From e7a77dbd948882f64fe2f2ce76dd8ee7ac756629 Mon Sep 17 00:00:00 2001 From: Craig McChesney Date: Thu, 10 Sep 2026 15:18:40 -0600 Subject: [PATCH 1/3] feat: allow the key-only attributes search on the five older criterion helpers (#40) The protos document an empty `values` list on every AttributesCriterion as a key-only / existence search: match every record possessing the key, whatever its value. dp-service implements exactly that -- four of the five criteria route to MongoQueryFilterBuilder.attributeFilter(), which returns Filters.exists("attributes.") on an empty list rather than an $in: [] that would match nothing, and MongoQueryFilterBuilderTest covers that path. Five helpers made it unreachable, each raising on empty values: PvMetadataQuery.attributes, ConfigurationQuery.attributes, ConfigurationActivationQuery.attributes, PvQuery.attr, ConfigQuery.attr. They now take `values: list[str] | None = None`, matching the shape #6 already shipped on DataSetQuery/AnnotationQuery, so all seven attribute helpers spell the concept the same way. The empty-key rejection stays, and matters more on the two v2 selectors: the server does not validate the key there, so a blank one would reach Mongo as an existence test on "attributes." and silently match nothing. Why this was worth doing rather than left as a documented workaround: - conventions.md justified the guard as refusing "a criterion that would silently match everything". True of tags([]) and pv_name(), but false here -- a key-only attributes criterion narrows the result set. The rule was generalized onto the one criterion it does not describe. - pv-metadata.md carried a whole section teaching users to drop through to the raw protobuf classes, a snippet needing `# cookbook:no-mypy` because the escape hatch is not statically checkable. That section is now a positive recipe and the checker type-checks all 104 snippets, none skipped for this reason. The guard was not an upstream-driven decision: it arrived in c101964 (2026-07-14) applying one rule across pv_name/aliases/tags/attributes at once, and all five proto comments predate it. Five existing tests asserted the rejection and are inverted rather than deleted, each covering both spellings (`attributes("k")` and `attributes("k", [])`). 721 unit tests pass. Plan: plan/tickets/40/plan.md. Verified against dp-grpc 6dfff3f and dp-service fddf692. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_019he3UCsAnqTDE2VQ73Djwn --- doc/cookbook/conventions.md | 19 +- doc/cookbook/machine-configuration.md | 4 + doc/cookbook/pv-metadata.md | 28 +-- doc/cookbook/query.md | 4 + plan/tickets/40/plan.md | 218 ++++++++++++++++++ .../client/machine_config_client.py | 36 +-- .../client/pv_metadata_client.py | 20 +- src/dp_python_lib/client/query_client.py | 43 ++-- .../test_machine_config_activation_client.py | 12 +- tests/unit/test_machine_config_client.py | 9 +- tests/unit/test_pv_metadata_client.py | 11 +- tests/unit/test_query_client.py | 17 +- 12 files changed, 354 insertions(+), 67 deletions(-) create mode 100644 plan/tickets/40/plan.md diff --git a/doc/cookbook/conventions.md b/doc/cookbook/conventions.md index 9545a7b..e8d608d 100644 --- a/doc/cookbook/conventions.md +++ b/doc/cookbook/conventions.md @@ -175,8 +175,8 @@ Name and alias criteria accept `exact`, `prefix`, and `contains` lists, which ma criteria = [Q.pv_name(prefix=["BPMS:"], contains=["GUNB"])] ``` -**The helpers reject empty input.** Every one of them raises `ValueError` rather than building a -criterion that would silently match everything: +**The helpers reject empty input.** They raise `ValueError` rather than building a criterion that +would silently match everything: ```python # cookbook:partial @@ -187,6 +187,21 @@ Q.pv_name() # ValueError: requires at least one non-empty of exa That is a deliberate guard — an empty criterion is nearly always a bug in the caller's filter construction, and failing loudly beats returning the whole collection. +**The one exception is `attributes()` / `attr()`, where an empty `values` list is meaningful.** It +is the protocol's key-only *existence* search: match every record possessing the attribute key, +whatever its value. That narrows the result set rather than matching everything, so the reasoning +above does not apply and the helpers allow it: + +```python +# cookbook:partial +Q.attributes("S") # every PV that has an S attribute at all +Q.attributes("S", []) # the same thing +Q.attributes("", ["0.49"]) # ValueError: the *key* is still required +``` + +This holds for all seven attribute helpers — `Q`, `C`, `CA`, `DS`, `AQ`, and the `PV.attr()` / +`CFG.attr()` query selectors. + ## Save semantics: full replace Methods named `save_*` are **full-replace upserts**, not partial updates. Omitted fields are diff --git a/doc/cookbook/machine-configuration.md b/doc/cookbook/machine-configuration.md index de3232b..98deb13 100644 --- a/doc/cookbook/machine-configuration.md +++ b/doc/cookbook/machine-configuration.md @@ -377,6 +377,10 @@ for configuration in client.annotation.machine_config.iter_configurations([ `C` offers `name`, `category`, `tags`, `attributes`, and `parent`; `CA` offers `timestamp`, `time_range`, `configuration_name`, `client_activation_id`, `category`, `tags`, and `attributes`. +Both `attributes(key, values)` helpers take the values list as optional: omit it for a **key-only +existence search** matching every record that has the key, whatever its value — say +`C.attributes("owner")` for "every configuration with an owner recorded". + ## Addressing a specific activation `get_configuration_activation()` and `delete_configuration_activation()` take **either** a client diff --git a/doc/cookbook/pv-metadata.md b/doc/cookbook/pv-metadata.md index 1ab995f..a01a65d 100644 --- a/doc/cookbook/pv-metadata.md +++ b/doc/cookbook/pv-metadata.md @@ -350,30 +350,24 @@ This reads every matching record to filter client-side, so keep the server-side as you can. Note also that string equality is exact: `"0.489650"` and `"0.48965"` are different attribute values even though the numbers are equal. -### Key-only (existence) search is not exposed +### Which PVs have this attribute at all? -The protocol supports matching on an attribute *key* regardless of value, by sending an -`AttributesCriterion` with an empty `values` list. The `Q.attributes()` helper does not allow -this — it raises `ValueError` on empty values, since an accidentally-empty list is far more often -a bug than a deliberate existence search. - -If you genuinely need it, build the criterion directly: +Omit the values list — or pass an empty one — for a **key-only existence search**: every PV +possessing the key matches, whatever its value. ```python # cookbook:partial -# cookbook:no-mypy (generated protobuf classes are built at import time; not statically visible) -from dp_python_lib.grpc import annotation_pb2 - -criterion = annotation_pb2.QueryPvMetadataRequest.QueryPvMetadataCriterion() -criterion.attributesCriterion.key = "S" # no values -> match any PV having an S attribute - -for record in client.annotation.pv_metadata.iter_pv_metadata([criterion]): +# every PV that has been surveyed, whatever its position +for record in client.annotation.pv_metadata.iter_pv_metadata([Q.attributes("S")]): print(record.pvName) ``` -Dropping to the generated stubs like this is the general escape hatch whenever a helper is -stricter than the protocol. The message classes are built dynamically at import time, so static -type checkers cannot see them — the code is correct, but your editor may flag it. +This is useful for finding gaps in the catalogue. Combine it with a positive criterion to ask +"which PVs in this area are *missing* a survey?" — the existence search finds the ones that have +it, and the difference is what you need to fill in. + +The *key* is still required: `Q.attributes("", ["0.49"])` raises `ValueError`. The same holds for +`C.attributes()`, `CA.attributes()`, and the `PV.attr()` / `CFG.attr()` query selectors. ### Other details diff --git a/doc/cookbook/query.md b/doc/cookbook/query.md index ec94194..d54d854 100644 --- a/doc/cookbook/query.md +++ b/doc/cookbook/query.md @@ -216,6 +216,10 @@ params = QueryParams( `CFG` offers `configuration_name`, `client_activation_id`, `category`, `tags`, and `attr(key, values)`. +On both `PV.attr()` and `CFG.attr()` the values list is optional: omit it for a **key-only +existence search** matching everything that has the key, whatever its value — `PV.attr("AREA")` +selects every PV with an area recorded. + ### The result covers several disjoint intervals If a configuration was active more than once inside the time range — two shifts in a day, say — diff --git a/plan/tickets/40/plan.md b/plan/tickets/40/plan.md new file mode 100644 index 0000000..698c6c2 --- /dev/null +++ b/plan/tickets/40/plan.md @@ -0,0 +1,218 @@ +# Issue #40 — Let the criterion helpers build the key-only attributes search the protos document + +- **Ticket**: [osprey-dcs/dp-python-lib#40](https://github.com/osprey-dcs/dp-python-lib/issues/40) +- **Companion**: [#41](https://github.com/osprey-dcs/dp-python-lib/issues/41) (optional `criteria` on the six + annotation-service query/iter methods) — same three source files, same three cookbook pages. Sequencing in + [Dependencies](#dependencies-and-sequencing). +- **Surfaced by**: `plan/tickets/6/plan.md` finding 5, whose new `DataSetQuery.attributes()` / + `AnnotationQuery.attributes()` accept the key-only form from the start. +- **Status**: written 2026-09-10 against dp-python-lib `dc477be`, dp-grpc `6dfff3f`, dp-service `fddf692` (the + same upstream commits `plan/tickets/6/plan.md` was verified against). Triage verified every premise in the + ticket body against the protos, the server source, and this repo's own history; three corrections to the + ticket are recorded below and folded into [Implementation tasks](#implementation-tasks). + +## Overview + +Five criterion helpers reject an empty `values` list, making the protocol's key-only ("does this record have +attribute `X` at all?") search unreachable through the library. This relaxes all five to accept an absent or +empty `values`, emitting a `key`-only criterion, while keeping the empty-`key` rejection. + +The change is small — five two-line edits — but it is worth doing for three reasons, in descending order: + +1. **The library is internally inconsistent right now.** Issue #6 shipped `DataSetQuery.attributes()` and + `AnnotationQuery.attributes()` accepting the key-only form; the five older helpers reject it. Seven + helpers spell the same protocol concept two different ways, and the two that differ are the *newest*, so + the inconsistency grows rather than ages out. +2. **The stated rationale for the guard does not hold for this criterion.** `doc/cookbook/conventions.md` + justifies rejecting empty input as refusing "a criterion that would silently match everything." That is + true of `tags([])` and `pv_name()`, but false of a key-only attributes criterion, which matches records + *possessing the key* — a narrowing filter, not a match-all. The guard was generalized to a case it does + not describe. +3. **The workaround is documented, which is evidence of demand, and it is a bad workaround.** + `doc/cookbook/pv-metadata.md` already devotes a section ("Key-only (existence) search is not exposed") to + dropping through to the generated protobuf classes — a snippet that must carry `# cookbook:no-mypy` + because the escape hatch is not statically checkable. The library forces users off its own type-checked + surface to reach a documented protocol feature. + +Non-breaking: every existing call passes a non-empty list and is unaffected. + +## Background / triage findings + +Verified against the sources, not taken from the ticket text. + +- **T1 — The five helpers and their files are exactly as the ticket lists them.** Confirmed at + `pv_metadata_client.py:92`, `machine_config_client.py:90` and `:235`, `query_client.py:143` and `:234`. All + five have the identical two-guard body (`if not key: raise` / `if not values: raise`), and all five + currently assign `criterion.attributesCriterion.values[:] = values` unconditionally. + +- **T2 — The proto comments say what the ticket quotes them as saying.** All five sites declare + `repeated string values` and document the empty-values existence search. The three annotation.proto sites + (`dp-grpc/src/main/proto/annotation.proto:1850`, `:2276`, `:2731`) carry the long form the ticket quotes; + the two query.proto sites (`query.proto:301`, `:384`) carry a compressed one-liner, `key required; empty + values = key-only existence search`. A documentation-density difference only, not a semantic one. + +- **T3 — The server genuinely implements it, and the behavior is pinned by server tests.** This is the + premise the ticket asserts but does not evidence, and it is the one that decides whether the ticket is + worth doing at all. Four of the five criteria route to one shared helper, + `dp-service/src/main/java/com/ospreydcs/dp/service/common/mongo/MongoQueryFilterBuilder.java:70-76`: + + ```java + public static Bson attributeFilter(String key, List values) { + final String mapKey = BsonConstants.BSON_KEY_ATTRIBUTES + "." + key; + if (values == null || values.isEmpty()) { + return Filters.exists(mapKey); + } + return Filters.in(mapKey, values); + } + ``` + + Empty values builds a real `exists` filter — not an `$in: []` that would match nothing. Callers: + `MongoSyncAnnotationClient.java:971` (PV metadata), `:1491` (activations), and + `QueryV2Resolver.java:309`, `:412` (the two v2 selectors). The fifth, Configurations, *inlines* the same + branch at `MongoSyncAnnotationClient.java:1225-1232` — duplicated code, behaviorally identical, no + divergence. Attributes are stored as a nested BSON document keyed by attribute name + (`DpBsonDocumentBase.java:31`, `Map attributes`) and backed by a wildcard index + (`MongoClientBase.java:187`), so the dotted-path `exists` is the correct and indexed form. + `MongoQueryFilterBuilderTest.java:114-139` covers the empty path explicitly, for both `emptyList()` and + `null`. Nothing on the server rejects empty values, on any of the five. + +- **T4 — Server-side validation checks a blank *key* and deliberately does not check values.** The three + annotation-service jobs reject a blank key (`QueryPvMetadataJob.java:68-74`, `QueryConfigurationsJob.java:74`, + `QueryConfigurationActivationsJob.java:97`). In each, the `TAGSCRITERION` arm immediately above *does* + reject empty values — so the omission for attributes is a decision, not an oversight. This is the + strongest available evidence that key-only is intended rather than merely tolerated. + +- **T5 — The two v2 selectors have no attributes validation at all**, so a blank key there falls through to + `Filters.exists("attributes.")` and silently matches nothing. Keeping the client-side non-blank-key + rejection on all five papers over that gap uniformly. This is an argument for the shape of the change + (relax `values`, keep `key`), not against the change. + +- **T6 — The guard was not an upstream-driven decision, and it postdates the semantics it overrides.** It + arrived in `c101964` ("Address Copilot review: criterion validation, dep pins, README RPC names", + 2026-07-14), which applied one rule — "raise on inputs that would build an empty criterion" — across + `pv_name`/`aliases`/`tags`/`attributes` at once. The proto comments documenting key-only search predate + it: `7b6ce5d` (2026-04-24) for PV metadata, `315b4f6` (2026-05-01) for machine configuration, `37ac710` + (2026-07-05) for query v2. So the guard did not reflect a considered reading of the protocol; it + generalized a sound rule onto the one criterion where it does not apply. This matters for the ticket's + framing: it is not overturning a deliberate design decision. + +- **T7 — Existing tests assert the rejection, and the ticket does not mention them.** Five assertions must + be inverted, not merely supplemented: `test_pv_metadata_client.py:151`, `test_machine_config_client.py:75`, + `test_machine_config_activation_client.py:89`, `test_query_client.py:128`, and — easy to miss — the + `lambda: ConfigQuery.attr("k", [])` entry inside the `test_empties_raise` loop at `test_query_client.py:167`. + The empty-key assertions beside them all stay. + +- **T8 — The documentation surface is wider than the ticket's three files.** Beyond + `pv-metadata.md` / `machine-configuration.md` / `query.md`, two more need edits: + - `doc/cookbook/conventions.md:178-189` states the empty-input guard as a uniform rule across all five + helper classes ("**The helpers reject empty input.** Every one of them raises `ValueError`…"). After + this change that sentence is false, and it is the one place a reader looks for the general rule. + - `doc/cookbook/pv-metadata.md:353-376`, the entire "Key-only (existence) search is not exposed" section, + becomes wrong. It is not a tweak: the section's *premise* is removed, so it should be replaced by a + short recipe using the helper, and the `# cookbook:no-mypy` escape-hatch snippet deleted. Deleting it + is a documentation improvement in its own right — that snippet exists only to work around this bug. + - `machine-configuration.md:377` and `query.md:216` are one-line helper inventories; they gain a clause. + Neither file currently has an attributes recipe worth extending, so no new prose is needed there. + +- **T9 — Baseline is green.** 719 unit tests pass at `dc477be` (plus 45 subtests). No integration test + touches these helpers, and no live server is required to verify this change. + +- **T10 — No current consumer exercises key-only, so this is a capability gap, not a reported break.** The + desktop app builds attribute criteria through `setIfBothPresent(attributeKeyCriterion, + attributeValueCriterion, …)` (`dp-desktop-app/.../DpApplication.java:1018`, `:1122`), requiring both key and + value. Recorded so the ticket is not oversold: nothing is broken in the field today. The case rests on + the internal inconsistency (Overview 1) and the documented-workaround cost (Overview 3). + +## Design decisions + +- **D1 — Match the #6 helpers exactly: `values: list[str] | None = None`, guarded by `if values:`.** The + five relaxed helpers should become copies of the shape already shipped at `dataset_client.py:193-214`, down + to the `if values:` truthiness test (which folds `None` and `[]` together, both meaning key-only). The + point of the ticket is to remove a divergence, so introducing a second spelling would defeat it. + Rejected: a distinct `attributes_exist(key)` helper. It would read more explicitly at the call site, but + it doubles the helper count on five classes, diverges from the two #6 helpers that just shipped, and has no + counterpart in the protocol, which models this as one criterion with an optional field. + +- **D2 — Keep the non-blank-key rejection on all five.** Unchanged behavior, but now load-bearing: per T5, + the v2 path has no server-side key check, so a blank key would silently match nothing. The client-side + guard is the only one there is for `PvQuery.attr` / `ConfigQuery.attr`. + +- **D3 — Invert the five empty-values assertions rather than deleting them.** Each becomes a positive + key-only build-request test covering *both* spellings (`attributes("k")` and `attributes("k", [])`), + mirroring `test_dataset_client.py:250-255`. Deleting them would leave the new behavior unpinned in exactly + the place a future reviewer would look for it. + +- **D4 — Correct `conventions.md`'s general rule rather than leaving it as a near-truth.** Reword to state + that helpers reject empty input *except* `attributes()`/`attr()`, whose empty `values` is the protocol's + key-only existence search — with the reason: an empty attributes criterion narrows, it does not match all. + This is the one doc edit that is not optional; the others are recipes. + +- **D5 — Replace, do not amend, the pv-metadata "not exposed" section.** Per T8, retitle to a positive + recipe ("Key-only (existence) search") showing `Q.attributes("S")`, and drop the raw-protobuf snippet and + its `# cookbook:no-mypy` directive. Keep the surrounding paragraph about dropping to the generated stubs + as a *general* escape hatch only if it still reads naturally without this example; otherwise let it go — + `conventions.md` is the right home for a general statement, and it is not this ticket's job to relocate it. + +## Implementation tasks + +Single PR; the work is one commit's worth. + +1. **`src/dp_python_lib/client/pv_metadata_client.py`** — `PvMetadataQuery.attributes` (line 92): signature to + `values: list[str] | None = None`; drop the `if not values: raise`; guard the assignment with `if values:`. + Docstring: mark `values` optional, state the key-only meaning, drop `values is empty` from `:raises:`. +2. **`src/dp_python_lib/client/machine_config_client.py`** — same for `ConfigurationQuery.attributes` (line 90) + and `ConfigurationActivationQuery.attributes` (line 235). +3. **`src/dp_python_lib/client/query_client.py`** — same for `PvQuery.attr` (line 143) and `ConfigQuery.attr` + (line 234). +4. **Tests** — per D3, in `test_pv_metadata_client.py`, `test_machine_config_client.py`, + `test_machine_config_activation_client.py`, and `test_query_client.py` (two helpers, including the + `test_empties_raise` loop entry at line 167). Each: a key-only build-request test asserting + `HasField("attributesCriterion")`, the key, and `values == []`; empty-key assertions retained. +5. **`doc/cookbook/conventions.md`** — reword the empty-input rule per D4. +6. **`doc/cookbook/pv-metadata.md`** — replace the "not exposed" section per D5. +7. **`doc/cookbook/machine-configuration.md:377` and `doc/cookbook/query.md:216-217`** — note the key-only + form in the helper inventories. +8. **Verify** — `.venv/bin/python -m pytest tests/unit/` (expect 719 + 5 new, all passing); + `ruff check .` and `ruff format --check .`; and + `.venv/bin/python .dev/tools/check-cookbook-snippets.py`, which must be run because task 6 changes a + snippet's directives. + +No `CLAUDE.md` change: it already describes the key-only form as #6's and names #40 as the back-port +([the DataSets/Annotations section](../../../CLAUDE.md)). That sentence should be updated to past tense when +this merges, which is a one-line edit best made in the same PR — added here so it is not forgotten. + +## Out of scope + +- **Optional `criteria` on the query/iter methods** — [#41](https://github.com/osprey-dcs/dp-python-lib/issues/41). +- **Relaxing any other empty-input guard** (`tags([])`, `pv_name()`, `parent([])`, …). For those the + `conventions.md` rationale holds exactly: they would match everything. No change, and D4's rewording must + not weaken the rule for them. +- **The server-side blank-key gap on the two v2 selectors** (T5). A dp-service issue if anyone wants it; + D2 makes it unreachable through this client. +- **De-duplicating the inlined Configurations attributes branch** (T3) — a dp-service refactor, no client + impact. + +## Dependencies and sequencing + +- **Nothing blocks this.** It needs no stub regeneration (the `key`/`values` fields are present in the + committed stubs, introspected: `['key', 'values']`), no server change, and no live server to verify. +- **Independent of #41**, which changes method signatures rather than criterion builders. They touch the + same three source files and overlapping cookbook pages, so shipping them **as two commits in one PR** is + reasonable and was the ticket's own suggestion; shipping separately is equally fine, with the second to + land rebasing over the first's `conventions.md` edit. Recommendation: one PR, two commits, #40 first — + its `conventions.md` edit is the smaller of the two. +- **No upstream release gating.** The behavior has been in dp-service since before `fddf692` and needs no + version banner: unlike the sample status API, this ships against servers already in use. + +## Open questions + +- **Q1 — Should `conventions.md` keep a single blanket sentence, or split the rule per helper?** + *Recommendation*: keep it a single sentence with a stated exception (D4). A per-helper table would be + accurate but out of proportion for one exception in one criterion type. + **Resolved 2026-09-10**: accepted. + +- **Q2 — Should the general "drop to the generated stubs" paragraph in `pv-metadata.md` survive the section + it illustrates?** *Recommendation*: decide while editing (D5). It is genuinely useful advice that + happens to have lost its example; if a natural one-sentence form remains, keep it, otherwise drop it + rather than inventing a contrived replacement example. + **Resolved 2026-09-10**: accepted — defer to the edit, no separate decision needed. diff --git a/src/dp_python_lib/client/machine_config_client.py b/src/dp_python_lib/client/machine_config_client.py index eb8bee1..6bfd67d 100644 --- a/src/dp_python_lib/client/machine_config_client.py +++ b/src/dp_python_lib/client/machine_config_client.py @@ -88,22 +88,26 @@ def tags( @staticmethod def attributes( - key: str, values: list[str] + key: str, values: list[str] | None = None ) -> "annotation_pb2.QueryConfigurationsRequest.QueryConfigurationsCriterion": """ - Builds a criterion matching configurations whose attribute with the given key has any of the specified values. + Builds a criterion matching configurations by attribute key and optional value(s). + + Omitting values (or passing an empty list) performs a key-only existence search: any configuration + possessing the key matches, whatever its value. That is a narrowing filter, not a match-all, which is + why it is allowed here while the other helpers still reject empty input. + :param key: Attribute key to match. - :param values: Attribute values to match for that key. + :param values: Attribute values to match for that key, or None for a key-only existence search. :return: A QueryConfigurationsCriterion with an attributesCriterion. - :raises ValueError: if key is empty or values is empty. + :raises ValueError: if key is empty. """ if not key: raise ValueError("attributes() requires a non-empty key") - if not values: - raise ValueError("attributes() requires a non-empty values list") criterion = ConfigurationQuery._Criterion() criterion.attributesCriterion.key = key - criterion.attributesCriterion.values[:] = values + if values: + criterion.attributesCriterion.values[:] = values return criterion @staticmethod @@ -233,22 +237,26 @@ def tags( @staticmethod def attributes( - key: str, values: list[str] + key: str, values: list[str] | None = None ) -> "annotation_pb2.QueryConfigurationActivationsRequest.QueryConfigurationActivationsCriterion": """ - Builds a criterion matching activations whose attribute with the given key has any of the specified values. + Builds a criterion matching activations by attribute key and optional value(s). + + Omitting values (or passing an empty list) performs a key-only existence search: any activation + possessing the key matches, whatever its value. That is a narrowing filter, not a match-all, which is + why it is allowed here while the other helpers still reject empty input. + :param key: Attribute key to match. - :param values: Attribute values to match for that key. + :param values: Attribute values to match for that key, or None for a key-only existence search. :return: A QueryConfigurationActivationsCriterion with an attributesCriterion. - :raises ValueError: if key is empty or values is empty. + :raises ValueError: if key is empty. """ if not key: raise ValueError("attributes() requires a non-empty key") - if not values: - raise ValueError("attributes() requires a non-empty values list") criterion = ConfigurationActivationQuery._Criterion() criterion.attributesCriterion.key = key - criterion.attributesCriterion.values[:] = values + if values: + criterion.attributesCriterion.values[:] = values return criterion diff --git a/src/dp_python_lib/client/pv_metadata_client.py b/src/dp_python_lib/client/pv_metadata_client.py index 49fd5d4..50c5d30 100644 --- a/src/dp_python_lib/client/pv_metadata_client.py +++ b/src/dp_python_lib/client/pv_metadata_client.py @@ -89,21 +89,27 @@ def tags(values: list[str]) -> "annotation_pb2.QueryPvMetadataRequest.QueryPvMet return criterion @staticmethod - def attributes(key: str, values: list[str]) -> "annotation_pb2.QueryPvMetadataRequest.QueryPvMetadataCriterion": + def attributes( + key: str, values: list[str] | None = None + ) -> "annotation_pb2.QueryPvMetadataRequest.QueryPvMetadataCriterion": """ - Builds a criterion matching PVs whose attribute with the given key has any of the specified values. + Builds a criterion matching PVs by attribute key and optional value(s). + + Omitting values (or passing an empty list) performs a key-only existence search: any PV possessing the + key matches, whatever its value. That is a narrowing filter, not a match-all, which is why it is + allowed here while the other helpers still reject empty input. + :param key: Attribute key to match. - :param values: Attribute values to match for that key. + :param values: Attribute values to match for that key, or None for a key-only existence search. :return: A QueryPvMetadataCriterion with an attributesCriterion. - :raises ValueError: if key is empty or values is empty. + :raises ValueError: if key is empty. """ if not key: raise ValueError("attributes() requires a non-empty key") - if not values: - raise ValueError("attributes() requires a non-empty values list") criterion = PvMetadataQuery._Criterion() criterion.attributesCriterion.key = key - criterion.attributesCriterion.values[:] = values + if values: + criterion.attributesCriterion.values[:] = values return criterion diff --git a/src/dp_python_lib/client/query_client.py b/src/dp_python_lib/client/query_client.py index fda07ec..6adb985 100644 --- a/src/dp_python_lib/client/query_client.py +++ b/src/dp_python_lib/client/query_client.py @@ -140,21 +140,29 @@ def tags(values: list[str]) -> "query_pb2.PvSelector.MetadataQuery.Criterion": return criterion @staticmethod - def attr(key: str, values: list[str]) -> "query_pb2.PvSelector.MetadataQuery.Criterion": + def attr(key: str, values: list[str] | None = None) -> "query_pb2.PvSelector.MetadataQuery.Criterion": """ - Builds a metadata criterion matching PVs whose attribute with the given key has any of the specified values. + Builds a metadata criterion matching PVs by attribute key and optional value(s). + + Omitting values (or passing an empty list) performs a key-only existence search: any PV possessing the + key matches, whatever its value. That is a narrowing filter, not a match-all, which is why it is + allowed here while the other helpers still reject empty input. + + The non-empty key check matters more here than on the annotation-service helpers: the server does not + validate the key on this selector, so a blank one would reach Mongo as an existence test on + "attributes." and silently match nothing. + :param key: Attribute key to match. - :param values: Attribute values to match for that key. + :param values: Attribute values to match for that key, or None for a key-only existence search. :return: A metadata Criterion with an attributesCriterion. - :raises ValueError: if key is empty or values is empty. + :raises ValueError: if key is empty. """ if not key: raise ValueError("attr() requires a non-empty key") - if not values: - raise ValueError("attr() requires a non-empty values list") criterion = PvQuery._MetaCriterion() criterion.attributesCriterion.key = key - criterion.attributesCriterion.values[:] = values + if values: + criterion.attributesCriterion.values[:] = values return criterion @@ -231,21 +239,28 @@ def tags(values: list[str]) -> "query_pb2.ConfigurationSelector.Criterion": return criterion @staticmethod - def attr(key: str, values: list[str]) -> "query_pb2.ConfigurationSelector.Criterion": + def attr(key: str, values: list[str] | None = None) -> "query_pb2.ConfigurationSelector.Criterion": """ - Builds a criterion matching activations whose attribute with the given key has any of the specified values. + Builds a criterion matching activations by attribute key and optional value(s). + + Omitting values (or passing an empty list) performs a key-only existence search: any activation + possessing the key matches, whatever its value. That is a narrowing filter, not a match-all, which is + why it is allowed here while the other helpers still reject empty input. + + As with PvQuery.attr(), the server does not validate the key on this selector, so the non-empty key + check here is the only one there is. + :param key: Attribute key to match. - :param values: Attribute values to match for that key. + :param values: Attribute values to match for that key, or None for a key-only existence search. :return: A ConfigurationSelector.Criterion with an attributesCriterion. - :raises ValueError: if key is empty or values is empty. + :raises ValueError: if key is empty. """ if not key: raise ValueError("attr() requires a non-empty key") - if not values: - raise ValueError("attr() requires a non-empty values list") criterion = ConfigQuery._Criterion() criterion.attributesCriterion.key = key - criterion.attributesCriterion.values[:] = values + if values: + criterion.attributesCriterion.values[:] = values return criterion diff --git a/tests/unit/test_machine_config_activation_client.py b/tests/unit/test_machine_config_activation_client.py index 1a09d68..9a7daae 100644 --- a/tests/unit/test_machine_config_activation_client.py +++ b/tests/unit/test_machine_config_activation_client.py @@ -84,9 +84,15 @@ def test_attributes_empty_key_raises(self): with self.assertRaises(ValueError): ConfigurationActivationQuery.attributes("", ["v"]) - def test_attributes_empty_values_raises(self): - with self.assertRaises(ValueError): - ConfigurationActivationQuery.attributes("owner", []) + def test_attributes_key_only(self): + # An absent/empty values list is a key-only existence search (issue #40), not a rejection. + for criterion in ( + ConfigurationActivationQuery.attributes("owner"), + ConfigurationActivationQuery.attributes("owner", []), + ): + self.assertTrue(criterion.HasField("attributesCriterion")) + self.assertEqual(criterion.attributesCriterion.key, "owner") + self.assertEqual(list(criterion.attributesCriterion.values), []) class TestBuildSaveActivationRequest(unittest.TestCase): diff --git a/tests/unit/test_machine_config_client.py b/tests/unit/test_machine_config_client.py index 95a0a4a..7db0db7 100644 --- a/tests/unit/test_machine_config_client.py +++ b/tests/unit/test_machine_config_client.py @@ -70,9 +70,12 @@ def test_attributes_empty_key_raises(self): with self.assertRaises(ValueError): ConfigurationQuery.attributes("", ["v"]) - def test_attributes_empty_values_raises(self): - with self.assertRaises(ValueError): - ConfigurationQuery.attributes("owner", []) + def test_attributes_key_only(self): + # An absent/empty values list is a key-only existence search (issue #40), not a rejection. + for criterion in (ConfigurationQuery.attributes("owner"), ConfigurationQuery.attributes("owner", [])): + self.assertTrue(criterion.HasField("attributesCriterion")) + self.assertEqual(criterion.attributesCriterion.key, "owner") + self.assertEqual(list(criterion.attributesCriterion.values), []) def test_parent(self): c = ConfigurationQuery.parent(["root-cfg"]) diff --git a/tests/unit/test_pv_metadata_client.py b/tests/unit/test_pv_metadata_client.py index 836068c..1761352 100644 --- a/tests/unit/test_pv_metadata_client.py +++ b/tests/unit/test_pv_metadata_client.py @@ -144,11 +144,16 @@ def test_tags_empty_raises(self): with self.assertRaises(ValueError): PvMetadataQuery.tags([]) - def test_attributes_empty_raises(self): + def test_attributes_key_only(self): + # An absent/empty values list is a key-only existence search (issue #40), not a rejection. + for criterion in (PvMetadataQuery.attributes("unit"), PvMetadataQuery.attributes("unit", [])): + self.assertTrue(criterion.HasField("attributesCriterion")) + self.assertEqual(criterion.attributesCriterion.key, "unit") + self.assertEqual(list(criterion.attributesCriterion.values), []) + + def test_attributes_empty_key_raises(self): with self.assertRaises(ValueError): PvMetadataQuery.attributes("", ["V"]) - with self.assertRaises(ValueError): - PvMetadataQuery.attributes("unit", []) class TestSendSavePvMetadata(unittest.TestCase): diff --git a/tests/unit/test_query_client.py b/tests/unit/test_query_client.py index 5f4064a..89dd1f9 100644 --- a/tests/unit/test_query_client.py +++ b/tests/unit/test_query_client.py @@ -123,9 +123,12 @@ def test_attr_empty_key_raises(self): with self.assertRaises(ValueError): PvQuery.attr("", ["V"]) - def test_attr_empty_values_raises(self): - with self.assertRaises(ValueError): - PvQuery.attr("unit", []) + def test_attr_key_only(self): + # An absent/empty values list is a key-only existence search (issue #40), not a rejection. + for criterion in (PvQuery.attr("unit"), PvQuery.attr("unit", [])): + self.assertTrue(criterion.HasField("attributesCriterion")) + self.assertEqual(criterion.attributesCriterion.key, "unit") + self.assertEqual(list(criterion.attributesCriterion.values), []) # ---------------------------------------------------------------------- @@ -164,12 +167,18 @@ def test_empties_raise(self): lambda: ConfigQuery.client_activation_id([]), lambda: ConfigQuery.category([]), lambda: ConfigQuery.tags([]), - lambda: ConfigQuery.attr("k", []), lambda: ConfigQuery.attr("", ["v"]), ): with self.assertRaises(ValueError): call() + def test_attr_key_only(self): + # An absent/empty values list is a key-only existence search (issue #40), not a rejection. + for criterion in (ConfigQuery.attr("owner"), ConfigQuery.attr("owner", [])): + self.assertTrue(criterion.HasField("attributesCriterion")) + self.assertEqual(criterion.attributesCriterion.key, "owner") + self.assertEqual(list(criterion.attributesCriterion.values), []) + # ---------------------------------------------------------------------- # QueryParams validation From 4a486f6e2e7bfc2b5aae1347e2e46e2fb0b17092 Mon Sep 17 00:00:00 2001 From: Craig McChesney Date: Thu, 10 Sep 2026 15:23:47 -0600 Subject: [PATCH 2/3] feat: make criteria optional on the annotation-service query/iter methods (#41) Six methods took `criteria` as a required positional argument, a shape dating from when the server rejected an empty criteria list. The server now treats an empty list as match-all, so "browse everything, paged" is a legitimate call -- but from Python it read `iter_configurations([])`, which looks like a mistake at the call site and was documented nowhere. `criteria` is now optional on query_pv_metadata / iter_pv_metadata, query_configurations / iter_configurations, and query_configuration_activations / iter_configuration_activations, plus their three private request builders, matching the shape #6 shipped on datasets and annotations. `iter_pv_metadata()` is the browse-all form. Non-breaking: criteria remains the first positional parameter. Server behavior verified rather than taken from the ticket. Each validation site carries a deliberate comment, not merely a missing check (QueryPvMetadataJob.java:38 and the two siblings): // An empty criteria list is match-all by contract (#245), not an // error, so there is deliberately no list-level emptiness check here. Per-criterion validation is retained everywhere, so this loosens the list and not its contents. Paging still applies: DEFAULT_QUERY_LIMIT = 100 in MongoSyncAnnotationClient, applied unconditionally -- dropping the last criterion does not change the page size -- which is why the docstrings point at iter_* rather than a bare query_* for browsing. Two corrections to the ticket body, recorded in the plan: - The default page size is NOT configurable. It is a hardcoded private static final int with no config key; the proto's "server-configured default page size" wording is loose and is deliberately not repeated in the client docs. - dp-grpc #245 / PR #147 covers only the three metadata queries. Datasets and annotations got match-all from the earlier #132 (7b2ea35) plus dp-service #248. The net behavior is as the ticket says; only the attribution was wrong. The v2 query methods are deliberately excluded: QueryParams still requires a PV selector or config criteria, since a time-series query with no selection is unbounded rather than a browse-all. Also adds a _ActivationCriterion TypeAlias -- the fully-qualified name is 109 characters and overflows the line limit in an annotated parameter. Using TypeAlias rather than a bare assignment keeps mypy happy in an annotation position, and net drops one pre-existing mypy error. 725 unit tests pass; ruff clean; 107 cookbook snippets checked. Plan: plan/tickets/41/plan.md. Verified against dp-grpc 6dfff3f and dp-service fddf692. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_019he3UCsAnqTDE2VQ73Djwn --- CLAUDE.md | 23 ++- doc/cookbook/conventions.md | 15 ++ doc/cookbook/machine-configuration.md | 12 ++ doc/cookbook/pv-metadata.md | 14 ++ plan/tickets/41/plan.md | 155 ++++++++++++++++++ .../client/machine_config_client.py | 68 ++++++-- .../client/pv_metadata_client.py | 29 +++- .../test_machine_config_activation_client.py | 8 + tests/unit/test_machine_config_client.py | 8 + tests/unit/test_pv_metadata_client.py | 20 +++ 10 files changed, 324 insertions(+), 28 deletions(-) create mode 100644 plan/tickets/41/plan.md diff --git a/CLAUDE.md b/CLAUDE.md index 437a3f1..b6509a1 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -614,10 +614,25 @@ Invariants worth knowing before touching this code: - **`save_annotation()` replaces in full, including calculations**: omitting them clears *and deletes* the stored object, and a replacement returns a new `calculationsId`. `get_annotation()` is the only method returning calculations inline; `query_annotations()` results carry the id with empty content. -- Two criterion-helper differences from the older `PvMetadataQuery` / `ConfigurationQuery` helpers, both following - the proto: `attributes(key)` accepts an absent `values` list as a key-only existence search, and `criteria` is - optional because the server treats an empty list as match-all. Back-porting these to the five existing helpers is - [#40](https://github.com/osprey-dcs/dp-python-lib/issues/40) / [#41](https://github.com/osprey-dcs/dp-python-lib/issues/41). +- Two conventions these clients introduced, both following the proto, **now shared by every criteria-based client** + after the [#40](https://github.com/osprey-dcs/dp-python-lib/issues/40) / + [#41](https://github.com/osprey-dcs/dp-python-lib/issues/41) back-ports (`plan/tickets/40/plan.md`, + `plan/tickets/41/plan.md`): + - **`attributes(key)` / `attr(key)` accept an absent or empty `values` list as a key-only existence search** — + match every record possessing the key, whatever its value. All seven attribute helpers now do this + (`PvMetadataQuery`, `ConfigurationQuery`, `ConfigurationActivationQuery`, `DataSetQuery`, `AnnotationQuery`, + `PvQuery.attr`, `ConfigQuery.attr`). It is the one exception to the "helpers reject empty input" rule, because + unlike `tags([])` it *narrows* the result set rather than matching everything. The **key** is still required, + and that check is load-bearing on the two v2 query selectors, where the server does not validate it: a blank key + would reach Mongo as an existence test on `"attributes."` and silently match nothing. Server side, empty values + build `Filters.exists("attributes.")` (`MongoQueryFilterBuilder.attributeFilter()`), not an `$in: []` + - **`criteria` is optional on every paged annotation-service query/iter method**, because the server treats an + empty list as match-all — `iter_pv_metadata()` with no arguments is the browse-all form. The server's default + page size (100, hardcoded in `MongoSyncAnnotationClient.DEFAULT_QUERY_LIMIT`, **not** configurable) applies + **unconditionally**: dropping the last criterion does not change the page size, so a bare `query_*()` still + returns one page and a token. That is why `iter_*` is the right call for browsing. Note the v2 query methods + are deliberately *not* included: `QueryParams` still requires a PV selector or config criteria, since a + time-series query with no selection is unbounded rather than a browse-all - `ExportFormat` makes the server-rejected `EXPORT_FORMAT_UNSPECIFIED` unreachable, and `ExportDataRequestParams` requires at least one of `dataset_id` / `data_blocks` / `calculations_spec`. The exported file lives on the **server's** filesystem and there is no retrieval RPC, so there is no download convenience. diff --git a/doc/cookbook/conventions.md b/doc/cookbook/conventions.md index e8d608d..f801912 100644 --- a/doc/cookbook/conventions.md +++ b/doc/cookbook/conventions.md @@ -137,6 +137,21 @@ Some things to keep in mind: - **Omitting `limit` does not mean "no limit".** The server applies its own default page size (currently 100) when `limit` is absent or zero, so a `query_*` call without one still returns a page, not the whole result set. Check `next_page_token`. +- **Omitting the criteria browses everything.** On the annotation-service queries — PV metadata, + configurations, activations, datasets, annotations — an omitted or empty criteria list matches + *all* records rather than being rejected, so `iter_pv_metadata()` with no arguments walks the + whole catalogue: + + ```python + # cookbook:partial + for record in client.annotation.pv_metadata.iter_pv_metadata(): + print(record.pvName) + ``` + + The default page size still applies unconditionally — it does not change just because you + dropped the last criterion — so the bare `query_*` form returns one page and a token, not the + collection. `iter_*` is the right call here. Mind how large the collection is before iterating + it. - **There is no total count.** The API deliberately omits it — computing one requires a separate expensive query — so you cannot know the result size in advance. - **Results come back in a stable order.** The server sorts each collection by its natural key — diff --git a/doc/cookbook/machine-configuration.md b/doc/cookbook/machine-configuration.md index 98deb13..67ede62 100644 --- a/doc/cookbook/machine-configuration.md +++ b/doc/cookbook/machine-configuration.md @@ -374,6 +374,18 @@ for configuration in client.annotation.machine_config.iter_configurations([ print(configuration.configurationName) ``` +Omit the criteria entirely to browse everything — an empty criteria list matches all records: + +```python +# cookbook:partial +for configuration in client.annotation.machine_config.iter_configurations(): + print(configuration.configurationName) +``` + +The default page size still applies, so prefer `iter_configurations()` over a bare +`query_configurations()` when you want the whole set. The same holds for +`iter_configuration_activations()`, which on a busy machine can be a much larger collection. + `C` offers `name`, `category`, `tags`, `attributes`, and `parent`; `CA` offers `timestamp`, `time_range`, `configuration_name`, `client_activation_id`, `category`, `tags`, and `attributes`. diff --git a/doc/cookbook/pv-metadata.md b/doc/cookbook/pv-metadata.md index a01a65d..1d0aeb3 100644 --- a/doc/cookbook/pv-metadata.md +++ b/doc/cookbook/pv-metadata.md @@ -350,6 +350,20 @@ This reads every matching record to filter client-side, so keep the server-side as you can. Note also that string equality is exact: `"0.489650"` and `"0.48965"` are different attribute values even though the numbers are equal. +### Browsing the whole catalogue + +Omit the criteria entirely to walk every record — an empty criteria list matches all: + +```python +# cookbook:partial +for record in client.annotation.pv_metadata.iter_pv_metadata(): + print(record.pvName) +``` + +The server's default page size still applies, so use `iter_pv_metadata()` rather than a bare +`query_pv_metadata()`, which returns only the first page. Check how big the catalogue is before +iterating all of it. + ### Which PVs have this attribute at all? Omit the values list — or pass an empty one — for a **key-only existence search**: every PV diff --git a/plan/tickets/41/plan.md b/plan/tickets/41/plan.md new file mode 100644 index 0000000..3bec7fb --- /dev/null +++ b/plan/tickets/41/plan.md @@ -0,0 +1,155 @@ +# Issue #41 — Make `criteria` optional on the annotation-service query/iter methods + +- **Ticket**: [osprey-dcs/dp-python-lib#41](https://github.com/osprey-dcs/dp-python-lib/issues/41) +- **Companion**: [#40](https://github.com/osprey-dcs/dp-python-lib/issues/40) (key-only `attributes()`), shipping + in the same PR as the preceding commit. Same three source files, overlapping cookbook pages. +- **Surfaced by**: `plan/tickets/6/plan.md` finding 5 / Q6 / D3, whose `query_datasets()` / `query_annotations()` + and their `iter_*` forms take `criteria` optional from the start. +- **Status**: written 2026-09-10 against dp-python-lib `e7a77db` (the #40 commit), dp-grpc `6dfff3f`, + dp-service `fddf692`. Every premise verified against the protos and the server source; two attribution + errors in the ticket body are corrected below. + +## Overview + +Six methods take `criteria` as a required positional argument, a shape that dates from when the server +**rejected** an empty criteria list. The server now treats an empty list as match-all, so "browse everything, +paged" is a legitimate call — but from Python it currently reads `iter_configurations([])`, which looks like a +mistake at the call site and is documented nowhere. + +Make `criteria` optional on all six, so `for cfg in mc.iter_configurations(): ...` is the browse-all form. + +Non-breaking: `criteria` stays the first parameter, so every positional caller is unaffected. + +## Background / triage findings + +Verified against the sources, not taken from the ticket text. + +- **T1 — The six methods are as listed**, plus three private request builders that must change with them: + `pv_metadata_client.py:405/445/471` (`_build_query_pv_metadata_request`, `query_pv_metadata`, + `iter_pv_metadata`), `machine_config_client.py:755/800/826` (configurations) and `:1113/1158/1184` + (activations). + +- **T2 — The server accepts an empty criteria list on all five annotation-service paged queries and treats it + as match-all.** Each validation site carries a deliberate comment rather than merely lacking a check — + `QueryPvMetadataJob.java:38`, `QueryConfigurationsJob.java:38`, `QueryConfigurationActivationsJob.java:38`: + + ```java + // An empty criteria list is match-all by contract (#245), not an error, so there is + // deliberately no list-level emptiness check here. Per-criterion validation below is + // unaffected: a criterion that IS supplied must still be well-formed. + ``` + + Datasets and annotations carry the same comment in `AnnotationServiceImpl.java:228` and `:679`. Per-criterion + validation is retained everywhere: a criterion that *is* supplied must still be well-formed, and + `CRITERION_NOT_SET` is still rejected. So this change loosens the list, not its contents. + +- **T3 — Paging still applies, and the default is 100.** `MongoSyncAnnotationClient.java:81` declares + `private static final int DEFAULT_QUERY_LIMIT = 100`, applied identically at all five call sites as + `request.getLimit() > 0 ? request.getLimit() : DEFAULT_QUERY_LIMIT`. The class comment at `:79` and the + proto both state the default is **unconditional** — it does not depend on whether criteria were supplied, so + removing the last criterion from a request does not change its page size. `:986` adds "limit is always + positive (DEFAULT_QUERY_LIMIT when unset), so there is no unbounded path." This confirms the ticket's claim + that `iter_*` remains the right call for browse-all: a bare `query_*()` returns 100 records and a page token, + not the collection. + +- **T4 — Correction to the ticket: the default page size is NOT configurable.** The ticket says "the server's + default page size still applies", which is true, but the proto wording for datasets/annotations + ("a server-configured default page size") is loose and should not be carried into client docs. + `DEFAULT_QUERY_LIMIT` is a hardcoded `private static final int` with no config key and no `configMgr()` + lookup. Do not promise users it is tunable. (Distinct from the genuinely config-driven + `DEFAULT_SAMPLE_STATUS_QUERY_DEFAULT_PAGE_SIZE = 10_000` in `MongoAnnotationHandler.java:34`, which belongs + to the sample status API — do not conflate them.) + +- **T5 — Correction to the ticket: the upstream attribution is too broad.** The ticket credits dp-grpc #245 / + PR #147 for all five RPCs. `2e6f849` (inside PR #147) changed only the three metadata queries — + `annotation.proto:1805`, `:2221`, `:2642` — replacing the prior text "An empty criteria list is rejected with + an ExceptionalResult; at least one criterion is required." Datasets and annotations got their match-all + wording from the earlier `7b2ea35` (dp-grpc #132) at `annotation.proto:929` and `:1471`, with dp-service #248 + implementing it. The net behavior is what the ticket says; only the provenance is wrong. Worth correcting + because this repo's plans cite upstream tickets as evidence. + +- **T6 — Server tests cover the empty-criteria path for all five**, including the paging claim specifically. + `PvMetadataClientIT.java:858` (`testQueryPvMetadataUnsetLimitReturnsDefaultPageSize`) saves 101 PVs, asserts + an unset-limit match-all returns exactly 100 with a non-empty `nextPageToken`, then repeats *with* criteria + present to pin that the default is unconditional. Also `PvMetadataClientIT.java:789`/`:812`, + `ConfigurationIT.java:237`/`:856`, `ConfigurationClientIT.java:1168`/`:1195`/`:1302`, + `QueryDataSetsIT.java:300`, `QueryAnnotationsIT.java:466`. + +- **T7 — A server-side inconsistency exists but is invisible from Python.** Four of the five build a sentinel + match-all filter (`Filters.exists()` on an always-present field: `MongoSyncAnnotationClient.java:369`, `:707`, + `:982`, `:1241`); activations use a literal empty document (`:1500`). Behaviorally equivalent, since the + probed fields are required on every document. Noted so a future reader does not mistake it for a + client-visible difference. Out of scope. + +- **T8 — The existing `iter_*` methods will do the right thing unchanged.** Each loops on the page token and + raises `RuntimeError` on a page error; none inspects `criteria` beyond passing it through. So the change is + confined to signatures, the `criteria or []` normalization, and docstrings. + +- **T9 — `check_at_most_one_text_criterion()` is not involved.** The three older query methods do not call it + (only #6's datasets/annotations do, since only those criterion types have a `textCriterion` arm). Confirmed + by `grep`: no call in `pv_metadata_client.py` or `machine_config_client.py`. Nothing to guard against an + empty list there. + +- **T10 — Baseline is green at the #40 commit**: 721 unit tests, ruff clean, 104 cookbook snippets checked. + +## Design decisions + +- **D1 — Mirror #6's shape exactly**, as #40 did for the criterion helpers: `criteria: list[...] | None = None` + on the public methods and the private builders; `criteria = criteria or []` at the top of each public method + (so the existing `len(criteria)` log lines stay correct); `if criteria:` guarding `request.criteria.extend()` + in the builders. Reference: `dataset_client.py:546-570` and `:588-615`. Rejected: a separate + `iter_all_*()` method per entity, which would double the surface for a case the protocol models as an empty + list. + +- **D2 — Keep `criteria` as the first positional parameter.** Making it keyword-only would be cleaner in + isolation but would break every existing positional caller for no benefit, and would diverge from #6. + +- **D3 — Document the page-size consequence at each call site, not just in `conventions.md`.** A user reaching + for `query_pv_metadata()` with no criteria expecting "everything" gets 100 records and a token. Each + docstring already says `limit` is per-page; the browse-all sentence should name `iter_*` as the way to get + the whole collection. Per T4, say "a server default" without claiming it is configurable. + +## Implementation tasks + +Second commit in the #40 PR. + +1. **`src/dp_python_lib/client/pv_metadata_client.py`** — `_build_query_pv_metadata_request` (405), + `query_pv_metadata` (445), `iter_pv_metadata` (471) per D1/D3. +2. **`src/dp_python_lib/client/machine_config_client.py`** — the same for configurations (755/800/826) and + activations (1113/1158/1184). +3. **Tests** — one build-request test per method asserting that omitted criteria produces an empty `criteria` + list rather than a rejection, in `test_pv_metadata_client.py`, `test_machine_config_client.py`, and + `test_machine_config_activation_client.py`. Add an `iter_*`-with-no-criteria test for at least one entity, + since the iterator is the method the browse-all case actually uses. +4. **`doc/cookbook/conventions.md`** — in the paging section, state that an omitted or empty criteria list + matches all records, and that the server's default page size still applies, so `iter_*` is the right call + for browse-all. +5. **`doc/cookbook/pv-metadata.md` / `machine-configuration.md`** — a short browse-all recipe where each file + introduces querying. +6. **`CLAUDE.md`** — update the sentence at line 617-620 that names #40/#41 as pending back-ports; both are now + done, so the five older helpers and six methods no longer diverge from #6's. +7. **Verify** — `pytest tests/unit/`, `ruff check .`, `ruff format --check .`, and + `.dev/tools/check-cookbook-snippets.py`. + +## Out of scope + +- **The two v2 query methods** (`query_samples` / `iter_query_samples`). Their `QueryParams` requires at least + one of `pv_selector` / `config_criteria`, which is a different contract — a time-series query with no PV + selection is not a browse-all, it is unbounded. No change. +- **`get_datasets()` / other non-criteria methods.** +- **The server's sentinel-vs-empty filter divergence** (T7) — a dp-service cosmetic issue, no client impact. +- **Making the server page size configurable** (T4) — a dp-service question if anyone wants it. + +## Dependencies and sequencing + +- **Depends on nothing**, but is sequenced **after #40** in the same PR so the `conventions.md` edits stack + cleanly (#40 rewrites the empty-input rule; #41 adds to the paging section — different sections, but the + smaller edit lands first). +- No stub regeneration, no server change, no live server needed to verify. +- No release gating: the behavior is in dp-service `fddf692` and predates it for datasets/annotations. + +## Open questions + +- **Q1 — Should the browse-all recipes warn that the collection may be large?** *Recommendation*: yes, one + clause, since `iter_*` will happily page through an unbounded catalogue. Not a separate section. + **Resolved 2026-09-10**: accepted. diff --git a/src/dp_python_lib/client/machine_config_client.py b/src/dp_python_lib/client/machine_config_client.py index 6bfd67d..53a5b74 100644 --- a/src/dp_python_lib/client/machine_config_client.py +++ b/src/dp_python_lib/client/machine_config_client.py @@ -1,6 +1,7 @@ import logging from collections.abc import Iterator from datetime import datetime, timezone +from typing import TypeAlias import grpc @@ -13,6 +14,13 @@ from dp_python_lib.client.time_conversions import TimestampInput, to_timestamp from dp_python_lib.grpc import annotation_pb2, annotation_pb2_grpc, common_pb2 +# The fully-qualified activation criterion name is 109 characters, which overflows the 120-column limit as soon +# as it appears in an annotated parameter. Alias it once rather than reflowing every signature that uses it. +# TypeAlias (not a bare assignment) so mypy accepts it in an annotation position. +_ActivationCriterion: TypeAlias = ( + annotation_pb2.QueryConfigurationActivationsRequest.QueryConfigurationActivationsCriterion +) + class ConfigurationQuery: """ @@ -754,20 +762,22 @@ def get_configuration(self, configuration_name: str) -> GetConfigurationApiResul def _build_query_configurations_request( self, - criteria: list[annotation_pb2.QueryConfigurationsRequest.QueryConfigurationsCriterion], + criteria: list[annotation_pb2.QueryConfigurationsRequest.QueryConfigurationsCriterion] | None = None, limit: int | None = None, page_token: str | None = None, ) -> annotation_pb2.QueryConfigurationsRequest: """ Builds a QueryConfigurationsRequest from the supplied criteria and paging parameters. - :param criteria: List of QueryConfigurationsCriterion objects (see ConfigurationQuery helpers). + :param criteria: List of QueryConfigurationsCriterion objects (see ConfigurationQuery helpers), or None to + match all records. :param limit: Maximum number of records to return per page (optional). :param page_token: Token for retrieving a subsequent page (optional). :return: A QueryConfigurationsRequest for the specified params. """ - self.logger.debug("Building QueryConfigurationsRequest with %d criteria", len(criteria)) + self.logger.debug("Building QueryConfigurationsRequest with %d criteria", len(criteria) if criteria else 0) request = annotation_pb2.QueryConfigurationsRequest() - request.criteria.extend(criteria) + if criteria: + request.criteria.extend(criteria) if limit is not None: request.limit = limit if page_token: @@ -799,18 +809,25 @@ def _send_query_configurations( def query_configurations( self, - criteria: list[annotation_pb2.QueryConfigurationsRequest.QueryConfigurationsCriterion], + criteria: list[annotation_pb2.QueryConfigurationsRequest.QueryConfigurationsCriterion] | None = None, limit: int | None = None, page_token: str | None = None, ) -> QueryConfigurationsApiResult: """ User-facing method for invoking the queryConfigurations() API method. Returns a single page of results; use iter_configurations() to page through all results transparently. - :param criteria: List of QueryConfigurationsCriterion objects (see ConfigurationQuery helpers). + + An omitted or empty criteria list matches all records. The server's default page size still applies, so + this returns one page and a next-page token rather than every configuration -- use iter_configurations() + to browse everything. + + :param criteria: List of QueryConfigurationsCriterion objects (see ConfigurationQuery helpers), or None + to match all records. :param limit: Maximum number of records to return per page (optional). :param page_token: Token for retrieving a subsequent page (optional). :return: A QueryConfigurationsApiResult with a single page of results and status information. """ + criteria = criteria or [] self.logger.info("Starting queryConfigurations operation with %d criteria", len(criteria)) request = self._build_query_configurations_request(criteria, limit=limit, page_token=page_token) @@ -825,7 +842,7 @@ def query_configurations( def iter_configurations( self, - criteria: list[annotation_pb2.QueryConfigurationsRequest.QueryConfigurationsCriterion], + criteria: list[annotation_pb2.QueryConfigurationsRequest.QueryConfigurationsCriterion] | None = None, limit: int | None = None, ) -> Iterator[common_pb2.Configuration]: """ @@ -834,7 +851,11 @@ def iter_configurations( Raises RuntimeError if any page returns an error, so callers can distinguish failure from an empty result set. - :param criteria: List of QueryConfigurationsCriterion objects (see ConfigurationQuery helpers). + Omit criteria (or pass an empty list) to browse every configuration: an empty list matches all records, + and this generator pages through them all. Mind the size of the collection before doing so. + + :param criteria: List of QueryConfigurationsCriterion objects (see ConfigurationQuery helpers), or None + to match all records. :param limit: Maximum number of records to return per page (optional). :return: An iterator over all matching Configuration records across all pages. """ @@ -1112,20 +1133,24 @@ def get_configuration_activation( def _build_query_configuration_activations_request( self, - criteria: list[annotation_pb2.QueryConfigurationActivationsRequest.QueryConfigurationActivationsCriterion], + criteria: list["_ActivationCriterion"] | None = None, limit: int | None = None, page_token: str | None = None, ) -> annotation_pb2.QueryConfigurationActivationsRequest: """ Builds a QueryConfigurationActivationsRequest from the supplied criteria and paging parameters. - :param criteria: List of QueryConfigurationActivationsCriterion objects (see ConfigurationActivationQuery). + :param criteria: List of QueryConfigurationActivationsCriterion objects (see + ConfigurationActivationQuery), or None to match all records. :param limit: Maximum number of records to return per page (optional). :param page_token: Token for retrieving a subsequent page (optional). :return: A QueryConfigurationActivationsRequest for the specified params. """ - self.logger.debug("Building QueryConfigurationActivationsRequest with %d criteria", len(criteria)) + self.logger.debug( + "Building QueryConfigurationActivationsRequest with %d criteria", len(criteria) if criteria else 0 + ) request = annotation_pb2.QueryConfigurationActivationsRequest() - request.criteria.extend(criteria) + if criteria: + request.criteria.extend(criteria) if limit is not None: request.limit = limit if page_token: @@ -1157,18 +1182,25 @@ def _send_query_configuration_activations( def query_configuration_activations( self, - criteria: list[annotation_pb2.QueryConfigurationActivationsRequest.QueryConfigurationActivationsCriterion], + criteria: list["_ActivationCriterion"] | None = None, limit: int | None = None, page_token: str | None = None, ) -> QueryConfigurationActivationsApiResult: """ User-facing method for invoking the queryConfigurationActivations() API method. Returns a single page of results; use iter_configuration_activations() to page through all results transparently. - :param criteria: List of QueryConfigurationActivationsCriterion objects (see ConfigurationActivationQuery). + + An omitted or empty criteria list matches all records. The server's default page size still applies, so + this returns one page and a next-page token rather than every activation -- use + iter_configuration_activations() to browse everything. + + :param criteria: List of QueryConfigurationActivationsCriterion objects (see ConfigurationActivationQuery), + or None to match all records. :param limit: Maximum number of records to return per page (optional). :param page_token: Token for retrieving a subsequent page (optional). :return: A QueryConfigurationActivationsApiResult with a single page of results and status information. """ + criteria = criteria or [] self.logger.info("Starting queryConfigurationActivations operation with %d criteria", len(criteria)) request = self._build_query_configuration_activations_request(criteria, limit=limit, page_token=page_token) @@ -1183,7 +1215,7 @@ def query_configuration_activations( def iter_configuration_activations( self, - criteria: list[annotation_pb2.QueryConfigurationActivationsRequest.QueryConfigurationActivationsCriterion], + criteria: list["_ActivationCriterion"] | None = None, limit: int | None = None, ) -> Iterator[common_pb2.ConfigurationActivation]: """ @@ -1192,7 +1224,11 @@ def iter_configuration_activations( Raises RuntimeError if any page returns an error, so callers can distinguish failure from an empty result set. - :param criteria: List of QueryConfigurationActivationsCriterion objects (see ConfigurationActivationQuery). + Omit criteria (or pass an empty list) to browse every activation: an empty list matches all records, and + this generator pages through them all. Mind the size of the collection before doing so. + + :param criteria: List of QueryConfigurationActivationsCriterion objects (see ConfigurationActivationQuery), + or None to match all records. :param limit: Maximum number of records to return per page (optional). :return: An iterator over all matching ConfigurationActivation records across all pages. """ diff --git a/src/dp_python_lib/client/pv_metadata_client.py b/src/dp_python_lib/client/pv_metadata_client.py index 50c5d30..43a4ce6 100644 --- a/src/dp_python_lib/client/pv_metadata_client.py +++ b/src/dp_python_lib/client/pv_metadata_client.py @@ -404,20 +404,22 @@ def get_pv_metadata(self, pv_name_or_alias: str) -> GetPvMetadataApiResult: def _build_query_pv_metadata_request( self, - criteria: list[annotation_pb2.QueryPvMetadataRequest.QueryPvMetadataCriterion], + criteria: list[annotation_pb2.QueryPvMetadataRequest.QueryPvMetadataCriterion] | None = None, limit: int | None = None, page_token: str | None = None, ) -> annotation_pb2.QueryPvMetadataRequest: """ Builds a QueryPvMetadataRequest from the supplied criteria and paging parameters. - :param criteria: List of QueryPvMetadataCriterion objects (see PvMetadataQuery helpers). + :param criteria: List of QueryPvMetadataCriterion objects (see PvMetadataQuery helpers), or None to + match all records. :param limit: Maximum number of records to return per page (optional). :param page_token: Token for retrieving a subsequent page (optional). :return: A QueryPvMetadataRequest for the specified params. """ - self.logger.debug("Building QueryPvMetadataRequest with %d criteria", len(criteria)) + self.logger.debug("Building QueryPvMetadataRequest with %d criteria", len(criteria) if criteria else 0) request = annotation_pb2.QueryPvMetadataRequest() - request.criteria.extend(criteria) + if criteria: + request.criteria.extend(criteria) if limit is not None: request.limit = limit if page_token: @@ -444,18 +446,25 @@ def _send_query_pv_metadata(self, request: annotation_pb2.QueryPvMetadataRequest def query_pv_metadata( self, - criteria: list[annotation_pb2.QueryPvMetadataRequest.QueryPvMetadataCriterion], + criteria: list[annotation_pb2.QueryPvMetadataRequest.QueryPvMetadataCriterion] | None = None, limit: int | None = None, page_token: str | None = None, ) -> QueryPvMetadataApiResult: """ User-facing method for invoking the queryPvMetadata() API method. Returns a single page of results; use iter_pv_metadata() to page through all results transparently. - :param criteria: List of QueryPvMetadataCriterion objects (see PvMetadataQuery helpers). + + An omitted or empty criteria list matches all records. The server's default page size still applies, so + this returns one page and a next-page token rather than the whole catalogue -- use iter_pv_metadata() to + browse everything. + + :param criteria: List of QueryPvMetadataCriterion objects (see PvMetadataQuery helpers), or None to + match all records. :param limit: Maximum number of records to return per page (optional). :param page_token: Token for retrieving a subsequent page (optional). :return: A QueryPvMetadataApiResult with a single page of results and status information. """ + criteria = criteria or [] self.logger.info("Starting queryPvMetadata operation with %d criteria", len(criteria)) request = self._build_query_pv_metadata_request(criteria, limit=limit, page_token=page_token) @@ -470,7 +479,7 @@ def query_pv_metadata( def iter_pv_metadata( self, - criteria: list[annotation_pb2.QueryPvMetadataRequest.QueryPvMetadataCriterion], + criteria: list[annotation_pb2.QueryPvMetadataRequest.QueryPvMetadataCriterion] | None = None, limit: int | None = None, ) -> Iterator[common_pb2.PvMetadata]: """ @@ -479,7 +488,11 @@ def iter_pv_metadata( Raises RuntimeError if any page returns an error, so callers can distinguish failure from an empty result set. - :param criteria: List of QueryPvMetadataCriterion objects (see PvMetadataQuery helpers). + Omit criteria (or pass an empty list) to browse the whole catalogue: an empty list matches all records, + and this generator pages through them all. Mind the size of the collection before doing so. + + :param criteria: List of QueryPvMetadataCriterion objects (see PvMetadataQuery helpers), or None to + match all records. :param limit: Maximum number of records to return per page (optional). :return: An iterator over all matching PvMetadata records across all pages. """ diff --git a/tests/unit/test_machine_config_activation_client.py b/tests/unit/test_machine_config_activation_client.py index 9a7daae..32fb111 100644 --- a/tests/unit/test_machine_config_activation_client.py +++ b/tests/unit/test_machine_config_activation_client.py @@ -419,6 +419,14 @@ def _result_response(self, ids, next_token=""): response.queryConfigurationActivationsResult.nextPageToken = next_token return response + def test_build_request_criteria_omitted_matches_all(self): + # An omitted or empty criteria list is match-all on the server (#41), not a rejection. + for request in ( + self.client._build_query_configuration_activations_request(), + self.client._build_query_configuration_activations_request([]), + ): + self.assertEqual(len(request.criteria), 0) + def test_build_request(self): criteria = [ConfigurationActivationQuery.configuration_name(["cfg-1"])] request = self.client._build_query_configuration_activations_request(criteria, limit=25, page_token="tok") diff --git a/tests/unit/test_machine_config_client.py b/tests/unit/test_machine_config_client.py index 7db0db7..66b069b 100644 --- a/tests/unit/test_machine_config_client.py +++ b/tests/unit/test_machine_config_client.py @@ -133,6 +133,14 @@ def test_build_delete_request(self): request = self.client._build_delete_configuration_request("cfg-1") self.assertEqual(request.configurationName, "cfg-1") + def test_build_query_request_criteria_omitted_matches_all(self): + # An omitted or empty criteria list is match-all on the server (#41), not a rejection. + for request in ( + self.client._build_query_configurations_request(), + self.client._build_query_configurations_request([]), + ): + self.assertEqual(len(request.criteria), 0) + def test_build_query_request_with_criteria_limit_token(self): criteria = [ ConfigurationQuery.name(prefix=["beamline-"]), diff --git a/tests/unit/test_pv_metadata_client.py b/tests/unit/test_pv_metadata_client.py index 1761352..a6eaf46 100644 --- a/tests/unit/test_pv_metadata_client.py +++ b/tests/unit/test_pv_metadata_client.py @@ -88,6 +88,14 @@ def test_build_query_request_limit_omitted_is_unset(self): self.client._build_query_pv_metadata_request([PvMetadataQuery.tags(["x"])]) self.assertEqual(assigned, [], "an omitted limit must not be assigned") + def test_build_query_request_criteria_omitted_matches_all(self): + # An omitted or empty criteria list is match-all on the server (#41), not a rejection. + for request in ( + self.client._build_query_pv_metadata_request(), + self.client._build_query_pv_metadata_request([]), + ): + self.assertEqual(len(request.criteria), 0) + def test_build_query_request(self): criteria = [ PvMetadataQuery.pv_name(prefix=["ABC:"]), @@ -458,6 +466,18 @@ def _page(self, pv_names, next_token): response.pvMetadataResult = result return response + def test_browse_all_with_no_criteria(self): + # The browse-all form: iter_pv_metadata() with no criteria pages through the whole catalogue (#41). + mock_stub = Mock() + mock_stub.queryPvMetadata.side_effect = [self._page(["ABC:1"], "tok1"), self._page(["ABC:2"], "")] + self.client._stub = mock_stub + + names = [pv.pvName for pv in self.client.iter_pv_metadata()] + + self.assertEqual(names, ["ABC:1", "ABC:2"]) + sent = mock_stub.queryPvMetadata.call_args_list[0].args[0] + self.assertEqual(len(sent.criteria), 0) + def test_pages_through_all_results(self): page1 = self._page(["ABC:1", "ABC:2"], "tok1") page2 = self._page(["ABC:3"], "") # empty next token = last page From facba6414e71f384702402d8a7b025d4caad6083 Mon Sep 17 00:00:00 2001 From: Craig McChesney Date: Thu, 10 Sep 2026 15:35:35 -0600 Subject: [PATCH 3/3] docs: refresh the #6 helper docstrings now that #40 has landed (Copilot review) DataSetQuery.attributes() and AnnotationQuery.attributes() still described themselves as differing from the older helpers "which require values", with relaxing those listed as pending issue #40 -- stale as of the previous commit in this PR, and contradicting the invariant CLAUDE.md now records. Both now state that every attribute helper behaves this way, with #40 named as the back-port that got them there. Also names the `key` argument in the pv-metadata cookbook's closing note rather than writing the helpers as bare `C.attributes()` / `PV.attr()`. Copilot read those as calls that would raise TypeError; they are inline prose references, not a fenced block, so nothing executes and the snippet checker never saw them -- but spelling them without their argument next to a real `Q.attributes("", ["0.49"])` call does read ambiguously. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_019he3UCsAnqTDE2VQ73Djwn --- doc/cookbook/pv-metadata.md | 3 ++- src/dp_python_lib/client/annotations_client.py | 4 ++-- src/dp_python_lib/client/dataset_client.py | 4 ++-- 3 files changed, 6 insertions(+), 5 deletions(-) diff --git a/doc/cookbook/pv-metadata.md b/doc/cookbook/pv-metadata.md index 1d0aeb3..7b21f42 100644 --- a/doc/cookbook/pv-metadata.md +++ b/doc/cookbook/pv-metadata.md @@ -381,7 +381,8 @@ This is useful for finding gaps in the catalogue. Combine it with a positive cr it, and the difference is what you need to fill in. The *key* is still required: `Q.attributes("", ["0.49"])` raises `ValueError`. The same holds for -`C.attributes()`, `CA.attributes()`, and the `PV.attr()` / `CFG.attr()` query selectors. +the `key` argument of `C.attributes`, `CA.attributes`, and the `PV.attr` / `CFG.attr` query +selectors. ### Other details diff --git a/src/dp_python_lib/client/annotations_client.py b/src/dp_python_lib/client/annotations_client.py index eb3fd03..387ac6e 100644 --- a/src/dp_python_lib/client/annotations_client.py +++ b/src/dp_python_lib/client/annotations_client.py @@ -193,8 +193,8 @@ def attributes( Builds a criterion matching annotations by attribute key and optional value(s). Omitting values (or passing an empty list) performs a key-only existence search: any annotation possessing - the key matches, whatever its value. This differs from the older PvMetadataQuery/ConfigurationQuery - helpers, which require values; relaxing those is issue #40. + the key matches, whatever its value. Every attribute helper in the library behaves this way (issue #40 + back-ported it to the older PvMetadataQuery/ConfigurationQuery helpers, which originally required values). :param key: Attribute key to match (maps to Attribute.name). :param values: Attribute values to match for that key, or None for a key-only existence search. diff --git a/src/dp_python_lib/client/dataset_client.py b/src/dp_python_lib/client/dataset_client.py index c62f468..03f217f 100644 --- a/src/dp_python_lib/client/dataset_client.py +++ b/src/dp_python_lib/client/dataset_client.py @@ -197,8 +197,8 @@ def attributes( Builds a criterion matching DataSets by attribute key and optional value(s). Omitting values (or passing an empty list) performs a key-only existence search: any DataSet possessing the - key matches, whatever its value. This differs from the older PvMetadataQuery/ConfigurationQuery helpers, - which require values; relaxing those is issue #40. + key matches, whatever its value. Every attribute helper in the library behaves this way (issue #40 + back-ported it to the older PvMetadataQuery/ConfigurationQuery helpers, which originally required values). :param key: Attribute key to match (maps to Attribute.name). :param values: Attribute values to match for that key, or None for a key-only existence search.