Add contributor + per-role operators to advanced search#2822
Add contributor + per-role operators to advanced search#2822bendichter wants to merge 20 commits into
Conversation
There was a problem hiding this comment.
my initial HI review (edit: actually it is "requesting changes")
Stacked on top of #2821 (which is itself stacked on #2814). Until those merge, ...
need to refresh you AI memory map of things -- #2814 is merged. For a number of "situations", I myself started to read/tune more of the AI output I give to folks for consumption and also "fold" some parts of the descriptions into <details>, like e.g. see freshish datalad/datalad#7852 ... on that end, started
| 'visualization': 'Visualization', | ||
| 'funder': 'Funder', | ||
| 'sponsor': 'Sponsor', | ||
| 'study_participant': 'StudyParticipant', |
There was a problem hiding this comment.
I itch with duplication allergy so badly: are those all duplicates of what we have in the dandi-schema mapped from snail_case to PythonCase?
❯ git grep -p -e ContactPerson -e Conceptualization -- dandischema/models.py
dandischema/models.py=class RoleType(Enum):
dandischema/models.py: #: Conceptualization
dandischema/models.py: Conceptualization = "dcite:Conceptualization"
dandischema/models.py: ContactPerson = "dcite:ContactPerson"
dandischema/models.py=class Contributor(DandiBaseModel):
dandischema/models.py: if role_names is not None and RoleType.ContactPerson in role_names:
dandischema/models.py=class Dandiset(CommonModel):
dandischema/models.py: if val.roleName and RoleType.ContactPerson in val.roleName:
dandischema/models.py: raise ValueError("At least one contributor must have role ContactPerson")
I feel like should be programmatically mapped from that dandischema.models.RoleType!
There was a problem hiding this comment.
Two duplications conflated here, taking them separately:
Within our code (parser allowlist + filter dispatch tables listing the same operator names): real, will fix. Plan is to move the dispatch tables into a small pure-Python module both parser.py and filters.py import from, then construct OPERATOR_KEYS as the union of those tables. Adding a new operator becomes one entry in the dispatch table, automatically known to the parser.
Between our code and dandischema.RoleType: I considered deriving _CONTRIBUTOR_ROLE_OPS programmatically from RoleType with an exclusion list, but I think the explicit allowlist is the right call here:
- The dispatch dict is a translation (snake_case operator name → PascalCase role name). Even with auto-derivation, we'd still have an exclusion list and a snake-case transformer — it's the same lines moved into a for loop, not fewer.
- API stability: a future PR that adds a new RoleType to dandi-schema shouldn't silently expand our public search syntax. With derivation, the new operator just appears with no review of the operator name, UX, or whether anyone wants it.
- Renames are a footgun: if dandi-schema ever renames
DataCurator→DataCuratorPerson, the auto-derivation would silently changedata_curator:todata_curator_person:and break every saved/shared user query. The explicit allowlist catches that as a diff to review here.
What I'll add as a guard: a unit test that every value in _CONTRIBUTOR_ROLE_OPS is an actual RoleType.name — one assertion, catches typos against the schema without auto-tracking it.
| # Note: `affiliation` is intentionally NOT here. Despite `dcite:Affiliation` | ||
| # existing as a RoleType, in real DANDI data affiliations live in a | ||
| # separate nested field — `Person.affiliation[]` — not as a contributor's | ||
| # role. The `affiliation:` operator queries that nested path; see |
There was a problem hiding this comment.
can exclude explicitly selected few if needed
There was a problem hiding this comment.
👍 see consolidated reply on the comment above — the exclusion list will live next to the dispatch dict (and a unit test will pin every entry against RoleType.name so drift is caught explicitly).
| { example: 'data_curator:Doe', description: 'Listed as a Data Curator' }, | ||
| { example: 'funder:NIH', description: 'Listed as a Funder' }, | ||
| { example: 'contact_person:Doe', description: 'Listed as the Contact Person' }, | ||
| { example: 'maintainer:Doe', description: 'Listed as a Maintainer (and many more — see API docs)' }, |
There was a problem hiding this comment.
"many more" is unclear here, of what more -- operators? or what it matches, or ...?
I feel that here we are getting into the land of "we need a UI" to support this variability. Here we do not even list all possible values I think from that earlier "duplicated" list, so how users would know? or is that the final list supported?
So I feel like we do need to figure out API to query for "available operators" and then "available values for the operator given current search query". Quick and dirty could be -- we could do smth like
- having "?" as an "operator" to trigger error listing all known operators, and we just present to user (with some minimal formatting tune up). ideally should return structured record of e.g.
{name:str : { description:str, available values: list[str] }}or alike - having "operator:?" returning similar record but now just for that operator.
In both cases -- would be with constraint given the rest of the query, e.g. "neuropixels species:?" would give error listing error like available 'species': "mus musculus", .... Then at least it becomes usable: referral to API is really not for user facing.
Then on top of that (in different PR) can bolt on nice frontend UI with dropbox for selection of values or operators to use for (which would inject "?" into query and run it, to render then result UI)
There was a problem hiding this comment.
Agreed the popover isn't the right surface for this many operators. Two-step plan:
This PR: replace the unhelpful "many more — see API docs" hand-wave with a real link to a docs page. The docs themselves will land in dandi/dandi-docs (separate repo); I'll add the page there and link from the popover here once it's published.
Future PR (your "?" idea): a discovery endpoint along the lines you sketched — ?search=? lists known operators; ?search=species:? lists candidate values, constrained by the rest of the query. That's a real improvement, but it's enough scope to deserve its own PR rather than getting bundled here.
On the user-doesn't-need-all-of-them point: agreed. 90%+ of contributor queries will be contributor: or author:. The 25 role-specific operators exist for the case where someone DOES want to find e.g. "dandisets with NIH as a Funder," and the docs page is the right place to enumerate them rather than dumping them in the popover.
| 'funder', | ||
| 'sponsor', | ||
| 'study_participant', | ||
| 'affiliation', |
There was a problem hiding this comment.
oy -- I think I saw those somewhere... duplication again? could be avoided?
There was a problem hiding this comment.
Same root cause as the role-list duplication thread above. Plan: move the dispatch tables into a small pure-Python module both parser.py and filters.py import from, and construct OPERATOR_KEYS as the union of those tables — one source of truth in our codebase. Adding a new operator becomes one entry; the parser automatically knows about it.
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
446eb7c to
845bb68
Compare
bendichter
left a comment
There was a problem hiding this comment.
Refreshed the description: dropped the stale "#2814 stacked" framing (since it merged) and folded the implementation-notes / files / test-plan sections into <details> per your suggestion. Inline replies on each thread are posted.
Two structural improvements + one product trim, in response to the review on dandi#2822: 1. New `dandiapi/api/services/search/operators.py` (pure Python, no Django) holds every operator-vocabulary constant: DATE_OPS, ASSET_OPS, OWNER_OPS, AFFILIATION_OPS, CONTRIBUTOR_ROLE_OPS, FILE_TYPE_ALIASES, ASSET_NAME_PATH_OPS, AFFILIATION_JSONPATH. OPERATOR_KEYS is now the union of those tables — single source of truth, no more duplication between parser.py (allowlist) and filters.py (dispatch). Adding a new operator is one entry; the parser automatically knows about it. 2. Trim the role-restricting shortcuts from 25 to 9. After review discussion: most RoleType values aren't operators users actually reach for (`conceptualization:`, `methodology:`, `validation:`, `visualization:`, etc.). Kept the ones that map to common search intents: contributor (catch-all), author, contact_person, data_collector, data_curator, data_manager, maintainer, project_lead, funder, sponsor The catch-all `contributor:` still matches anyone in any role; only the role-restricting shortcuts are pruned. `project_lead:` is intentionally shorter than the schema name `ProjectLeader`. 3. Shrank the verbose docstrings on private filter helpers (the rationale stays in commit messages, not as documentation rot on internal API). 4. Added test_contributor_role_ops_match_actual_dandischema_roletype as a drift guard: every non-catch-all CONTRIBUTOR_ROLE_OPS value must be a real RoleType.name. Renames or removals on the schema side trip the test, forcing an explicit decision instead of silently changing public search syntax. OpenAPI help text and the search popover updated to reflect the trimmed list (`project_lead`, `data_collector`, `data_manager`, `sponsor` now shown; the misleading "many more" tail removed).
Per dandi#2822 review discussion: the old semantics required all asset operators to be satisfied by a SINGLE asset, which meant `species:mouse species:rat` only matched dandisets with a multi-species recording (rare). The natural user reading is "the dandiset has mouse data AND has rat data" — those can be on different assets, and that's the common case for comparative-species dandisets. Implementation: each asset operator now builds an independent AssetSearch subquery and the dandiset queryset is filtered with `id__in=...` per operator. Django generates one subquery per operator and AND's them at the dandiset level. Cross-key likewise: `species:mouse approach:electrophysiological` now matches any dandiset that has SOME mouse asset AND SOME ephys asset, not just dandisets with a mouse-ephys asset. Tests updated: - `test_advanced_search_repeated_same_key_operator_combines_with_and` is now `..._combines_at_dandiset_level`, with a new fixture that has two separate assets (one mouse, one rat) to actually exercise the cross-asset case the old semantic excluded. - `test_advanced_search_repeated_asset_operators_intersect` is now `test_advanced_search_asset_operators_combine_at_dandiset_level`, with a similar two-assets-split fixture that demonstrates the new inclusive behavior. Contributor / affiliation semantics unchanged — those still AND on the same Version's metadata (since contributors live per-version, not per-asset). Within that single version, predicates can match different contributor[] entries.
|
here is the doc page on this that would be added into dandi-docs pasted (?) content from above: click to expandAdvanced SearchThe dandiset list's search box accepts a Gmail/GitHub-style syntax that lets you mix Quick examplesOperators combine with AND. Quoted phrases ( How operators combine
Operator referenceDatesAll take an ISO date in the form
Asset contentSubstring matches (case-insensitive) against the dandiset's asset metadata.
Owner
If a name matches multiple users (e.g. two Smiths), dandisets owned by any ContributorsThe contributor operators search the dandiset's
The role-restricting operators map to the DANDI schema's Affiliation
RecipesFind recent NWB dandisets from a particular lab. Find dandisets where I'm the contact person. Find dandisets funded by NIH with mouse data. Find dandisets that cite a particular ORCID as an author. Find your own dandisets in the listing. (Or use the My Dandisets tab if you're signed in — it's the same set.) Quoting rules
Error messagesInvalid syntax doesn't fail silently. Common cases:
Typo suggestions are produced by Using from the APIThe same syntax works against the REST API — the search string lives in the curl 'https://api.dandiarchive.org/api/dandisets/?search=species:mouse+author:Doe'import requests
r = requests.get(
'https://api.dandiarchive.org/api/dandisets/',
params={'search': 'species:mouse author:Doe', 'draft': 'true', 'empty': 'true'},
)
r.json()The OpenAPI description on Limitations and notes
|
|
Thank you @bendichter . Great work -- I think we are converging. Could you please refine PR description to correspond to current changes, since I think edit: also rebase/merge master to get advantage of #2820 since now renders skinny |
yarikoptic
left a comment
There was a problem hiding this comment.
overall looks great to my eye... let's see if more eyes could have a peek
| { example: 'contact_person:Doe', description: 'Listed as the Contact Person' }, | ||
| { example: 'maintainer:Doe', description: 'Listed as a Maintainer' }, | ||
| { example: 'project_leader:Doe', description: 'Listed as the Project Leader (also: data_collector, data_manager, sponsor)' }, | ||
| { example: 'affiliation:Stanford', description: 'Has a contributor affiliated with the named organization (or ROR ID)' }, |
There was a problem hiding this comment.
I feel we would really need a URL to docs there now ... best even not to delay but preempt location?
|
The PR description is outdated at this point. A update can help conveying the intent of the PR more clearly. (For example, the trimming of the role-specific operators in 5240487 is not reflected in the description.) |
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
…okup 29 new operators total: catch-all `contributor:` plus one per dandi-schema RoleType (`author`, `data_curator`, `funder`, `contact_person`, etc.). Independent-operator semantics — `author:Doe funder:NIH` returns dandisets where SOME contributor has Doe-as-Author AND SOME contributor (possibly different) has NIH-as-Funder. Each role-specific operator constrains a single contributor[] element to have BOTH the name match AND the role. Implementation: - A single `_CONTRIBUTOR_ROLE_OPS` dict drives both the parser allowlist and the filter dispatch; adding a future role is one new entry. - `_contributor_jsonpath()` builds a Postgres jsonb_path_exists predicate that ORs across `name`, `email`, AND `identifier` (so ORCID for Persons and ROR URL for Organizations both work, including bare-ID substring forms like `01cwqze88` matching the full ROR URL). - All contributor operators in a single query AND on the same Version's metadata so a draft + published version with disjoint contributor lists never combine into a spurious match. Why 29 separate operators rather than a `contributor: + role:` pair: independent operators compose cleanly (cross-key AND falls out naturally; no ambiguity about which role applies to which contributor when there are multiple). Same precedent as Gmail's `from:`/`to:`/`cc:`. The 28 role names come straight from `dandischema.RoleType`. Test: one consolidated test covers catch-all + role-specific lookup, case-insensitivity, identifier (ORCID + ROR + bare-ID substring), role-substring matching `dcite:`-prefixed stored values, role + ORCID composition (positive and negative), and independent cross-role AND. Plus a separate test for the typo → 400-with-suggestion path. Anonymous test fixtures use generic Doe placeholders, no real names. OpenAPI help text and the search popover updated.
The previous commit treated `affiliation:` as a role-name match (looking for `dcite:Affiliation` in `contributor[].roleName`), but real DANDI data never uses that role; affiliations live in a separate nested field `contributor[].affiliation[]`. The operator silently returned 0 hits despite plenty of (e.g.) Stanford-affiliated contributors. Fix: route `affiliation:` through a dedicated jsonpath that scans `$.contributor[*].affiliation[*]` and matches against the affiliation's `name` OR `identifier` (case-insensitive substring). So: affiliation:Stanford → matches Stanford University affiliation:"University College London" → quoted multi-word affiliation:00f54p054 → matches via ROR ID substring Composes with role/contributor operators on the same Version, same as the other contributor-style operators (independent-operator AND). Also refactored `_apply_contributor_filters` to accept a list of (where, params) pairs rather than (value, role) — cleaner since both the role-based and affiliation operators now share the same dispatch.
Per review: `other:` would be a thin surface for "uncategorized contributors" — not a useful filter — and `ethics_approval:` isn't a contributor-style role users would search by. Removing them tightens the operator vocabulary to the 25 substantive RoleType values + the contributor catch-all + affiliation.
Two structural improvements + one product trim, in response to the review on dandi#2822: 1. New `dandiapi/api/services/search/operators.py` (pure Python, no Django) holds every operator-vocabulary constant: DATE_OPS, ASSET_OPS, OWNER_OPS, AFFILIATION_OPS, CONTRIBUTOR_ROLE_OPS, FILE_TYPE_ALIASES, ASSET_NAME_PATH_OPS, AFFILIATION_JSONPATH. OPERATOR_KEYS is now the union of those tables — single source of truth, no more duplication between parser.py (allowlist) and filters.py (dispatch). Adding a new operator is one entry; the parser automatically knows about it. 2. Trim the role-restricting shortcuts from 25 to 9. After review discussion: most RoleType values aren't operators users actually reach for (`conceptualization:`, `methodology:`, `validation:`, `visualization:`, etc.). Kept the ones that map to common search intents: contributor (catch-all), author, contact_person, data_collector, data_curator, data_manager, maintainer, project_lead, funder, sponsor The catch-all `contributor:` still matches anyone in any role; only the role-restricting shortcuts are pruned. `project_lead:` is intentionally shorter than the schema name `ProjectLeader`. 3. Shrank the verbose docstrings on private filter helpers (the rationale stays in commit messages, not as documentation rot on internal API). 4. Added test_contributor_role_ops_match_actual_dandischema_roletype as a drift guard: every non-catch-all CONTRIBUTOR_ROLE_OPS value must be a real RoleType.name. Renames or removals on the schema side trip the test, forcing an explicit decision instead of silently changing public search syntax. OpenAPI help text and the search popover updated to reflect the trimmed list (`project_lead`, `data_collector`, `data_manager`, `sponsor` now shown; the misleading "many more" tail removed).
- Variable renames: ds_baker_curator → ds_doe_curator, ds_baker_author_only → ds_doe_author_only (the test data was already Doe; only the variable names still carried the old name). - One stale query string `AUTHOR:baker` updated to `AUTHOR:doe`. - One fixture email field `'jane.doe.com'` (broken: no @) restored to `'jane.doe@example.com'` — leftover from the earlier perl rename that stripped @example out.
Per dandi#2822 review discussion: the old semantics required all asset operators to be satisfied by a SINGLE asset, which meant `species:mouse species:rat` only matched dandisets with a multi-species recording (rare). The natural user reading is "the dandiset has mouse data AND has rat data" — those can be on different assets, and that's the common case for comparative-species dandisets. Implementation: each asset operator now builds an independent AssetSearch subquery and the dandiset queryset is filtered with `id__in=...` per operator. Django generates one subquery per operator and AND's them at the dandiset level. Cross-key likewise: `species:mouse approach:electrophysiological` now matches any dandiset that has SOME mouse asset AND SOME ephys asset, not just dandisets with a mouse-ephys asset. Tests updated: - `test_advanced_search_repeated_same_key_operator_combines_with_and` is now `..._combines_at_dandiset_level`, with a new fixture that has two separate assets (one mouse, one rat) to actually exercise the cross-asset case the old semantic excluded. - `test_advanced_search_repeated_asset_operators_intersect` is now `test_advanced_search_asset_operators_combine_at_dandiset_level`, with a similar two-assets-split fixture that demonstrates the new inclusive behavior. Contributor / affiliation semantics unchanged — those still AND on the same Version's metadata (since contributors live per-version, not per-asset). Within that single version, predicates can match different contributor[] entries.
Postgres jsonpath quirk: `like_regex` requires its pattern to be a STRING LITERAL inside the jsonpath text — not a `$variable`. The contributor + affiliation builders I wrote tried to use the `vars` argument of `jsonb_path_exists` for the regex pattern, which Postgres rejects with `syntax error at or near "$val" of jsonpath input`. (The asset operators avoid this by concatenating `to_jsonb(?::text)::text` into the jsonpath at SQL execution time — the regex pattern ends up as a properly-quoted JSON string literal in the path. The user value is still bound as a parameter, never inlined into the SQL.) Refactor: applied the same SQL-time concatenation trick to the contributor + affiliation builders. Three new helpers — `_contributor_where`, `_affiliation_where`, and a shared `_LIKE_REGEX_PATTERN` constant — replace the old `_contributor_role_jsonpath` + `_build_jsonpath_where` pair that relied on the broken `vars` mechanism. Removed the unused `AFFILIATION_JSONPATH` constant from operators.py and dropped the `json` import from filters.py since we no longer marshal `vars` objects. Net behavior unchanged; the failing CI tests should pass now.
CI surfaced an assertion that AUTHOR:doe should match the same set as author:doe. The old _TOKEN_RE / _BARE_OP_RE only accepted lowercase operator keys, so uppercase tokens fell through to free text and returned 0 results. Accept either case in the regex and lowercase the captured key before validation/dispatch. Matches user expectations (GitHub's search operators are case-insensitive on the key side too).
Co-authored-by: Isaac To <candleindark@users.noreply.github.com>
Per @candleindark's review: a contributor can be an Organization as well as a Person, and the affiliation jsonpath (which traverses `contributor[*].affiliation[*]`) should walk past Organizations (which have no `affiliation` field of their own) without exploding. Added Organization contributors to both `ds_stanford` and `ds_ucl`: NIH as a Funder on ds_stanford and Wellcome Trust as a Funder on ds_ucl. The new assertions confirm: - `affiliation:Stanford` (and the other affiliation queries) keep working with mixed Person/Organization contributors. - The Organization's own `identifier` is NOT matched by `affiliation:` (it's not an affiliation; the test pins this). - Cross-key with `funder:NIH affiliation:Stanford` works — different contributor elements on the same Version. Also: used `National Institutes of Health (NIH)` for the org name so the `funder:NIH` substring test actually matches (the abbreviation isn't part of the spelled-out form alone). Realistic — DANDI contributors often use this parenthetical form.
1889be6 to
f9a8155
Compare
|
@yarikoptic @candleindark — refreshed the PR description: dropped the stale "25 role-specific operators" framing (now 9), called out the case-insensitive operator keys, the explicit-allowlist rationale, and the RoleType drift-guard test. Also rebased on master to pick up #2820 (skinny popover render) — both branches force-pushed. |
Per @yarikoptic's review (PR dandi#2822). The deferred imports inside test_contributor_role_ops_match_actual_dandischema_roletype were a holdover from when this file deliberately avoided dandischema imports; that constraint no longer applies, and module-level imports are the project convention. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
| Operators thus AND on the same Version (a draft and a published version | ||
| with disjoint contributor lists never combine into a spurious match). |
There was a problem hiding this comment.
It may be a good idea to add a test assure that there is no spurious match. Such a test can safeguard against a future that break this behavior.
There was a problem hiding this comment.
Good idea — added in 0cbdca7. The new test_advanced_search_contributor_operators_and_on_same_version seeds a dandiset whose draft has Author=Doe and whose published version has Funder=NIH (no contributor overlap), then asserts author:Doe funder:NIH rejects it (while a positive-control dandiset with both contributors on the same version still matches). A future change that ANDs per-operator subqueries against unrelated Version rows would let the spurious match through and trip the test.
All concerns I raised have been addressed, deferring to other reviewers for full approval.
Per @candleindark's review (PR dandi#2822). Pins down the "AND on the same Version" semantics with a regression test: a dandiset whose draft has `Author=Doe` and whose published version has `Funder=NIH` (with no overlap) must NOT match `author:Doe funder:NIH`. A future change that chains the predicates against unrelated Version rows would let this spurious match through, and would now trip the test. A positive control (a single version that holds both contributors) confirms the operator composition itself still works. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…tributor # Conflicts: # dandiapi/api/services/search/filters.py # dandiapi/api/services/search/parser.py
Stacked on top of #2821. Adds operators for filtering dandisets by their version-metadata contributors:
contributor:— catch-all (any role); matches by name/email/identifierauthor,contact_person,data_collector,data_curator,data_manager,maintainer,project_leader,funder,sponsor. Each matches a contributor by name/email/identifier AND requires that contributor to hold the correspondingdandischema.RoleTyperole.affiliation:— queries the nestedPerson.affiliation[]field by organization name or ROR identifier. Structurally distinct from the role operators (see below).Behavior
contributor:"Doe, Jane"author:Doedata_curator:Doefunder:NIHauthor:Doe funder:NIHcontributor:0000-0002-2990-9889contributor:01cwqze88https://ror.org/01cwqze88via substringdata_curator:0000-0002-2990-9889affiliation:Stanfordaffiliation:00f54p054https://ror.org/00f54p054URL)author:Doe affiliation:Stanforddata_curatr:DoeDid you mean "data_curator"?Each role/contributor operator's lookup ORs across
name,email, ANDidentifier(case-insensitive substring), so ORCIDs for Persons and ROR URLs for Organizations both work. Substring matching means bare-ID forms like01cwqze88work without typing the full URL prefix. Operator keys themselves are also case-insensitive (AUTHOR:Doeis the same asauthor:Doe).The role list is deliberately concise — most RoleType values aren't operators users reach for. The catch-all
contributor:still finds anyone in any role; the role-restricting shortcuts cover the common search intents (authors, curators, funders, etc.).Affiliation is special
Affiliationexists indandischema.RoleType, but in practice no real DANDI contributor usesdcite:Affiliationas aroleName. Affiliations live in a separate nested field —Person.affiliation[]— populated for ~all real Person contributors (dandiset 000409 alone has 40+ contributors each affiliated with their respective universities).So
affiliation:is not a per-role operator. It uses a dedicated jsonpath ($.contributor[*].affiliation[*]) that ORs across the affiliation'snameandidentifier. Implementation-wise it shares the same batch-AND-on-same-Version dispatch as the role operators, so it composes with them as expected.Why per-role operators (Option D) over
contributor: + role:Considered combining a generic
contributor:and a separaterole:qualifier with same-element semantics, but per-role operators won on:author:Doe funder:NIHcleanly means two independent constraints (no ambiguity about whichrole:applies to whichcontributor:when both repeat).from:/to:/cc:and falls out of the same AND-across-keys semantics our other operators already have.Implementation cost is the same either way (one dict + one jsonpath builder).
Implementation notes
CONTRIBUTOR_ROLE_OPSdict indandiapi/api/services/search/operators.pydrives the parser allowlist + the role-based dispatch. Adding a future role is one new entry. Kept explicit (rather than auto-derived fromdandischema.RoleType) so a schema-level rename or addition can't silently change our public search syntax — see discussion thread. A unit test (test_contributor_role_ops_match_actual_dandischema_roletype) pins each value against an actualRoleType.nameso drift is caught explicitly.to_jsonb(%s::text)::text) to inline the bound regex into the jsonpath text, because Postgres' jsonpathlike_regexrequires its pattern as a string literal (not a$variable). Same shape as the asset operators._apply_contributor_filters()accumulates(where, params)pairs and chains them asVersion.objects.extra(...)calls, so role-based and affiliation predicates share the same batch logic.Version.metadataso a draft + published version with disjoint contributor lists never combine into a spurious match. Cross-operator AND on the same dandiset, but each operator may match a different contributor element within that version's array.Files
dandiapi/api/services/search/operators.py— new pure-Python module with all operator vocabulary + dispatch tables;OPERATOR_KEYSis now derived as the union (no more duplication between parser and filter)dandiapi/api/services/search/parser.py— importsOPERATOR_KEYSfrom operators.py; key matching is now case-insensitivedandiapi/api/services/search/filters.py—_contributor_where(),_affiliation_where(),_apply_contributor_filters()dandiapi/api/views/serializers.py— OpenAPI help text appendedweb/src/components/DandisetSearchField.vue— popover entries for the catch-all, 6 common roles, andaffiliation:dandiapi/api/tests/test_dandiset.py— one consolidated test for catch-all/role/case-insensitivity/identifier/role-substring/composition (per @yarikoptic's "denser tests" preference); separate tests for affiliation (org name + ROR id + Person/Organization mix + composition) and the did-you-mean-on-typo path. Anonymous Doe placeholders.dandiapi/api/tests/test_search_parser.py— drift-guard test againstdandischema.RoleType.Test plan
tox -e lint— cleantox -e test -- dandiapi/api/tests/test_dandiset.py dandiapi/api/tests/test_search_parser.py -k "search_parser or advanced_search or contributor or affiliation"— 53 passcurl 'http://localhost:8000/api/dandisets/?search=author:Doe',affiliation:Stanford,data_curator:0000-0002-2990-9889, etc.Once #2821 merges, this PR's base will collapse to master automatically.