fix: validation edge cases, schema consistency, and tooling reliability#124
Merged
Merged
Conversation
Fix numeric/IP/date-time constraint validation, safeParse exception handling, union/enum/lazy-schema edge cases, and generator diagnostics; clean up workspace tooling and remove stale IDE/build artifacts.
|
To preview the documentation for this pull request, visit the following URL:
|
There was a problem hiding this comment.
Pull request overview
This PR hardens the Ack monorepo across three main areas: runtime schema validation correctness/immutability (packages/ack), generator alignment and diagnostics (packages/ack_generator), and workspace/release/tooling reliability (scripts, Melos, docs).
Changes:
- Tighten and fail-fast runtime validation (e.g.,
multipleOf, strict RFC 3339 datetime including leap-second handling rules, IPv6 parsing) and make schema factories snapshot caller-owned collections for immutability. - Fix lazy-schema recursion-depth accounting so wrapper/fluent-copy paths are correctly bounded, and improve schema/collection equality behavior.
- Modernize generator/tooling: align with current
source_generror types, improve generator failure messaging, make changelog/API scripts more reliable and add root script tests; update workspace scripts/docs accordingly.
Reviewed changes
Copilot reviewed 73 out of 76 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/README.md | Switch Node install instructions to npm ci and point users to dart run melos run --list. |
| tools/npm-shrinkwrap.json | Update pinned Node tool dependencies (ajv/fast-uri/zod). |
| test/scripts/release_changelog_test.dart | Add unit/integration coverage for atomic changelog batch updates and exact heading matching. |
| test/scripts/api_check_test.dart | Add script-level tests ensuring activation failures and stale report handling fail correctly. |
| setup.sh | Simplify setup to validate Flutter/Dart availability, bootstrap via Melos, and optionally install Node tooling via npm ci. |
| scripts/update_release_changelog.dart | Make updates atomic across package changelogs; delegate content logic to pure helper. |
| scripts/src/release_changelog.dart | Introduce pure updateReleaseChangelog helper (exact version heading match, remove duplicates, idempotent). |
| scripts/setup.sh | Remove legacy setup script. |
| scripts/api_check.dart | Expand API checks to all packages, pin dart_apitool, delete stale reports, and treat missing reports/errors as failures. |
| README.md | Update workspace command examples to dart run melos ... and remove --delete-conflicting-outputs. |
| pubspec.yaml | Adjust Melos scripts (run via dart run melos ...), add root dart test test, disable IntelliJ generation. |
| PUBLISHING.md | Update release steps to use dart run melos ... and updated script flows. |
| packages/ack/test/schemas/transform_optional_nullable_test.dart | Clean up transform semantics tests; remove debug prints; strengthen expectations. |
| packages/ack/test/schemas/schema_equality_test.dart | Add equality tests for built-in transformers/codecs. |
| packages/ack/test/schemas/refine_test.dart | Add coverage ensuring safeParse converts refinement exceptions into failures. |
| packages/ack/test/schemas/lazy_schema_test.dart | Add regression tests ensuring recursion caps apply through aliases/wrappers/fluent copies and encode paths. |
| packages/ack/test/schemas/extensions/numeric_extensions_test.dart | Add tests for multipleOf validation (negative/non-finite divisors; tiny/large scale behavior). |
| packages/ack/test/schemas/datetime_validation_test.dart | Add strict datetime validation tests (normalized calendar/time components; leap-second rules). |
| packages/ack/test/schemas/core_schema_test.dart | Add coverage for rejecting unions that can produce nullable list items. |
| packages/ack/test/schemas/any_of_null_and_default_test.dart | Change empty anyOf behavior expectation to fail-fast at construction. |
| packages/ack/test/reference_semantics_test.dart | Add invariant tests for snapshotting factories, strict JSON Schema keyword merging, and exception-to-failure behavior. |
| packages/ack/test/documentation/error_recovery_examples_test.dart | Remove leftover debug comments. |
| packages/ack/test/constraints/string_ip_constraint_test.dart | Update IP tests for IPv6 behavior (case, scope rejection, invalid strings) + construction validation. |
| packages/ack/melos_ack.iml | Remove committed IntelliJ module artifact. |
| packages/ack/lib/src/validation/schema_result.dart | Clarify getOrNull/ifOk nullable-payload semantics in docs. |
| packages/ack/lib/src/utils/collection_utils.dart | Fix deepEquals for Set to consume matches once (multiset-like matching). |
| packages/ack/lib/src/schemas/schema.dart | Convert constraint/refinement/validation exceptions into contextual failures; add equality for constraint message override. |
| packages/ack/lib/src/schemas/object_schema.dart | Snapshot object properties map via Map.unmodifiable. |
| packages/ack/lib/src/schemas/list_schema.dart | Reject nullable item schemas using acceptsNull (covers nullable unions). |
| packages/ack/lib/src/schemas/lazy_schema.dart | Track recursion via per-schema token and dedicated recursion context; preserve token across copyWith. |
| packages/ack/lib/src/schemas/extensions/string_schema_extensions.dart | Validate IP version at construction; use stable tear-offs for built-in transforms (improves equality). |
| packages/ack/lib/src/schemas/enum_schema.dart | Document immutability expectations for low-level constructor; prefer validated factory. |
| packages/ack/lib/src/schemas/codec_schema.dart | Include decoder/encoder identity in ==/hashCode to fix codec schema equality. |
| packages/ack/lib/src/schemas/any_of_schema.dart | Document immutability expectations for low-level constructor; prefer validated factory. |
| packages/ack/lib/src/schema_model/ack_schema_model_builder.dart | Preserve conflicting JSON Schema keywords by pushing disagreements into allOf instead of overwriting. |
| packages/ack/lib/src/constraints/string_ip_constraint.dart | Replace IPv6 mega-regex with Uri.parseIPv6Address. |
| packages/ack/lib/src/constraints/pattern_constraint.dart | Replace permissive DateTime.tryParse datetime check with strict regex + field-range + calendar validity + leap-second rules. |
| packages/ack/lib/src/constraints/comparison_constraint.dart | Add fail-fast argument validation; improve multipleOf to use exact int path + magnitude-relative tolerance for doubles; emit exact list/object constraint keywords. |
| packages/ack/lib/src/ack.dart | Make factories snapshot and validate inputs (anyOf/enum); fix datetime leap-second codec rule; use stable tear-offs for codec callbacks. |
| packages/ack/CHANGELOG.md | Add Unreleased notes documenting key fixes/behavior changes. |
| packages/ack_json_schema_builder/melos_ack_json_schema_builder.iml | Remove committed IntelliJ module artifact. |
| packages/ack_generator/tool/update_goldens.dart | Remove obsolete golden-update utility. |
| packages/ack_generator/tool/show_generator_output.dart | Remove obsolete debug utility. |
| packages/ack_generator/test/test_utils/generation_test_utils.dart | Add shared helper for asserting generation failures via severe logs. |
| packages/ack_generator/test/run_tests.sh | Remove obsolete test runner script. |
| packages/ack_generator/test/integration/example_folder_build_test.dart | Remove --delete-conflicting-outputs and noisy prints; keep structural output checks. |
| packages/ack_generator/test/integration/ack_type_getter_test.dart | Update nullable list element behavior: now rejected (matches runtime contract) and use shared failure helper. |
| packages/ack_generator/test/integration/ack_type_discriminated_test.dart | Refactor to shared generation-failure helper. |
| packages/ack_generator/test/integration/ack_type_cross_file_resolution_test.dart | Refactor to shared generation-failure helper. |
| packages/ack_generator/test/bugs/schema_variable_bugs_test.dart | Update tests to current source_gen error type (InvalidGenerationSource). |
| packages/ack_generator/test/block_emission_test.dart | Remove exploratory test file. |
| packages/ack_generator/README.md | Update build_runner instructions to drop --delete-conflicting-outputs. |
| packages/ack_generator/melos_ack_generator.iml | Remove committed IntelliJ module artifact. |
| packages/ack_generator/lib/src/generator.dart | Align error type to InvalidGenerationSource; remove env-gated debug file writing; include generated output in error message. |
| packages/ack_generator/lib/src/analyzer/schema_ast_analyzer.dart | Align error type; reject nullable list element schemas (direct or referenced) during analysis. |
| packages/ack_generator/CHANGELOG.md | Add Unreleased notes for generator alignment/diagnostics and nullable list element rejection. |
| packages/ack_generator/build.yaml | Remove unused/obsolete builder config blocks. |
| packages/ack_firebase_ai/README.md | Update Melos command examples to dart run melos .... |
| packages/ack_firebase_ai/melos_ack_firebase_ai.iml | Remove committed IntelliJ module artifact. |
| packages/ack_annotations/README.md | Update build_runner instructions to drop --delete-conflicting-outputs. |
| packages/ack_annotations/melos_ack_annotations.iml | Remove committed IntelliJ module artifact. |
| melos_ack_workspace.iml | Remove committed IntelliJ module artifact. |
| llms.txt | Update canonical API notes (1.0.1), document leap-second behavior, snapshotting, non-throwing safeParse, and strict construction rules. |
| example/README.md | Update bootstrap/build instructions to use dart run melos ... and drop --delete-conflicting-outputs. |
| example/melos_ack_example.iml | Remove committed IntelliJ module artifact. |
| docs/getting-started/installation.mdx | Drop --delete-conflicting-outputs from build instructions. |
| docs/core-concepts/validation.mdx | Document leap-second acceptance for string datetime schema and rejection for datetime codec. |
| docs/core-concepts/typesafe-schemas.mdx | Update build command and clarify list item nullability rule (Ack.list(item).nullable()). |
| docs/core-concepts/schemas.mdx | Document leap-second distinction between Ack.string().datetime() and Ack.datetime(). |
| docs/core-concepts/codecs.mdx | Document leap-second rejection in Ack.datetime() codec and recommended alternative. |
| docs/api-reference/index.mdx | Update datetime docs and build_runner command examples. |
| coverage/lcov.info | Remove committed build artifact. |
| CONTRIBUTING.md | Update setup and common commands to use dart run melos .... |
| CHANGELOG.md | Add root Unreleased summary for this set of fixes. |
| .gitignore | Ignore *.iml and stop keeping committed coverage outputs. |
| .github/copilot-instructions.md | Update repo contributor guidance to match new commands and remove golden-tool references. |
Files not reviewed (1)
- tools/npm-shrinkwrap.json: Generated file
Comments suppressed due to low confidence (2)
packages/ack/lib/src/schemas/lazy_schema.dart:90
LazySchema.parseWithContextenters a dedicated recursion context, but then applies the lazy schema’s own constraints/refinements using the parentcontext. That drops the recursion token from the active context chain and can misattribute errors (and potentially allow recursive validation triggered by constraints/refinements to bypass the depth cap). UserecursionContextfor the post-parseapplyConstraintsAndRefinementscall.
final recursionContext = _enterRecursion(value, context);
final depthResult = _checkMaxDepth<Runtime>(recursionContext);
if (depthResult != null) return depthResult;
final target = _target;
final result = target.parseWithContext(value, recursionContext);
if (result.isFail) return SchemaResult.fail(result.getError());
final runtime = result.getOrNull();
if (runtime == null) return SchemaResult.ok(null);
return applyConstraintsAndRefinements(runtime, context);
}
packages/ack/lib/src/schemas/lazy_schema.dart:113
- Same issue in
validateRuntimeWithContext: constraints/refinements are applied with the parentcontextinstead of therecursionContextcreated for this lazy invocation. This can drop the recursion marker and lose attribution in failures.
final recursionContext = _enterRecursion(value, context);
final depthResult = _checkMaxDepth<Runtime>(recursionContext);
if (depthResult != null) return depthResult;
final target = _target;
final result = target.validateRuntimeWithContext(value, recursionContext);
if (result.isFail) return SchemaResult.fail(result.getError());
final runtime = result.getOrNull();
if (runtime == null) return SchemaResult.ok(null);
return applyConstraintsAndRefinements(runtime, context);
}
Comment on lines
+142
to
+147
| result = await Process.run('dart', [ | ||
| 'pub', | ||
| 'global', | ||
| 'run', | ||
| 'dart_apitool:main', | ||
| 'diff', |
Comment on lines
+121
to
+129
| final recursionContext = _enterRecursion(value, context); | ||
| final depthResult = _checkMaxDepth<Boundary>(recursionContext); | ||
| if (depthResult != null) return depthResult; | ||
|
|
||
| final ownChecked = applyConstraintsAndRefinements(value, context); | ||
| if (ownChecked.isFail) return SchemaResult.fail(ownChecked.getError()); | ||
|
|
||
| return _target.encodeWithContext(ownChecked.getOrThrow()!, context); | ||
| final target = _target; | ||
| return target.encodeWithContext(ownChecked.getOrThrow()!, recursionContext); |
…age list Behavior-preserving simplifications on top of the validation/schema/tooling fixes. Each item removes a single source of duplication or redundant state: - ack.dart: extract _requireNonEmpty/_requireUniqueBy for the enumValues, enumString, and anyOf argument guards - schema.dart: extract _failFromThrown for the three thrown-error catch blocks - codec_schema.dart: drop redundant _encoderIdentity (provably identical to _encoder in every path; _decoderIdentity retained as it fixes a real bug) - lazy_schema.dart: drop write-only _LazyRecursionContext.owner (duplicated the inherited SchemaContext.schema) - ack_schema_model_builder.dart: hoist a single const _deepEquality - schema_ast_analyzer.dart: extract _rejectIfReferencesNullableSchema (2 sites) - api_check.dart: extract _writeProcessStderr - update_release_changelog.dart: drop redundant record `path` field - scripts: share publishableAckPackages across api_check and update_release_changelog (new scripts/src/workspace_packages.dart) - enum_schema/any_of_schema: point docs to the canonical AckSchema policy dart analyze clean; ack (955) + ack_generator (127) + scripts tests pass.
Split runtime behavior changes out of the Fixed/Changed changelog sections into dedicated "Behavior changes" sections with migration notes, noting no public API changed (verified against 1.0.1 with dart_apitool). Document that multipleOf checks integers exactly and doubles with a scale-relative (~4 ULP) tolerance, and record the rationale for the relative tolerance as a code comment so it is not reverted to a fixed epsilon.
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.
fix: validation edge cases, schema consistency, and tooling reliability
Summary
Hardens ACK's runtime validation, makes schema construction fail fast on invalid
configuration, snapshots caller-owned collections so schemas are immutable, fixes
lazy-schema recursion-depth accounting, aligns the generator with the current
source_genAPI, and makes the release/API tooling reliable and unit-tested.No public API is added. Several validation paths now reject inputs that
previously passed or silently misbehaved — see Behavior changes below.
Validation correctness (
packages/ack)multipleOf— replaced the fixed1e-10epsilon overremainder()with anexact integer path plus a magnitude-relative tolerance for doubles. The fixed
epsilon produced both false positives and false negatives depending on operand
scale (large numbers, currency
0.01, percentage0.1multiples).DateTime.tryParseheuristicwith a strict regex plus explicit field-range and calendar-validity checks, and
added leap-second handling.
Ack.string().datetime()accepts and preservesannounced leap seconds (
:60);Ack.datetime()rejects them, because Dartnormalizes
:60to the next minute and cannot round-trip the instant.Uri.parseIPv6Address,which is correct and maintained (the regex had known false positives/negatives).
bounds must be finite,
numberRangerequiresmin <= max,multipleOfmust befinite and
> 0, andstring().ip(version:)must be4or6. Invalid configused to build nonsensical schemas that failed silently or at odd times.
contextual
SchemaValidationError, and top-levelparse/safeParseno longerthrow, restoring the non-throwing
safeParsecontract.Schema consistency & immutability (
packages/ack)Ack.anyOf,Ack.enumValues,Ack.enumString,Ack.object)snapshot inputs with
List/Map.unmodifiable; empty unions and empty/duplicateenum inputs are rejected. Callers could previously mutate the passed list after
construction and corrupt a live schema.
CodecSchemanow factors decoder/encoder identity into==/hashCode, and the built-in codecs use top-level tear-offs instead of freshclosures so equal codecs compare equal (two
Ack.datetime()used to be unequal).deepEqualsonSetnow does multiset matching;_ConstraintMessageOverridegained value equality — both fixed order/duplicate-sensitive equality bugs.
allOfinstead of the last constraint silently overwriting; exact list/objectconstraints now emit
min/maxItemsandmin/maxProperties.Ack.list(and unions feeding it) reject nullable item schemas; nullabilitybelongs on the list (
Ack.list(item).nullable()).Lazy schema recursion (
packages/ack)Recursion depth is now tracked with a per-schema token carried by a dedicated
_LazyRecursionContextand preserved acrosscopyWith. The oldidentical(context.schema, this)check miscounted once a lazy schema was copiedby a fluent method or wrapped, so
maxDepthdid not bound recursion through thosepaths.
Generator (
packages/ack_generator)InvalidGenerationSourceError→InvalidGenerationSource(the currentsource_genname, already inside the allowed>=3.0.0range — no dependencybump).
contract; removed the env-gated debug-file writer (drops the
dart:iodependency) and folds the generated source directly into the error message.
Tooling & workspace
api_check.dart— pinsdart_apitool, runs it viadart pub global run(no
PATHassumption), checks all five packages, deletes stale reports first,and exits non-zero on activation failure, API changes, or a missing report.
update_release_changelog.dart— extracted a pure, unit-testedupdateReleaseChangelog; heading matching is now anchored (no morecontains()false hits such as
1.0.0matching1.0.0-beta.1); the batch is atomic —nothing is written if any changelog fails validation.
dart run melos …;npm install→npm ci;IntelliJ project-file generation disabled.
test/scriptsnow runs in CI viamelos run test.coverage/lcov.info,*.iml) and added*.imlto.gitignore.Docs
Updated to describe leap-second behavior, list nullability, non-throwing
safeParse, snapshotting factories, andbuild_runner build(dropped--delete-conflicting-outputs).Behavior changes (call out in release notes)
The fail-fast validations and the stricter date-time / IPv6 /
multipleOf/ enumrules reject some inputs that previously passed. All are corrections, but code
relying on the lax behavior will now see
ArgumentErrors or validation failures.Testing
dart analyze --fatal-infosclean. All tests pass:packages/ack955,packages/ack_generator127, roottest/scripts7.