Repository navigation
Conversation
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the
 |
Mutation testing (cargo-mutants,
|
| g.jsonArrays = nil | ||
| named := map[string]string{} | ||
| for _, a := range g.TargetOptions.ElementArrays() { | ||
| name, err := jsonPathName(a.Path) |
There was a problem hiding this comment.
A root-array document that orders a member (ordered=$[*].name, which plainPath accepts and the engine orders) generated fine before this change (At.Name). Now ElementArrays yields the base $[*], jsonPathName("$[*]") skips the * step, gets an empty name and errors, so planJSON refuses the whole field with "the ordered path $[*] has no Go accessor name" — naming a path the user never wrote. Suggest naming a root array Root (as jsonPathName("$") does) or skipping the accessor rather than failing generation.
There was a problem hiding this comment.
Fixed in 7f403aa. A root array ($[*], or $[*][*]) is now Elements.Root, the same name jsonPathName("$") gives. The README says so. ordered=$[*].name;$[*].age generates At.Name as it did before, plus Elements.Root with the members name, age. A new case in TestRunGeneratesJSONSearchFields covers it.
Naming an array no longer stops generation for any reason. An array whose keys give no name ($.__[*]) is skipped with a notice. The notice names the ordered paths as the tag writes them ($.__[*].x), never the derived $[*].
| return fieldErr(f.typeName, g.GoName, "%v", err) | ||
| } | ||
| if prior, ok := named[name]; ok { | ||
| return fieldErr(f.typeName, g.GoName, "the arrays %s and %s both name the accessor Elements.%s; order members beneath one of them only", prior, a.Path, name) |
There was a problem hiding this comment.
This is a new refusal of a tag that generated before: ordered=$.a_b[*].x;$.aB[*].y has distinct At names (ABX, ABY) and was accepted, and now stops stashgen because the two arrays share the Elements name AB. The suggested remedy ("order members beneath one of them only") takes the orderings away from one array to free an accessor name. Consider skipping the Elements accessor for the colliding arrays (or de-duplicating the name) instead of failing the field.
There was a problem hiding this comment.
Fixed in 7f403aa. A tag that generated before still generates. jsonPathName does not disambiguate At names; it refuses a collision. A suffix chosen by order would rename an accessor whenever a path is added. So instead of renaming, the colliding arrays are skipped:
- Both arrays that share an
Elementsname, and any array whose keys give no name, get no accessor. stashgenexits 0 and prints a notice, e.g.stashgen: Customer.Profile: no Elements accessor for the elements of $.a_b[*] (ordered $.a_b[*].x): another array's keys give the same accessor name, Elements.AB…. The generated file repeats it in a comment at the top.Atis unchanged (ABX,ABY), and other arrays keep their accessors.
The tests generate ordered=$.a_b[*].x;$.aB[*].y;$.c[*].z and check exit 0, the unchanged At, Elements.C, the comment and the stderr notice. The bare $.a_b[*].x;$.aB[*].y tag generates with no Elements at all. The refusal case is gone, and the stashgen README describes the rule.
| // text is case- and accent-folded, so "sibling" equals "Sibling", and | ||
| // integers past 2^53 can tie. For an exact equality of one member, use the | ||
| // field's EqAt at the member's full path, which any element can answer. | ||
| func MemberEq(member string, v any) ElementPredicate { |
There was a problem hiding this comment.
Altitude: the member is a string checked at run time (gensupport.JSONElementMatch looks it up in ElementArrays), while stashgen already knows every ordered member beneath each array when it writes Elements.<Array>. The At.<Path> accessors exist precisely so a path is a compile-time identifier rather than a string (Go principle 1, "find mistakes at the earliest stage"). Generating a per-member predicate builder (e.g. Elements.Visits.Place.Eq("home"), Elements.Visits.At.Gt(15)) would make a misspelt or unordered member a compile error and remove the ErrTerm path and its orderedMembers message entirely.
There was a problem hiding this comment.
Not changed in this PR, and left to the owner to decide. #1442 specifies the string-member form.
Here is the trade-off. Generated per-member builders (Elements.Visits.Place.Eq(v)) would turn a misspelt or unordered member into a compile error. But the customer this serves builds the predicate list at run time: a filter compiler turns emergency_contacts:(relationship = "Sibling" AND previous_call_count > 5) into members it only knows as strings. For that caller, typed builders mean a member-name switch in their own code that stashgen would have to keep in step.
The string form keeps the predicates as plain data. It still refuses an unordered member before the engine is asked, naming the member, the array and the members the field orders. Both forms could also coexist, with the builders producing ElementPredicate values.
| if op == "between" { | ||
| keys = []string{"low", "high"} | ||
| } | ||
| if len(values) != len(keys) { |
There was a problem hiding this comment.
Dead branch: every constructor fixes the arity (between → 2 values, the rest → 1) and a zero ElementPredicate is already refused above, so len(values) != len(keys) is unreachable through the public API. Either drop it or, if kept as a guard, it deserves a comment saying so — as written it reads like a reachable user error.
There was a problem hiding this comment.
Removed in 7f403aa. The keys come from the op, and a comment above them says each constructor gives its op its values: between the low and the high, every other op one. A zero ElementPredicate, the only value without an op, is refused before this point.
| w.p("func new%s(f %s) %s {", g.fieldType, field, g.fieldType) | ||
| if ordered { | ||
| switch { | ||
| case ordered && !arrays: |
There was a problem hiding this comment.
The ordered && !arrays case and the ordered case emit the same At literal in two layouts; the first exists only to keep the single-line return X{field: f, At: XAt{...}} form. Emitting the multi-line form unconditionally (and Elements only when arrays) removes a duplicated loop and one case; gofmt accepts both.
There was a problem hiding this comment.
Fixed in 7f403aa. The constructor now writes the multi-line At literal whenever the field orders a path, and adds Elements only when there are arrays. The duplicated loop and the extra case are gone.
go generate ./... changes no committed file: every committed field that orders a path already had Elements. The TestRunGeneratesTypedJSONDocuments case with ordered=$.age covers the form without Elements.
|
|
||
| // Values are the predicate's values: one, or for between the low bound | ||
| // then the high. They are plaintext. For generated code. | ||
| func (p ElementPredicate) Values() []any { return p.values } |
There was a problem hiding this comment.
Values() returns the internal slice, so a caller can mutate a predicate after it was built (p.Values()[0] = x), and it exposes the plaintext on the public API for anyone, not only generated code. Returning slices.Clone(p.values) keeps the predicate immutable; alternatively keep the accessor unexported-by-convention (an internal interface gensupport asserts) so plaintext has one reader.
There was a problem hiding this comment.
Fixed in 7f403aa. Values() now returns slices.Clone(p.values), and its doc says the result is a copy. A test changes the returned slice and checks that the predicate is unchanged.
I kept the reader public rather than adding a hidden accessor. The SDK already exposes the plaintext a caller handed it in the same way (gensupport.JSONText, JSONValue.JSON, the NewJSONRead-style "for generated code" functions). The values are ones the caller passed to the constructor. Printing and logging still hide them.
There was a problem hiding this comment.
Still present at cb6a190: slices.Clone is shallow, so a json.RawMessage, []byte, map or slice passed to MemberEq is shared with the caller, and mutating it after construction changes the predicate (encrypt/json_elements.go:79). The "changing the copy does not change the predicate" doc holds only for scalar values.
Generated by Claude Code
There was a problem hiding this comment.
Fixed in 6ea438c (folded into the commit that adds ElementPredicate). The constructors and Values() now copy the bytes of a json.RawMessage or []byte value, so changing the caller's bytes after MemberEq and the others, or changing the bytes Values() returns, leaves the predicate unchanged. A value of any other reference type (a map, a slice or a pointer) is still shared. A general deep copy of any is not well defined, and the documented value types are numbers, strings and json.RawMessage. The type doc and the Values doc now say exactly that. TestElementPredicatesCopyTheirByteValues changes both the caller's bytes and the returned bytes for every constructor and checks that the predicate is unchanged.
ebb4c28 to
bb9a5c9
Compare
6a31ad7 to
500aa58
Compare
bb9a5c9 to
cb6a190
Compare
|
Rebased onto #1446's new head (500aa58); new head cb6a190. Rebase only: no change to this PR's own commits. Why: the three EQL PRs below changed under review (#1444's feedback, the move of the JSON tree SQLx tests onto documents Stack Encrypt seals, and the clippy fix in #1445 that gates |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
cipherstash-agent
left a comment
cipherstash-agent
left a comment
There was a problem hiding this comment.
Review attestation — automated /code-review medium plus cross-check against existing PR feedback, at cb6a190.
- Critical / blocking: none.
- New findings: none.
- Adds to existing: one minor reply on the
Values()thread (slices.Cloneis shallow, so reference-typed values stay mutable). - Dropped: four candidate findings. The
len(values)guard was already raised and removed in this PR. The member-spelling refusal is disclosed in the PR's review notes. The per-callElementArrays()regroup and thestrings.Trimcutset injsonArrayNameare unreachable or negligible today.
Not checked: CI results, and the Codex security review, which was still running.
Generated by Claude Code
500aa58 to
6109e38
Compare
…ield A filter such as "a contact who is a sibling and was called more than five times" had no Go method. An EqAt and an ordering at the contacts' paths can each be answered by a different contact, so the application had to read the array back and re-apply the predicate itself. The engine and EQL already answer it: element_match derives one eql_v4.query_json_elements operand, and json_tree_element_match is true of a row where one element passes every predicate. This wires it into the Go SDK. The EQL guest's resolver now names the elements domain, so the engine lists element_match (input path_predicates) on a field that orders a path, and the checker and stashgen read the operator. The compiled predicate domain stays unnamed: match is #1443's. stashgen groups the tag's ordered paths by the array their last wildcard names (record.TargetOptions.ElementArrays) and writes one accessor per array under Elements, named as At names a path, with Match. An array with no ordered member beneath it gets none, and a root array ($[*]) is Root. Two arrays whose keys give one name, or an array whose keys give none, get no accessor and a notice naming their ordered paths, so a tag that generated before still generates, with its At unchanged. Match takes encrypt.ElementPredicates made with MemberEq, MemberLt, MemberLte, MemberGt, MemberGte and MemberBetween, writes each value with gensupport.JSONText as the orderings do, and returns an eql.JSONSearchElementsQuery. The member check runs in the SDK, against the members the declaration orders beneath the array, before the guest is called: the engine's json_path_not_ordered names no member, so the first error a caller sees names the member, the array and the members it orders. It wraps ErrTerm, as the engine's refusal does; no predicates, a zero predicate and a member with a wildcard wrap ErrEncoding. Every other refusal is the engine's Diagnostic. Tests: the hermetic suite pins the operand against the subtree reads and the orderings of the same members, every pre-engine refusal, and each engine refusal's sentinel and code through the guest. The PostgreSQL suite (go:test:postgres) finds the row where one contact passes both predicates and not the row where two contacts each pass one, which the EqAt and ordering superset returns, and raises SQLSTATE 22023 past the bound. The guest test now pins the elements domain named and match not listed. Closes #1442 Claude-Session: https://claude.ai/code/session_01MrxG73fnX1p7oiizh3GRG5
The stashgen README's "Encrypted JSON documents" table gets the Match row and its SQL, with when Elements is generated, how a member is written, what is refused before the engine, the folded equality and the 22023 bound. The SDK README and package doc show the call, and the CHANGELOG records it under Unreleased. The typed-document example orders each visit's at and place, and finds the customer with one visit at home after 15: Ada has a visit at home and a visit after 15, but no one visit that is both. Its generated file is regenerated from the new tag. Refs #1442
cb6a190 to
52802ef
Compare
Summary
A Go program can now ask "does one array element of this encrypted JSON document pass several tests at once?", for example "a contact who is a sibling and was called more than five times". Before, the Go SDK could only ask each half on its own (
EqAtand an ordering at a path), and each half could be answered by a different contact, so the application had to read the array back, decrypt it and re-apply the filter by hand.Terms used below:
JsonSearchfield is a Go struct, map orjson.RawMessagefield stored as one encrypted JSON document in apublic.eql_v4_json_searchcolumn.ordered=$.contacts[*].calls) whose numbers and strings get an ordering term, so the database can compare them without decrypting.stashgenis the Go generator that writes the typedFieldsvalue from the struct tags.This is the Go half of #1440 and stacks on #1446 (which stacks on #1445 and #1444). It does not touch the engine or the EQL SQL.
Changes
Public Go API
JsonSearchfield,Fields.<Field>.Elements.<Array>.Match(ctx, cipher, predicates, opts): one accessor per array that has at least one ordered member beneath its wildcard, named asAtnames a path ($.extra.emergency_contacts[*]isElements.ExtraEmergencyContacts; a root array$[*]isElements.Root). Two arrays whose keys give one name, and an array whose keys give none, get no accessor:stashgenstill generates the field and itsAtunchanged, exits 0, and prints a notice naming the ordered paths as written, which the generated file repeats in a comment. So no tag that generated before this PR stops generating.Matchreturnseql.JSONSearchElementsQuery(already generated by feat(eql): one-parameter jsonpath predicates on json_tree documents (json_tree_match, @@, match) #1446), which binds as the one operand ofWHERE eql_v4.json_tree_element_match(col, $1::eql_v4.query_json_elements).encrypt.ElementPredicate/encrypt.ElementPredicates, made withencrypt.MemberEq,MemberLt,MemberLte,MemberGt,MemberGte,MemberBetween. Values are Go values written to JSON by the samegensupport.JSONTextthe orderings use. A predicate prints and logs its member and comparison, never its values;Values()returns a copy, so a built predicate cannot be changed.Refusals
ElementPredicate, a member with a wildcard (allencrypt.ErrEncoding), and a member whose full path the field does not order (encrypt.ErrTerm). The last one names the member, the array and the members the field orders there, because the engine's ownjson_path_not_orderednames none.*encrypt.Diagnostic, wrappingErrEncodingfor a data-plan reason andErrTermfor an index refusal, asSubtreeandAt.<P>.Ltalready surface theirs.JSONReadOptions.WildcardBoundkeeps the meaning it has forSubtree: 0 sends no bound and the engine's default, 64, applies. So the engine'sjson_wildcard_bound_zerocannot be reached throughMatch; it is still tested throughCipher.QueryTerm.Wiring
languages/golang/encrypt/guest/src/targets.rs): the EQL resolver nameseql_v4.query_json_elements. The compiled-predicate domain stays unnamed (matchis Go SDK: addMatch(ctx, cipher, predicate, vars)on a JsonSearch field, binding one jsonpath-style predicate as one parameter, on top of #1441 #1443). The guest test now asserts the elements domain is named,element_matchis listed on a field that orders a path, andmatchis not listed.parseOperatorNameand the operator input parser acceptelement_match/path_predicates; stashgen'sjsonOperatorstoo.record.TargetOptions.ElementArraysgroups ordered paths by the array their last wildcard names; stashgen uses it to decide which accessors to write, and gensupport uses it to check members at run time.element_match's data-plan refusals areError::Source(encoding) and its index refusalsTermError(term); none falls into the internal catch-all.Tests
Subtreetokens of the full path at the same bound (bound + 1, the probe last);eqisgteandlteat one value and equalsAt.<P>.Gte;gtequalsAt.<P>.Gt;betweenequalsAt.<P>.Between; Go values and their JSON text derive the same operand; the default bound gives 65 tokens. Every pre-engine refusal returns its sentinel with noDiagnostic. Every engine refusal listed in feat(eql): test equality and ranges on one array element of a json_tree document #1445 comes back through the guest with its sentinel and its reason or code.go:test:postgres, newTestJSONElementMatchInPostgres): one family has a sibling and, separately, a contact called more than five times; another has one contact who is both.Matchreturns only the second; theEqAt+ ordering superset returns both. Also a range, three members, folded equality ("sibling"matches,EqAtdoes not), a bound of 1 raising SQLSTATE 22023, and an unordered member refused before the engine.$.tags[*],$.age), named asAtnames paths, a root array namedRoot, colliding or unnameable arrays skipped with a notice (exit 0,Atunchanged), and an engine that answers noelement_matchrefused.testusers.Entity, mirroring Go SDK: no way to test an equality and a range on the same array element of a JsonSearch field — add an element-scopedMatchon top of #1440 #1442's example;customer_stash.goandsubscriber_stash.goare regenerated.Docs
cmd/stashgen/README.md"Encrypted JSON documents": aMatchrow with its SQL, and a section on whenElementsexists, the folded equality and the 22023 bound.encrypt/README.md,encrypt/doc.go,languages/golang/CHANGELOG.md(the Go module records changes there, not in changesets).encrypt/example/json) now orders each visit'satandplace, and finds the one customer with a visit at home after 15. Its generated file is regenerated.Verification
mise run wasm:guest:test: fmt, clippy (8 feature/target combinations), rustdoc, nextest 73 passed (default) and 77 passed (eql).wasm:guest:build,wasm:auth-guest:build,wasm:guest:build:deterministic,wasm:guest:build:eql,wasm:guest:build:eql:deterministic.mise run go:test: every package ok.go test -json ./...: 545 passed, 12 skipped (the 6 PostgreSQL tests withoutDATABASE_URL, 4 live ZeroKMS tests without credentials, 2 platform-specific memory tests).mise run go:test:postgresagainst a freshpostgres:17container with the EQL v4 bundle built from this tree: all 6 tests passed, including the new one.go vet ./...,gofmt -l .(empty),mise run go:lint(0 issues),go generate ./...leaves the tree clean.Related
Closes #1442
Stacked on #1446.
Review notes
Elements.Visits.Place.Eq(v)) were suggested in review and are not in this PR: Go SDK: no way to test an equality and a range on the same array element of a JsonSearch field — add an element-scopedMatchon top of #1440 #1442 specifies the string-member form, which a filter compiler can build at run time. Left to the owner.encrypt/gensupport/json.go(JSONElementMatch),stashgen/emit.go(planJSON,jsonElements),internal/record/record.go(ElementArrays).relationship,address.city). A member spelled another way (["relationship"]) is refused as not ordered, though the engine would accept it. Tag paths are plain JSONPath, so the tag spelling is the only one a caller needs.Match(ctx, cipher, predicate, vars)on a JsonSearch field, binding one jsonpath-style predicate as one parameter, on top of #1441 #1443: thematchoperator and itspredicateinput shape, namingeql_v4.query_json_predicatein the guest, mappingError::Predicateto an input status in the guest'sstatus.rs(it falls into the internal catch-all today), and flipping the guest test'smatchassertion.