Repository navigation
Fix #150: reject list filter values with a clear error instead of silently using CONTAINS - #158
Closed
rudra337-lgtm wants to merge 1 commit into
Closed
rudra337-lgtm wants to merge 1 commit into
rudra337-lgtm wants to merge 1 commit into
Conversation
Filtering on a list of values (e.g. short_name=[Equals("FOO"), Equals("BAR")])
silently ignored the Regex/Equals marker and generated a CONTAINS filter whose
value was the whole list, changing the meaning of the query without warning.
For link-typed properties a list value would instead crash with an AttributeError
from str.startswith being called on the list.
The KG query API only supports a single value per filter, so there is no way to
express "any of these values". Raise a TypeError for list filter values instead
of silently altering the query, and document the single-value restriction in the
Equals docstring.
Fixes HumanBrainProject#150
Member
|
Thanks for working on this! This was unfortunate timing, as a solution was already in the works - #150 has now been fixed by #156, which supports lists of filter values: each value becomes its own filter, and a result must match all of them. So I'm closing this PR. I appreciate you taking the time, and contributions on other open issues are very welcome. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #150.
get_query_filter_property()picked the filter operator from the type of the wholevalue, so a list of
RegexorEqualsobjects fell through toCONTAINS. Thegenerated query then had a
CONTAINSfilter whose value was the entire list, silentlychanging the meaning of the query. For link-typed properties, a list value would instead
crash with an
AttributeErrorbecausestr.startswith()was called on the list.The KG query API only supports a single value per filter (the available operators are
IS_EMPTY,STARTS_WITH,ENDS_WITH,CONTAINS,EQUALS,REGEX, each taking onestring), so there is no way to express "any of these values". As the issue suggests,
this change rejects list filter values with a clear
TypeErrorinstead of silentlyaltering the query.
Changes
fairgraph/queries.py: raise aTypeErrorwhen a filter value is a list, with amessage explaining that the KG applies a single value per filter.
fairgraph/queries.py: correct theEqualsdocstring, which documented the old(wrong) behaviour that a list of
Equalsis filtered withCONTAINS.test/test_queries.py: add regression tests covering lists ofEquals,Regex,plain strings, and instance IDs for a link-typed property.
Testing
pytest test→ 397 passed, 65 skipped (the 3 new parametrized cases and the linkcase fail on
masterwithDID NOT RAISE TypeError, and pass with this fix).flake8 --select=E9,F63,F7,F82(the CI lint selection) → clean.