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

FieldIndex value of None should be unindexed, not just ignored #100

Closed
ewohnlich opened this issue May 5, 2020 · 6 comments
Closed

FieldIndex value of None should be unindexed, not just ignored #100

ewohnlich opened this issue May 5, 2020 · 6 comments

Comments

@ewohnlich
Copy link
Contributor

Reported here first plone/Products.CMFPlone#3064

There is a note here from @hannosch that a value of None should not be indexed because of limitations in BTrees 4.0+. That makes sense, but if the value has changed from something to None I think it would make sense to also remove the old value if one exists.

My original case was on a Plone site:

  1. Plone object "pub1" with attribute "pmid" set to "1"
  2. index with FieldIndex for "pmid"
  3. Assign value of None to pub1.pmid, reindex

The result is pub1 is still found for a query on pmid="1"

@jamadden
Copy link
Member

jamadden commented May 5, 2020

That comment is no longer valid with BTrees >= 4.4.0 (2017-01-11). Those versions again allow None.

ewohnlich pushed a commit that referenced this issue May 5, 2020
@ewohnlich
Copy link
Contributor Author

I was about to submit a PR but given your comment, perhaps this condition should just be removed entirely?

@d-maurer
Copy link
Contributor

d-maurer commented May 5, 2020 via email

@ewohnlich
Copy link
Contributor Author

I will fix up that branch as submit as a PR along those lines then.

Also, I would like to hear more about @andbag's work on that if someone could direct me to it. I have a use case where users need to be able to search for content that have a None value (or some other python False value like an empty string). To do that I am currently using Plone indexers that assign some magic value in those cases. It would be much better if the Index itself could handle this natively. But this need to be connected with a fix here by any means.

@d-maurer
Copy link
Contributor

d-maurer commented May 6, 2020 via email

ewohnlich added a commit that referenced this issue May 8, 2020
* proposed fix for #100, case where index value is changed to None after previously being indexed

* remove pycharm junk

* remove old comment as no longer relevant in BTrees. But we still want
to not index None values

* use assertRaises instead of try/except for expected KeyError

Co-authored-by: wohnlice <wohnlice@imsweb.com>
@wesleybl
Copy link
Member

Fixed in: #103

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

No branches or pull requests

4 participants