-
-
Notifications
You must be signed in to change notification settings - Fork 252
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Rendering JSON with ActiveModel::Serializers returns object #199
Comments
Can you provide a simple test I can run? |
@ohler55 I will prepare a test by the end of this week. |
I can reproduce this without AMS by doing a simple |
I'm about ready to make a release. There were some changes around ActiveSupport. Maybe that will help. Anyway, if not, I don't have ActiveRecord setup with a database. A simple test would be one that uses only some number of gems and some ruby code you provide. I'm still not sure what the expected JSON is or how to reproduce what you are doing. |
Here's a Rails app with the behaviour: https://github.com/Soliah/oj-test. There are 2 branches, master and no-oj. The only difference betwen the 2 branches is the inclusion of oj in master. The expected output is just the properties of the User model. Edit:Rails 4.1.7, Ruby 2.1.4. no-oj branch:
master branch with oj:
|
I haven't done anything special in above setup like use AMS. Just a simple |
Since I have not used rails that much it is going to take some time for me to figure out how to get you test working and see what object class is being serialized. |
I'm happy to help so please let me know if you want me to try anything. |
Please try the latest. I just released 2.11.0. If that does not work then if you can make a simple ruby script that creates an instance of the object you serialized that would be great. The User object from the looks of it. |
I've just tried this in the Rails app and the behavior is the same. What I'm not clear on is if I'm supposed to call I've tried reproducing this with ActiveRecord outside of Rails and can't get the same behaviour. This leads me to believe that it's something to do with Rails further up in the rendering section. Maybe something here: https://github.com/rails/rails/blob/master/actionpack/lib/action_controller/metal/renderers.rb though I can't be sure. I might just work around this and do what was done here http://brainspec.com/blog/2012/09/28/lightning-json-in-rails/ |
For interests sake here is ActiveRecord without Rails:
require 'active_record'
require 'oj'
Oj.mimic_JSON()
# Requiring json here seems to stop conflicts when requiring json in other files.
begin
require 'json'
rescue Exception
# ignore
end
ActiveRecord::Base.logger = Logger.new(STDERR)
ActiveRecord::Base.establish_connection(
:adapter => "sqlite3",
:database => ":memory:"
)
ActiveRecord::Schema.define do
create_table :users do |table|
table.column :first_name, :string
table.column :last_name, :string
table.column :email, :string
end
end
class User < ActiveRecord::Base
end
User.find_or_create_by(first_name: "John", last_name: "Smith", email: "[email protected]")
puts User.first.as_json |
The Oj.mimic_JSON should take care of spoofing the json gem. If it is not, let me know and I'll get it back to that state. There are some things that will help you understand what is going on. Oj has different modes. If you are trying to serialize an object you can use the default :object mode in Oj. As you have seen that creates a JSON representation with all the active record extra stuff in it. Probably not what you want. If you can as_json() and serialize that using the lighting approach you pointed to that should work well. Notice I use "should". Heres where it might get tricky. the as_json() method is only calling on compat mode. mimic_JSON puts Oj in compat mode though. All good so far. If you have the :use_to_json option set to true the dump should work as expected. If it is false it will most likely not. Can you verify the :use_to_json is true? Another twist. If to_json is defined and you call that :use_to_json is automatically set to false. This is to avoid the recursive calls active makes when calling to_json. It basically calls the serializer (Oj) which is expected to call to_json. This goes on until the app dies. The key may be to add another :use_as_json or something. Lets continue exploring. |
Ok it seems to me that in Rails, Passing options to However, going |
That is useful information. Passing arguments to to_json() will not work. You can set the default mode like this.
From what you describe it sounds like the problem may be in the :use_to_json flag being set to false when to_json() is called. I may have to change that. Can you verify that on Oj.dump(User.first) the as_json() method is called? |
Putting a debugger here https://github.com/rails/rails/blob/master/actionpack/lib/action_controller/metal/renderers.rb#L117 the following behaviour is observed:
What interesting is EditThe
|
Just confirmed that doing |
okay, I guess I have to allow as_json. Let me look at some of the issue and make sure as_json does not get into an endless recursive loop with some of the other rails libraries. I the mean time I can create a branch for you with the change if that helps. |
One last thing, is this issue has sort of diverged potentially from the original issue with ActiveModelSerializers. I have a feeling it's somewhat related but maybe not completely. I would prefer for this to work with AMS but that might be another issue. |
Do you know what the AMS call? I'm sure it is related but not sure the path the AMS is taking. |
The current stable version 0.9.x uses master which will be 0.10 is a big refactor from what I understand, so the API will change. @kurko or @guilleiguaran might be able to offer some suggestions. |
Try the as_json-ok branch. I think it might be okay. I have a second level check for recursion. The problem is that active support has as_json return the object itself in some cases like for core types. |
Ok trying out 00b368c (as_json-ok branch)
With However if I change it to In both cases, |
Thats not good. There is a way out but it isn't very nice. A dump routine can be registered. It is not efficient though. I suspect the problem is the collection is a Hash or an Array and Active has added a to_json() method that calls the JSON.dump() which is now Oj and Oj then would call the to_json on the object if the :use_to_json was enabled. Works fine up to that point but then the use_to_json is still set to false when trying to convert the User object to json but at that point all attributes will be serialized. Let me sleep on this and see if I can come up with a work around. I think the key will be getting the as_json methods to be called. |
I think @chancancode can help understanding how the to_json/as_json works in the most recent versions of active support 😃 |
@chancancode, can we figure out a way to get this working? |
I'll take a look in a few hours 👍 |
Thanks, I'll be offline by then but will pick it up in the morning. |
Hello! The situation we got into is quite unfortunate, as we have multiple JSON libraries fighting over each other about very generic method names like Quick Summary
Before we get into the details, here is some backstory and history of how we got ourselves into the current situation. Rails JSON encoderAncient History (Rails 2.3 era)Once upon a time, Ruby doesn't ship with any JSON library. As Rails needed to generate JSON objects, we wrote our own encoder, and eventualy settled on the Rails 3 eraAt some point, Rubyists started writing other JSON libraries/wrappers that are more efficient than the pure-Ruby encoder that comes with Rails, such as The key observation here is that most application programmers don't override Thus, For backwards compatibility reasons, the (*the Rails 4 eraWhen Ruby 1.9.1 was released, the As the popularity of the JSON gem grew though, another annoying issue arose. As mentioned above, the Rails 4.1+I picked up the task of cleaning up some of these mess. A few things changed:
Rails controllerWhen you call Active Model Serializerv0.8 & 0.9These two versions of AM::S has a completely different codebase, but for our purpose they are virtually the same. When you do a If you are using Rails 4.2, you'll need 0.8.2 / 0.9.0 for things to work properly because Rails renamed some of the controller hook methods. v0.10This is another complete rewrite, but currently it defines (@guilleiguaran we need to ✂️ these Where things went wrong with Oj + RailsThe Rails codebase relies on Where does that leave us...Option 1: Explicitly opt-in
This is pretty much how things always worked with previous versions of Oj. (Again MultiJson never actually helped you for encoding.) Presumably, you can monkey patch the json renderer to do this automatically for you (in that case the AM::S renderer additions should Just Work™ also). Option 2: Automatically use Oj for everything...This requires patching Oj to offer a mode that would override Also, it's quite likely that we would deviate slightly on the encoding result, but if anything comes up I'm happy to work with you to figure out what the "correct" behavior should be (or whether that's an edge case that should be left undefined). Option 3: Automatically use Oj for everything (Rails 4.1+ only)...If supporting only Rails 4.1 and above is an option, there's a slightly less invasive option. (In this case it probably shouldn't try to mimic json gem.) It still has the "deviate slightly on the encoding result" problem, but it'd at least shield you from the json gem compat |
wdyt @jeremy? |
Fantastic explanation. Thanks. I was aiming for option 1 with the exception of the to_json override. The override is there now to mitigate the rails monkey patching the core/base types. It may make sense for Oj to change some of the options and their behaviour so that it can be made to work with both Rails and the Active libraries. I'll make a branch of Oj that follows the semi-formal description for as_json and see how it works. I believe the key difference will be in handling the primitive and base type. |
Let me know how it goes! Treat the gist as a "note", more than anything, we can certainly review and fix anything that doesn't make sense. As for the immediate problem described in this ticket, I am pretty sure it's just that |
I'm doing some planning and wanted to run a few things past the group. I can't remove the to_json override since many (maybe most) call JSON.dump which creates a recursive loop unless the to_json call turns off Oj calling to_json when it encountered an Object that responds to that method. The alternative would be to never call to_json from within Oj. Any opinions? The as_json would always be called if the Object responded to that method unless it is a primitive. I will leave in the check for an object returning itself though. |
I'm not 100% sure if I understand your problem, can you explain...
Here is what the
I suspect point number 1 is why we don't have the same problem that you are dealing with. For point number 2, we were able to do that because |
I think part of the problem is I'm trying got be compatible with both older and the latest rails. Maybe the trick is the ::JSON::State object. I'm not exactly sure that works yet though. It is clear that to_json and as_json need to be considered separately though. Let me try to put something together and see how it works. Not sure if I will get to it tonight to tomorrow. |
Simply not turning off as_json with to_json seems to be enough. I have to put together a test with and ActiveSupport collection though. If anyone has simple one I can use that would be great. Otherwise I'll do some web searching and write something tomorrow afternoon. |
I'm wondering if yajl json's approach would work here or if it's the same issue: |
I'd prefer not to take the yawl/json_gem approach as it forces all object to use to_s as a json representation if ActiveSupport is not defined. Not all Oj users are using ActiveSupport. |
Here is a simple test that works but I'm not sure how to make sure Oj is used for the to_json call without using Oj.mimicJSON. It seems to do the right thing in the as_json-ok branch.
|
Does the latest release fix this? |
I'll give it ago and report back. |
Is oj_mimic_json still required? |
the mimic_JSON call is needed if you are making JSON calls or calling to_json(). If you are using Oj directly then mimic_JSON is not needed. |
Working for myself with active_model_serializers using the current HEAD. Loading about 10k records in development takes about half a second off rendering: Without oj: The objects are being serialized correctly with or without AMS. |
Excellent! |
For smaller data sets the rendering time difference isn't particularly different than the built in JSON library. Is there a way I can confirm that oj is being used? |
Try setting the default options indent to 2 and look at the output. |
Ok that confirms that oj is definitely working. ❤️ ✨ |
great, are we okay to close this issue? |
Yup. Would be great to have a new version up on Rubygems. |
2.11.1 is there now. What version have you been using? |
Oh sorry, I was just pulling down current HEAD on master (374c113). The version on Rubygems doesn't give me the right JSON back. |
Thats strange. The code is the same except for some travis changes. And the changes to floats tonight. |
Sorry, looks like I was referencing the wrong version in my Gemfile.lock. Good to close! |
great, thanks |
@chancancode @Soliah How does this fix the AMS problem? |
I made an upgrade of oj from version 2.10.2 to 2.10.4, and I got a serialized object instead of expected JSON.
I use rails-api 0.3.1 and active_model_serializers 0.9.0
The text was updated successfully, but these errors were encountered: