Skip to content

Fix uid: search to match by prefix instead of substring - #357

Open
harriiinnii wants to merge 2 commits into
lucc:mainfrom
harriiinnii:fix/uid-search-prefix-matching
Open

Fix uid: search to match by prefix instead of substring#357
harriiinnii wants to merge 2 commits into
lucc:mainfrom
harriiinnii:fix/uid-search-prefix-matching

Conversation

@harriiinnii

Copy link
Copy Markdown

Summary

khard list displays a short unique UID prefix for each contact (e.g. a for a contact whose UID starts with a and is the only one with that prefix). However, uid:a matched by substring, so it would match both aaabbbb and bbbaaaa instead of only aaabbbb. This made the displayed prefix unusable as a selector.

Problem

parse("uid:xxx") returned a plain FieldQuery("uid", "xxx"), which inherits TermQuery.match and does term in value.lower() — a substring check. The displayed prefix is a prefix, not a substring, so the two behaviours are inconsistent.

Solution

Add a UidQuery subclass of FieldQuery that overrides _match_union to use str.startswith for string values. Update parse() to return a UidQuery when the field is "uid".

The change is intentionally minimal: only the UID field is affected, and the rest of the query machinery (AndQuery, OrQuery, equality, hashing, __str__) works as before.

Testing

  • Added TestUidQuery in test/test_query.py covering: prefix match, substring-only non-match, exact match, case insensitivity, empty term with and without uid set, and disambiguation of two contacts sharing a common prefix.
  • Added test_uid_field_creates_uid_query_instance to TestParser verifying that parse("uid:xxx") returns a UidQuery.
  • All existing tests are preserved unchanged.

Fixes #327

khard list displays a short unique UID prefix for each contact, but
uid:xxx searched by substring so uid:a would match both "aaabbbb" and
"bbbaaaa". Introduce UidQuery which uses startswith so the displayed
prefix can be used directly to select a contact unambiguously.

Fixes lucc#327
UidQuery is a subclass of FieldQuery so isinstance always returns True;
use type() equality to assert that parse() returns a UidQuery specifically.

@lucc lucc left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Looks good, I just have some nitpicks.

Comment thread khard/query.py
class AndQuery(Query):

"""A query to combine multiple queries with "and"."""
"""A query to combine multiple queries with \"and\"."""

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

The backslash is not needed inside triple quotes. Please remove it.

Comment thread khard/query.py
class OrQuery(Query):

"""A query to combine multiple queries with "or"."""
"""A query to combine multiple queries with \"or\"."""

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

see above

Comment thread khard/query.py
def __init__(self, value: str) -> None:
super().__init__("uid", value)

def _match_union(self, value: "str | datetime | list | dict[str, Any]"

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Please remove the quotes around the type. It works without them as none of these types has to be lazy loaded. See the other _match_union implementations.

Comment thread test/test_query.py
actual = parse("uid:abc123")
expected = UidQuery("abc123")
self.assertEqual(actual, expected)
self.assertEqual(type(actual), UidQuery)

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

As you test for equality I think you do not need to test the type again afterwards.

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

Successfully merging this pull request may close these issues.

Restrict UID search query to prefixes, matching displayed ones

2 participants