Repository navigation
Conversation
|
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueComment |
Mutation testing (cargo-mutants,
|
|
|
||
| ### Changed | ||
|
|
||
| - **Every `encrypt/eql` type prints and logs redacted (#1443).** `String`, |
There was a problem hiding this comment.
Redacted fmt output is a silent behaviour change and reads as Breaking, not Changed. Before this PR every encrypt/eql type was a bare []byte to fmt, so fmt.Sprint(q), %s, %v and %x all yielded the JSON text. Any caller that relied on that (string-building SQL, json.RawMessage(fmt.Sprint(q)), logging a value for debugging, a test fixture comparing %s output) now gets eql.X{[redacted]} with no error and no compile failure. Consider listing it under Breaking so a reader scanning for upgrade hazards sees it.
There was a problem hiding this comment.
Moved to Breaking in 5902c80. The entry now says what changed for a caller: %s and %q used to give the JSON (fmt.Sprint, %v and %x gave the bytes as numbers or hex), and all of them now give the redacted text with no error.
To get the JSON, convert the value (string(v), json.RawMessage(v)) or call v.MarshalJSON(); Value is unchanged too. There is no named accessor method for the JSON, because the value itself is the JSON, and the entry says so. The eql package doc says the same, and TestValuesPrintAndLogRedacted checks that the conversion still gives the JSON.
| } | ||
| // A nil map, slice or pointer, or an empty json.RawMessage, which | ||
| // encoding/json writes as null: no variables. | ||
| if trimmed := bytes.TrimSpace(text); len(trimmed) != 0 && !bytes.Equal(trimmed, []byte("null")) { |
There was a problem hiding this comment.
A json.RawMessage that is only whitespace (json.RawMessage(" ")) trims to empty and is silently treated as "no variables", rather than being refused as not-JSON the way json.RawMessage("{") is. With a predicate that names $v the failure then surfaces as json_predicate_unknown_variable, pointing the caller at the predicate rather than the vars. Only an empty message (len(text) == 0) and a literal null mean "none"; whitespace-only should still go to the engine.
There was a problem hiding this comment.
Fixed in e6fe661. Only these mean "no variables": a nil vars, a nil map, slice or pointer (which encoding/json writes as null), or a json.RawMessage that is the literal null after trimming whitespace. Every other text goes to the engine, so an empty, nil or whitespace-only json.RawMessage is refused as json_syntax, the same way { is. A nil json.RawMessage is sent as empty bytes rather than left out.
New cases in TestMatchRefusalsNameTheConstructAndItsOffset cover json.RawMessage{}, json.RawMessage(nil) and " \t\n ". The cases where no variables are sent now include a nil struct pointer, null and " null\n".
| // for a refused Match predicate, where the refused construct begins in the | ||
| // predicate string. ok is false for an error that gives no offset. | ||
| func (d *Diagnostic) At() (offset int, ok bool) { | ||
| at, isOffset := d.Fields["at"].(uint64) |
There was a problem hiding this comment.
At() answers only when the transport delivered a uint64. The guest writes (*at as u64) today, but the value transport also carries UInt32, Int32 and Int64, and vcffi.Unmarshal returns each as its own Go type. If the encoder ever narrows a small offset (or a later guest sends usize through a different conversion), At() returns ok == false and the caller silently loses the offset; TestConstructAndAtReadARefusedPredicate even pins rejection of int64(4). Accepting any non-negative integer kind (uint32, uint64, int32, int64) would make this robust to the encoder's width choice.
There was a problem hiding this comment.
Fixed in e6fe661. At() now reads every Go integer type: int, int8 to int64, and uint to uint64. It also reads a float32 or float64 that holds a whole number below 2^53. vcffi.Unmarshal can return int32, int64, uint32, uint64, float32 and float64, and its scalar path int8, int16 and uint16.
It refuses a negative, a fraction, NaN, infinity, a value past int, and anything that is not a number. The test now checks that all twelve numeric types read 4, where it used to check that int64(4) was refused, and it checks each refusal.
| } | ||
| input := vcvalue.Object{{Key: "predicate", Value: predicate}} | ||
| if vars != nil { | ||
| if t := throughPointer(vars); t != nil && t.Kind() == reflect.String && writesItself(vars) == "" { |
There was a problem hiding this comment.
This duplicates JSONQuery's Go-string refusal (throughPointer(...).Kind() == reflect.String && writesItself(...) == "") with a near-identical message. A shared helper, e.g. func isGoString(v any) bool, used by both keeps the two rules from drifting the next time one of them learns a new case (a named string type with MarshalText, say).
There was a problem hiding this comment.
Fixed in e6fe661. isGoString(v any) bool in gensupport is now the one rule: a string kind, or a pointer to one, with no MarshalJSON or MarshalText of its own. JSONQuery (for the needle) and JSONMatch (for vars) both call it.
|
|
||
| // logValue is what slog records of an EQL value: [redacted], never its | ||
| // bytes. | ||
| func logValue([]byte) slog.Value { |
There was a problem hiding this comment.
logValue ignores its argument, so a log line records "q":"[redacted]" with no type name, while String() on the same value gives eql.JSONSearchPredicateQuery{[redacted]} and an empty value prints eql.X{}. Returning slog.StringValue(text(b, kind)) would make the two consistent and drop the unused parameter. Separately, once Format exists fmt never reaches GoString (Formatter wins for %#v too), so the generated GoString is reachable only by direct call.
There was a problem hiding this comment.
Fixed in 2c469ff, in the generator, and eql_gen.go is regenerated rather than hand-edited.
LogValuenow returnslogValue(v, "X"), which isslog.StringValue(text(b, kind)): the same textStringgives, type name included, andeql.X{}when empty.- The generated
GoStringis gone.Formatanswers every verb,%#vincluded, soString,FormatandLogValueall write one text.
The codegen test checks the three methods on every type and that no GoString is written. The Go tests check the slog records, "email":"eql.TextEq{[redacted]}" and the empty eql.TextEq{}. types:check and codegen:parity pass.
| - `&&`, `||`, `!(…)`, parentheses and `/* */` comments. Keywords are read in any case. | ||
|
|
||
| An ordering (`<` and the rest) needs its path in `ordered=`, and so does every member of a filter of more than one condition, which compiles to the element match above. | ||
| Anything else is refused before any term is derived, as an `*encrypt.Diagnostic` wrapping `encrypt.ErrEncoding`: `Reason()` says why (`json_predicate_syntax`, `json_predicate_unsupported`, `json_predicate_unknown_variable`, `json_predicate_value_type`, `json_predicate_not_equal_at_wildcard`, `json_predicate_too_deep`, `json_path_position`, `json_path_no_wildcard`, `json_path_wildcard`), `Construct()` names the construct in the grammar's own words (`` `like_regex` ``, `a position (`[2]`)`), and `At()` is the byte offset in the predicate where it starts. |
There was a problem hiding this comment.
The Reason() list is missing two reasons the tests pin: json_syntax (a literal JSON cannot hold such as 1e999, or vars that are not JSON) and json_input (vars that are not an object). A caller that switches on this list to classify a refusal will treat both as unknown.
There was a problem hiding this comment.
Fixed in 5902c80. The reasons are now a list, one line each, with json_syntax and json_input added:
json_syntax: a literal that does not parse (1e999, which has an offset), orvarsthat are not JSON, including an empty or whitespace-onlyjson.RawMessage(no offset).json_input:varsthat are not an object.
| # A -run pattern that matches no test (one renamed, say) prints "no tests | ||
| # to run" and PASS, so each test has to be seen to pass by name. | ||
| for test in TestEQLTypesRoundTripAndQueryInPostgres TestEQLOrderMatchesPlaintextInPostgres TestQueriesRunAgainstPostgres TestJSONDocumentsQueryAndReadInPostgres TestJSONTypedDocumentsInPostgres TestJSONElementMatchInPostgres; do | ||
| for test in TestEQLTypesRoundTripAndQueryInPostgres TestEQLOrderMatchesPlaintextInPostgres TestQueriesRunAgainstPostgres TestJSONDocumentsQueryAndReadInPostgres TestJSONTypedDocumentsInPostgres TestJSONElementMatchInPostgres TestJSONPredicateMatchInPostgres; do |
There was a problem hiding this comment.
The seven test names now live in two places: the -run regex a few lines up and this loop. They have to agree exactly or the regex runs a test the loop never checks (or vice versa). Deriving one from the other, e.g. TESTS="A B C" then -run "^($(echo $TESTS | tr ' ' '|'))$", removes the second copy.
There was a problem hiding this comment.
Fixed in e6fe661. The task now names the tests once, in a bash array TESTS=(...). The -run pattern is built from it, "^($(IFS='|'; echo "${TESTS[*]}"))$", and the pass-by-name loop walks "${TESTS[@]}".
I ran mise run go:test:postgres with it against a fresh container, and all seven tests ran and were each seen to pass by name.
| w.p("\treturn gensupport.JSONNodeToken(ctx, f.field, c, \"child_count\", path)") | ||
| w.p("}") | ||
| } | ||
| if answered["match"] { |
There was a problem hiding this comment.
This guard is dead: planJSON now refuses any document field the engine answers no match on before jsonEntry runs, so answered["match"] is always true here. Either drop the condition (and emit Match unconditionally, as the comment in planJSON says) or drop the refusal in planJSON -- keeping both leaves two places that disagree about which one is the rule.
There was a problem hiding this comment.
| panic!("a query term is an object") | ||
| }; | ||
| assert!( | ||
| matches!(&term[3].1, Tree::String(s) if s.as_bytes() == b"eql_v4.query_json_predicate") |
There was a problem hiding this comment.
term[3].1 and term[4].1 pin the positional order of the query-term object's fields. A field added or reordered in query_term's output panics with an index error rather than failing on the named field. Looking the entry up by key (term.iter().find(|(k, _)| k == b"sql_domain")) would keep the assertion about the domain name instead of the position.
There was a problem hiding this comment.
Fixed in e6fe661. The test now finds each entry by its key with term.iter().find(|(k, _)| k == key) and checks operator, domain and encoding. A missing key, or a value that is not a string, fails with the key's name.
This only changes the guest's test module, so the built guests did not need rebuilding. mise run wasm:guest:test passes (73 default, 78 eql).
95a22e9 to
5902c80
Compare
bb9a5c9 to
cb6a190
Compare
A Go EQL value is the JSON its column stores: a stored value holds ciphertext and terms, and a query value holds terms. Until now each type was a bare []byte to fmt and slog, so %s or a log line wrote those bytes out. The compiled predicate Match returns (#1443) is the first operand a caller is likely to log whole, so every generated type now gets String, Format and LogValue, written by eql-codegen beside Value and Scan so no type can be missed. All three write one text, the type's name and [redacted] (or {} when empty); Format answers every verb, %#v included, so no GoString is generated. Value, MarshalJSON and a []byte or string conversion still give the JSON. The helpers (text, format, logValue) are hand-written in eql.go, so the generated methods stay one-line block bodies gofmt leaves alone. A codegen test checks every `type X []byte` the file declares has all three.
…meter An application that compiles a filter language into SQL targets jsonb_path_match(col, $1, $2): the predicate and its variables, two parameters. On an encrypted column the same filter was one Go call per leaf and a hand-assembled WHERE, one cast per parameter. The engine's match operator (#1441) compiles the predicate into one operand; this is the Go call on top (#1443). Fields.<Field>.Match(ctx, cipher, predicate, vars, opts) is generated on every JsonSearch field and returns eql.JSONSearchPredicateQuery, which binds as col @@ $1::eql_v4.query_json_predicate or eql_v4.json_tree_match(col, $1::...). The predicate crosses as text and vars as JSON text written by gensupport.JSONText, so Go adds no grammar: nil, or a value that writes null, sends no vars, and a Go string is refused as a Contains needle is. opts.WildcardBound bounds the filters that compile to the element match, as Elements.<Array>.Match takes it. The guest now names eql_v4.query_json_predicate, so the engine lists match (input predicate) on every JsonSearch field; the checker and stashgen read it, and stashgen refuses a document field the engine answers no match on. The guest maps Error::Predicate to STATUS_ENCODING: a refused predicate is the caller's input, not an internal failure. Diagnostic gains Construct and At, which read the construct and the byte offset the refusal carries. testusers.Entity grows the RC2 model's members (score, weight, address, country, preferred languages; $.weight ordered). The PostgreSQL test runs the seven filters of the customer's request and checks @@, json_tree_match, the per-operator query and jsonb_path_match over the plaintexts return the same rows; it joins go:test:postgres.
The stashgen README's "Encrypted JSON documents" table gains a Match row with both SQL forms, and a section on the grammar, the refusals and what Diagnostic's Reason, Construct and At give, the four places the answer parts from jsonb_path_match, the 22023 bound, and the index the per-operator form engages that json_tree_match does not. encrypt/README.md and doc.go say the same in short. The typed-document example finds Bea by one predicate that combines an ordering with the visit filter. The Go module's CHANGELOG records Match, the Diagnostic accessors and the redacted EQL types (#1443).
5902c80 to
be8052a
Compare
|
Rebased onto #1451's new head (cb6a190); new head be8052a. Rebase only: no change to this PR's own commits. Why: the 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 |
Summary
A Go program can now send a whole JSON filter to an encrypted column as one SQL parameter, the way it would call
jsonb_path_match(col, $1, $2)on a plaintextjsonbcolumn. Before, each part of the filter was its own Go call, its own parameter and its own cast, and the application had to assemble theWHEREclause by hand. A filter compiler (such as a customer's AIP-160 engine, which turns Google's list-filter syntax into SQL) can now emit one template for both.Terms used below:
JsonSearchfield is a Go struct, map orjson.RawMessagefield stored as one encrypted JSON document in apublic.eql_v4_json_searchcolumn.$.country == $val && $.weight < $max. Its variables (vars) are the values named$valand$max.ordered=$.weight) whose numbers and strings get an ordering term, so the database can compare them without decrypting.This is the Go half of #1441 and the last issue of the "JSONB path match on encrypted JSON" milestone. It stacks on #1451 (which stacks on #1446, #1445 and #1444). It does not change the engine or the EQL SQL.
Changes
Public Go API
JsonSearchfield:Fields.<Field>.Match(ctx, cipher, predicate string, vars any, opts encrypt.JSONReadOptions) (eql.JSONSearchPredicateQuery, error).col @@ $1::eql_v4.query_json_predicate, oreql_v4.json_tree_match(col, $1::eql_v4.query_json_predicate). Both forms return the same rows.varsis any valueencoding/jsonwrites as a JSON object: a map, a struct, or ajson.RawMessageof one. It is written by the samegensupport.JSONTextthe other methods use.nil, a nil map, slice or pointer (which encoding/json writes asnull), or ajson.RawMessagethat is the literalnullsends no variables.json.RawMessageis refused asjson_syntax, as{is.Containsneedle, because it writes one JSON string, not an object. One helper,isGoString, holds that rule for both.opts.WildcardBoundworks as it does forElements.<Array>.Match: 0 means the engine's default, 64.EqAt,Selector,At.<Path>andElements.<Array>.Matchalready derive.Errors
*encrypt.Diagnosticwrappingencrypt.ErrEncoding, with codestack_encrypt::dynamic_predicate. It is refused; it is never compiled to a tree that silently drops a leaf.Diagnostic.Construct()andDiagnostic.At()read the two fields the engine already sent.Construct()names the refused construct in the grammar's own fixed words (`like_regex`,a position ([2])).At()is the byte offset in the predicate where that construct starts; it reads the offset in any integer width the transport carries, or a float holding a whole number, and refuses a negative or fractional one.Reason()says why (json_predicate_unsupportedand the others). None of them carries text from the predicate or a variable's value.encrypt.ErrTermwith codestack_encrypt::json_path_not_ordered, as the per-path orderings report it.Guest and wiring
encrypt/guest/src/targets.rs) now names theeql_v4.query_json_predicatedomain. The engine therefore listsmatch(inputpredicate) on everyJsonSearchfield, with or without ordered paths.parseOperatorNameand the operator input parser inchecker.goacceptmatchandpredicate. So does stashgen'sjsonOperators.status.rsmapsError::PredicatetoSTATUS_ENCODING(bad input). Before this, it fell into the catch-all and a refused predicate was reported asErrInternal.Matchon everyJsonSearchfield. It stops with an error if the engine lists nomatchfor a document field: that would mean the generator and its engine disagree, the same rule feat(go): test several members of one array element of a JsonSearch field #1451 applies to a missingelement_match.Generated EQL types print redacted (outside the issue's strict scope; see Review notes)
encrypt/eqlnow hasString,FormatandLogValue, which all write one text:eql.TextEq{[redacted]}, oreql.TextEq{}when empty, and never the bytes.Formatanswers every verb,%#vincluded, so noGoStringis generated. Before this, every one of them was a bare[]bytetofmtandslog.eql.go.%sused to give a value's JSON. To get the JSON, convert the value (string(v),json.RawMessage(v)) or callMarshalJSON;Valueis unchanged. There is no named accessor method, because the value itself is the JSON.Tests
json_match_test.go, deterministic guest):and(contains, exists, not(contains), cmp.lt), and each leaf equals the operand its own method derives (EqAt,At.Weight.Selector,EqAtwithnil,At.Weight.Lt).Elements.ExtraEmergencyContacts.Matchat the same bound.varsgiven as a map, a struct, a pointer to a struct, ajson.RawMessageand amap[string]json.RawMessageall derive the same operand. With literals in place of variables,nil, a nil map, a nil struct pointer,nulland" null\n"asjson.RawMessage, and{}derive it too. The comparison treats a containment needle'ssv/ellists as sets, because the engine shuffles them on every query.json_predicate_syntax,json_predicate_unsupported(5 constructs),json_predicate_unknown_variable,json_predicate_value_type(2),json_predicate_not_equal_at_wildcard,json_predicate_too_deep,json_path_position,json_path_no_wildcard,json_path_wildcardandjson_syntax.varsthat are not an object (json_input);varsthat are not JSON, an empty, nil or whitespace-onlyjson.RawMessageincluded (json_syntax);json_path_not_orderedfor a range and for an unordered filter member; a Go stringvars, before the engine is asked; andjson_wildcard_bound_zero, throughQueryTerm.TestJSONPredicateMatchInPostgres, added togo:test:postgres, whose tests are now named once in aTESTSarray that both the-runpattern and the pass-by-name check come from): the seven filters of the customer's request, with the predicates from the RC2 note written verbatim. Each runs four ways:@@,json_tree_match, the hand-written per-operator query, andjsonb_path_matchover the plaintext (stored beside each row). All four return the expected rows.a_document_names_the_leaf_element_and_predicate_domainsasserts the predicate domain is named andmatchis listed on fields with and without ordered paths.Error::Predicate→STATUS_ENCODING.matchthrough the query exports: one predicate deriveseql_v4.query_json_predicate, and alike_regexone isSTATUS_ENCODING.native_ops's catalog listing now includesmatch.Matchis generated on an ordered and an unordered document field, and an engine that answers nomatchis refused.testusers.Entitygains the RC2 note's members (score, weight, address, country, preferred languages), and$.weightis ordered.customer_stash.go,subscriber_stash.go,entity_stash.goand the example'scustomer_stash.goare regenerated.Docs
cmd/stashgen/README.md, "Encrypted JSON documents": aMatchrow with both SQL forms, and a section covering:jsonb_path_match(lax unwrapping not applied; lax wrapping of a non-array under[*]not applied; a string compared with a number is false under==and true under!=; orderings and multi-condition filters compare folded terms, so such a filter's==is of folded values);@>and->, not insidejson_tree_match, so a query that needs an index usesEqAt,ContainsandAt.<Path>.encrypt/README.mdandencrypt/doc.gosay the same in short. The typed-document example's README andmain.goadd aMatchcall.languages/golang/CHANGELOG.mdgainsAddedentries forMatchand the Diagnostic accessors, and aBreakingentry for the redacted types. The stashgen README lists everyReason()a refusal can give,json_syntaxandjson_inputincluded, one line each. The Go module records changes there, not in changesets.Verification
mise run wasm:guest:test: fmt, clippy, rustdoc; nextest 73 passed (default features) and 78 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 ./...: 550 passed, 13 skipped. The skips are 7 PostgreSQL tests withoutDATABASE_URL, 4 live ZeroKMS tests without credentials and 2 platform-specific memory tests.mise run go:test:postgresagainst a freshpostgres:17container with the EQL v4 bundle built from this tree: all 7 tests passed, including the new one, on the final head.go vet ./...,gofmt -l .(empty),mise run go:lint(0 issues), andgo generate ./...leaves the committed tree unchanged.cargo nextest run -p eql-codegen202 passed,cargo clippy -p eql-codegen --all-targets --all-features -D warningsandcargo fmt --checkclean.mise run types:checkandmise run codegen:parity(frompackages/eql) passed after commit.Related
Closes #1443
Stacked on #1451.
Review notes
%sto see its JSON now seeseql.X{[redacted]}; it converts the value (string(v)) or callsMarshalJSONinstead. Listed under Breaking. There is no named JSON accessor method; say if one should be added.TermError::JsonPathNotOrdered. It carries no offset and no path text, as feat(eql): one-parameter jsonpath predicates on json_tree documents (json_tree_match, @@, match) #1446 left it, soAt()is false there; the docs name its sentinel and code. Wrapping it inError::Predicatewith the path's offset is a small engine follow-up, if wanted.varsthat writenull. A nil map or pointer, orjson.RawMessage("null"), is treated as "no variables" rather than sent asnull, which the engine would refuse asjson_input. An empty or whitespace-onlyjson.RawMessageis not: it is refused asjson_syntax. Plaintextjsonb_path_match(col, p, NULL)returns NULL, so this differs from passing SQL NULL. It is the Go-idiomatic reading of a nil map. Tell me if you would rather refuse it.encrypt/gensupport/json.go(JSONMatch),stashgen/emit.go(jsonMatch, and thematchcheck inplanJSON),encrypt/guest/src/status.rs, andjson_match_postgres_test.go.