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-30951: Searching for Content with empty field will result on Error when using Solr #149

Closed
wants to merge 1 commit into from

Conversation

mateuszbieniek
Copy link
Contributor

JIRA issue: https://jira.ez.no/browse/EZP-30951

This PR adds handling for Field Criterion where value NULL is passed for EQ.

Copy link
Member

@alongosz alongosz left a comment

Choose a reason for hiding this comment

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

This needs integration tests coverage.

@andrerom
Copy link
Contributor

andrerom commented Sep 21, 2019

And does this even work on LegacySearch? Afaik IsEmpty Criterion is the missing feature for this use case.

@mateuszbieniek
Copy link
Contributor Author

@andrerom yeah, it works correctly in LegacySearch.

@mateuszbieniek
Copy link
Contributor Author

Side note: failing tests are not related to this PR.

@mateuszbieniek
Copy link
Contributor Author

@andrerom @alongosz I have a problem here which came out after I have written missing Integration Test - maybe you have any idea how to handle it:
In eZ Platform fields that have null value are not stored in Solr (so, null is basically an absence of field for document).

This part: https://github.com/ezsystems/ezplatform-solr-search-engine/pull/149/files#diff-01ed98326f3cd2df926d20e7b8386feeR69 doest exactly this: returns all documents that do not have field X in them, which will return not only Content with field X set to null but as well all other Contents that does not have X field in ContentType.

Therefore, currently, I don't see a way to fix it - or even if it is possible.

The best way would be to handle null with some default value, which would be stored in Solr and then there it is possible to check against it - but that would be big BC break - maybe it is a valid candidate for v3?

The second approach would be to somewhat retrieve a list of ContentTypes, check which ones have this field defined and then add an additional condition for the query (ContentType) so behaviour would be the same as for Legacy, but I have no idea how to achieve it without a significant performance hit.

A third possible approach is to leave it as it is, but with a note that it should only be used with Solar alongside ContentTypeIdentifier.

Forth one is to create a custom Criterion, that would merge Field and ContentTypeIdentifier Critetions into one.

WDYT?

@mateuszbieniek
Copy link
Contributor Author

ping @andrerom @alongosz

@andrerom
Copy link
Contributor

andrerom commented Oct 8, 2019

I'd suggest we setup a meeting for this, I'm not entirely following here.

@mateuszbieniek
Copy link
Contributor Author

Close in favor #157

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

Successfully merging this pull request may close these issues.

3 participants