diff --git a/.agents/skills/loom-framework/SKILL.md b/.agents/skills/loom-framework/SKILL.md index e31f2a7d2..e95df8d71 100644 --- a/.agents/skills/loom-framework/SKILL.md +++ b/.agents/skills/loom-framework/SKILL.md @@ -75,6 +75,24 @@ consumer validation. ## Architecture Boundaries +- `expr.IsNullable` supplies effective nullability to prepared JSON schema + matching and lowered `JSONShape` descriptions. Nullable null bypasses value + predicates, including enums, while ordinary Any null still obeys its enum. + Inline JSON Schema and OpenAPI lift named occurrence refinements inside the + nullable value domain. Runtime descriptors validate before matching; null + never hides malformed descriptions or changes untagged union uniqueness. + Keep the nullable-enum model beside `pkg/tla/untagged_json` and schema/codec + agreement tests aligned. + +- Shared `recurseValidationCode` guards optional native array/map occurrences + before their predicates. Nil is absence only there; required roots, elements + and extracted Optional/Nullable values retain their concrete-value validation. + `AttributeContext.CollectionNilIsEmpty` records protobuf's nil-as-empty + storage independently of scalar pointers; context copies retain it so wrapped + and streamed collection predicates still run. Keep named occurrence guards and + the collection-presence model under + `codegen/tla/validation_carrier` aligned; do not patch individual transports. + - `execution/` owns typed activity names, options, registration/execution contracts, and futures. `execution/adapter` owns concrete-handler reflection; it does not own serialization. `execution/selvedge` uses native generic diff --git a/.agents/skills/loom/SKILL.md b/.agents/skills/loom/SKILL.md index 6649722e5..6fa16065f 100644 --- a/.agents/skills/loom/SKILL.md +++ b/.agents/skills/loom/SKILL.md @@ -394,6 +394,11 @@ For maps, `MinLength` and `MaxLength` count entries and export as `minProperties` and `maxProperties`, including named map types. Regenerate contracts after upgrading so schema validators enforce the declared map bounds. +Optional array/map fields may be omitted despite `MinLength`. Nil native +collections represent absence; nonnil empty collections must satisfy the bounds. +Required fields still need presence and valid contents. Regenerate services and +transports after upgrading to adopt the optional collection validation fix. + Unsigned integer schemas use `uint32` for `UInt32` and `uint64` for `UInt64` and `UInt`. Regenerate after upgrading and review generated clients. These format annotations describe the domain; validator format support is optional. @@ -818,6 +823,9 @@ Correct the key design and regenerate instead of editing generated declarations. `encoding/json/jsontext.Value`. Do not use the legacy raw-message type. - Treat requiredness and nullability as independent. `Required("field")` requires presence; `Nullable()` allows a present field to contain JSON null. +- `Nullable()` plus `Enum(...)` admits null and restricts concrete values to the + enum. Ordinary validators and untagged union codecs follow the same rule, + including inherited nullability. Upgrade Loom and regenerate to adopt the fix. - Generated nullable fields use `loom.Nullable[T]`. Its zero value is absent; use `loom.NullValue[T]()` for explicit null and `loom.NullableValue(value)` for a concrete value. Inspect it with `Present`, `IsNull`, and `Value`. diff --git a/codegen/generator/generation_determinism_test.go b/codegen/generator/generation_determinism_test.go index 06e2e2df9..a5d47fbc2 100644 --- a/codegen/generator/generation_determinism_test.go +++ b/codegen/generator/generation_determinism_test.go @@ -14,6 +14,7 @@ import ( servicetestdata "github.com/CaliLuke/loom/codegen/service/testdata" "github.com/CaliLuke/loom/eval" grpctestdata "github.com/CaliLuke/loom/grpc/codegen/testdata" + httptestdata "github.com/CaliLuke/loom/http/codegen/testdata" ) const generationDeterminismHelperPath = "LOOM_GENERATION_DETERMINISM_HELPER_PATH" @@ -28,6 +29,8 @@ var generationDeterminismDSLs = []struct { {"PkgPathMultipleDSL", servicetestdata.PkgPathMultipleDSL}, {"StructMetaTypeDSL", grpctestdata.StructMetaTypeDSL}, {"PayloadWithNestedTypesDSL", grpctestdata.PayloadWithNestedTypesDSL}, + {"OptionalCollectionValidationDSL", optionalCollectionValidationDSL}, + {"UntaggedArraysDSL", httptestdata.UntaggedArraysDSL}, } // TestGeneratedFilesAreByteStableAcrossIndependentProcesses generates the diff --git a/codegen/generator/optional_collection_validation_test.go b/codegen/generator/optional_collection_validation_test.go new file mode 100644 index 000000000..3a9881b61 --- /dev/null +++ b/codegen/generator/optional_collection_validation_test.go @@ -0,0 +1,150 @@ +package generator + +import ( + "testing" + + dsl "github.com/CaliLuke/loom/dsl" +) + +func TestOptionalCollectionValidationGenerated(t *testing.T) { + runDesignHarness(t, "example.com/collections", optionalCollectionValidationDSL, optionalCollectionValidationHarness) +} + +func optionalCollectionValidationDSL() { + dsl.API("collections", func() { + dsl.JSONRPC(func() {}) + }) + bounds := func() { + dsl.MinLength(2) + dsl.MaxLength(4) + } + list := dsl.Type("List", dsl.ArrayOf(dsl.String), bounds) + settings := dsl.Type("Settings", func() { + dsl.Field(1, "levels", dsl.ArrayOf(dsl.String), bounds) + dsl.Field(2, "labels", dsl.MapOf(dsl.String, dsl.String), bounds) + dsl.Field(3, "named", list) + dsl.Field(4, "required", dsl.ArrayOf(dsl.String), bounds) + dsl.Required("required") + }) + arraySettings := dsl.Type("ArraySettings", func() { + dsl.Attribute("levels", dsl.ArrayOf(dsl.String), bounds) + dsl.Attribute("named", list) + dsl.Attribute("required", dsl.ArrayOf(dsl.String), bounds) + dsl.Required("required") + }) + absent := dsl.Type("Absent", func() { + dsl.Attribute("missing", dsl.Boolean) + dsl.Required("missing") + }) + choice := dsl.Type("Choice", dsl.OneOf(arraySettings, absent), func() { + dsl.Untagged() + }) + dsl.Service("collections", func() { + dsl.Method("read", func() { + dsl.Result(choice) + dsl.HTTP(func() { + dsl.GET("/read") + }) + }) + dsl.Method("check", func() { + dsl.Payload(settings) + dsl.Result(settings) + dsl.HTTP(func() { + dsl.POST("/check") + }) + dsl.GRPC(func() {}) + }) + }) + dsl.Service("collectionsrpc", func() { + dsl.JSONRPC(func() { + dsl.POST("/rpc") + }) + dsl.Method("check", func() { + dsl.Payload(settings) + dsl.Result(settings) + dsl.JSONRPC(func() {}) + }) + }) +} + +const optionalCollectionValidationHarness = `package collections_test + +import ( + "context" + "encoding/json/v2" + "net/http/httptest" + "strings" + "testing" + + "github.com/stretchr/testify/require" + loomhttp "github.com/CaliLuke/loom/http" + svc "example.com/collections/gen/collections" + rpc "example.com/collections/gen/collectionsrpc" + httpserver "example.com/collections/gen/http/collections/server" + rpcserver "example.com/collections/gen/jsonrpc/collectionsrpc/server" +) + +func TestCollectionPresence(t *testing.T) { + for _, tc := range []struct { + name, body string + valid, arraysValid bool + }{ + {"omitted", "{\"required\":[\"a\",\"b\"]}", true, true}, + {"supplied", "{\"required\":[\"a\",\"b\"],\"levels\":[\"a\",\"b\"],\"labels\":{\"a\":\"a\",\"b\":\"b\"},\"named\":[\"a\",\"b\"]}", true, true}, + {"empty array", "{\"required\":[\"a\",\"b\"],\"levels\":[]}", false, false}, + {"short array", "{\"required\":[\"a\",\"b\"],\"levels\":[\"a\"]}", false, false}, + {"long array", "{\"required\":[\"a\",\"b\"],\"levels\":[\"a\",\"b\",\"c\",\"d\",\"e\"]}", false, false}, + {"empty map", "{\"required\":[\"a\",\"b\"],\"labels\":{}}", false, true}, + {"short map", "{\"required\":[\"a\",\"b\"],\"labels\":{\"a\":\"a\"}}", false, true}, + {"long map", "{\"required\":[\"a\",\"b\"],\"labels\":{\"a\":\"a\",\"b\":\"b\",\"c\":\"c\",\"d\":\"d\",\"e\":\"e\"}}", false, true}, + {"empty named", "{\"required\":[\"a\",\"b\"],\"named\":[]}", false, false}, + {"missing required", "{}", false, false}, + {"empty required", "{\"required\":[]}", false, false}, + } { + t.Run(tc.name, func(t *testing.T) { + var value svc.Settings + require.NoError(t, json.Unmarshal([]byte(tc.body), &value)) + err := svc.ValidateArraySettings(&svc.ArraySettings{Levels: value.Levels, Named: value.Named, Required: value.Required}) + require.Equal(t, tc.arraysValid, err == nil, "%v", err) + for _, protocol := range []string{"http", "rpc"} { + t.Run(protocol, func(t *testing.T) { + calls := 0 + endpoint := func(_ context.Context, payload any) (any, error) { + calls++ + encoded, err := json.Marshal(payload) + require.NoError(t, err) + var got svc.Settings + require.NoError(t, json.Unmarshal(encoded, &got)) + require.Equal(t, value, got) + return payload, nil + } + mux := loomhttp.NewMuxer() + path, body := "/check", tc.body + if protocol == "http" { + httpserver.Mount(mux, httpserver.New(&svc.Endpoints{Check: endpoint}, mux, loomhttp.RequestDecoder, loomhttp.ResponseEncoder, nil, nil)) + } else { + rpcserver.Mount(mux, rpcserver.New(&rpc.Endpoints{Check: endpoint}, mux, loomhttp.RequestDecoder, loomhttp.ResponseEncoder, nil)) + path = "/rpc" + body = "{\"jsonrpc\":\"2.0\",\"id\":1,\"method\":\"check\",\"params\":" + tc.body + "}" + } + recorder := httptest.NewRecorder() + request := httptest.NewRequest("POST", path, strings.NewReader(body)) + request.Header.Set("Content-Type", "application/json") + mux.ServeHTTP(recorder, request) + if tc.valid { + require.Equal(t, 1, calls, recorder.Body.String()) + require.Equal(t, 200, recorder.Code, recorder.Body.String()) + } else { + require.Zero(t, calls) + if protocol == "http" { + require.Equal(t, 400, recorder.Code, recorder.Body.String()) + } else { + require.Contains(t, recorder.Body.String(), "\"error\":") + } + } + }) + } + }) + } +} +` diff --git a/codegen/testdata/golden/validation_array-pointer.go.golden b/codegen/testdata/golden/validation_array-pointer.go.golden index def295081..959dd309e 100644 --- a/codegen/testdata/golden/validation_array-pointer.go.golden +++ b/codegen/testdata/golden/validation_array-pointer.go.golden @@ -5,8 +5,10 @@ func Validate() (err error) { if len(target.RequiredArray) < 5 { err = loom.MergeErrors(err, loom.InvalidLengthError("target.required_array", len(target.RequiredArray), 5, true)) } - if len(target.DefaultArray) > 3 { - err = loom.MergeErrors(err, loom.InvalidLengthError("target.default_array", len(target.DefaultArray), 3, false)) + if target.DefaultArray != nil { + if len(target.DefaultArray) > 3 { + err = loom.MergeErrors(err, loom.InvalidLengthError("target.default_array", len(target.DefaultArray), 3, false)) + } } for _, e := range target.Array { if !(e == 0 || e == 1 || e == 1 || e == 2 || e == 3 || e == 5) { diff --git a/codegen/testdata/golden/validation_array-required.go.golden b/codegen/testdata/golden/validation_array-required.go.golden index def295081..959dd309e 100644 --- a/codegen/testdata/golden/validation_array-required.go.golden +++ b/codegen/testdata/golden/validation_array-required.go.golden @@ -5,8 +5,10 @@ func Validate() (err error) { if len(target.RequiredArray) < 5 { err = loom.MergeErrors(err, loom.InvalidLengthError("target.required_array", len(target.RequiredArray), 5, true)) } - if len(target.DefaultArray) > 3 { - err = loom.MergeErrors(err, loom.InvalidLengthError("target.default_array", len(target.DefaultArray), 3, false)) + if target.DefaultArray != nil { + if len(target.DefaultArray) > 3 { + err = loom.MergeErrors(err, loom.InvalidLengthError("target.default_array", len(target.DefaultArray), 3, false)) + } } for _, e := range target.Array { if !(e == 0 || e == 1 || e == 1 || e == 2 || e == 3 || e == 5) { diff --git a/codegen/testdata/golden/validation_array-use-default.go.golden b/codegen/testdata/golden/validation_array-use-default.go.golden index def295081..959dd309e 100644 --- a/codegen/testdata/golden/validation_array-use-default.go.golden +++ b/codegen/testdata/golden/validation_array-use-default.go.golden @@ -5,8 +5,10 @@ func Validate() (err error) { if len(target.RequiredArray) < 5 { err = loom.MergeErrors(err, loom.InvalidLengthError("target.required_array", len(target.RequiredArray), 5, true)) } - if len(target.DefaultArray) > 3 { - err = loom.MergeErrors(err, loom.InvalidLengthError("target.default_array", len(target.DefaultArray), 3, false)) + if target.DefaultArray != nil { + if len(target.DefaultArray) > 3 { + err = loom.MergeErrors(err, loom.InvalidLengthError("target.default_array", len(target.DefaultArray), 3, false)) + } } for _, e := range target.Array { if !(e == 0 || e == 1 || e == 1 || e == 2 || e == 3 || e == 5) { diff --git a/codegen/testdata/golden/validation_map-pointer.go.golden b/codegen/testdata/golden/validation_map-pointer.go.golden index 1ce2591c7..39634b60f 100644 --- a/codegen/testdata/golden/validation_map-pointer.go.golden +++ b/codegen/testdata/golden/validation_map-pointer.go.golden @@ -7,8 +7,10 @@ func Validate() (err error) { if len(target.RequiredMap) < 5 { err = loom.MergeErrors(err, loom.InvalidLengthError("target.required_map", len(target.RequiredMap), 5, true)) } - if len(target.DefaultMap) > 3 { - err = loom.MergeErrors(err, loom.InvalidLengthError("target.default_map", len(target.DefaultMap), 3, false)) + if target.DefaultMap != nil { + if len(target.DefaultMap) > 3 { + err = loom.MergeErrors(err, loom.InvalidLengthError("target.default_map", len(target.DefaultMap), 3, false)) + } } for k, v := range target.Map { err = loom.MergeErrors(err, loom.ValidatePatternCompiled("target.map.key", k, loomPatternTest0)) diff --git a/codegen/testdata/golden/validation_map-required.go.golden b/codegen/testdata/golden/validation_map-required.go.golden index 1ce2591c7..39634b60f 100644 --- a/codegen/testdata/golden/validation_map-required.go.golden +++ b/codegen/testdata/golden/validation_map-required.go.golden @@ -7,8 +7,10 @@ func Validate() (err error) { if len(target.RequiredMap) < 5 { err = loom.MergeErrors(err, loom.InvalidLengthError("target.required_map", len(target.RequiredMap), 5, true)) } - if len(target.DefaultMap) > 3 { - err = loom.MergeErrors(err, loom.InvalidLengthError("target.default_map", len(target.DefaultMap), 3, false)) + if target.DefaultMap != nil { + if len(target.DefaultMap) > 3 { + err = loom.MergeErrors(err, loom.InvalidLengthError("target.default_map", len(target.DefaultMap), 3, false)) + } } for k, v := range target.Map { err = loom.MergeErrors(err, loom.ValidatePatternCompiled("target.map.key", k, loomPatternTest0)) diff --git a/codegen/testdata/golden/validation_map-use-default.go.golden b/codegen/testdata/golden/validation_map-use-default.go.golden index 1ce2591c7..39634b60f 100644 --- a/codegen/testdata/golden/validation_map-use-default.go.golden +++ b/codegen/testdata/golden/validation_map-use-default.go.golden @@ -7,8 +7,10 @@ func Validate() (err error) { if len(target.RequiredMap) < 5 { err = loom.MergeErrors(err, loom.InvalidLengthError("target.required_map", len(target.RequiredMap), 5, true)) } - if len(target.DefaultMap) > 3 { - err = loom.MergeErrors(err, loom.InvalidLengthError("target.default_map", len(target.DefaultMap), 3, false)) + if target.DefaultMap != nil { + if len(target.DefaultMap) > 3 { + err = loom.MergeErrors(err, loom.InvalidLengthError("target.default_map", len(target.DefaultMap), 3, false)) + } } for k, v := range target.Map { err = loom.MergeErrors(err, loom.ValidatePatternCompiled("target.map.key", k, loomPatternTest0)) diff --git a/codegen/tla/validation_carrier/CollectionPresence.tla b/codegen/tla/validation_carrier/CollectionPresence.tla new file mode 100644 index 000000000..68780c6a2 --- /dev/null +++ b/codegen/tla/validation_carrier/CollectionPresence.tla @@ -0,0 +1,26 @@ +--------------------- MODULE CollectionPresence --------------------- +EXTENDS Naturals +CONSTANT Mode +VARIABLES required, supplied, size, nilIsEmpty, phase, valid +vars == <> +Init == /\ required \in BOOLEAN + /\ supplied \in BOOLEAN + /\ size \in 0..5 + /\ nilIsEmpty \in BOOLEAN + /\ (nilIsEmpty => supplied) + /\ (~supplied => size = 0) + /\ phase = "input" + /\ valid = FALSE +WithinBounds == size >= 2 /\ size <= 4 +Validate == /\ phase = "input" + /\ valid' = CASE Mode = "legacy" -> WithinBounds + [] Mode = "native-only" -> + (~required /\ (~supplied \/ (nilIsEmpty /\ size = 0))) \/ WithinBounds + [] OTHER -> (~required /\ ~supplied) \/ WithinBounds + /\ phase' = "validated" + /\ UNCHANGED <> +Next == Validate +Spec == Init /\ [][Next]_vars +Omission == phase = "validated" /\ ~required /\ ~supplied => valid +Bounds == phase = "validated" /\ (required \/ supplied) => (valid = WithinBounds) +===================================================================== diff --git a/codegen/tla/validation_carrier/README.md b/codegen/tla/validation_carrier/README.md index f0678437c..c124ddc46 100644 --- a/codegen/tla/validation_carrier/README.md +++ b/codegen/tla/validation_carrier/README.md @@ -46,6 +46,43 @@ coverage deliberately abstracts physical Go carrier agreement. Primitive aliases byte slices and named Nullable declarations remain independent regression controls. Do not commit checker states or raw output; remove task-owned output after review. +## Optional collections (#1042) + +`CollectionPresence.tla` distinguishes absent optional collections from supplied +empty collections. It enumerates lengths zero through five with bounds two and +four. Required occurrences also represent collection elements and extracted +Optional/Nullable values, whose presence has already been established. It also +models protobuf's nil-as-empty storage: a present collection wrapper supplies +its collection even if decoding produces a nil slice/map. The model abstracts +Go nil detection and does not model JSON decoding, defaults or naming. + +Run with the pinned tool above, substituting `CollectionPresence.tla`, +`collection-legacy.cfg`, `collection-native-only.cfg` or `collection-checked.cfg`, +and separate temporary state directories. Legacy fails `Omission` after 27 +distinct states: native absence is treated as a supplied empty collection. +The first repair (`native-only`) fails `Bounds` after 29 states because protobuf +nil contents bypass minimum length. Checked passes all 52 states, preserving +bounds for supplied and required values. These runs used Go source base +`05e01174e` and the candidate's explicit collection-storage context. + +`recurseValidationCode` owns the optional native array/map guard. +`AttributeContext.CollectionNilIsEmpty`, set by `protoBufTypeContext` and retained +by `Dup`, prevents that guard where native storage does not carry absence. +Named calls retain their existing occurrence guards; required roots, collection +elements and unwrapped presence values continue to validate concrete values. +`TestOptionalCollectionValidationPresence` checks the emitted root guard and +both bounds; `TestOptionalCollectionValidationGenerated` compiles service, +HTTP, JSON-RPC and gRPC output, then exercises service validation and mounted +HTTP/JSON-RPC omission, empty, valid, out-of-bound and required-field cases. +`TestProtoCollectionPredicatesDoNotInferAbsence` checks the protobuf context; +existing `TestCollectionWrapperGeneratedContracts` and +`TestGeneratedStreamingRootValidation` reproduce the first repair's regression +and verify corrected generated validation and mounted protobuf exchanges. +The HTTP decoder goldens also retain the guard for optional array headers and +query arrays/maps, including named collection aliases. `TestClientDecode` and +`TestDecode` compare those emitted sections; element predicates remain outside +the collection's nil guard and continue to validate supplied entries. + ## Alias spine extraction (#892) `AliasPresence.tla` refines the root-versus-child distinction for inherited diff --git a/codegen/tla/validation_carrier/collection-checked.cfg b/codegen/tla/validation_carrier/collection-checked.cfg new file mode 100644 index 000000000..96429f03e --- /dev/null +++ b/codegen/tla/validation_carrier/collection-checked.cfg @@ -0,0 +1,4 @@ +CONSTANT Mode = "checked" +SPECIFICATION Spec +INVARIANT Omission Bounds +CHECK_DEADLOCK FALSE diff --git a/codegen/tla/validation_carrier/collection-legacy.cfg b/codegen/tla/validation_carrier/collection-legacy.cfg new file mode 100644 index 000000000..32047add7 --- /dev/null +++ b/codegen/tla/validation_carrier/collection-legacy.cfg @@ -0,0 +1,4 @@ +CONSTANT Mode = "legacy" +SPECIFICATION Spec +INVARIANT Omission Bounds +CHECK_DEADLOCK FALSE diff --git a/codegen/tla/validation_carrier/collection-native-only.cfg b/codegen/tla/validation_carrier/collection-native-only.cfg new file mode 100644 index 000000000..197f9a8b3 --- /dev/null +++ b/codegen/tla/validation_carrier/collection-native-only.cfg @@ -0,0 +1,4 @@ +CONSTANT Mode = "native-only" +SPECIFICATION Spec +INVARIANT Omission Bounds +CHECK_DEADLOCK FALSE diff --git a/codegen/transformer.go b/codegen/transformer.go index 631ae816f..817071f3a 100644 --- a/codegen/transformer.go +++ b/codegen/transformer.go @@ -60,6 +60,10 @@ type ( // map values use loom.Optional in a generated JSON decoding type so null // is rejected by decoding before conversion to the service type. CollectionElementPresence bool + // CollectionNilIsEmpty indicates that native collection storage cannot + // distinguish absence from empty, as with protobuf repeated/map fields. + // Validation must apply collection predicates even when that storage is nil. + CollectionNilIsEmpty bool // JSONPresenceTypes records named types whose physical object fields use // loom.Optional even when the root conversion itself uses native presence. // It is keyed by the hash of the user type, which names its Go type. @@ -375,6 +379,7 @@ func (a *AttributeContext) Dup() *AttributeContext { SamePackageConversion: a.SamePackageConversion, JSONPresence: a.JSONPresence, CollectionElementPresence: a.CollectionElementPresence, + CollectionNilIsEmpty: a.CollectionNilIsEmpty, JSONPresenceTypes: a.JSONPresenceTypes, PresencePointerTypes: a.PresencePointerTypes, PresenceUseDefaultTypes: a.PresenceUseDefaultTypes, diff --git a/codegen/validation_optional_collection_test.go b/codegen/validation_optional_collection_test.go new file mode 100644 index 000000000..ab0d943db --- /dev/null +++ b/codegen/validation_optional_collection_test.go @@ -0,0 +1,39 @@ +package codegen + +import ( + "fmt" + "strings" + "testing" + + "github.com/stretchr/testify/require" + + "github.com/CaliLuke/loom/expr" +) + +func TestOptionalCollectionValidationPresence(t *testing.T) { + minimum, maximum := 2, 4 + for _, typ := range []struct { + name string + typeOf expr.DataType + }{ + {"array", &expr.Array{ElemType: &expr.AttributeExpr{Type: expr.String}}}, + {"map", &expr.Map{KeyType: &expr.AttributeExpr{Type: expr.String}, ElemType: &expr.AttributeExpr{Type: expr.String}}}, + } { + for _, required := range []bool{false, true} { + for _, pointer := range []bool{false, true} { + for _, nilIsEmpty := range []bool{false, true} { + t.Run(fmt.Sprintf("%s/required=%t/pointer=%t/nilIsEmpty=%t", typ.name, required, pointer, nilIsEmpty), func(t *testing.T) { + attribute := &expr.AttributeExpr{Type: typ.typeOf, Validation: &expr.ValidationExpr{MinLength: &minimum, MaxLength: &maximum}} + ctx := NewAttributeContext(pointer, false, true, "", NewNameScope()) + ctx.CollectionNilIsEmpty = nilIsEmpty + ctx = ctx.Dup() + code := ValidationCode(attribute, nil, ctx, required, false, false, "value") + require.Equal(t, !required && !nilIsEmpty, strings.HasPrefix(code, "if value != nil {"), code) + require.Contains(t, code, "len(value) < 2") + require.Contains(t, code, "len(value) > 4") + }) + } + } + } + } +} diff --git a/codegen/validation_recurse.go b/codegen/validation_recurse.go index 0ea17e4e3..f755db706 100644 --- a/codegen/validation_recurse.go +++ b/codegen/validation_recurse.go @@ -23,6 +23,11 @@ func recurseValidationCode(att *expr.AttributeExpr, put expr.UserType, attCtx *A // Write validations on attribute if any. validation := validationCode(att, attCtx, req, alias, target, context) if validation != "" { + // Native collection nil is absence only at an optional occurrence. + // Required and extracted values still need their value predicates. + if !req && !attCtx.validationValue && !attCtx.CollectionNilIsEmpty && (expr.IsArray(att.Type) || expr.IsMap(att.Type)) { + validation = renderSimplePointerWrappedValidation(true, target, validation) + } fmt.Fprint(buf, validation) first = false } diff --git a/docs/dsl-reference.md b/docs/dsl-reference.md index f08e7ff52..53680361f 100644 --- a/docs/dsl-reference.md +++ b/docs/dsl-reference.md @@ -100,6 +100,11 @@ Its zero value is absent. Construct the other states with `Present`, `IsNull`, and `Value`. A required nullable attribute still uses the wrapper so that generated validation can reject absence while accepting null. +`Nullable()` also admits null when combined with `Enum`: the enum restricts +concrete values, such as `"open"` and `"closed"`. Ordinary record validation and +untagged union matching use this same rule, including inherited nullability. +Upgrade Loom and regenerate services and transports to adopt the matching fix. + | Contract | Absent JSON property | JSON `null` | Concrete JSON value | Generated service form | |---|---|---|---|---| | Required, non-null | Rejected | Rejected | Accepted | Native `T` | @@ -610,6 +615,14 @@ a default applies are unchanged by this representation correction. - `MinLength(n)` - Minimum number of elements - `MaxLength(n)` - Maximum number of elements +Optional arrays and maps may be omitted even when they declare `MinLength`. +In native Go values, a nil optional collection is absent; a nonnil empty +collection is supplied and must satisfy its bounds. Required fields still need +to be present and satisfy their bounds. Regenerate services and transports to +adopt the corrected optional collection validation. +Protobuf repeated/map fields retain their wire format's absence/empty collapse; +see the collection mapping rules in the [gRPC guide](grpc-guide.md). + For `Bytes`, these bounds count decoded bytes. Built-in JSON represents the value as padded base64, and the generated schema projects the decoded bounds into constraints on that string. See [Bytes in JSON bodies](http-guide.md#bytes-in-json-bodies). diff --git a/expr/json_schema_inline.go b/expr/json_schema_inline.go index 8437080c3..939e458f1 100644 --- a/expr/json_schema_inline.go +++ b/expr/json_schema_inline.go @@ -121,7 +121,9 @@ func buildInlineJSONSchema(attr *AttributeExpr, visited map[any]struct{}, contex if err != nil { return nil, err } - if attr.Nullable && !inlineTypeAlreadyNullable(attr.Type) { + // Occurrence constraints refine the non-null domain even when nullability + // comes from a named type. Lift the complete refinement into the value arm. + if IsNullable(attr) && (!inlineTypeAlreadyNullable(attr.Type) || attr.Validation != nil) { return nullableInlineJSONSchema(schema), nil } return schema, nil diff --git a/expr/value_plan_json_shape.go b/expr/value_plan_json_shape.go index 587b4e3d5..7562f31b8 100644 --- a/expr/value_plan_json_shape.go +++ b/expr/value_plan_json_shape.go @@ -34,7 +34,9 @@ func lowerJSONShape(node *valuePlanNode, seen map[*valuePlanNode]*loom.JSONShape if node == nil || node.codec != ValueCodecJSON { return nil, fmt.Errorf("JSON matching cannot lower an opaque codec") } - shape := &loom.JSONShape{Nullable: node.nullable, Closed: !node.schemaUnknown, NonNullableElements: node.nonNullableElements} + // The runtime descriptor receives effective nullability, including named + // aliases, so it need not reconstruct declaration-level presence policy. + shape := &loom.JSONShape{Nullable: IsNullable(node.attribute), Closed: !node.schemaUnknown, NonNullableElements: node.nonNullableElements} seen[node] = shape rules, err := lowerJSONShapeRules(node) if err != nil { diff --git a/expr/value_plan_nullable_enum_test.go b/expr/value_plan_nullable_enum_test.go new file mode 100644 index 000000000..9bcb0d6b5 --- /dev/null +++ b/expr/value_plan_nullable_enum_test.go @@ -0,0 +1,61 @@ +package expr + +import ( + "testing" + + "github.com/stretchr/testify/require" + "github.com/xeipuuv/gojsonschema" +) + +func TestNullableEnumMatchingAgreement(t *testing.T) { + base := &UserTypeExpr{TypeName: "Status", AttributeExpr: &AttributeExpr{ + Type: String, Nullable: true, Validation: &ValidationExpr{Values: []any{"open", "closed"}}, + }} + for _, tc := range []struct { + name string + attribute *AttributeExpr + valid, invalid string + }{ + {"string", base.Attribute(), `"open"`, `"other"`}, + {"inherited", &AttributeExpr{Type: base}, `"open"`, `"other"`}, + {"inherited refined", &AttributeExpr{Type: base, Validation: &ValidationExpr{Values: []any{"open"}}}, `"open"`, `"closed"`}, + {"conjoined", &AttributeExpr{Type: String, Nullable: true, Validation: &ValidationExpr{EnumClauses: [][]any{{"open", "closed"}, {"open"}}}}, `"open"`, `"closed"`}, + {"number", &AttributeExpr{Type: Int, Nullable: true, Validation: &ValidationExpr{Values: []any{1, 2}}}, `1`, `3`}, + {"boolean", &AttributeExpr{Type: Boolean, Nullable: true, Validation: &ValidationExpr{Values: []any{true}}}, `true`, `false`}, + {"array", &AttributeExpr{Type: &Array{ElemType: &AttributeExpr{Type: String}}, Nullable: true, Validation: &ValidationExpr{Values: []any{[]string{"open"}}}}, `["open"]`, `[]`}, + } { + t.Run(tc.name, func(t *testing.T) { + context := NewValueContext() + occurrence, err := context.NewOccurrence(tc.attribute) + require.NoError(t, err) + plan, err := context.NewValuePlan(occurrence, ValuePlanRequest{Target: DupAtt(tc.attribute), Codec: ValueCodecJSON, Use: ValuePlanDocumentation}) + require.NoError(t, err) + shape, err := plan.JSONShape() + require.NoError(t, err) + schema, err := InlineJSONSchema(tc.attribute) + require.NoError(t, err) + for _, value := range []struct { + wire string + want bool + }{ + {"null", true}, {tc.valid, true}, {tc.invalid, false}, + } { + t.Run(value.wire, func(t *testing.T) { + result, err := gojsonschema.Validate(gojsonschema.NewBytesLoader(schema), gojsonschema.NewStringLoader(value.wire)) + require.NoError(t, err) + require.Equal(t, value.want, result.Valid(), "%s", schema) + t.Run("prepared", func(t *testing.T) { + matched, err := plan.MatchJSONSchema([]byte(value.wire)) + require.NoError(t, err) + require.Equal(t, value.want, matched) + }) + t.Run("lowered", func(t *testing.T) { + matched, err := shape.Match([]byte(value.wire)) + require.NoError(t, err) + require.Equal(t, value.want, matched) + }) + }) + } + }) + } +} diff --git a/expr/value_projection_schema.go b/expr/value_projection_schema.go index 940168f77..38f70cd46 100644 --- a/expr/value_projection_schema.go +++ b/expr/value_projection_schema.go @@ -13,8 +13,8 @@ import ( var valueBase64Language = regexp.MustCompile(`\A(?:[A-Za-z0-9+/]{4})*(?:[A-Za-z0-9+/]{2}==|[A-Za-z0-9+/]{3}=)?\z`) -// The body and enum gate are deliberately separate: returning from a body -// branch must never bypass whole-array/object/map/union enum constraints. +// The body and enum gate are deliberately separate: a successful non-null body +// branch must still satisfy whole-array/object/map/union enum constraints. func schemaJSON(plan *valuePlanNode, wire jsontext.Value) (bool, error) { if plan == nil { return false, fmt.Errorf("missing JSON schema plan") @@ -23,6 +23,11 @@ func schemaJSON(plan *valuePlanNode, wire jsontext.Value) (bool, error) { if err != nil || !matched { return matched, err } + // Nullable adds null to the complete value domain. Value predicates, + // including enums, constrain only the non-null branch of that domain. + if wire.Kind() == 'n' && IsNullable(plan.attribute) { + return true, nil + } matched, err = schemaLocalRules(plan, wire) if err != nil || !matched { return matched, err diff --git a/grpc/codegen/proto_streaming_validation_test.go b/grpc/codegen/proto_streaming_validation_test.go index e0e347686..4548c3ec9 100644 --- a/grpc/codegen/proto_streaming_validation_test.go +++ b/grpc/codegen/proto_streaming_validation_test.go @@ -5,9 +5,26 @@ import ( "github.com/stretchr/testify/require" + "github.com/CaliLuke/loom/codegen" + "github.com/CaliLuke/loom/expr" "github.com/CaliLuke/loom/grpc/codegen/testdata" ) +func TestProtoCollectionPredicatesDoNotInferAbsence(t *testing.T) { + for _, collection := range []expr.DataType{ + &expr.Array{ElemType: &expr.AttributeExpr{Type: expr.String}}, + &expr.Map{KeyType: &expr.AttributeExpr{Type: expr.String}, ElemType: &expr.AttributeExpr{Type: expr.String}}, + } { + t.Run(collection.Name(), func(t *testing.T) { + attribute := &expr.AttributeExpr{Type: collection, Validation: &expr.ValidationExpr{MinLength: new(2)}} + context := protoBufTypeContext("pb", codegen.NewNameScope(), false) + validation := codegen.ValidationCode(wrapperAttribute(attribute), nil, context, true, false, false, "message") + require.Contains(t, validation, "len(message.Field) < 2") + require.NotContains(t, validation, "if message.Field != nil") + }) + } +} + func TestProtoStreamingRootValidation(t *testing.T) { root := RunGRPCDSL(t, testdata.StreamingValidationDSL) var rendered string diff --git a/grpc/codegen/protobuf.go b/grpc/codegen/protobuf.go index 49937d56b..fec5eb3e1 100644 --- a/grpc/codegen/protobuf.go +++ b/grpc/codegen/protobuf.go @@ -57,6 +57,7 @@ func (p *protoBufScope) Scope() *codegen.NameScope { func protoBufTypeContext(pkg string, scope *codegen.NameScope, useDefault bool) *codegen.AttributeContext { ctx := codegen.NewAttributeContext(true, false, useDefault, pkg, scope) ctx.Scope = &protoBufScope{scope: scope} + ctx.CollectionNilIsEmpty = true return ctx } diff --git a/http/codegen/openapi/internal/ir/analyzer.go b/http/codegen/openapi/internal/ir/analyzer.go index 390e2d94b..a04d720ff 100644 --- a/http/codegen/openapi/internal/ir/analyzer.go +++ b/http/codegen/openapi/internal/ir/analyzer.go @@ -205,7 +205,9 @@ func (a *Analyzer) analyzeSchema(attr *expr.AttributeExpr, context string, noref return a.analyzeUnderlyingSchema(componentAttribute(attr, t), context, true) } s := a.analyzeUserType(attr, t, context, len(noref) > 0) - if attr.Nullable && !expr.IsNullable(t.Attribute()) { + // A named occurrence's refinement constrains its non-null domain; + // inherited nullability must surround that complete refinement too. + if expr.IsNullable(attr) && (!expr.IsNullable(t.Attribute()) || hasUserTypeValidationOverlay(attr, t.Attribute())) { a.applySchemaExample(s, attr, context) a.applyNullableSchema(s) } else if expr.AllowsNull(t.Attribute()) && len(attr.UserExamples) > 0 { diff --git a/http/codegen/openapi/internal/ir/nullable_enum_test.go b/http/codegen/openapi/internal/ir/nullable_enum_test.go new file mode 100644 index 000000000..81ae92ab2 --- /dev/null +++ b/http/codegen/openapi/internal/ir/nullable_enum_test.go @@ -0,0 +1,29 @@ +package ir + +import ( + "testing" + + "github.com/stretchr/testify/require" + + "github.com/CaliLuke/loom/expr" +) + +func TestNullableEnumOccurrenceRefinement(t *testing.T) { + for _, nullable := range []bool{false, true} { + base := &expr.UserTypeExpr{TypeName: "Status", AttributeExpr: &expr.AttributeExpr{ + Type: expr.String, Nullable: nullable, + Validation: &expr.ValidationExpr{Values: []any{"open", "closed"}}, + }} + attribute := &expr.AttributeExpr{Type: base, Validation: &expr.ValidationExpr{Values: []any{"open"}}} + analyzer := NewAnalyzer(expr.NewRandom("nullable-enum"), false) + schema := analyzer.AnalyzeSchema(attribute) + if nullable { + require.Len(t, schema.AnyOf, 2) + require.Equal(t, "null", schema.AnyOf[1].Type) + require.Empty(t, schema.Enum) + schema = schema.AnyOf[0] + } + require.Equal(t, []any{"open"}, schema.Enum) + require.Len(t, schema.AllOf, 1, "retain the base type's constraints") + } +} diff --git a/http/codegen/openapi/v3/untagged_arrays_test.go b/http/codegen/openapi/v3/untagged_arrays_test.go index 8f56ae273..29f99155d 100644 --- a/http/codegen/openapi/v3/untagged_arrays_test.go +++ b/http/codegen/openapi/v3/untagged_arrays_test.go @@ -6,6 +6,7 @@ import ( "testing" "github.com/stretchr/testify/require" + "github.com/xeipuuv/gojsonschema" openapiv3 "github.com/CaliLuke/loom/http/codegen/openapi/v3" "github.com/CaliLuke/loom/http/codegen/testdata" @@ -45,6 +46,28 @@ func TestUntaggedArrayObjectContract(t *testing.T) { } } require.True(t, found, "missing mixed array/object oneOf") + foundRecord := false + for name, raw := range schemas { + schema := raw.(map[string]any) + properties, ok := schema["properties"].(map[string]any) + if !ok || properties["status"] == nil { + continue + } + foundRecord = true + root := map[string]any{"components": spec["components"], "$ref": "#/components/schemas/" + name} + for _, value := range []struct { + wire string + want bool + }{ + {`{"status":null}`, true}, {`{"status":"open"}`, true}, + {`{"status":"closed"}`, false}, {`{"status":"other"}`, false}, {`{}`, false}, + } { + result, err := gojsonschema.Validate(gojsonschema.NewGoLoader(root), gojsonschema.NewStringLoader(value.wire)) + require.NoError(t, err) + require.Equal(t, value.want, result.Valid(), "%s: %s", name, value.wire) + } + } + require.True(t, foundRecord, "missing nullable enum record") if os.Getenv("LOOM_OPENAPI_CONTRACT") != "" { dir := t.TempDir() path := filepath.Join(dir, "openapi.yaml") diff --git a/http/codegen/testdata/golden/client_decode_header-string-array-validate.go.golden b/http/codegen/testdata/golden/client_decode_header-string-array-validate.go.golden index 62213bf02..ab2bea5ed 100644 --- a/http/codegen/testdata/golden/client_decode_header-string-array-validate.go.golden +++ b/http/codegen/testdata/golden/client_decode_header-string-array-validate.go.golden @@ -12,8 +12,10 @@ func DecodeMethodAResponse(decoder func(*http.Response) loomhttp.Decoder, restor ) array = loomhttp.HeaderValues(resp.Header, "Array") - if len(array) < 5 { - err = loom.MergeErrors(err, loom.InvalidLengthError("array", len(array), 5, true)) + if array != nil { + if len(array) < 5 { + err = loom.MergeErrors(err, loom.InvalidLengthError("array", len(array), 5, true)) + } } if err != nil { return nil, loomhttp.ErrValidationError("ServiceHeaderStringArrayValidateResponse", "MethodA", err) diff --git a/http/codegen/testdata/golden/server_decode_decode-query-array-alias-validate.go.golden b/http/codegen/testdata/golden/server_decode_decode-query-array-alias-validate.go.golden index c82b5648f..af1db6970 100644 --- a/http/codegen/testdata/golden/server_decode_decode-query-array-alias-validate.go.golden +++ b/http/codegen/testdata/golden/server_decode_decode-query-array-alias-validate.go.golden @@ -24,8 +24,10 @@ func DecodeMethodARequest(mux loomhttp.Muxer, _ func(*http.Request) loomhttp.Dec } } } - if len(array) < 3 { - err = loom.MergeErrors(err, loom.InvalidLengthError("array", len(array), 3, true)) + if array != nil { + if len(array) < 3 { + err = loom.MergeErrors(err, loom.InvalidLengthError("array", len(array), 3, true)) + } } for _, e := range array { if e < 0xa { diff --git a/http/codegen/testdata/golden/server_decode_decode-query-map-alias-validate.go.golden b/http/codegen/testdata/golden/server_decode_decode-query-map-alias-validate.go.golden index ba7c19282..609227a5f 100644 --- a/http/codegen/testdata/golden/server_decode_decode-query-map-alias-validate.go.golden +++ b/http/codegen/testdata/golden/server_decode_decode-query-map-alias-validate.go.golden @@ -34,8 +34,10 @@ func DecodeMethodARequest(mux loomhttp.Muxer, _ func(*http.Request) loomhttp.Dec } } } - if len(map_) < 5 { - err = loom.MergeErrors(err, loom.InvalidLengthError("map", len(map_), 5, true)) + if map_ != nil { + if len(map_) < 5 { + err = loom.MergeErrors(err, loom.InvalidLengthError("map", len(map_), 5, true)) + } } if err != nil { return payload, err diff --git a/http/codegen/testdata/golden/server_decode_decode-query-map-bool-array-string-validate.go.golden b/http/codegen/testdata/golden/server_decode_decode-query-map-bool-array-string-validate.go.golden index 14fc2a202..dd4e93dd2 100644 --- a/http/codegen/testdata/golden/server_decode_decode-query-map-bool-array-string-validate.go.golden +++ b/http/codegen/testdata/golden/server_decode_decode-query-map-bool-array-string-validate.go.golden @@ -45,8 +45,10 @@ func DecodeMethodQueryMapBoolArrayStringValidateRequest(mux loomhttp.Muxer, _ fu } } } - if len(q) < 1 { - err = loom.MergeErrors(err, loom.InvalidLengthError("q", len(q), 1, true)) + if q != nil { + if len(q) < 1 { + err = loom.MergeErrors(err, loom.InvalidLengthError("q", len(q), 1, true)) + } } for k, v := range q { if !(k == true) { diff --git a/http/codegen/testdata/untagged_arrays_dsl.go b/http/codegen/testdata/untagged_arrays_dsl.go index d199edd38..c5de41326 100644 --- a/http/codegen/testdata/untagged_arrays_dsl.go +++ b/http/codegen/testdata/untagged_arrays_dsl.go @@ -8,12 +8,17 @@ var UntaggedArraysDSL = func() { API("arrayunion", func() { Meta("openapi:closed-objects", "true") }) + status := Type("Status", String, func() { + Nullable() + Enum("open", "closed") + }) item := Type("Item", func() { Attribute("name", String, func() { MinLength(2) }) Attribute("label", String, func() { Nullable() + Enum("open", "closed") }) Required("name") Meta("openapi:additionalProperties", "false") @@ -27,6 +32,16 @@ var UntaggedArraysDSL = func() { result := Type("Result", OneOf(items, page), func() { Untagged() }) + record := Type("Record", func() { + Attribute("status", status, func() { + Enum("open") + }) + Required("status") + Meta("openapi:additionalProperties", "false") + }) + recordOrPage := Type("RecordOrPage", OneOf(record, page), func() { + Untagged() + }) signed := Type("Signed", ArrayOf(Int32)) unsigned := Type("Unsigned", ArrayOf(UInt32)) numbers := Type("Numbers", OneOf(signed, unsigned), func() { @@ -52,6 +67,10 @@ var UntaggedArraysDSL = func() { Untagged() }) Service("listing", func() { + Method("record", func() { + Result(recordOrPage) + HTTP(func() { GET("/record") }) + }) Method("exact", func() { Result(exact) HTTP(func() { GET("/exact") }) diff --git a/http/codegen/testdata/untagged_arrays_harness.go b/http/codegen/testdata/untagged_arrays_harness.go index 70069fbd8..55ac1b301 100644 --- a/http/codegen/testdata/untagged_arrays_harness.go +++ b/http/codegen/testdata/untagged_arrays_harness.go @@ -21,6 +21,30 @@ import ( "github.com/stretchr/testify/require" ) +func TestNullableEnumRecord(t *testing.T) { + for _, wire := range []string{"{\"status\":null}", "{\"status\":\"open\"}"} { + var record svc.Record + require.NoError(t, json.Unmarshal([]byte(wire), &record)) + require.NoError(t, svc.ValidateRecord(&record)) + value := svc.NewRecordOrPageRecord(&record) + encoded, err := json.Marshal(value, json.Deterministic(true)) + require.NoError(t, err) + require.Equal(t, wire, string(encoded)) + var decoded svc.RecordOrPage + require.NoError(t, json.Unmarshal(encoded, &decoded)) + require.Equal(t, value, decoded) + } + for _, wire := range []string{"{}", "{\"status\":\"closed\"}", "{\"status\":\"other\"}"} { + var record svc.Record + require.NoError(t, json.Unmarshal([]byte(wire), &record)) + require.Error(t, svc.ValidateRecord(&record)) + _, err := json.Marshal(svc.NewRecordOrPageRecord(&record)) + require.Error(t, err) + var value svc.RecordOrPage + require.Error(t, json.Unmarshal([]byte(wire), &value)) + } +} + func TestServiceCodecs(t *testing.T) { for _, wire := range []string{"[]", "[{\"name\":\"ok\",\"label\":null}]", "{\"total\":0}"} { var value svc.Result @@ -78,7 +102,7 @@ func TestHTTPCodecs(t *testing.T) { address, err := url.Parse(host.URL) require.NoError(t, err) c := client.NewClient(address.Scheme, address.Host, host.Client(), loomhttp.RequestEncoder, loomhttp.ResponseDecoder, false) - for _, wire := range []string{"[]", "[{\"name\":\"ok\"}]", "{\"total\":0}"} { + for _, wire := range []string{"[]", "[{\"name\":\"ok\"}]", "[{\"name\":\"ok\",\"label\":null}]", "[{\"name\":\"ok\",\"label\":\"open\"}]", "{\"total\":0}", "{\"total\":1,\"items\":[{\"name\":\"ok\",\"label\":null}]}"} { payload, err := client.BuildEchoPayload(wire) require.NoError(t, err) result, err := c.Echo()(t.Context(), payload) @@ -93,6 +117,7 @@ func TestHTTPCodecs(t *testing.T) { {"", true, false}, {" \t\r\n", true, false}, {"{x}", false, false}, {"[", false, false}, {"null", false, false}, {"[null]", false, false}, {"{\"total\":0,\"extra\":1}", false, false}, + {"[{\"name\":\"ok\",\"label\":\"other\"}]", false, false}, {"[{\"name\":\"x\"}]", false, false}, {"[]", false, true}, } { before := calls.Load() diff --git a/jsonrpc/codegen/untagged_arrays_test.go b/jsonrpc/codegen/untagged_arrays_test.go index b26233051..053f39a6d 100644 --- a/jsonrpc/codegen/untagged_arrays_test.go +++ b/jsonrpc/codegen/untagged_arrays_test.go @@ -20,6 +20,10 @@ func TestJSONRPCUntaggedArrayObject(t *testing.T) { items := Type("Items", ArrayOf(String)) page := Type("Page", func() { Attribute("total", Int) + Attribute("status", String, func() { + Nullable() + Enum("open", "closed") + }) Required("total") }) choice := Type("Choice", OneOf(items, page), func() { @@ -70,12 +74,14 @@ func TestJSONRPCArrayObject(t *testing.T) { host := httptest.NewServer(mux) defer host.Close() c := client.NewClient("http", strings.TrimPrefix(host.URL, "http://"), host.Client(), loomhttp.RequestEncoder, loomhttp.ResponseDecoder, false) - for _, wire := range []string{"[]", "[\"ok\"]", "{\"total\":0}"} { + for _, wire := range []string{"[]", "[\"ok\"]", "{\"total\":0}", "{\"total\":0,\"status\":null}", "{\"total\":0,\"status\":\"open\"}"} { var payload svc.Choice require.NoError(t, json.Unmarshal([]byte(wire), &payload)) result, err := c.Echo()(t.Context(), &payload) require.NoError(t, err) require.Equal(t, &payload, result) } + var invalid svc.Choice + require.Error(t, json.Unmarshal([]byte("{\"total\":0,\"status\":\"other\"}"), &invalid)) } ` diff --git a/pkg/json_shape.go b/pkg/json_shape.go index 5d17837d6..f2571684f 100644 --- a/pkg/json_shape.go +++ b/pkg/json_shape.go @@ -149,7 +149,7 @@ func (s *JSONShape) match(wire jsontext.Value, depth int) (bool, error) { return false, fmt.Errorf("invalid or cyclic JSON matching description") } if wire.Kind() == 'n' && s.Nullable { - return s.rules(wire, -1) + return true, nil } length := -1 switch s.Kind { diff --git a/pkg/json_union_test.go b/pkg/json_union_test.go index 3e395cf7c..216b90ce7 100644 --- a/pkg/json_union_test.go +++ b/pkg/json_union_test.go @@ -124,6 +124,9 @@ func TestJSONShapePredicates(t *testing.T) { {"exclusive bound", &JSONShape{Kind: "number", Rules: JSONShapeRules{ExclusiveMinimum: "1"}}, `1.1`, `1`}, {"conjoined enums", &JSONShape{Kind: "integer", Rules: JSONShapeRules{Enums: [][]jsontext.Value{{[]byte(`1`), []byte(`2`)}, {[]byte(`2`)}}}}, `2.0`, `1`}, {"nullable member", &JSONShape{Kind: "string", Nullable: true}, `null`, `3`}, + {"nullable enum", &JSONShape{Kind: "string", Nullable: true, Rules: JSONShapeRules{Enums: [][]jsontext.Value{{[]byte(`"open"`)}}}}, `null`, `"other"`}, + {"nullable enum alias", &JSONShape{Kind: "alias", Nullable: true, Child: &JSONShape{Kind: "string"}, Rules: JSONShapeRules{Enums: [][]jsontext.Value{{[]byte(`"open"`)}}}}, `null`, `"other"`}, + {"nonnullable any enum", &JSONShape{Kind: "any", Rules: JSONShapeRules{Enums: [][]jsontext.Value{{[]byte(`"open"`)}}}}, `"open"`, `null`}, {"non-null elements", &JSONShape{Kind: "array", Child: &JSONShape{Kind: "string", Nullable: true}, NonNullableElements: true}, `["x"]`, `[null]`}, {"format", &JSONShape{Kind: "string", Rules: JSONShapeRules{Formats: []Format{FormatEmail}}}, `"user@example.com"`, `"invalid"`}, {"pattern", &JSONShape{Kind: "string", Rules: JSONShapeRules{Patterns: []string{"^a", "z$"}}}, `"az"`, `"ab"`}, @@ -144,9 +147,28 @@ func TestJSONShapePredicates(t *testing.T) { } { _, err := shape.Match([]byte(`[]`)) require.Error(t, err, "invalid configuration must fail even when wire does not match") + shape.Nullable = true + _, err = shape.Match([]byte(`null`)) + require.Error(t, err, "nullable null must not bypass description validation") } } +func TestNullableEnumsRetainUnionAmbiguity(t *testing.T) { + branches := make([]JSONUnionBranch, 0, 2) + for _, member := range []jsontext.Value{[]byte(`"open"`), []byte(`"closed"`)} { + branches = append(branches, JSONUnionBranch{ + Schema: &JSONShape{Kind: "object", Fields: []JSONShapeField{{Name: "status", Required: true, Shape: &JSONShape{ + Kind: "string", Nullable: true, Rules: JSONShapeRules{Enums: [][]jsontext.Value{{member}}}, + }}}}, + Decode: func(jsontext.Value) (bool, error) { + return true, nil + }, + }) + } + _, err := MatchUntaggedJSON("Status", []byte(`{"status":null}`), branches) + require.ErrorContains(t, err, "matched 2 branches in schema") +} + func TestJSONShapeEvaluatorFailureIsFatal(t *testing.T) { valid := JSONUnionBranch{Schema: &JSONShape{Kind: "number"}, Decode: func(jsontext.Value) (bool, error) { return true, nil }} broken := JSONUnionBranch{Schema: &JSONShape{Kind: "number", Rules: JSONShapeRules{Minimum: "not a number"}}, Decode: valid.Decode} diff --git a/pkg/tla/untagged_json/NullableChecked.cfg b/pkg/tla/untagged_json/NullableChecked.cfg new file mode 100644 index 000000000..9a20c1e92 --- /dev/null +++ b/pkg/tla/untagged_json/NullableChecked.cfg @@ -0,0 +1,4 @@ +CONSTANT Mode = "checked" +SPECIFICATION Spec +INVARIANT Agreement +CHECK_DEADLOCK FALSE diff --git a/pkg/tla/untagged_json/NullableEnum.tla b/pkg/tla/untagged_json/NullableEnum.tla new file mode 100644 index 000000000..389a95318 --- /dev/null +++ b/pkg/tla/untagged_json/NullableEnum.tla @@ -0,0 +1,29 @@ +------------------------ MODULE NullableEnum ------------------------ +EXTENDS Naturals +CONSTANT Mode +VARIABLES nullable, inherited, required, wire, phase, matched +vars == <> +Init == /\ nullable \in BOOLEAN + /\ inherited \in BOOLEAN + /\ required \in BOOLEAN + /\ wire \in {"missing", "null", "member", "other"} + /\ phase = "input" + /\ matched = FALSE +Expected == CASE wire = "missing" -> ~required + [] wire = "null" -> nullable \/ inherited + [] OTHER -> wire = "member" +BodyAdmits == wire \in {"member", "other"} \/ + (wire = "null" /\ (nullable \/ inherited)) +NullAdmits == CASE Mode = "legacy" -> FALSE + [] Mode = "local-only" -> nullable + [] OTHER -> nullable \/ inherited +ValueGate == (wire = "null" /\ NullAdmits) \/ wire = "member" +Match == /\ phase = "input" + /\ matched' = IF wire = "missing" THEN ~required + ELSE BodyAdmits /\ ValueGate + /\ phase' = "matched" + /\ UNCHANGED <> +Next == Match +Spec == Init /\ [][Next]_vars +Agreement == phase = "matched" => matched = Expected +===================================================================== diff --git a/pkg/tla/untagged_json/NullableLegacy.cfg b/pkg/tla/untagged_json/NullableLegacy.cfg new file mode 100644 index 000000000..d9773a5b9 --- /dev/null +++ b/pkg/tla/untagged_json/NullableLegacy.cfg @@ -0,0 +1,4 @@ +CONSTANT Mode = "legacy" +SPECIFICATION Spec +INVARIANT Agreement +CHECK_DEADLOCK FALSE diff --git a/pkg/tla/untagged_json/NullableLocalOnly.cfg b/pkg/tla/untagged_json/NullableLocalOnly.cfg new file mode 100644 index 000000000..4cb69d4a2 --- /dev/null +++ b/pkg/tla/untagged_json/NullableLocalOnly.cfg @@ -0,0 +1,4 @@ +CONSTANT Mode = "local-only" +SPECIFICATION Spec +INVARIANT Agreement +CHECK_DEADLOCK FALSE diff --git a/pkg/tla/untagged_json/README.md b/pkg/tla/untagged_json/README.md index 5850a91d8..a7a048f6a 100644 --- a/pkg/tla/untagged_json/README.md +++ b/pkg/tla/untagged_json/README.md @@ -43,3 +43,50 @@ The model does not prove Go refinement, unlimited recursion, arbitrary codecs, or full OpenAPI equivalence. Opaque codecs are rejected by DSL validation. Keep checker state, traces and logs outside the repository and remove task-owned outputs after validation and independent review. + +## Nullable enum membership (#1037) + +`NullableEnum.tla` models a record field's missing, null, enum-member and +non-member states. Requiredness and occurrence/inherited nullability vary +independently. `Nullable()` adds null to the value domain; enum predicates +apply to concrete values. This is distinct from Any's intrinsic admission of +null, which does not bypass authored enum membership. + +Use the pinned TLC command above with `NullableEnum.tla` and the +`NullableLegacy.cfg`, `NullableLocalOnly.cfg` and `NullableChecked.cfg` +configurations, each with its own temporary state directory. Legacy reproduces +enum rejection of an admitted null. Local-only preserves direct nullability +but loses named-type nullability. Checked retains both and leaves required +absence and concrete non-member rejection intact. +With Java 25.0.5 and TLC 1.7.4, both negative controls fail `Agreement` after +42 distinct states; checked passes all 64 states. The implementation comparison +uses base `a2df44b82` and Go 1.27.3. The initial local-only repair failed the +inherited-null direct test before effective nullability was added to lowering. + +`schemaJSON` uses the captured attribute's `IsNullable` fact after syntax/body +admission; `lowerJSONShape` puts that effective fact in the runtime descriptor. +`JSONShape.match` accepts admitted null before value predicates, while `Match` +still validates the full description first. `TestNullableEnumMatchingAgreement` +compares prepared and lowered matching with rendered schemas for primitive, +inherited, inherited-with-occurrence-refinement, conjoined and array enums. +Review exposed a separate rendering gap: an occurrence enum outside a nullable +named schema removed null. Inline JSON Schema and OpenAPI now wrap the complete +refinement in the nullable domain. Direct IR checks and the rendered 3.1/3.2 +specimen verify null/open acceptance and closed/other/missing rejection. +The extended untagged-array fixtures check +ordinary required record validation, union marshal/unmarshal, HTTP and JSON-RPC +round trips, and invalid concrete values. Runtime tests retain ambiguous-null +rejection and fatal malformed-description errors. + +The bounded before/after generation comparison uses the six designs in +`generationDeterminismDSLs` with identical fixture inputs on both revisions. +Intended differences are inherited `Nullable: true` descriptors in the service +and both HTTP union codecs, and null-preserving refinement wrappers in the +OpenAPI JSON/YAML specimen. The five control designs remain byte-identical. +The generated HTTP fixture compiles and runs; independent-process generation +checks exact bytes across five runs. + +The model assumes the nullability query, JSON parser and enum evaluator; it does +not prove arbitrary alias graphs, Go codecs or JSON Schema rendering. Those +boundaries remain direct and generated-test obligations, and the original +union uniqueness/transaction model remains separate.