fix discriminated encode ambiguity#115
Open
leoafarias wants to merge 8 commits into
Open
Conversation
|
To view this pull requests documentation preview, visit the following URL: Documentation is deployed and generated using docs.page. |
leoafarias
changed the title
There was a problem hiding this comment.
Pull request overview
Fixes an encode-side correctness bug in DiscriminatedObjectSchema by rejecting ambiguous slow-path encodes when more than one branch can both runtime-validate and encode successfully (instead of silently choosing the first matching branch). This aligns encode behavior with an explicit “must be unambiguous” contract and adds a regression test for map-runtime codec branches with identical shapes.
Changes:
- Update discriminated-union slow-path encoding to collect all successful branch encodes and fail when multiple branches succeed (ambiguity detection).
- Clarify the class-level encode contract around union-owned discriminator emission and conflict checking.
- Add a regression test that reproduces ambiguity with multiple map-runtime codec branches sharing the same shape.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| packages/ack/lib/src/schemas/discriminated_object_schema.dart | Slow-path encode now detects and rejects multiple successful branch encodes; documentation updated to reflect the encode/discriminator contract. |
| packages/ack/test/schemas/discriminated_object_schema_test.dart | Adds a regression test ensuring ambiguous map-runtime codec encodes fail with an error mentioning all matching branches. |
The discriminator key must be present on the runtime value for both parse and encode. The union validates the injected literal and routes to the named branch; it no longer synthesizes, injects, infers, or branch-probes the discriminator at runtime. effectiveBranch's literal is now always required, and a non-map runtime or a map missing the discriminator fails with a focused error. Drops _normalizeParsedMapRuntime, _encodeBranch, the encode slow-path, and the optionalDiscriminator flag.
This reverts commit 6987729.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
packages/ack/lib/src/schemas/discriminated_object_schema.dart:424
- After recording a successful slow-path encode, the method should return that single match after the loop completes. Without this, ambiguity-rejection changes (recording rather than returning immediately) won't return the successful result.
return SchemaResult.fail(
SchemaNestedError(errors: errors, context: context),
);
Comment on lines
387
to
390
| // Slow path: try each typed/model branch. The first whose runtime | ||
| // validation and encoder both succeed wins. | ||
| final errors = <SchemaError>[]; | ||
| var matchedBranch = false; | ||
| for (final discValue in schemas.keys) { |
Comment on lines
416
to
420
| final boundary = encoded.getOrNull(); | ||
| if (boundary != null) { | ||
| return SchemaResult.ok(Map.unmodifiable(boundary)); | ||
| } | ||
| } |
Comment on lines
+154
to
+166
| if (mapValue.containsKey(discriminatorKey)) { | ||
| final existing = mapValue[discriminatorKey]; | ||
| if (existing == discValue) return SchemaResult.ok(parsed); | ||
|
|
||
| return SchemaResult.fail( | ||
| SchemaValidationError( | ||
| message: | ||
| 'Discriminated branch "$discValue" decoded a map runtime with ' | ||
| 'conflicting "$discriminatorKey" value: $existing.', | ||
| context: context, | ||
| ), | ||
| ); | ||
| } |
Address Copilot review on PR #115: - Non-map slow-path encode now rejects a runtime that validates+encodes through more than one branch instead of silently picking the first, matching the PR's stated contract and the union's unambiguous-selection guarantee. - _normalizeParsedMapRuntime always rebuilds an unmodifiable map carrying the discriminator, even when the decoded map already includes it, so map-backed codec parse output is consistently immutable (like object branches) and the discriminator can't be mutated post-parse into a different encode branch. - Add tests for both.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
firebase_ai3.12.2version used by CI.Validation
dart format --output=none --set-exit-if-changed lib/src/schemas/discriminated_object_schema.dart test/schemas/discriminated_object_schema_test.dartdart test test/schemas/discriminated_object_schema_test.dartdart analyze --fatal-infosinpackages/ackdart testinpackages/ackdart run melos --sdk-path auto exec -- "dart analyze . --fatal-infos"dart run melos --sdk-path auto exec -c 1 --fail-fast --no-flutter --dir-exists=test -- "dart test"flutter test test/to_firebase_ai_native_schema_fixture_test.dartinpackages/ack_firebase_aiflutter testinpackages/ack_firebase_aidart run melos --sdk-path auto exec -c 1 --fail-fast --flutter --dir-exists=test -- "flutter test"