Skip to content
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

Issue with the latest upgrade 0.9.1 #746

Closed
kapso opened this issue Dec 4, 2014 · 31 comments
Closed

Issue with the latest upgrade 0.9.1 #746

kapso opened this issue Dec 4, 2014 · 31 comments

Comments

@kapso
Copy link

kapso commented Dec 4, 2014

After the upgrade render json: { ok: true } is returning this {"sessions"=>[["ok", true]]} where sessions is the name of the controller. Its weird.

adding a to_json fixes it render json: { ok: true }.to_json ??

@masda70
Copy link

masda70 commented Dec 5, 2014

I confirm this, happens after upgrading to 0.9.1 from 0.9.0.

EDIT: After some debugging, this behavior was most likely introduced after the following change
90343ce

The change affects serializing behavior for Enumerables, though its effect on Hash might be undesirable.

@shwoodard
Copy link

Hi, I am experiencing the same problem,

render json: @this.returns_a_hash, root: false # hash {a: b}; output: [['a', 'b']]

Is this intended new behavior? Is it fixed in master? Can we bump priority on this and release? Locking to 0.9.0 in the interim.

Another question to consider is, if the json being rendered is a Hash and not an object that includes ActiveModel::Serialization or other applicable ActiveModel modules, should ams intervene at all?

@steveklabnik
Copy link
Contributor

No, this is a bug.

@shwoodard
Copy link

Lemme see if I can fix :)

@shwoodard
Copy link

I created a pull request within my fork, shwoodard#1, with a failing spec. I made it from my repo to my repo instead to rails-api/ams because it doesn't work. However, I time boxed this and the fix wasn't apparent. I'm happy to make another pull request against rails-api/ams with just the failing spec, if desired.

@ashanbrown
Copy link

@andrusha pointed this out in the discussion at the end of #665 but unfortunately it appears to have been included in the v0.9.1. It's not clear to me why @andreychernih wanted this change. If the desired payload is the kind of Enumerable that wants to be rendered as an array, then maybe the payload should just define to_ary.

@mgonzalez-msights
Copy link

+1 completely broke my app

@magg
Copy link

magg commented Dec 10, 2014

+1

1 similar comment
@woohoou
Copy link

woohoou commented Dec 10, 2014

+1

@msaint
Copy link

msaint commented Dec 12, 2014

+1

@tadman
Copy link

tadman commented Dec 12, 2014

Just discovered this problem and it pretty much trashes any render(json: ...) calls in an application. Shouldn't 0.9.1 be yanked until this is fixed?

@steveklabnik
Copy link
Contributor

Yanking is always the wrong thing to do. I would like a fix, however.

@tadman
Copy link

tadman commented Dec 12, 2014

It is a bit drastic, but this is a pretty severe bug. I've found that several people I work with have encountered it independently and have had to roll back to 0.9.0. This is included as a dependency in popular gems like ember-rails so a lot of people are probably tripping over this without even realizing why.

Republishing 0.9.0 as 0.9.2 would not be a bad idea until this can be properly resolved.

@steveklabnik
Copy link
Contributor

Republishing 0.9.0 as 0.9.2 would not be a bad idea until this can be properly resolved.

I like this idea. I'll do this later today. :(

@steveklabnik
Copy link
Contributor

I have re-released 0.9.0 as 0.9.2 until we can get a fix.

@ashanbrown
Copy link

Bummer about all this. Could you let us know what the release plan is? Does everyone need to resubmit their pull requests against v0.9.0 or should the owners of their respective features submit fixes for model-less json serialization bug and the namespace lookup bug? Thanks.

@steveklabnik
Copy link
Contributor

Keep submitting to master. I just made https://github.com/rails-api/active_model_serializers/tree/0-9-2, which as you can see is just a version bump commit off of https://github.com/rails-api/active_model_serializers/releases/tag/v0.9.0

steveklabnik referenced this issue Dec 14, 2014
0.9.1 introduces unacceptable regressions in the 0.9.x series, and
yanking isn't cool. So re-release 0.9.0 as 0.9.2, so that new users
aren't messed up.
@tadman
Copy link

tadman commented Dec 15, 2014

Thanks for fixing that. Helps keep things up to date.

@zamith
Copy link

zamith commented Dec 19, 2014

With 0.9.2 I get an error that no serializer is used. I have this in the controller:

render json: Member.all

And I have a serializers/member_serializer.rb. It works perfectly fine with 0.9.1, but it just sends the entire object across, with 0.9.2.

@zamith
Copy link

zamith commented Dec 19, 2014

Figured out that it was because of how rails v4.2.0-rc3 handles the hooking of renderers. It is not using _render_option_json anymore, it is still using _render_with_renderer_json though.

Since AMS 0.8.x defines both it works, but in 0.9.x _render_with_renderer_json stopped being defined.

@shwoodard
Copy link

@zamith IMHO these last 2 comments are not related to #746. Maybe open a separate issue?

@zamith
Copy link

zamith commented Dec 19, 2014

@shwoodard I thought it was related, but it turned out not to be. It's not an issue for me anymore, you can just disregard it.

@steveklabnik
Copy link
Contributor

0.9.2 is 0.9.0, so you'll get the same bugs you had in it.

@zamith
Copy link

zamith commented Dec 22, 2014

@steveklabnik The bug is only present in 0.9.0 and 0.9.2, in 0.9.1 it has been fixed in 447d969

@steveklabnik
Copy link
Contributor

Exactly.

@kurko
Copy link
Member

kurko commented Jan 13, 2015

We lost many, many good commits (almost 4 months) doing this. 0.9.1 was good except for 1 commit (#665 ?). We had support for Rails 4.2 and a bunch of other fixes.

Could someone kindly pick the 0.9.1 tag, revert just that one commit and open a PR? I really can't do this because I don't use 0.9 :( but your work will help tons of people.

@nateberkopec
Copy link

@kurko #775 but the merge isn't clean

EDIT: #776

@kurko
Copy link
Member

kurko commented Jan 13, 2015

#776, not #775.

@kurko
Copy link
Member

kurko commented Jan 13, 2015

Could everyone here please test #776? It's basically the 0.9.1 (full of fixes) minus the problematic commit that breaks the universe. If #776 works on your apps, then it could potentially become 0.9.3.

@recurrence
Copy link

Wouldn't this be better than 0.9.2 even if there is a small regression or two? Seems to be working fine for me if this naive and completely unscientific data point helps any :) Ship It! :)

@kurko
Copy link
Member

kurko commented Jan 21, 2015

Released https://rubygems.org/gems/active_model_serializers/versions/0.9.3. It's basically 0.9.1 (tons of bugfixes) minus the commit that caused tons of regressions. Please test and let us know how it goes 😁

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests