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

0.1.1.beta6 + active_model_serializers include: "blargh.**" and "blargh.*" appears to be broken #53

Open
ghost opened this issue Nov 18, 2016 · 4 comments

Comments

@ghost
Copy link

ghost commented Nov 18, 2016

Using active_model_serializers version 0.10.2 and rails 4.2.7.1

I'm currently trying to get this down to an easy test that you can reproduce but thought I'd go ahead and report it while I work on making it.

We have an app with a render invocation using activemodel_serializers that looks like:

render json: @object,
           adapter: :json,
           root: 'something', 
           include: 'blah, blargh.**',

There's some other render options provided that shouldn't matter.

With jsonapi 0.1.1.beta2 this works as expected and there is a blargh key with nested stuff under it in the resulting json.

However, with jsonapi 0.1.1.beta6 there is no blargh key at all. If I remove the .** then the blargh key is back and has data, but obviously no nesting. If I switch to just blargh.* I get the same bad result as .** where I have no blargh key.

I will work on getting that test ready so you can take a look yourself. I'm "blaming" this on jsonapi gem instead of active_model_serializers because upgrading the jsonapi gem is the direct cause of the breakage.

@beauby
Copy link
Owner

beauby commented Nov 18, 2016

Hi @mordocai – the AMS dependency on jsonapi should have been pinned (which it is now, but the next release isn't out yet), as jsonapi is still in beta and its API is subject to change.
However, I'm not sure where the issue comes from, as the piece of code used by AMS (the IncludeDirective) has not been touched since 0.1.1.beta2. If you can make a test that reproduces this behavior I'll happily look into it.
Not however that you can pin jsonapi to 0.1.1.beta2 in your Gemfile, and AMS won't complain.

@ghost
Copy link
Author

ghost commented Nov 18, 2016

@beauby Well so far the only progress I have really made is that 0.1.1.beta3 is the first one that breaks for me (checking it out via bundle's github support since it was released on rubygems). The version of jsonapi-renderer (beta 1 or 2) doesn't seem to make a difference.

@ghost
Copy link
Author

ghost commented Nov 18, 2016

@beauby Figured it out. This works:

render json: @object,
           adapter: :json,
           root: 'something', 
           include: 'blah,blargh.**',

Having the space after the comma breaks the include directive (i've only tested this in beta6 but given my previous experiences I imagine the problem starts in beta3). It makes a hash with :" blargh" instead of :blargh as a key which of course doesn't work.

So a simple test is just to try it with a space after each comma.

Edit: Just to be absolutely clear:

      str = "blah, blargh"
      include_directive = JSONAPI::IncludeDirective.new(str)
      include_directive.key?(:blargh)
      # => false
      str = "blah,blargh"
      include_directive = JSONAPI::IncludeDirective.new(str)
      include_directive.key?(:blargh)
      # => true

@beauby
Copy link
Owner

beauby commented Nov 18, 2016

Oh, you are right, this did change at some point. There was a tradeoff between convenience for the API developper vs ability from the client to specify fields with spaces (which is allowed by the spec).
Edit: Although, spaces are not allowed at the beginning of member names, so it wouldn't have been an issue. For the actual rationale, see #23.

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

No branches or pull requests

1 participant