Skip to content

Change how renderer output is converted to JSON #1

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

Merged
merged 3 commits into from
Sep 27, 2021

Conversation

malcolmbaig
Copy link

@malcolmbaig malcolmbaig commented Sep 24, 2021

Context

  • In TTAPI, we're using jsonapi-rb/jsonapi-rails v0.4.0. This is the latest release at time of writing, and was published 7th Mar 2019.
  • That version has an in-built caching mechanism that caches the assembly and serialisation of JSON documents.
  • This mechanism has a bug which incorrectly serialises part of the document, returning arrays of over-escaped strings rather than arrays of JSON hashes in the data field.
  • This bug was fixed in master (but never released).
  • The fix introduced another bug which leads to inconsistencies in the output JSON. This PR attempts to fix that bug.

Changes proposed in this pull request

Update the railtie to fix issues with the way renderer output is
converted into a JSON string in the controller.

Cached and non-cached resources need slightly different handling, so
make a logical check to tell the difference and use the appropriate
method.

Guidance to review

  • The original library is essentially unmaintained at this stage, so I'm opting to fork it to implement this fix for use in TTAPI.
  • I originally planned to roll changes in master all the way back to the 0.4.0 tag, so that this bugfix is the only new thing we introduce to TTAPI. Having reviewed the diff, however, there isn't anything in there that's particularly concerning. We also get a passing test suite and deprecation fixes in the process. Views on this welcome.
  • I've tested this with TTAPI locally. Test suite is green and manual testing suggests caching and cache busting works as expected. I'm planning to add a spec for the caching when adding this change to TTAPI.

Link to Trello card

https://trello.com/c/lvTXTofp

Update the railtie to fix issues with the way renderer output is
converted into a JSON string in the controller.

Cached and non-cached resources need slightly different handling, so
make a logical check to tell the difference and use the appropriate
method.
Copy link

@duncanjbrown duncanjbrown left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent comment! 👏

Can we prove this in a spec?

These specs test the caching version of the render call more thoroughly.

- Assert that the expected data structure is stored in the cache
- Assert that the response is the same whether using the caching version
  of this call or the regular one
- Git ignore tmp cache files
- Add pry-byebug to dev dependencies
@malcolmbaig
Copy link
Author

@duncanjbrown added some specs!

Copy link

@gpeng gpeng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • bonus points for naming the user Johnny Cache 🤣

@duncanjbrown
Copy link

  • bonus points for naming the user Johnny Cache 🤣

+1 👏👏👏

Copy link

@duncanjbrown duncanjbrown left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 👍 👍

@malcolmbaig malcolmbaig merged commit 359d794 into master Sep 27, 2021
@remear
Copy link

remear commented Oct 5, 2021

@malcolmbaig Fantastic work on this! I just encountered some of these issues while picking up some long-overdue maintenance work on jsonapi-rb. Would you mind if I merge these changes upstream?

@malcolmbaig
Copy link
Author

@remear Sure! Raised a PR for the change here jsonapi-rb#133.

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

Successfully merging this pull request may close these issues.

4 participants