Correctly support circular dependencies, bonus: 7.5x faster test suite

[rezonant]

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 the nyancat fixture is currently, as reuse_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 used reuse_entries: false in the create_client calls which maintains the coverage of that feature without having reuse_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 by ResourceBuilder are getting cleared when an Asset is involved in the hydration process. You may want to expand the test cases around this related to the max_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).

[rezonant]

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 includes and the new entries hash, I'm not sure the parameters can be reduced.

[dlitvakb]

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

[dlitvakb]

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 reuse_entries to be false? Also, please add the flag on the README with it's explanation.

If you want a new section can be created as well explaining the difference between reuse_entries values. And it's impact on performance.

Once again, I really appreciate this work, it's of great value!

Cheers

[rezonant]

Sure. I believe it shouldn't cause any issues but I suppose better safe than sorry. Will update PR today

[rezonant]

OK, PR is updated to disable reuse_entries by default and to document the feature in README.md

[dlitvakb]

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

[rezonant]

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 .raw has the original JSON, perhaps implementing as_json to return the raw JSON would be a good addition.

[dlitvakb]

Hey @rezonant,

I'm closing this PR, merge will happen with #165.

Cheers

[dlitvakb]

Regarding the as_json behaviour that is due to an ActiveRecord override, using #raw is a valid solution.

In the case of FieldResources you can use #raw_with_links instead.

[dlitvakb]

Merged! 🎉