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

EZP-27101: Recursion protection for retrieving metadata doesn't work properly #1371

Conversation

jacek-foremski
Copy link

@jacek-foremski jacek-foremski commented Jun 27, 2018

JIRA issue: EZP-27101

When retrieving the search metadata from objects containing "Object relation" and "Object relations" field type, there is a recursion protection in place that ensures that there is no infinite recursion caused by circular references. However, it only checks if the certain object was already processed and this is causing problems when the same object is referenced from two different fields (as described in EZP-27101). To fix that I introduced new recursion protection that differs in two aspects:

  • Besides object id, it also stores the field id which contains this object. Circular reference only happens if we are processing the same object from the same field as before.
  • It works like a stack instead of a one-dimensional array, meaning that only references above in the recursion chain are taken into the account when detecting circular references.

There is another PR which tries to fix this issue (#1288), but it can cause an infinite recursion (as described in #1288 (comment)).

@jacek-foremski jacek-foremski changed the title Fix EZP-27101: Recursion protection for retrieving metadata doesn't work properly EZP-27101: Recursion protection for retrieving metadata doesn't work properly Jun 27, 2018
@pkamps
Copy link
Contributor

pkamps commented Jun 27, 2018

We also had issues with the way the metaData was retrieved. Not only technical because the logic was fetching unrelated data which is resulting in bad search results.
We fixed the issue in this pull request:
mugoweb#55

@jacek-foremski
Copy link
Author

@pkamps Yes, I saw your PR. In general I agree that the search retrieves too much information for most use cases, but I'm not sure if we can change the logic now since that would be a BC break.

@pkamps
Copy link
Contributor

pkamps commented Jun 27, 2018

The recursive lookup is currently technically broken and we agree it's a bad/broken concept. Not sure why you want to keep it. It's unrealistic that somebody relies on a broken "feature".

@jacek-foremski jacek-foremski force-pushed the EZP-27101_fix_recursion_protection_when_retrieving_metadata branch from eebebc6 to 35cbadc Compare June 27, 2018 08:45
@jacek-foremski
Copy link
Author

After the internal discussion, we decided to close this PR. I'll make another one which will remove the recursive logic in the least BC-breaky way as possible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants