Enable sync validation. - #1029
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces synchronous validation capabilities to the json_schema_builder package. It refactors the validation core to be synchronous (validateSync), while keeping the existing asynchronous validate method as a thin wrapper that automatically fetches remote references as needed. It also introduces SchemaRegistry.resolveSync, SchemaRegistry.fetch, and a new SchemaResolutionRequiredException thrown when a synchronous validation encounters an unresolved remote reference. Comprehensive tests have been added to verify the correctness and equivalence of the synchronous validation path. There are no review comments, so I have no additional feedback to provide.
gspencergoog
left a comment
There was a problem hiding this comment.
I like that this splits up the concerns, but when can you ever run validateSync without first running a validate or fetch? It seems like you need to always do an async resolve step before you can validate synchronously anyhow.
| while (true) { | ||
| try { | ||
| return body(); | ||
| } on SchemaResolutionRequiredException catch (e) { |
There was a problem hiding this comment.
Isn't this the anti-pattern of using exceptions for control flow? Wouldn't it be better to return some sort of union or result type that allows branching instead?
There was a problem hiding this comment.
I do not see how it is possible. Added comment to explain in detail.
There was a problem hiding this comment.
Why couldn't you have a prefetch that you run before doing validateSync? I.e. do something like this:
Future<T> _withRemoteRefs<T>(
ValidationContext context,
Schema schema,
T Function() body,
) async {
// 1. Discover all missing remote references in the schema AST
// and fetch them in parallel with Future.wait.
await context.schemaRegistry.prefetchDependencies(schema, baseUri: context.sourceUri);
// 2. Run the synchronous validation once.
return body();
}Or, even simpler, the async validate method can call it directly:
Future<List<ValidationError>> validate(
Object? data, {
SchemaRegistry? schemaRegistry,
...
}) async {
final registry = schemaRegistry ?? SchemaRegistry(...);
// Fetch any missing transitive references in parallel:
await registry.prefetchDependencies(this, baseUri: sourceUri);
// Run synchronous validation directly:
return validateSync(data, schemaRegistry: registry, ...);
}This way validateSync stays the same, but the exception is a real exception that happens if you call validateSync without all of the dependencies, but now they can be fetched in parallel instead of serially, and we don't need to use an exception for flow control. In the current setup, each time the exception is thrown, it starts the resolution over again.
There was a problem hiding this comment.
Thank you for amazing suggestion! Applied.
| return _getSchemaFromFragment(uri, _schemas[uriWithoutFragment]!); | ||
| final Schema? schema = _schemas[uriWithoutFragment]; | ||
| if (schema == null) { | ||
| throw SchemaResolutionRequiredException(uriWithoutFragment); |
There was a problem hiding this comment.
Just a comment: I was hoping that we could check and see if the URI was a file URI and then resolve it sync here, but I guess if we're going to be using this on the web ever, then that will mean that dart:io isn't available, so sync file reading also wouldn't be.
Nope. |
| For example, in Dart, keep | ||
| the `_` prefix and annotate with `@visibleForTesting`. |
There was a problem hiding this comment.
I don't think this will work, actually. Symbols starting with _ are private to the module. Marking them with @visibleForTesting won't export them to the test. You can only mark public functions as @visibleForTesting.
But I agree with the sentiment of keeping things private. For the skill, I'd word it like this:
Make every code element as private as it can be. If tests need access, design the API for testability with
dependency injection, abstraction, role interfaces, ports and adaptors, or other factoring improvements.
There was a problem hiding this comment.
Oh, yes, this is standard source of confusion for me. Will send correction.
Contributes to a2ui-project/a2ui#2356, a2ui-project/a2ui#2373.
Addresses: a2ui-project/a2ui#2439 (comment).
Add synchronous validation to
json_schema_builderMotivation
Schema.validateis asynchronous, but validation is not. Acrosslib/thereare 34
awaits and only two of them are real I/O, both inschema_cache.dart:fetching the target of a remote
$refoverfile:orhttp(s):. Everythingelse is contagion up the recursion — the tree walk itself is pure computation
over a schema and a value.
That cost is paid by callers who never fetch anything. The A2UI Dart SDK
(
a2ui_core) pre-inlines every$refand documents that it never fetches aschema over the network, yet the inherited
Futureforces itsMessageProcessor.processMessagesto returnFuture<void>, diverging from theTypeScript SDK's synchronous
processMessages(messages): void(a2ui-project/a2ui#2439 (comment)).
The split
Resolution is asynchronous; validation is not. This PR separates the two rather
than duplicating the ~1,170-line traversal.
The traversal is now synchronous.
validateSchemaSync,validateSubSchemaSync,validateObjectSync,validateListSync,validateTypeSpecificKeywordsSync,resolveRefSyncandresolveDynamicRefSyncare the same code as before with theasync/awaitremoved. References resolve from the in-memory
SchemaRegistryonly. If thewalk reaches a reference whose target is not registered, it throws the new
SchemaResolutionRequiredExceptionnaming that target — it does not skip thesubschema, which would silently treat it as unconstrained and turn a missing
fetch into a passing validation.
The asynchronous API is a thin wrapper.
validateand each of thepreviously asynchronous helpers now call the synchronous core through one
small retry loop (
_resolvingRemoteRefs): run it, and if it asks for a schema,fetch that schema into the registry and run it again. Each retry resolves one
more remote reference; a schema with no remote references runs the core
exactly once. Deriving the URI to fetch from the core itself, rather than from
a separate pre-scan, is what keeps base-URI and
$idresolution semantics fromdrifting between the two paths — there is only one implementation of them.
validateSyncis the new public entry point, mirroringvalidate'ssignature and results. Its dartdoc states the precondition (every reference
resolves from the schema itself or from a registry the caller populated,
including any
$schemameta-schema) and what happens when it is violated.Supporting changes:
SchemaRegistrygainsresolveSync(registry-onlyresolution) and
fetch(fetch and register), withresolvenow expressed interms of
fetch.ValidationContextrecords the outcome of the fetches made forone validation, so a fetch that failed is not repeated within that validation and
is not remembered beyond it — a later
validateretries it exactly as it didbefore.
SchemaFetchExceptionandSchemaResolutionRequiredExceptionare nowexported.
Compatibility
No behavior change for existing async callers.
validatekeeps its signature andits results, and every previously asynchronous member still exists with its
asynchronous signature, so this is additive: nothing that compiled before stops
compiling.
The one observable difference is the number of HTTP requests inside a single
validatecall: a remote target that fails to fetch is now attempted once pervalidation instead of once per reference to it. The resulting errors are
identical.
Trade-off
A cold registry costs one extra traversal per distinct remote reference, since
each retry restarts the walk. Validation is pure CPU over the data, the count is
bounded by the number of remote references, and the registry stays warm
afterwards, so subsequent validations are single-pass. The alternative — a
separate pre-scan that predicts which URIs will be needed — would duplicate the
base-URI and
$idresolution rules and drift from them.Testing
test/test_suite_test.dart(the official JSON Schema Test Suite, draft2020-12) passes unchanged, and now runs every one of its 1261 cases through
the synchronous path as well, asserting the two paths produce identical error
strings. A
tearDownAllassertion fails the suite if any case stops beingcovered synchronously, so the coverage cannot erode silently.
test/sync_validation_test.dartcovers the split directly, in four parts:the synchronous path (agreement with
validate,strictFormat, local$defsreferences, registry-backed references, a registry warmed by
validate, andthe failure modes — an unregistered
$ref, an unregistered$schemameta-schema, and a reference whose fetch previously failed); the asynchronous
wrappers, each called directly; the reference resolution failures
(
$dynamicRef, meta-schema, and a fetch that yields no schema); andSchemaRegistry.resolve/resolveSync.dart analyzeclean,dart formatapplied, full package suite green (1317tests).
packages/genui's validation tests pass against the change.coverage_baseline.yamlis updated to the new high-water mark.