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

Fix double request parsing for default actions #45

Merged
merged 10 commits into from
Dec 26, 2016

Conversation

tiagopog
Copy link
Owner

@tiagopog tiagopog commented Dec 17, 2016

Fixes #28 and #44

TODO

  • Fix double request instantiation for default actions by overriding the JSONAPI::ActsAsResourceController#process_request method – it's no longer required to override the default actions in order to skip this behavior;
  • Write specs covering the case for default actions;
  • Improve README.md;
  • Improve code doc;
  • Update version;
  • Update .travis.yml.

@tiagopog tiagopog self-assigned this Dec 17, 2016
@tiagopog tiagopog added the bug label Dec 17, 2016
@tiagopog tiagopog changed the title [WIP] Fix request parsing for default actions [WIP] Fix double request parsing for default actions Dec 17, 2016
@tiagopog tiagopog added this to the v0.5.0 milestone Dec 17, 2016
@tiagopog
Copy link
Owner Author

👀 @yellow5 @delner

@delner
Copy link

delner commented Dec 25, 2016

So I just spotted this in the Rails 5.0.1 release notes:

Added ActionController::Parameters#deep_dup which actually creates a params copy, instead of refereing to old references in params.

Fixes #26566.

rails/rails#26566

It's possible that using deep_dup on that params object instead of dup in 5.0.1 might circumvent this entire issue. Worth trying?

@tiagopog
Copy link
Owner Author

tiagopog commented Dec 25, 2016

Cool! I didn't know about the existence of that method, thanks for sharing.

It's certainly an easy fix for that issue of changing the internals of params, however, it would still make double instantiation on the @request instance variable , which I'm also trying to fix with this PR.

The fix proposed here – override the JSONAPI::ActsAsResourceController#process_request – will probably be temporary since the proper fix would be a memoization on @request.

@tiagopog tiagopog changed the title [WIP] Fix double request parsing for default actions Fix double request parsing for default actions Dec 25, 2016
@tiagopog tiagopog merged commit 6233b5c into master Dec 26, 2016
@tiagopog tiagopog deleted the feature/fix-request-parsing-for-default-actions branch December 26, 2016 14:38
@yellow5
Copy link
Contributor

yellow5 commented Dec 27, 2016

👍 👏

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

Successfully merging this pull request may close these issues.

None yet

3 participants