Skip to content

Conversation

@tiagopog
Copy link
Owner

@tiagopog tiagopog commented May 23, 2016

Fixes #12

Problem

Due to a change on how JSONAPI::Resources deals with the setup and check of requests around its operations, the methods JSONAPI::Utils#(setup_request|json_api_(render|serialize)) had to change as well, since the request validation and error rendering that once were handled by JR now needed to be handled by JU.

Unfortunately the implementation caused a bug when dealing with invalid requests:

  1. An invalid request was made (e.g. an invalid param);
  2. Resource was persisted with no errors, since its validation has passed;
  3. When it tries to render the response then an error message was rendered showing the issues with the request's invalid param.

Fix

Now it sets up and checks the request before the action executes. This makes much more sense than validating the request after the actual operation is complete.

  • Set up and validate the request before action;
  • Write specs.

@tiagopog
Copy link
Owner Author

@mecampbellsoup could you also test this PR, please? I'm leaving work now and I may write some specs to cover this case when I get home.

Cheers!

@tiagopog tiagopog self-assigned this May 24, 2016
@tiagopog tiagopog added the bug label May 24, 2016
@tiagopog
Copy link
Owner Author

I also ran a suite of automated tests including this fix on an API we (Beauty Date) keep in production. As tests passed with no issues, I will merge this branch and generate a patch release.

@tiagopog tiagopog merged commit e7755e4 into master May 24, 2016
@tiagopog tiagopog deleted the hotfix/setup-and-check-request branch May 24, 2016 22:42
@mecampbellsoup
Copy link
Contributor

Hey nice job @tiagopog! Sorry I didn't help out after opening the issue :/

@tiagopog
Copy link
Owner Author

Hey! That's all right, @mecampbellsoup. Thanks for reporting the bug and feel free to share any thoughts you have on the gem :-)

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.

3 participants