Add owner: operator to advanced search#2821
Conversation
bendichter
changed the title
yarikoptic
left a comment
yarikoptic
left a comment
There was a problem hiding this comment.
Initial HI review. Overall -- love it.
| alice_ds = DandisetFactory.create(owners=[alice]) | ||
| DraftVersionFactory.create(dandiset=alice_ds) | ||
|
|
||
| assert _search_ids(api_client, 'owner:alice@example.com') == {alice_ds.identifier} |
There was a problem hiding this comment.
could you please ask your agent to remember to make "each test matter more" kinda... I will try to figure out where/how I did it to mine later -- e.g. these two tests could be 1, and likely others could also be folded in where more testing done on the same "setup" thus making overall testing faster and without loss of any coverage. I do understand that there is a tradeoff (individual aspect tests might be easier to debug etc) but still -- my Allergy to duplication is triggering AIlergy too
There was a problem hiding this comment.
oh, I generally prefer more modular tests, but yes I can adjust for DANDI projects
There was a problem hiding this comment.
Done in 8a65f3b — consolidated the 10 owner tests down to 3 denser ones (lookup-paths-and-combinations, me-magic-and-literal-escape, superuser-non-inflation). Same coverage; DB setup runs ~3x instead of ~10x. Saved a memory so my agent prefers the denser style for DANDI PRs going forward.
| # though their username is an email address. | ||
| user = UserFactory.create( | ||
| username='ben.dichter@gmail.com', | ||
| email='ben.dichter@gmail.com', |
There was a problem hiding this comment.
might better to use some alice.m.nobody@example.com from above instead of leaking personal real emails
There was a problem hiding this comment.
Fixed in 8a65f3b — switched the fixture user to a generic Super/User name, no personal email.
| `get_objects_for_user` so we can intersect across multiple matched users | ||
| in a single query, and to bypass the superuser-sees-everything default. | ||
| """ | ||
| if value.lower() == 'me': |
There was a problem hiding this comment.
now I started to "feel" for the unlikely Me Someoneyou being unable to look himself without authorization... I wonder if we could make it more explicit, e.g. owner:%me% or owner:me: or alike -- any other system like github or google having smth like that? could also be owner-me: keyword . This should make it explicit and it is better than implicit
There was a problem hiding this comment.
owner:"me" should search for the name "Me". I can test that. Also, if you wanted to search for an owner named "Me Someoneyou", you could use owner:"Me Someoneyou".
There was a problem hiding this comment.
Done in 8a65f3b. Addressed via a parser-level quoted/unquoted distinction:
owner:me(unquoted) → magic alias for the requesting user (anonymous → 400 with a hint pointing at the quoted form)owner:"me"(quoted) → bypasses the magic and matches the literal user named Meowner:"Me Someoneyou"→ matches the full-name display
Implementation: ParsedSearch.operators is now a list of Operator dataclasses (key, value, quoted) instead of bare 2-tuples; only the owner filter inspects quoted for now, but the same signal could be used by other operators that ever want a magic-alias escape hatch. New integration test test_advanced_search_owner_me_magic_and_literal_escape pins all three cases plus the anonymous-400 path.
GitHub uses @me for the same idea, but that's because @ is GitHub's user sigil — we don't have that convention in DANDI, so the @ would just be an arbitrary literal here. The quoted-form escape feels more in line with the Gmail-style operator syntax we already have (which uses "foo:bar" as the colon escape).
Round-2 review feedback on dandi#2821: - @yarikoptic flagged that owner:me silently shadows a real user named "Me". Fix: distinguish quoted vs unquoted at the parser level. Unquoted owner:me → magic alias for the requesting user. Quoted owner:"me" → literal lookup (matches a user whose first/last name is "Me"). Same pattern lets owner:"Me Someoneyou" reach the literal full-name match while keeping the convenient owner:me shortcut. Implementation: ParsedSearch.operators is now a list of `Operator` dataclasses (key, value, quoted) instead of bare tuples. Filters consume the new shape and the owner filter switches on the quoted flag. - Replaced personal email (ben.dichter@gmail.com) in the full-name test fixture with a generic example user. - Consolidated 10 small owner-tests into 3 denser ones that share setup per @yarikoptic's "make each test matter more" feedback. Coverage is unchanged (every documented lookup path is asserted; cross-key AND with another operator; multi-user union via shared last name; unknown user → 0; superuser non-inflation; owner:me magic; owner:"me" literal-escape; anonymous owner:me → 400). DB setup runs ~3x instead of ~10x. Updated OpenAPI help text and the search popover to mention the owner:me alias and the quoted-escape.
Filters dandisets to those owned by a given user. The value is matched case-insensitively against User.username OR User.email. The special form `owner:me` resolves to the requesting user (consistent with the existing ?user=me query parameter) and returns 400 if the request is anonymous. Implementation reuses the existing `get_owned_dandisets()` permission helper. We pass `with_superuser=False` so `owner:admin` returns only what admin explicitly owns — guardian's default would otherwise inflate to the entire archive for any superuser. Unknown users return zero results (not an error): a search for a nonexistent owner is a valid 0-hit query. Tests cover username/email lookup, case-insensitivity, unknown user, `owner:me` for an authenticated user, anonymous `owner:me` → 400, the superuser non-inflation guarantee, and combination with other operators. OpenAPI help text and the frontend operator popover updated.
Real users encounter the dandiset list with owners shown by display name (e.g. "Super User"), not by username. Searching that string was returning 0 because the lookup only matched username/email. Now matches case-insensitively against username, email, first_name, last_name, OR "first_name last_name" — so owner:"Super User" works the same as owner:ben.dichter@gmail.com. Multiple users may match (e.g. shared last name); we union dandisets owned by any of them via a direct DandisetUserObjectPermission query. Updated OpenAPI help text and the frontend popover example to `owner:"Jane Doe"` so users discover the new shape.
Round-2 review feedback on dandi#2821: - @yarikoptic flagged that owner:me silently shadows a real user named "Me". Fix: distinguish quoted vs unquoted at the parser level. Unquoted owner:me → magic alias for the requesting user. Quoted owner:"me" → literal lookup (matches a user whose first/last name is "Me"). Same pattern lets owner:"Me Someoneyou" reach the literal full-name match while keeping the convenient owner:me shortcut. Implementation: ParsedSearch.operators is now a list of `Operator` dataclasses (key, value, quoted) instead of bare tuples. Filters consume the new shape and the owner filter switches on the quoted flag. - Replaced personal email (ben.dichter@gmail.com) in the full-name test fixture with a generic example user. - Consolidated 10 small owner-tests into 3 denser ones that share setup per @yarikoptic's "make each test matter more" feedback. Coverage is unchanged (every documented lookup path is asserted; cross-key AND with another operator; multi-user union via shared last name; unknown user → 0; superuser non-inflation; owner:me magic; owner:"me" literal-escape; anonymous owner:me → 400). DB setup runs ~3x instead of ~10x. Updated OpenAPI help text and the search popover to mention the owner:me alias and the quoted-escape.
The unquoted owner:me → current-user shortcut required threading a `quoted` flag through the parser and a `request_user` arg through the filter dispatch — non-trivial machinery to support one alias. Per dandi#2822 review discussion, removing it from this PR keeps the owner operator focused on literal lookup-by-value (username / email / first / last / "first last") and avoids the design debate about the right escape mechanism for "I literally want a user named Me." The alias can come back in a focused follow-up PR if/when there's appetite for it. Concrete drops: - owner:me magic + 400-on-anonymous in `_apply_owner_filter` - `Operator.quoted` field on the parser dataclass - `quoted` and `request_user` parameters on `_apply_owner_filter` - `get_owned_dandisets` import (no longer used here) - `test_advanced_search_owner_me_magic_and_literal_escape` test - The two `owner-me-quoted` / `owner-me-unquoted` parser test cases - "owner:me" mentions in OpenAPI help text and the popover entry
bendichter
force-pushed
the
advanced-search-owner
branch
from
30535f9 to
a328da1
Compare
2 participants
Adds an
owner:operator to the advanced-search syntax (which landed in #2814).owner:VALUEreturns dandisets owned by users matchingVALUE. The lookup is case-insensitive againstUser.username,User.email,User.first_name,User.last_name, or"first_name last_name"(so the display name shown in the dandiset list also works). Multiple users may match (common with last-name lookups); we union dandisets owned by any of them. Unknown user → 0 results, not 400.Implementation notes
_apply_owner_filterindandiapi/api/services/search/filters.pydoes the lookup. Direct query againstDandisetUserObjectPermission(rather than guardian'sget_objects_for_user) so we can union across multiple matched users in a single query and bypass the superuser-sees-everything default —owner:adminreturns only what admin explicitly owns, not the entire archive.DandisetQueryParameterSerializer.searchlists the operator alongside the others.owner:"Jane Doe".Test plan
tox -e linttox -e test -- dandiapi/api/tests/test_dandiset.py -k "advanced_search_owner"— three consolidated tests cover username/email/case-insensitivity/full-name/multi-user-union/unknown-user/superuser-non-inflation/cross-key combination.curl 'http://localhost:8000/api/dandisets/?search=owner:%22Jane%20Doe%22'.