Resolve v0.1.0 open questions and fix production-readiness issues
POC: /workspace/alktype-builder-poc/ (18/18 tests pass, findings in
docs/research/alktype-builder-poc/findings.md). Round 2 adds the SFTP
Packet validate_bytes tests (7 new: valid Init/Read/Write/Status,
short buffer, unknown discriminator, over-maxLength Bytes).
Open questions resolved (OQ-004 through OQ-008):
- OQ-004: Discriminator::Field name is String (already implemented;
docs updated to mark resolved)
- OQ-005: Both union discriminator kinds return the same shape:
{__discriminator, ...variant-fields}. Field-name path also had a
real offset bug (returned start, not end) - fixed.
- OQ-006: builder.md Example 3 now wraps the Union in a
Schema::struct_().field("payload", ...) and merges $defs via
Definitions::merge_into (matches the engine's AlkType:Struct-at-root
constraint and the SFTP wire shape)
- OQ-007: Bytes materialization is array-of-u8 (Value::Array of
Value::Number, one entry per byte 0..=255). BytesValidator accepts
both Value::String (validate_json) and Value::Array (validate_bytes).
maxLength = max byte count. Replaces the lossy from_utf8_lossy path
that corrupted non-UTF-8 bytes and broke maxLength semantics.
- OQ-008 (new): UnionValidator now dispatches to variant schemas via
sub-validators built at factory time. AlkTypeEngine::compile calls
schema::inline_union_variant_refs before build_validator to inline
$refs in union mapping entries (necessary because union_factory
receives the union node, but $defs live at the schema root).
Production-readiness fixes in src/ (no stubs/hedges in a published crate):
- materialize.rs: Record stub -> full count-prefixed key/value pair
implementation per schema-layer.md TRecord
- materialize.rs: root_of() was broken (returned the current node, not
the schema root) -> root schema threaded through every recursive call
so resolve_ref_or_inline can resolve $refs for nested composites
- builder.rs: LengthPrefixed encoding setter was a no-op when the
keyword was already in object form -> complete the branch (updates
the encoding entry in place for both LengthPrefixed and OffsetIndirect)
- builder.rs, engine.rs: POC-referencing comments cleaned up; the
round-trip test's or_else fallback (papering over write_field being
aligned-only) replaced with direct byte writes
Documentation:
- builder.md: Example 3 updated; Discriminator::Field spec shows String;
Open Questions section updated (OQ-004 resolved)
- validation.md: AlkType:Bytes and AlkType:Union validator descriptions
updated for array-of-u8 form and variant dispatch
- open-questions.md: OQ-004/005/006/007/008 marked resolved; new
Validation theme entries
- questions/004-008: individual OQ files updated with resolutions
- findings.md: round 2 results documented
Verification:
- cargo test: 369 -> 391 tests pass (22 new: 14 materialize, 5
inline_union_variant_refs, 3 validation/builder)
- cargo clippy --all-targets: clean
- POC: 11 -> 18 tests (7 new SFTP Packet tests); all pass
This commit is contained in:
1 parent
c6893eece8
commit
c0217d91a8
15 files changed
+1553
-493
No files matched your search
@@ -2,26 +2,18 @@
|
||||
|
||||
- **Origin**: [../builder.md](../builder.md) §"Open Questions" (raised
|
||||
during ADR-009 spec drafting)
|
||||
- **Status**: open
|
||||
- **Status**: resolved
|
||||
- **Door type**: Two-way (the field is internal to the `Discriminator`
|
||||
enum; changing the type is a local refactor with no schema-level impact)
|
||||
- **Priority**: low
|
||||
- **Impacts**: Blocks finalizing the `Discriminator::Field` variant's
|
||||
signature in `src/builder.rs`. Does NOT block any current POC — the
|
||||
chunk header uses `Discriminator::Byte`, not `Discriminator::Field`.
|
||||
The field-name discriminator is exercised by the SFTP Packet POC
|
||||
(next round), so this must be resolved before that POC.
|
||||
- **Investigation target**: Decide whether the `name` field of
|
||||
`Discriminator::Field` should be `&str` (borrows) or `String` (owns).
|
||||
The builder consumes `Self` on each setter (`.field()`, `.mapping()`,
|
||||
etc.), so the discriminator's `name` needs to outlive the `Schema`
|
||||
it's embedded in. `&str` would require a lifetime parameter on
|
||||
`Discriminator` (and transitively on `Schema::union_`); `String`
|
||||
owns its content and keeps `Discriminator` `'static`. The likely
|
||||
resolution is `String` for ownership simplicity — the builder owns
|
||||
its content until `.build()` produces `Value`, and `String` avoids
|
||||
lifetime pollution across the builder API.
|
||||
- **Resolution**: Not yet decided. Confirm during the next POC round
|
||||
(SFTP Packet) when `Discriminator::Field` is first exercised.
|
||||
- **Resolution**: `String`. The builder consumes `Self` on each setter
|
||||
(`.field()`, `.mapping()`, etc.), so `&str` would require a lifetime
|
||||
parameter on `Discriminator` (and transitively on `Schema::union_`),
|
||||
polluting the builder API. `String` owns its content, keeps
|
||||
`Discriminator` `'static`, and matches the rest of the builder's
|
||||
ownership model (the builder owns its content until `.build()`
|
||||
produces `Value`). Implemented in `src/builder.rs` since the initial
|
||||
v0.1.0 builder implementation; confirmed correct during the SFTP
|
||||
Packet POC (round 2) which first exercises `Discriminator::Field`.
|
||||
- **Cross-references**: [ADR-009](../decisions/009-builder-api.md),
|
||||
[builder.md](../builder.md)
|
||||
@@ -2,39 +2,27 @@
|
||||
|
||||
- **Origin**: [../../research/alktype-builder-poc/findings.md](../../research/alktype-builder-poc/findings.md)
|
||||
§"Open Questions surfaced" (raised during the v0.1.0 POC)
|
||||
- **Status**: open
|
||||
- **Status**: resolved
|
||||
- **Door type**: Two-way (the materializer is internal; the shape it
|
||||
produces is consumed only by the existing `jsonschema` validator,
|
||||
which accepts arbitrary objects — changing the shape is a local
|
||||
refactor)
|
||||
- **Priority**: medium
|
||||
- **Impacts**: Blocks the SFTP Packet `validate_bytes` POC (next round).
|
||||
The current `materialize_union_packed` returns inconsistent shapes:
|
||||
byte-offset discriminators produce `{ "__discriminator": <u32>,
|
||||
...variant-fields }`; field-name discriminators produce the struct
|
||||
fields directly (the discriminator is just another field). For
|
||||
`validate_bytes` callers, the shape matters for how the validator
|
||||
dispatches on the variant. Does NOT block the chunk header or call
|
||||
input schema (no unions in either).
|
||||
- **Investigation target**: Decide on one consistent shape for
|
||||
materialized unions. Two candidates:
|
||||
|
||||
1. **Both return a tagged object**: `{ "__discriminator": <value>,
|
||||
...variant-fields }` for both discriminator kinds. The validator
|
||||
sees the discriminator value and the variant fields in one
|
||||
object. This is what the byte-offset path already does.
|
||||
2. **Both return the variant struct directly** (no `__discriminator`
|
||||
wrapper): the discriminator is consumed during dispatch and not
|
||||
included in the materialized `Value`. The field-name path
|
||||
approximates this today.
|
||||
|
||||
Option 1 is more uniform and lets the validator re-check the
|
||||
discriminator; option 2 is cleaner but loses the discriminator in
|
||||
the `Value` tree. Resolve by working through the SFTP Packet
|
||||
`validate_bytes` example — what does the `jsonschema` validator need
|
||||
to see for a `AlkType:Union` field?
|
||||
- **Resolution**: Not yet decided. Work through the SFTP Packet POC
|
||||
to surface the validator's requirements.
|
||||
- **Resolution**: Both discriminator kinds return the same shape:
|
||||
`{ "__discriminator": <value>, ...variant-fields }`. The
|
||||
`__discriminator` entry carries the mapping key (stringified
|
||||
discriminator value for byte-offset, string value for field-name).
|
||||
For field-name discriminators, the original typed discriminator value
|
||||
is also preserved under the discriminator field's name (it's a
|
||||
regular field in the struct). If the variant materializes as a
|
||||
non-object (a leaf kind), it is nested under `"__variant"`.
|
||||
Implemented in `src/materialize.rs` (`materialize_union_packed`).
|
||||
|
||||
Additionally, the field-name path had a real correctness bug
|
||||
(`Ok((value, offset))` returned the start offset, not the end —
|
||||
subsequent fields after a field-name union would read from the wrong
|
||||
position). This is now fixed: the path returns the new offset after
|
||||
the variant struct.
|
||||
- **Cross-references**: [ADR-010](../decisions/010-generalized-validation-validate-bytes.md),
|
||||
`src/materialize.rs` (`materialize_union_packed`),
|
||||
[../../research/alktype-builder-poc/findings.md](../../research/alktype-builder-poc/findings.md)
|
||||
@@ -2,34 +2,17 @@
|
||||
|
||||
- **Origin**: [../../research/alktype-builder-poc/findings.md](../../research/alktype-builder-poc/findings.md)
|
||||
§"Findings" #1 (raised during the v0.1.0 POC)
|
||||
- **Status**: open
|
||||
- **Status**: resolved
|
||||
- **Door type**: Two-way (documentation fix — the builder already
|
||||
supports both shapes; the spec example just shows the wrong one)
|
||||
- **Priority**: low
|
||||
- **Impacts**: Blocks nothing functional. The builder spec's Example 3
|
||||
(SFTP Packet) currently shows a top-level `Schema::union_(...)`,
|
||||
which won't compile via `AlkTypeEngine::compile` — the engine
|
||||
requires `AlkType:Struct` at the top level (existing constraint from
|
||||
`OffsetMap::compute` / `SequentialReader::new`, ADR-002). The POC
|
||||
worked around this by wrapping the union in a
|
||||
`Schema::struct_().field("payload", Schema::union_(...))`, which is
|
||||
the realistic wire shape anyway (SFTP's `[length:u32][type:u8][payload]`
|
||||
is a struct with a union payload field). The spec example should
|
||||
match the realistic shape.
|
||||
- **Investigation target**: Update [../builder.md](../builder.md)
|
||||
Example 3 to wrap the `Union` in a `Struct`. Two options:
|
||||
|
||||
1. **Wrap in a struct** (preferred — matches the realistic wire
|
||||
shape): `Schema::struct_().field("payload", Schema::union_(...))`.
|
||||
2. **Document the constraint explicitly**: keep the top-level union
|
||||
in the example but add a note that `AlkTypeEngine::compile`
|
||||
requires `AlkType:Struct` at the root; a top-level `Union` is for
|
||||
`jsonschema`-only validation (no `validate_bytes`).
|
||||
|
||||
Option 1 is preferred — it shows the shape the engine actually
|
||||
consumes and matches the SFTP wire format.
|
||||
- **Resolution**: Not yet applied. Documentation fix in builder.md,
|
||||
not an implementation change.
|
||||
- **Resolution**: Applied. [../builder.md](../builder.md) Example 3
|
||||
now wraps the `Union` in a `Schema::struct_().field("payload", ...)`
|
||||
and merges `$defs` via `Definitions::merge_into`. This matches the
|
||||
realistic SFTP wire shape (`[type:u8][payload-struct]`) and the
|
||||
engine's constraint that `AlkTypeEngine::compile` requires
|
||||
`AlkType:Struct` at the top level (inherited from
|
||||
`OffsetMap::compute` / `SequentialReader::new`, ADR-002).
|
||||
- **Cross-references**: [ADR-009](../decisions/009-builder-api.md),
|
||||
[builder.md](../builder.md) §"Example 3: SFTP Packet union",
|
||||
[ADR-002](../decisions/002-two-layout-modes-packed-vs-aligned.md)
|
||||
@@ -2,43 +2,33 @@
|
||||
|
||||
- **Origin**: [../../research/alktype-builder-poc/findings.md](../../research/alktype-builder-poc/findings.md)
|
||||
§"Open Questions surfaced" (raised during the v0.1.0 POC)
|
||||
- **Status**: open
|
||||
- **Status**: resolved
|
||||
- **Door type**: One-way (the choice affects what the validator sees;
|
||||
changing it after consumers depend on the shape is a semantic break)
|
||||
- **Priority**: medium
|
||||
- **Impacts**: Blocks the SFTP use case for `validate_bytes`. SFTP
|
||||
packets have binary `handle` and `data` fields (`AlkType:Bytes`)
|
||||
that are not valid UTF-8. The current materializer uses
|
||||
`String::from_utf8_lossy` to convert raw bytes to a JSON string
|
||||
(because `jsonschema`'s `BytesValidator` expects a string — JSON
|
||||
has no native byte type; see schema-layer.md §TBytes). Lossy
|
||||
conversion corrupts non-UTF-8 bytes (replacing invalid sequences
|
||||
with `U+FFFD`), so the validator sees corrupted content. Does NOT
|
||||
block the chunk header (no `Bytes` fields) or call input schema
|
||||
(no AlkType kinds).
|
||||
- **Investigation target**: Decide how `AlkType:Bytes` should be
|
||||
materialized for validation. Three candidates:
|
||||
- **Resolution**: Array of u8. The materializer produces
|
||||
`Value::Array` of `Value::Number` (one entry per byte, 0..=255) for
|
||||
`AlkType:Bytes` fields. The `BytesValidator` accepts both
|
||||
`Value::String` (for `validate_json` consumers that hold a JSON
|
||||
representation) and `Value::Array` (for `validate_bytes`
|
||||
materialization). `maxLength` is interpreted as max byte count
|
||||
(array length for the array form, string byte length for the string
|
||||
form).
|
||||
|
||||
1. **Lossy UTF-8 (current)**: `String::from_utf8_lossy`. Simple;
|
||||
corrupts non-UTF-8. Acceptable only for UTF-8-only `Bytes` fields.
|
||||
2. **Base64 encode**: encode raw bytes as base64, validate against
|
||||
a `format: "base64"` constraint. Round-trips cleanly but the
|
||||
validator sees base64, not raw bytes — `maxLength` semantics
|
||||
change (base64 length ≠ byte length).
|
||||
3. **Array of u8**: materialize as `Value::Array` of `Value::Number`
|
||||
(one entry per byte). The validator sees the actual byte values;
|
||||
`maxLength` becomes "max array length." Semantically clean but
|
||||
produces large `Value` trees for big `Bytes` fields.
|
||||
The array form is the round-trippable form for non-UTF-8 bytes —
|
||||
`String::from_utf8_lossy` corrupts non-UTF-8 byte sequences
|
||||
(replacing invalid sequences with `U+FFFD`, which is 3 bytes in
|
||||
UTF-8, breaking `maxLength` semantics). Base64 encoding changes
|
||||
`maxLength` semantics (base64 length != byte length). Array of u8
|
||||
is semantically clean: each entry is the actual byte value, and
|
||||
`maxLength` is the actual byte count.
|
||||
|
||||
The right answer depends on what the `BytesValidator` (in
|
||||
`src/validation.rs`) should check. Today it checks string length
|
||||
against `maxLength` — which assumes the string form. If we keep
|
||||
the string form, base64 is the only round-trippable option. If we
|
||||
want raw bytes, the validator needs to accept `Value::Array` of
|
||||
numbers.
|
||||
- **Resolution**: Not yet decided. Work through the SFTP `handle`/
|
||||
`data` fields in the next POC round to surface what
|
||||
`validate_bytes` callers actually need.
|
||||
Trade-off: array-of-u8 produces larger `Value` trees for big
|
||||
`Bytes` fields (one `Value::Number` per byte). For the SFTP use
|
||||
case (handles up to 256 bytes, data up to 32768 bytes), this is
|
||||
acceptable. A future "validate bytes without materializing" path
|
||||
(two-way door, out of scope for v0.1.0 per ADR-010) would avoid the
|
||||
allocation for hot paths.
|
||||
- **Cross-references**: [ADR-010](../decisions/010-generalized-validation-validate-bytes.md),
|
||||
`src/materialize.rs` (`AlkTypeKind::Bytes` branch),
|
||||
`src/validation.rs` (`BytesValidator`),
|
||||
|
||||
@@ -0,0 +1,38 @@
|
||||
# OQ-008: `UnionValidator` variant dispatch — validate variant fields against variant schema
|
||||
|
||||
- **Origin**: Raised during the v0.1.0 POC round 2 (SFTP Packet
|
||||
`validate_bytes` POC). Surfaced when the over-`maxLength` `Bytes`
|
||||
test failed: the materializer read the bytes correctly, but the
|
||||
validator did not check the variant's `maxLength` constraint because
|
||||
the `UnionValidator` was structural-only (just `is_object`).
|
||||
- **Status**: resolved
|
||||
- **Door type**: One-way (the validator now recurses into variant
|
||||
schemas; consumers that depended on the structural-only behavior
|
||||
would see new validation errors for previously-accepted instances)
|
||||
- **Priority**: medium
|
||||
- **Resolution**: `UnionValidator` now builds a sub-validator for each
|
||||
variant at factory time (when the parent validator tree is
|
||||
constructed) and dispatches on the `__discriminator` field in the
|
||||
materialized instance at validation time. The sub-validators are
|
||||
full `jsonschema::Validator`s built via `build_validator` (so nested
|
||||
AlkType kinds inside variants are validated correctly).
|
||||
|
||||
To make this work, `AlkTypeEngine::compile` now calls
|
||||
`schema::inline_union_variant_refs` after `normalize_refs` and before
|
||||
`build_validator`. This inlines `$ref`s in union `mapping` entries by
|
||||
resolving them against the schema root — necessary because the
|
||||
`union_factory` receives the union node as `parent`, but `$defs` live
|
||||
at the schema root (not on the union node). After inlining, each
|
||||
variant in the `mapping` is a full inline schema, so the factory can
|
||||
build a sub-validator directly.
|
||||
|
||||
The materialized shape for unions is
|
||||
`{ "__discriminator": <value>, ...variant-fields }` (OQ-005
|
||||
resolution). The `UnionValidator` reads `__discriminator`, looks up
|
||||
the corresponding variant sub-validator, and validates the full
|
||||
instance (including the variant fields) against that sub-validator.
|
||||
- **Cross-references**: `src/validation.rs` (`UnionValidator`,
|
||||
`union_factory`), `src/schema.rs` (`inline_union_variant_refs`),
|
||||
`src/engine.rs` (`AlkTypeEngine::compile`),
|
||||
[OQ-005](005-union-materialization-shape.md),
|
||||
[ADR-010](../decisions/010-generalized-validation-validate-bytes.md)
|
||||
Reference in new issue
Block a user