Skip to content

Commit

Permalink
Generate special (#654)
Browse files Browse the repository at this point in the history 
* Work around the special case of rails and generate

* Alternate approach to generate issue
  • Loading branch information
@ohler55
ohler55 committed Apr 14, 2021
1 parent b12cbfb commit a298dd0
Show file tree
Hide file tree
Showing 4 changed files with 16 additions and 5 deletions.
5 changes: 5 additions & 0 deletions CHANGELOG.md
View file
Expand Up @@ -5,8 +5,13 @@
- Code re-formatted with clang-format. Thanks goes to BuonOmo for
suggesting and encouraging the use of a formatter and getting the
effort started.

- Added support for `GC.compact` on `Oj::Doc`

- Fixed compatibility issue with Rails and the JSON gem that requires
a special case when using JSON.generate versus call `to_json` on an
object.

## 3.11.3 - 2021-03-09

- Fixed `respond_to?` method on `Oj::EasyHash`.
Expand Down
4 changes: 2 additions & 2 deletions ext/oj/dump.c
View file
Expand Up @@ -599,8 +599,8 @@ void oj_dump_obj_to_json(VALUE obj, Options copts, Out out) {
void oj_dump_obj_to_json_using_params(VALUE obj, Options copts, Out out, int argc, VALUE *argv) {
if (0 == out->buf) {
out->buf = ALLOC_N(char, 4096);
out->end = out->buf + 4095 -
BUFFER_EXTRA; // 1 less than end plus extra for possible errors
// 1 less than end plus extra for possible errors
out->end = out->buf + 4095 - BUFFER_EXTRA;
out->allocated = true;
}
out->cur = out->buf;
Expand Down
10 changes: 8 additions & 2 deletions ext/oj/mimic_json.c
View file
Expand Up @@ -384,8 +384,14 @@ static VALUE mimic_generate_core(int argc, VALUE *argv, Options copts) {
rb_raise(rb_eTypeError, "nil not allowed.");
}
*/
oj_dump_obj_to_json_using_params(*argv, copts, &out, argc - 1, argv + 1);
if (1 < argc) {
oj_dump_obj_to_json_using_params(*argv, copts, &out, argc - 1, argv + 1);
} else {
VALUE active_hack[1];

active_hack[0] = rb_funcall(state_class, oj_new_id, 0);
oj_dump_obj_to_json_using_params(*argv, copts, &out, 1, active_hack);
}
if (0 == out.buf) {
rb_raise(rb_eNoMemError, "Not enough memory.");
}
Expand Down Expand Up @@ -755,7 +761,7 @@ static VALUE mimic_object_to_json(int argc, VALUE *argv, VALUE self) {
// seem to prefer the option of changing that.
// oj_dump_obj_to_json(self, &mimic_object_to_json_options, &out);
oj_dump_obj_to_json_using_params(self, &copts, &out, argc, argv);
if (0 == out.buf) {
if (NULL == out.buf) {
rb_raise(rb_eNoMemError, "Not enough memory.");
}
rstr = rb_str_new2(out.buf);
Expand Down
2 changes: 1 addition & 1 deletion ext/oj/oj.h
View file
Expand Up @@ -247,7 +247,7 @@ extern void oj_parse_options(VALUE ropts, Options copts);

extern void oj_dump_obj_to_json(VALUE obj, Options copts, Out out);
extern void
oj_dump_obj_to_json_using_params(VALUE obj, Options copts, Out out, int argc, VALUE *argv);
oj_dump_obj_to_json_using_params(VALUE obj, Options copts, Out out, int argc, VALUE *argv);
extern void oj_write_obj_to_file(VALUE obj, const char *path, Options copts);
extern void oj_write_obj_to_stream(VALUE obj, VALUE stream, Options copts);
extern void oj_dump_leaf_to_json(Leaf leaf, Options copts, Out out);
Expand Down

11 comments on commit a298dd0

@espen
Copy link

@espen espen commented on a298dd0 Apr 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

3.11.4/5 is an issue for me. I assume due to this commit. I'm not able to replicate the issue in a simple repo yet but in my project I am using Sidekiq to perform a job from a Rails controller and it is sending a hash to Sidekiq (Using Sidekiq inline testing, so jobs are not going through the Sidekiq server). In 3.11.3 I get a hash in the Sidekiq job but with 3.11.4 I get an error saying it is a string.

@ohler55
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only significant change is is mimic_json.c line 387 so I suspect it has something to do with the active support work around which means the state objects are most likely different in some way. To debug we will need to look at the object when it works vs when it doesn't. Do you know enough about C to be able to put in that kind of debug information? If note is there a was to set up a test case? Or an image I can work with?

@kochalex
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ran into an issue like this - in our case for two jobs, in error we were calling Sidekiq's perform_async with objects as the argument.

Previously, these objects were automatically converted to a hash or json in the process of sending the job to the background queue and after updating from 3.11.3 to 3.11.5, the object itself was being stored as a string.

@ohler55
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you see different behaviour when comparing the json gem and oj in that situation?

Any chance of a stack trace to see what is being called?

@espen
Copy link

@espen espen commented on a298dd0 Apr 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While there was definitively some change in behaviour here I had a closer look at the code that was the root of my problem. Turns out I was passing some objects (ActionController::Parameters) instead of a hash to Sidekiq. I modified this to correct usage, converting it to a hash, and I am no longer experiencing this issue.

I only ran my tests for this to occur so not sure if this was an issue when running a normal server. For testing Sidekiq will run jobs differently than with a server. It will execute the jobs immediately (inline testing). I see that it will generate JSON for the input data.

@ohler55
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, so problem solved, right?

@mrbongiolo
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @ohler55, thanks for your amazing work on Oj! I found an "issue" in the way BigDecimal is handled when upgrading from v3.11.3 to v3.11.4 when using JSON.generate.

It's a rails project with using those configs:

Oj.optimize_rails
Oj.default_options = {
  mode: :rails,
  float_precision: 0,
  bigdecimal_as_decimal: true,
  bigdecimal_load: :bigdecimal,
  trace: true
}

When using v3.11.3:

[1] pry(main)> JSON.generate({:ticker=>"ASAI3", :price=>18.7.to_d, :rate=>-0.8.to_d})
#0:dump_compat.c:938:Oj:}: dump Hash
#0:dump_compat.c:938:Oj:}:   dump String
#0:dump_compat.c:963:Oj:{:   dump String
#0:dump_compat.c:938:Oj:}:   dump BigDecimal
#0:dump_compat.c:120:Oj:>: to_json BigDecimal
#0:      rails.c:1493:Oj:}: dump BigDecimal
#0:      rails.c:1504:Oj:{: dump BigDecimal
#0:dump_compat.c:128:Oj:<: to_json BigDecimal
#0:dump_compat.c:963:Oj:{:   dump BigDecimal
#0:dump_compat.c:938:Oj:}:   dump BigDecimal
#0:dump_compat.c:120:Oj:>: to_json BigDecimal
#0:      rails.c:1493:Oj:}: dump BigDecimal
#0:      rails.c:1504:Oj:{: dump BigDecimal
#0:dump_compat.c:128:Oj:<: to_json BigDecimal
#0:dump_compat.c:963:Oj:{:   dump BigDecimal
#0:dump_compat.c:963:Oj:{: dump Hash
=> "{\"ticker\":\"ASAI3\",\"price\":18.7,\"rate\":-0.8}"

[3] pry(main)> {:ticker=>"ASAI3", :price=>18.7.to_d, :rate=>-0.8.to_d}.to_json
#0:      rails.c:1493:Oj:}: dump Hash
#0:      rails.c:1493:Oj:}:   dump String
#0:      rails.c:1504:Oj:{:   dump String
#0:      rails.c:1493:Oj:}:   dump BigDecimal
#0:      rails.c:1504:Oj:{:   dump BigDecimal
#0:      rails.c:1493:Oj:}:   dump BigDecimal
#0:      rails.c:1504:Oj:{:   dump BigDecimal
#0:      rails.c:1504:Oj:{: dump Hash
=> "{\"ticker\":\"ASAI3\",\"price\":18.7,\"rate\":-0.8}"

BigDecimal values are converted to numerical values.

When using v3.11.4:

[1] pry(main)> JSON.generate({:ticker=>"ASAI3", :price=>18.7.to_d, :rate=>-0.8.to_d})
#0:dump_compat.c:939:Oj:}: dump Hash
#0:dump_compat.c:939:Oj:}:   dump String
#0:dump_compat.c:964:Oj:{:   dump String
#0:dump_compat.c:939:Oj:}:   dump BigDecimal
#0:dump_compat.c:121:Oj:>: to_json BigDecimal
#0:dump_compat.c:129:Oj:<: to_json BigDecimal
#0:dump_compat.c:964:Oj:{:   dump BigDecimal
#0:dump_compat.c:939:Oj:}:   dump BigDecimal
#0:dump_compat.c:121:Oj:>: to_json BigDecimal
#0:dump_compat.c:129:Oj:<: to_json BigDecimal
#0:dump_compat.c:964:Oj:{:   dump BigDecimal
#0:dump_compat.c:964:Oj:{: dump Hash
=> "{\"ticker\":\"ASAI3\",\"price\":\"18.7\",\"rate\":\"-0.8\"}"

[2] pry(main)> {:ticker=>"ASAI3", :price=>18.7.to_d, :rate=>-0.8.to_d}.to_json
#0:      rails.c:1491:Oj:}: dump Hash
#0:      rails.c:1491:Oj:}:   dump String
#0:      rails.c:1502:Oj:{:   dump String
#0:      rails.c:1491:Oj:}:   dump BigDecimal
#0:      rails.c:1502:Oj:{:   dump BigDecimal
#0:      rails.c:1491:Oj:}:   dump BigDecimal
#0:      rails.c:1502:Oj:{:   dump BigDecimal
#0:      rails.c:1502:Oj:{: dump Hash
=> "{\"ticker\":\"ASAI3\",\"price\":18.7,\"rate\":-0.8}"

In this case, BigDecimal values are being converted to string values when JSON.generate is used. Is this the correct behaviour or is this a bug?

I found this when some ActiveJobs (with Sidekiq as the queue adapter) started to fail, those jobs receive BigDecimal values and now those values are being processed as string when the job deserializes the arguments. As far as I could debug it seems like Sidekiq uses JSON.generate when pushing the job data to the Redis database, thus creating inconsistency.

@ohler55
Copy link
Owner Author

@ohler55 ohler55 commented on a298dd0 Aug 12, 2021 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The traces help, thanks. I'll look into it and see what changed. @Watson1978 was kind enough to spend some time optimizing dumping so maybe something in the changes caused the issue. I think this is a different issue though. Would you mind creating a separate issue for this?

@grk
Copy link

@grk grk commented on a298dd0 Aug 27, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're running into a similar issue with timestamps:

3.11.3:

JSON.generate(Time.current)
#0:dump_compat.c:938:Oj:}: dump ActiveSupport::TimeWithZone
#0:dump_compat.c:120:Oj:>: to_json ActiveSupport::TimeWithZone
#0:      rails.c:1493:Oj:}: dump ActiveSupport::TimeWithZone
#0:      rails.c:1504:Oj:{: dump ActiveSupport::TimeWithZone
#0:dump_compat.c:128:Oj:<: to_json ActiveSupport::TimeWithZone
#0:dump_compat.c:963:Oj:{: dump ActiveSupport::TimeWithZone
=> "\"2021-08-27T07:19:00.634Z\""

3.11.4:

pry(main)> JSON.generate(Time.current)
#0:dump_compat.c:939:Oj:}: dump ActiveSupport::TimeWithZone
#0:dump_compat.c:121:Oj:>: to_json ActiveSupport::TimeWithZone
#0:dump_compat.c:129:Oj:<: to_json ActiveSupport::TimeWithZone
#0:dump_compat.c:964:Oj:{: dump ActiveSupport::TimeWithZone
=> "\"2021-08-27 07:18:43 UTC\""

running with Oj.optimize_rails

@ohler55
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please create a separate issue for this. The original had nothing to do with time string formats.

@ohler55
Copy link
Owner Author

@ohler55 ohler55 commented on a298dd0 Aug 27, 2021 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@grk when you create the new issue please let me know what results you get without Oj and just using the JSON gem with rails. I get the 3.11.4 output without Oj and just using the JSON gem.

Please sign in to comment.