Repository navigation
feat: add metadata_raw_search for the raw metadata search endpoint - #331
Conversation
The server-side API gained POST /v3/search/metadata/raw, which runs a caller-authored OpenSearch query body and overlays the tenant filter itself. This is the client for it. The body is passed through untouched and the community travels in the query string, which is why the method uses the inline-dict _single form rather than the generic builder: on a POST the builder puts every parameter in the JSON body, and the server reads community/communities out of that body when it resolves scope. A community folded into the body would therefore choose the scope via a key the server's own grammar then rejects. A POST is never paginated, so _single returns the one composite result - hits and aggregations together - rather than a generator. Tests are in the respx tier rather than the e2e one: what they pin is that the body arrives verbatim and the community is not in it, and neither is visible from the far side of a live call. The live lifecycle test follows separately.
A VCR lifecycle test per transport, each with its own cassette recorded against a freshly booted stack running the matching server branch. Both replay offline for the unit jobs and both pass with TESTS_VCR=off against the live stack, which is the invariant that keeps the cassettes an efficiency cache rather than the thing under test. The query aggregates on `extension` rather than a mimetype field: the metadata document has no artifact.mimetype at all (the mimetype it does carry is scan.mimetype), and a terms aggregation on an unmapped field is not an error - it returns an empty bucket list, so the assertion fails with nothing to say why. The respx example moved to scan.mimetype for the same reason, even though a mock would have accepted either. Recording these needed the stack's ai-es-dispatch-pending step: it is the only thing that turns a claimed es_metadata row into an Elasticsearch write, so without it the metadata index is never created and every metadata test polls an empty corpus until it gives up.
ReviewThe code is correct. The inline 1. Spec drift:
2. Tests are in the wrong tier for request-shape checks (
|
poll_equals absorbs NotFoundException as "the index has not caught up". That is right for a document and wrong for an endpoint: a search matching nothing is a 200 with total 0, so a 404 from this route means the server side is not deployed. Absorbed as lag it burned 90 polls and then failed as `assert None == 1`, naming nothing - and because the e2e job runs with TESTS_VCR=off against a live stack, that is what develop would do if this landed before the server release, blocking the :latest promotion every other repo's pipeline pulls. The reader now skips with the reason instead. It costs no extra request - the first poll is the probe - so the cassettes still replay. Drop it once the route ships. Also document two things the method's contract depends on and its docstring left out: omitting `size` returns no documents, because the server defaults it to 0 so an aggregation caller does not pay for hits they did not ask for; and `partial` is a list of reasons, whose absence - not an empty `hits` - is what says a count is whole.
|
Review against AGENTS.md / specs. Base 1. Spec drift:
2. E2e tests skip on any 404, which can hide SDK regressions ( 3. The docstring and the tests disagree on the shape of |
The docstring told callers that partial's absence means a count is whole. It does not: the server suppresses the hit-total signal when the body asked for no hits, because a total capped at track_total_hits is what every large query returns and reporting it would mark all of them partial. Exactness of a count is hits.total.relation - eq or gte - which is the engine's own field and passes through untouched. partial speaks for the aggregations and the search as a whole.
|
Review against AGENTS.md and
Gitflow is fine: the PR targets |
…specs AGENTS.md step 5 asks for specs/03-endpoints.md in the same PR as a new resource, and the class tree in specs/02-resources.md lists every BaseJsonResource. Both omitted this one.
|
Review against AGENTS.md / specs. Base
|
The e2e job runs TESTS_VCR=off against a live stack, so a skip let a renamed or withdrawn route report green and take the :latest promotion with it. AGENTS.md already orders the server endpoint first, so a 404 is a real failure rather than a merge-ordering inconvenience to absorb. The early exit stays: it still names the cause immediately instead of spending 90 polls to fail as `assert None == 1`. Renamed to _raw_hit_count, since it no longer skips.
The fixture mocked a dict, so the test demonstrated a shape the server never sends and the docstring does not promise.
|
Checked against AGENTS.md and specs/02, 03, 04. The endpoint code is correct: the body goes through unchanged, 1. Request-shape tests are in the wrong tier, because there is no builder (spec drift, specs/04 invariant 7). class MetadataRawSearch(core.BaseJsonResource):
RESOURCE_ENDPOINT = '/search/metadata/raw'
@classmethod
def search(cls, api, body, community=None):
return core.PolyswarmRequest(api=api, method='POST',
url=f'{api.uri}{cls.RESOURCE_ENDPOINT}',
params={'community': community or api.community},
json=body, result_parser=cls)Then 2. Gitflow / hygiene: the head branch Not blocking: no version bump is correct here, since this is an additive method and the CLI pins published releases. Base |
…-unit AGENTS.md routes a new resource through a builder classmethod on the resource, and specs/04 invariant 7 puts request-shape assertions in the pure-unit tier. The inline request dict forced the opposite: body-vs-query routing could only be observed through respx, which that invariant reserves for what the e2e stack cannot produce. MetadataRawSearch.search returns the PolyswarmRequest, so the routing reads off request_parameters with no httpx and no transport parametrisation. A mutation folding the community into the body kills three of the new tests.
|
The code is clean against AGENTS.md and specs/. The
|
The docstring stated the exception as the rule: it said a capped total is not reported as partial, when the server reports exactly that whenever the body asked for hits, and omits it only when it asked for none. A caller following the old text would read a complete aggregation as truncated and re-run a narrowed query, paying for another metered search that can never clear the marker.
|
Code, spec updates (
|
The two paragraphs contradicted each other: one said partial speaks for the search as a whole, the next that a large query normally carries one. Both are true of different reasons, which is the point - so the docstring now names the closed set of codes and says to branch on them. The reasons are prose no program can discriminate, which the server fixed by giving each a code. Also drops a stray blank line before _raw_hit_count.
|
Checked against AGENTS.md and specs 02, 03 and 04. The code is correct and follows the conventions. The builder is pure and keeps
The merge order in the PR description is important (entitlement service first, then the server, then this PR). Merging to |
AGENTS.md asks for the spec in the same PR as the code, and the Files list in specs/04-testing.md names every test module. specs/02 and 03 were updated for this endpoint; 04 was missed.
|
Reviewed against AGENTS.md and specs/02, 03, 04, and 05. The code is clean: One thing to fix before merge:
|
TL;DR
metadata_raw_search(body, community=None)to both clients, for the new server-sidePOST /v3/search/metadata/raw.hitsandaggregationstogether — not a generator.develop → masterstep).The endpoint
The server runs a caller-authored OpenSearch query body against the metadata corpus and overlays the tenant filter itself. It accepts a closed grammar — anything it does not recognise is refused rather than passed through — and it is entitlement-gated and metered per query, including a query it refuses after running.
Callers get
result.json['hits']andresult.json['aggregations'], plusresult.json['partial']when the server could not establish that the answer is complete — a list of every reason that fired (a timeout, a lost shard, a clamped bucket list). Apartialmarker is the only thing separating a truncated answer from a whole one — both are a 200 — so it is deliberately preserved rather than flattened away, and a test pins that.Two contract details the docstring now states, because neither is guessable. Omitting
sizereturns no documents — the server defaults it to 0, so an aggregation caller does not pay for hits they did not ask for.And
partialdoes not speak for the hit count. The server suppresses that signal when the body asked for no hits, because a total capped attrack_total_hitsis what every large query returns and reporting it would mark all of them partial. Exactness of a count ishits.total.relation—eqorgte— the engine's own field, passed through untouched.partialspeaks for the aggregations and the search as a whole. Reading its absence as "this count is exact" would be wrong, which is why the docstring now separates the two.Why a hand-written builder instead of the generic one
This is the one design choice worth a reviewer's time. On a POST, the generic builder puts every parameter into the JSON body. That cannot express
?community=…, and it is not merely inconvenient: the server readscommunity/communitiesout of the POST body when it resolves scope, while its grammar rejects those keys. A community folded into the body would therefore select the scope via a key the server then refuses — scope chosen by one layer and rejected by the next.So
MetadataRawSearch.searchsetsparamsandjsonseparately, the way 34 other resource builders already do: body stays a pure_searchdocument, control stays in the query string. Keeping the builder on the resource is also what makes the routing assertable without httpx — see Testing.A POST is never paginated (
core.pytestsmethod == 'GET'), so_singlereturns the single composite envelope rather than a generator — which is what we want here, since hits and aggregations arrive together.Testing
Request shape — pure-unit tier (
specs/04-testing.mdinvariant 7). What these pin is that the body arrives byte-for-byte and that the community is not in it. Neither is visible from the far side of a live call, which only shows that some scoped search ran. They readrequest_parameters, the projection of what is sent —PolyswarmRequest.jsonis reused for the parsed response and isNoneuntil the call runs. A mutation folding the community into the body kills three of them.Lifecycle — live e2e, one cassette per transport. Recorded against a freshly booted stack running the matching server branch, verified to pass with
TESTS_VCR=offagainst that stack and to replay offline for the unit jobs. Full suite: 410 passed; unasync mirror regenerates byte-identical.Two things found while recording, both worth knowing:
artifact.mimetype— the mimetype it carries isscan.mimetype. A terms aggregation on an unmapped field is not an error; it returns an empty bucket list, so the test failed with nothing to say why. The query now aggregates onextension, which is present and aggregatable.metadata-*index is ever created, and every metadata test — including the pre-existingtest_metadata_search— polls an empty corpus until it gives up. Worth remembering when a local stack is brought up with commands disabled.Requires — merge order is load-bearing
Merge in this order. Merging this PR first turns the e2e stage red in every repo, not just here.
:latest, the entitlement check answers 400, which the server turns into aBadResponseExceptionon every call.POST /v3/search/metadata/raw. Until it is on:latest, the route 404s.Why the blast radius is wider than this repo: merging to
developpromotes the SDK test image to:latest, and "every other repo's e2e pipeline resolvespolyswarm-api:latestby default" (AGENTS.md). That image runs this whole suite withTESTS_VCR=off(docker/Dockerfile) against a stack whose other services are their:latest. So the two new live tests would POST a route that stack does not serve. This PR's own e2e is green because the identical branch name pairs it with the matching server image — a pairing unrelated pipelines do not get.AGENTS.md already states the rule this follows: "New endpoints land there first; the SDK PR comes after."
The live tests fail rather than skip on a 404, deliberately.
poll_equalstreatsNotFoundExceptionas "the index has not caught up", which is right for a document and wrong for an endpoint — a search matching nothing is a 200 with total 0, so a 404 means the route is absent. Absorbed as lag it burned 90 polls and then failed asassert None == 1, naming nothing. A skip would be worse than either: the e2e job runsTESTS_VCR=off, so a renamed or withdrawn route would report green forever and take the:latestpromotion with it. Failing is what makes the ordering above enforce itself instead of rotting into a permanently-skipped test. It costs no extra request — the first poll is the probe — so the cassettes still replay.Only the live tier depends on any of this; the pure-unit tier and the recorded cassettes are self-contained.
Merging
Squash-merge this one, with a written message rather than the default. Two things otherwise reach the public history on
develop: the head branch name carries an internal ticket ID, and GitHub's default squash message concatenates every commit body — several of which describe intermediate states this branch later reversed. The first commit's inline-dict_singleform became a builder in3d2bfbe, and the reader that "skips with the reason" became a hard failure inc351e76. Each body is accurate for the commit it belongs to; concatenated into one message they read as a description of the final state, which they are not.The PR title plus a two-line summary is enough.