Unify canonical sequence index semantics for version decoding
This commit is contained in:
@@ -631,8 +631,10 @@ and redacted inspection.
|
|||||||
|
|
||||||
Schema path traversal uses these same bindings. Explicit key segments address
|
Schema path traversal uses these same bindings. Explicit key segments address
|
||||||
mappings, explicit index segments address sequences, and parsed numeric pointer
|
mappings, explicit index segments address sequences, and parsed numeric pointer
|
||||||
tokens become indices only in a sequence. Sequence indices cannot have leading
|
tokens become indices only in a sequence. Sequence indices must use canonical
|
||||||
zeroes; fixed-array indices are bounds checked. Dynamic `any` and inline-map entries
|
nonnegative decimal spelling: no signs or leading zeroes except `0` itself.
|
||||||
|
Tokens such as `+0` and `-0` remain literal keys in mappings but do not identify
|
||||||
|
sequence elements. Fixed-array indices are bounds checked. Dynamic `any` and inline-map entries
|
||||||
remain schema-valid for application-defined descendants, but dynamic values cannot
|
remain schema-valid for application-defined descendants, but dynamic values cannot
|
||||||
introduce typed secret references.
|
introduce typed secret references.
|
||||||
|
|
||||||
|
|||||||
87
docs/roadmap/remediation.md
Normal file
87
docs/roadmap/remediation.md
Normal file
@@ -0,0 +1,87 @@
|
|||||||
|
# Integer schema version remediation
|
||||||
|
|
||||||
|
Review baseline: `fd8a68d`, covering implementation commits `f5bbc2b` through
|
||||||
|
`fd8a68d` against `b3e7b62`. Status: finding resolved. This document records
|
||||||
|
required corrective work, not an implemented contract or release approval.
|
||||||
|
|
||||||
|
## R1 — Unify sequence-path interpretation between checking and decoding
|
||||||
|
|
||||||
|
Priority: medium. Status: resolved.
|
||||||
|
|
||||||
|
Resolution: sequence traversal now requires canonical nonnegative decimal index
|
||||||
|
tokens in the shared `schemaIndex` helper. Version decoding reuses
|
||||||
|
`matchesPathShape`, which shares that helper with traversal; its independent
|
||||||
|
numeric matcher was removed. Signed tokens remain literal mapping keys but no
|
||||||
|
longer alias sequence elements. Public-load regression coverage verifies numeric
|
||||||
|
and quoted input, metadata lookup, mapping keys, and explicit segment intent.
|
||||||
|
Focused Version/Decode/Path tests and the everyday checker passed on Linux.
|
||||||
|
|
||||||
|
### Finding and evidence
|
||||||
|
|
||||||
|
The new `pathMatches` helper in [validation.go](../../validation.go) implements
|
||||||
|
sequence-token matching independently of the existing `schemaIndex` helper in
|
||||||
|
[schema.go](../../schema.go). `checkVersion` looks up its node through
|
||||||
|
`documentAtPath`, which uses `schemaIndex`; typed integer-to-string conversion
|
||||||
|
instead uses `pathMatches`.
|
||||||
|
|
||||||
|
These rules disagree. `schemaIndex` currently accepts parsed `+0` and `-0` as
|
||||||
|
sequence index zero through `strconv.Atoi`, but `pathMatches` requires the exact
|
||||||
|
canonical spelling returned by `strconv.Itoa`, namely `0`.
|
||||||
|
|
||||||
|
A public-API probe against the review baseline used a configuration with
|
||||||
|
`Versions []string` bound to `versions`, a no-op version checker, and each path
|
||||||
|
returned by `ParsePath`:
|
||||||
|
|
||||||
|
| Configured path | YAML value | Checker receives | Load result |
|
||||||
|
| --- | --- | --- | --- |
|
||||||
|
| `/versions/0` | `[4]` | Present, text `4` | Success |
|
||||||
|
| `/versions/0` | `['4']` | Present, text `4` | Success |
|
||||||
|
| `/versions/+0` | `[4]` | Present, text `4` | Decode failure |
|
||||||
|
| `/versions/+0` | `['4']` | Present, text `4` | Success |
|
||||||
|
| `/versions/-0` | `[4]` | Present, text `4` | Decode failure |
|
||||||
|
| `/versions/-0` | `['4']` | Present, text `4` | Success |
|
||||||
|
|
||||||
|
Thus the checker accepts and identifies a version node, but the decoder does not
|
||||||
|
recognize that same node as the configured version path. Canonical index paths and
|
||||||
|
the ordinary Notarius `version` mapping key work correctly. The permissive lookup
|
||||||
|
predates these commits; the new, separately implemented matcher introduces the
|
||||||
|
checker/decoder disagreement. The existing canonical-index tests do not catch it.
|
||||||
|
|
||||||
|
### Required correction
|
||||||
|
|
||||||
|
Establish one sequence-token interpretation for version lookup and conversion,
|
||||||
|
using shared semantic-path machinery rather than independent numeric spelling
|
||||||
|
rules. Preserve explicit mapping-key versus index intent and exact string matching
|
||||||
|
for mapping keys. Do not simply substitute a helper without checking its semantics:
|
||||||
|
metadata matching also compares pointer spellings before interpreting indices.
|
||||||
|
|
||||||
|
Decide explicitly whether noncanonical signed sequence tokens are supported or
|
||||||
|
rejected. If retaining current lookup behavior, decoding must identify the same
|
||||||
|
node. If tightening it to canonical JSON Pointer sequence indices, apply that rule
|
||||||
|
consistently to lookup and matching, document the compatibility impact, and ensure
|
||||||
|
the checker cannot report a present version that conversion refuses to recognize.
|
||||||
|
Do not broaden mapping-key matching or unrelated scalar coercion as a workaround.
|
||||||
|
|
||||||
|
### Acceptance and verification
|
||||||
|
|
||||||
|
- Add public-load regressions for integer and string input with canonical and
|
||||||
|
signed parsed sequence tokens, asserting checker presence/text and decoding
|
||||||
|
outcomes under the chosen policy.
|
||||||
|
- Retain coverage for literal numeric/signed mapping keys, escaped keys, explicit
|
||||||
|
`Key`/`Index` distinctions, and unrelated string fields.
|
||||||
|
- Verify lookup, version checking, and conversion share the chosen semantics;
|
||||||
|
check other users of any shared path helper changed by the fix.
|
||||||
|
- Run `go test . -run 'Version|Decode|Path'`, `./scripts/check.sh`, and the full
|
||||||
|
release checker before selecting a corrected release candidate.
|
||||||
|
|
||||||
|
## Review validation and remaining release work
|
||||||
|
|
||||||
|
The full release checker passed on Linux at the review baseline with Go 1.25.5
|
||||||
|
and Go 1.26.7, including race tests, examples, documentation links, and cross-builds.
|
||||||
|
The independent public-API probe above exposed behavior outside that suite.
|
||||||
|
No production code or existing tests were modified during review.
|
||||||
|
|
||||||
|
Release selection, version-matched notes, retirement of completed roadmap material,
|
||||||
|
and exact-candidate native Linux/macOS validation remain governed by the
|
||||||
|
[release procedure](../release.md). They are separate from resolving R1; older
|
||||||
|
release validation is not evidence for a newly changed candidate.
|
||||||
@@ -434,5 +434,5 @@ func schemaIndex(segment pathSegment) (int, bool) {
|
|||||||
return 0, false
|
return 0, false
|
||||||
}
|
}
|
||||||
index, err := strconv.Atoi(segment.key)
|
index, err := strconv.Atoi(segment.key)
|
||||||
return index, err == nil && index >= 0
|
return index, err == nil && index >= 0 && strconv.Itoa(index) == segment.key
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -2,7 +2,6 @@ package configkit
|
|||||||
|
|
||||||
import (
|
import (
|
||||||
"reflect"
|
"reflect"
|
||||||
"strconv"
|
|
||||||
|
|
||||||
"gitea.maximumdirect.net/eric/ConfigKit/internal/document"
|
"gitea.maximumdirect.net/eric/ConfigKit/internal/document"
|
||||||
)
|
)
|
||||||
@@ -238,26 +237,7 @@ func isIntegerVersion(node *document.Node) bool {
|
|||||||
}
|
}
|
||||||
|
|
||||||
func pathMatches(configured *Path, current Path) bool {
|
func pathMatches(configured *Path, current Path) bool {
|
||||||
if configured == nil || !configured.valid() || !current.valid() || len(configured.segments) != len(current.segments) {
|
return configured != nil && current.valid() && matchesPathShape(*configured, current)
|
||||||
return false
|
|
||||||
}
|
|
||||||
for i, expected := range configured.segments {
|
|
||||||
actual := current.segments[i]
|
|
||||||
if expected.kind == pathSegmentParsed {
|
|
||||||
if actual.kind == pathSegmentIndex {
|
|
||||||
if expected.key != strconv.Itoa(actual.index) {
|
|
||||||
return false
|
|
||||||
}
|
|
||||||
} else if expected.key != actual.key {
|
|
||||||
return false
|
|
||||||
}
|
|
||||||
continue
|
|
||||||
}
|
|
||||||
if expected.kind != actual.kind || expected.key != actual.key || expected.index != actual.index {
|
|
||||||
return false
|
|
||||||
}
|
|
||||||
}
|
|
||||||
return true
|
|
||||||
}
|
}
|
||||||
|
|
||||||
func documentLocationPointer(location document.Location) *Location {
|
func documentLocationPointer(location document.Location) *Location {
|
||||||
|
|||||||
90
version_path_test.go
Normal file
90
version_path_test.go
Normal file
@@ -0,0 +1,90 @@
|
|||||||
|
package configkit
|
||||||
|
|
||||||
|
import "testing"
|
||||||
|
|
||||||
|
func TestVersionPathsShareCanonicalSequenceSemantics(t *testing.T) {
|
||||||
|
t.Parallel()
|
||||||
|
type config struct {
|
||||||
|
Versions []string `yaml:"versions"`
|
||||||
|
}
|
||||||
|
for _, token := range []string{"0", "+0", "-0", "00", "+1"} {
|
||||||
|
for _, value := range []string{"4", "'4'"} {
|
||||||
|
t.Run(token+"/"+value, func(t *testing.T) {
|
||||||
|
path, err := ParsePath("/versions/" + token)
|
||||||
|
if err != nil {
|
||||||
|
t.Fatal(err)
|
||||||
|
}
|
||||||
|
var checked Version
|
||||||
|
result, err := Load[config](Bytes("version.yml", []byte("versions: ["+value+"]\n")), Options[config]{Definition: Definition[config]{
|
||||||
|
Version: &VersionRule{Path: path, Check: func(v Version, _ *Validation) error { checked = v; return nil }},
|
||||||
|
}})
|
||||||
|
want := Version{}
|
||||||
|
if token == "0" {
|
||||||
|
want = Version{Present: true, Text: "4"}
|
||||||
|
}
|
||||||
|
if checked != want {
|
||||||
|
t.Fatalf("checked = %#v, want %#v", checked, want)
|
||||||
|
}
|
||||||
|
if token != "0" && value == "4" {
|
||||||
|
requireDiagnostic(t, err, CodeDecodeType, "/versions/0")
|
||||||
|
return
|
||||||
|
}
|
||||||
|
if err != nil || result.Config.Versions[0] != "4" {
|
||||||
|
t.Fatalf("result = %#v, err = %v", result, err)
|
||||||
|
}
|
||||||
|
_, found := result.Metadata().Lookup(path)
|
||||||
|
if found != (token == "0") {
|
||||||
|
t.Fatalf("metadata lookup = %v", found)
|
||||||
|
}
|
||||||
|
})
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
func TestVersionPathsPreserveLiteralMappingKeysAndSegmentIntent(t *testing.T) {
|
||||||
|
t.Parallel()
|
||||||
|
for _, key := range []string{"0", "+0", "-0", "00", "a/b~"} {
|
||||||
|
t.Run(key, func(t *testing.T) {
|
||||||
|
path, err := ParsePath(Keys(key).String())
|
||||||
|
if err != nil {
|
||||||
|
t.Fatal(err)
|
||||||
|
}
|
||||||
|
result, err := Load[map[string]string](Bytes("version.yml", []byte("'"+key+"': 4\n")), Options[map[string]string]{Definition: Definition[map[string]string]{
|
||||||
|
Version: &VersionRule{Path: path, Check: func(v Version, _ *Validation) error {
|
||||||
|
if !v.Present || v.Text != "4" {
|
||||||
|
t.Errorf("checked = %#v", v)
|
||||||
|
}
|
||||||
|
return nil
|
||||||
|
}},
|
||||||
|
}})
|
||||||
|
if err != nil || result.Config[key] != "4" {
|
||||||
|
t.Fatalf("result = %#v, err = %v", result, err)
|
||||||
|
}
|
||||||
|
if _, ok := result.Metadata().Lookup(path); !ok {
|
||||||
|
t.Fatal("literal key missing from metadata")
|
||||||
|
}
|
||||||
|
})
|
||||||
|
}
|
||||||
|
for _, test := range []struct {
|
||||||
|
name string
|
||||||
|
path Path
|
||||||
|
input string
|
||||||
|
}{
|
||||||
|
{"key cannot address sequence", Keys("versions", "0"), "versions: [4]\n"},
|
||||||
|
{"index cannot address mapping", Keys("versions").Index(0), "versions: {'0': 4}\n"},
|
||||||
|
} {
|
||||||
|
t.Run(test.name, func(t *testing.T) {
|
||||||
|
type config struct {
|
||||||
|
Versions any `yaml:"versions"`
|
||||||
|
}
|
||||||
|
_, err := Load[config](Bytes("version.yml", []byte(test.input)), Options[config]{Definition: Definition[config]{Version: &VersionRule{Path: test.path, Check: func(v Version, validation *Validation) error {
|
||||||
|
if v.Present {
|
||||||
|
t.Errorf("wrong segment kind found version: %#v", v)
|
||||||
|
}
|
||||||
|
validation.Error(Keys("versions"), "version absent")
|
||||||
|
return nil
|
||||||
|
}}}})
|
||||||
|
requireDiagnostic(t, err, CodeVersionError, "/versions")
|
||||||
|
})
|
||||||
|
}
|
||||||
|
}
|
||||||
Reference in New Issue
Block a user