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

GPL-nnn Update jsonapi-resource or investigate alternatives #2857

Closed
2 tasks
JamesGlover opened this issue Jul 23, 2020 · 8 comments
Closed
2 tasks

GPL-nnn Update jsonapi-resource or investigate alternatives #2857

JamesGlover opened this issue Jul 23, 2020 · 8 comments
Labels
Enhancement New feature or request

Comments

@JamesGlover
Copy link
Contributor

User story
We have been stuck on version 0.9.0 of jsonapi-resources for a while, this is mostly as upgrades are causing test failures.

Contributing factors:

  1. Monkey patches to handle eager loading of necessary records in Api::V2::BaseResource app/resources/api/v2/base_resource.rb
  2. Monkey patches to handle polymorphic relationships in config/initializers/patch_json_api_resource.rb

More information on previous depfu attempts to update resources:
#2149
#2035

Attempts to fix earlier 0.9.1 branches (With some success, although the build now appears broken)
https://github.com/JamesGlover/sequencescape/tree/depfu/update/jsonapi-resources-0.9.5
(Inclusions weren't working quite right for relationships, but didn't have a proper chance to investigate more fully.)

Version 0.10 makes some nice improvements, but seems to break has_many 'polymorphic' associations. In particular I was seeing issues with plate relationships. This appears to be because its assuming your polymorphism relates to joins onto different tables, rather than sti. As a result its expecting an association_type column somewhere for building up the relationship data (a combination of type and id). It also seems to expect to see this on labware, which is a little odd. I've made a little progress into fixing some aspects of this, but it does raise questions if this is the right gem. (I had thought it was abandoned, but instead it seems to just get updates somewhat sporadically)

Unfortunately heavy use of sti and polymorphic associations do limit our options a little.

Who are the primary contacts for this story
e.g. John S (don't include surnames in public repos)

Acceptance criteria
To be considered successful the solution must allow:

  • add a list of acceptance criteria here
  • ...

Dependencies
This story is blocked by the following dependencies:

  • #<issue_no.>
  • sanger/#<issue_no.>

Additional context
Add any other context or screenshots about the feature request here.

@JamesGlover JamesGlover added the Enhancement New feature or request label Jul 23, 2020
@JamesGlover
Copy link
Contributor Author

I'll push up the 0.10 experimental branch once I've finished running down my current avenue of investigation.

@JamesGlover
Copy link
Contributor Author

@JamesGlover
Copy link
Contributor Author

Hmm, looks like the performance issues I refer to in #2149 might exist anyway. Briefly:

  • We patched in the ability to supply default_includes to automatically load associated records that appear inline (eg. uuid)

  • This works when retrieving a record directly

  • However something about loading associations seems to nuke this.

  • As best I can tell, is we do load the associated record, and their resources from the database

  • However when JSON-API resource attempts to render the associated objects, it does its own fetching from the database, and these records don't have the associations pre-loaded, so we fetch them all again.

@JamesGlover
Copy link
Contributor Author

JamesGlover commented Jun 10, 2021

Had another crack with json-api-resources 0.10.5 [Edit: ow 0.10.7]. Almost got there with a couple of monkey patches, but running into some major issues with relationships and STI. This branch has the work, along with a couple of added tests to catch edge cases:

https://github.com/sanger/sequencescape/tree/json_api_update_v2

Things that didn't work:

  1. Making the relationships polymorphic resulted in an undefined method polymorphic_type' for #<JSONAPI::Relationship::ToMany:0x00007fe0b7a23ab0` which suggests that polymoprhic has_many realtionships are a little broken. But a glance at the code indicates that this probably isn't what we want anyway, and is more geared up to handle database level polymorprhism rather than STI.
  2. Removing class name and defining abstract resources tends to result in a cascade of a few minor issued, but essentially results in relationships names 'parents' rather than 'labware' which is probably even worse.

I think the trick will be to pull back the sti_type columns, in the pluck for STI relationships and use them,. but that's getting a little larger than I'd like for a monkey patch.

Related issues:

= Monkey patched
cerebris/jsonapi-resources#1371 [Now merged - no new release currentl ]
cerebris/jsonapi-resources#1369

= Outstanding
cerebris/jsonapi-resources#772 # Although we're more concerned with the relation objects than the endpoints, I supsect any fix will cover both
Potential PR fix: cerebris/jsonapi-resources#748

@JamesGlover
Copy link
Contributor Author

Looked at https://github.com/stas/jsonapi.rb in context of unified Warehouse:

  1. It is mostly just serialization, deserialization with a few helpers to handle parsing json api arguments
  2. There doesn't seem to be any handling of eager-loading off the shelf, so we'd have to roll our own
  3. Fewer overall users based on stars and forks
  4. Last major change was 6 months ago, with minor readme updates 2 months ago. Slightly less active then json-api resources
  5. https://github.com/jsonapi-serializer/jsonapi-serializer which it is based on is more active, but still overall less so than json-api resources

So not sure I'd suggest a switch, although the simplicity and lack of magic is nice. Also haven't really battle tested it yet against more complicated setup, so its possible that it will run into the same kinds of issues with STI/polymorphisms

Had a bit of a poke around at other alternatives, but nothing appears to be in great health.

@JamesGlover
Copy link
Contributor Author

This is blocking Rails 6.1 release, so increases in importance

@JamesGlover
Copy link
Contributor Author

Looked into this a bit more, and it turns out there is an official fix underway, although it hasn't move much for a while:
https://github.com/cerebris/jsonapi-resources/commits/sti_v0-10

There's a minor issue with the approach I can see here in that it uses a hard-coded type column, whereas we have overridden the rails defaults (for reasons I was told once about 10 years ago, and have since forgotten). I've popped a comment to that effect on the PR, but I think if there was movement on this (without a fix) it would be worth renaming our columns to the Rails defaults. (Assuming the 10 year old reasons aren't still valid)

@JamesGlover
Copy link
Contributor Author

JamesGlover commented Apr 29, 2022

We've also now got a new issue with some of the plate creation spec failing, but I think this is just due to the way readonly now works. (Ie. it does, and was previously a little broken)

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

No branches or pull requests

2 participants