-
Notifications
You must be signed in to change notification settings - Fork 63
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
Correctly support circular dependencies, bonus: 7.5x faster test suite #164
Conversation
…performance improvement
…nd of the superclass initializer (needed for contentful_model)
Note: I have suppressed Rubocop wrt the amount of parameters used by the BaseResource/FieldsResource/Entry class heirarchy initializers. I tried to work around the parameter length here but short of a large refactoring to use something like a ResourceStore class to replace |
Hey @rezonant, This is amazing work! 👍 I'll look into it on detail today, run a few tests and see if we can merge it. Looks super promising! Thank you so much for the work! Cheers |
Hey @rezonant, One comment I have, In order to keep backwards compatibility and avoid the need of a new major release, can you please change the default value of If you want a new section can be created as well explaining the difference between Once again, I really appreciate this work, it's of great value! Cheers |
Sure. I believe it shouldn't cause any issues but I suppose better safe than sorry. Will update PR today |
OK, PR is updated to disable |
Hey @rezonant, I'm about to start doing some intensive testing on this. Will most likely get it merged tomorrow. By the way, if this works as good as I expect it, and caching still works as expected, then I'll be adding this same solution for our Python SDK. So, thank you very much for the inspiration. Cheers |
No problem. One interesting side effect I ran into yesterday is Rails' as_json. The default implementation would yield a stack overflow with a circular graph, but |
Regarding the In the case of |
Merged! 🎉 |
This changeset introduces a new option, enabled by default, called
reuse_entries
. When enabled, the gem will reuse Entry/Asset objects constructed during the current hydration cycle. This means if a request has cyclical relationships, only one object per type/ID is hydrated and that object is reused in the appropriate place in the dependency graph.This changeset improves the woes expressed in #124 and elsewhere.
This change makes the previous
max_include_resolution_depth
configuration setting useful only for genuinely deep object structures, where there are a large amount of unique objects within the request.Although I haven't done the work for it in this PR, I think the
max_include_resolution_depth
test fixtures should be changed to use a flat depth structure and not a cyclical one as thenyancat
fixture is currently, asreuse_entries
makes those tests fail because the gem never needs to use a ContentfulLink to represent links which are too deep (it just reuses the existing Entry instead). I think the flag still has value but it would no longer come into play for cyclical structures. So for those tests specifically I have usedreuse_entries: false
in thecreate_client
calls which maintains the coverage of that feature without havingreuse_entries
interfere.As a side note, I experienced very high amounts of time hydrating a cyclical structure within our test Contentful database (which contains a design our product team is building) even with
max_include_resolution_depth: 3
. In my tests none of the hydrations ever finished, instead memory usage just kept growing. I believe this may be related to an Asset being attached to one or both of the content objects which link to each other. Perhaps the@depth
value inspected byResourceBuilder
are getting cleared when anAsset
is involved in the hydration process. You may want to expand the test cases around this related to themax_include_resolution_depth
configuration setting.With my change however, that problem is a non-issue as
max_include_resolution_depth
is not needed to prevent runaway hydration times.As a bonus, because the library is not hydrating more copies of objects than necessary, the entire test suite is running about 7.5x faster (30 seconds without the feature enabled to 4 seconds with it enabled).