From c32aeef725c5fb7216463f455c76696fa63bc446 Mon Sep 17 00:00:00 2001 From: Polina Cherkasova Date: Tue, 20 Oct 2026 12:03:55 -0700 Subject: [PATCH 1/2] test(conformance): cover reference reading and composition cost in the core suites Add v0.8 cases for a plain string property that names a component id, v0.9 and v1.0 cases for an array property whose items are ComponentId, and a timing case for processing messages with a catalog that declares allowedParents and allowedChildren. List the cases the Python and Dart cores fail on main as known gaps. --- .../core/message_processor_scaling.yaml | 20 +++++ conformance/core/validator_v0_8.yaml | 67 +++++++++++++++++ conformance/core/validator_v0_9.yaml | 72 ++++++++++++++++++ conformance/core/validator_v1_0.yaml | 74 +++++++++++++++++++ .../catalogs/composition_catalog_v1_0.json | 40 ++++++++++ ...ge_processor_scaling_conformance_test.dart | 11 ++- python/a2ui_core/tests/test_conformance.py | 11 ++- 7 files changed, 293 insertions(+), 2 deletions(-) create mode 100644 conformance/test_data/catalogs/composition_catalog_v1_0.json diff --git a/conformance/core/message_processor_scaling.yaml b/conformance/core/message_processor_scaling.yaml index 353c84dd48..8d5cf4cb83 100644 --- a/conformance/core/message_processor_scaling.yaml +++ b/conformance/core/message_processor_scaling.yaml @@ -66,3 +66,23 @@ - "]" sizes: [200, 800] max_ratio: 8 + +- name: test_scaling_process_single_component_messages_composition_v10 + description: Processing n messages of one component each, under a validation config and with a catalog whose components declare allowedParents and allowedChildren, takes about four times as long at 4n. A message that adds a child to a container checks the constraints the child changes, rather than every child of the container. + action: measure_growth + protocolVersion: "v1.0" + catalogPaths: + - "conformance/test_data/catalogs/composition_catalog_v1_0.json" + args: + validation_config: relaxed + operation: process_messages + payload: + - '[{"version": "v1.0", "createSurface": {"surfaceId": "s1", "catalogId": "conformance/composition"}}, {"version": "v1.0", "updateComponents": {"surfaceId": "s1", "components": [{"id": "root", "component": "Column", "children": [' + - repeat: '"item_{i}"' + separator: ", " + - "]}]}}, " + - repeat: '{"version": "v1.0", "updateComponents": {"surfaceId": "s1", "components": [{"id": "item_{i}", "component": "Text", "text": "Item number {i}"}]}}' + separator: ", " + - "]" + sizes: [1000, 4000] + max_ratio: 8 diff --git a/conformance/core/validator_v0_8.yaml b/conformance/core/validator_v0_8.yaml index 56a119acde..271e4d7614 100644 --- a/conformance/core/validator_v0_8.yaml +++ b/conformance/core/validator_v0_8.yaml @@ -476,6 +476,73 @@ text: literalString: Reachable +- name: test_v08_topology_plain_string_property_not_treated_as_child_ref + description: Tests that a plain string property whose value happens to match another component's ID does not make that component reachable in v0.8 strict mode. Text.usageHint is an enum, not a child reference, even when a component has the ID it names. + action: validate + protocolVersion: v0.8 + catalogPaths: + - specification/v0_8/json/standard_catalog_definition.json + strictMode: true + steps: + - messages: + - beginRendering: + surfaceId: s1 + root: root + - surfaceUpdate: + surfaceId: s1 + components: + - id: root + component: + Column: + children: + explicitList: [title] + - id: title + component: + Text: + text: + literalString: Heading + usageHint: h1 + - id: h1 + component: + Text: + text: + literalString: Still orphaned + expectError: + category: ValidationError + message: h1 + +- name: test_v08_topology_plain_string_property_does_not_close_cycle + description: Tests that a plain string property whose value happens to match an ancestor's ID is not read as a reference that closes a cycle in v0.8 strict mode. Here root -> h1 -> title is a tree, and title's usageHint names h1. + action: validate + protocolVersion: v0.8 + catalogPaths: + - specification/v0_8/json/standard_catalog_definition.json + strictMode: true + steps: + - messages: + - beginRendering: + surfaceId: s1 + root: root + - surfaceUpdate: + surfaceId: s1 + components: + - id: root + component: + Column: + children: + explicitList: [h1] + - id: h1 + component: + Column: + children: + explicitList: [title] + - id: title + component: + Text: + text: + literalString: Heading + usageHint: h1 + - name: test_v08_invalid_data_model_path_error description: Tests that a dataModelUpdate path with an invalid JSON Pointer escape ('~2') fails validation in v0.8. action: validate diff --git a/conformance/core/validator_v0_9.yaml b/conformance/core/validator_v0_9.yaml index 3b1bdbae48..5875acd4c7 100644 --- a/conformance/core/validator_v0_9.yaml +++ b/conformance/core/validator_v0_9.yaml @@ -1307,6 +1307,78 @@ category: ValidationError message: orphan_id +- name: test_v09_topology_component_id_array_dangling_reference_error + description: Tests that every item of an array property whose items are ComponentId is a child reference, whatever the property is called, so an item naming no component fails validation in v0.9 strict mode even when no other item names one. + action: validate + protocolVersion: v0.9 + catalog: + catalogId: custom + components: + Stack: + type: object + properties: + component: {const: Stack} + slots: + type: array + items: + $ref: "common_types.json#/$defs/ComponentId" + required: + - component + - slots + strictMode: true + steps: + - messages: + - version: v0.9 + createSurface: + surfaceId: s1 + catalogId: custom + - version: v0.9 + updateComponents: + surfaceId: s1 + components: + - id: root + component: Stack + slots: [missing_slot] + expectError: + category: ValidationError + message: missing_slot + +- name: test_v09_topology_component_id_array_child_reachable + description: Tests that a component named in an array property whose items are ComponentId is reachable from the root in v0.9 strict mode, whatever the property is called. + action: validate + protocolVersion: v0.9 + catalog: + catalogId: custom + components: + Stack: + type: object + properties: + component: {const: Stack} + slots: + type: array + items: + $ref: "common_types.json#/$defs/ComponentId" + required: + - component + - slots + strictMode: true + steps: + - messages: + - version: v0.9 + createSurface: + surfaceId: s1 + catalogId: custom + - version: v0.9 + updateComponents: + surfaceId: s1 + components: + - id: root + component: Stack + slots: [first] + - id: first + component: Stack + slots: [] + - name: test_v09_child_list_template_missing_path_error description: Tests that a ChildList template object missing its required 'path' property fails schema validation in v0.9. action: validate diff --git a/conformance/core/validator_v1_0.yaml b/conformance/core/validator_v1_0.yaml index 45adf2b9e5..58b1ee45ac 100644 --- a/conformance/core/validator_v1_0.yaml +++ b/conformance/core/validator_v1_0.yaml @@ -1425,6 +1425,80 @@ category: ValidationError message: orphan_id +- name: test_v10_topology_component_id_array_dangling_reference_error + description: Tests that every item of an array property whose items are ComponentId is a child reference, whatever the property is called, so an item naming no component fails validation in v1.0 strict mode even when no other item names one. + action: validate + protocolVersion: v1.0 + catalog: + catalogId: custom + components: + Stack: + type: object + properties: + id: {type: string} + component: {const: Stack} + slots: + type: array + items: + $ref: "common_types.json#/$defs/ComponentId" + required: + - component + - slots + strictMode: true + steps: + - messages: + - version: v1.0 + createSurface: + surfaceId: s1 + catalogId: custom + - version: v1.0 + updateComponents: + surfaceId: s1 + components: + - id: root + component: Stack + slots: [missing_slot] + expectError: + category: ValidationError + message: missing_slot + +- name: test_v10_topology_component_id_array_child_reachable + description: Tests that a component named in an array property whose items are ComponentId is reachable from the root in v1.0 strict mode, whatever the property is called. + action: validate + protocolVersion: v1.0 + catalog: + catalogId: custom + components: + Stack: + type: object + properties: + id: {type: string} + component: {const: Stack} + slots: + type: array + items: + $ref: "common_types.json#/$defs/ComponentId" + required: + - component + - slots + strictMode: true + steps: + - messages: + - version: v1.0 + createSurface: + surfaceId: s1 + catalogId: custom + - version: v1.0 + updateComponents: + surfaceId: s1 + components: + - id: root + component: Stack + slots: [first] + - id: first + component: Stack + slots: [] + - name: test_v10_child_list_template_missing_path_error description: Tests that a ChildList template object missing its required 'path' property fails schema validation in v1.0. action: validate diff --git a/conformance/test_data/catalogs/composition_catalog_v1_0.json b/conformance/test_data/catalogs/composition_catalog_v1_0.json new file mode 100644 index 0000000000..e65fcab540 --- /dev/null +++ b/conformance/test_data/catalogs/composition_catalog_v1_0.json @@ -0,0 +1,40 @@ +{ + "$schema": "https://json-schema.org/draft/2020-12/schema", + "$id": "https://a2ui.org/conformance/test_data/catalogs/composition_catalog_v1_0.json", + "protocolVersion": "1.0", + "title": "Conformance Composition Catalog (v1.0)", + "description": "A two component catalog whose components declare allowedParents and allowedChildren, for the timing cases of core/message_processor_scaling.yaml.", + "catalogId": "conformance/composition", + "components": { + "Text": { + "type": "object", + "description": "Displays a run of text.", + "allowedParents": ["Column"], + "properties": { + "component": { "const": "Text" }, + "text": { "$ref": "common_types.json#/$defs/DynamicString" } + }, + "required": ["component", "text"] + }, + "Column": { + "type": "object", + "description": "A layout that stacks its children vertically.", + "allowedParents": ["Surface", "Column"], + "allowedChildren": ["Text", "Column"], + "properties": { + "component": { "const": "Column" }, + "children": { "$ref": "common_types.json#/$defs/ChildList" } + }, + "required": ["component", "children"] + } + }, + "$defs": { + "anyComponent": { + "oneOf": [ + { "$ref": "#/components/Text" }, + { "$ref": "#/components/Column" } + ], + "discriminator": { "propertyName": "component" } + } + } +} diff --git a/dart/a2ui_core/test/conformance/message_processor_scaling_conformance_test.dart b/dart/a2ui_core/test/conformance/message_processor_scaling_conformance_test.dart index dde1212331..72e471980b 100644 --- a/dart/a2ui_core/test/conformance/message_processor_scaling_conformance_test.dart +++ b/dart/a2ui_core/test/conformance/message_processor_scaling_conformance_test.dart @@ -35,11 +35,20 @@ void main() { group('conformance $suite', () { test('suite is not empty', () => expect(cases, isNotEmpty)); for (final testCase in cases) { - test(testCase['name']! as String, () => _runCase(testCase)); + final name = testCase['name']! as String; + test(name, () => _runCase(testCase), skip: _knownGaps[name]); } }); } +/// Cases this package fails, with the reason for each. +const Map _knownGaps = { + 'test_scaling_process_single_component_messages_composition_v10': + 'Once a catalog the surface draws on declares allowedParents or ' + 'allowedChildren, the processor checks the composition constraints ' + 'of every component on the surface for each updateComponents.', +}; + void _runCase(ConformanceTestCase testCase) { expect(testCase['action'], 'measure_growth'); expect(testCase['operation'], 'process_messages'); diff --git a/python/a2ui_core/tests/test_conformance.py b/python/a2ui_core/tests/test_conformance.py index b1bec867ef..7233862ea4 100644 --- a/python/a2ui_core/tests/test_conformance.py +++ b/python/a2ui_core/tests/test_conformance.py @@ -68,7 +68,16 @@ "1.0", } -SKIP_TEST_NAMES: set[str] = set() +SKIP_TEST_NAMES: set[str] = { + # The topology checks read a v0.8 string property as a child reference + # when it names a component the surface holds, and an array property whose + # items are ComponentId as a reference only when one of its items names a + # held component. #3326 reads both from the catalog alone. + "test_v08_topology_plain_string_property_not_treated_as_child_ref", + "test_v08_topology_plain_string_property_does_not_close_cycle", + "test_v09_topology_component_id_array_dangling_reference_error", + "test_v10_topology_component_id_array_dangling_reference_error", +} # Transition skip list containing specific test suite files or basenames to skip entirely. SKIP_TEST_SUITES: set[str] = set() From 1d056e3e49c1ec663671b88acef8464a05ce8012 Mon Sep 17 00:00:00 2001 From: Polina Cherkasova Date: Tue, 20 Oct 2026 12:30:07 -0700 Subject: [PATCH 2/2] perf(dart/core): check composition constraints against what a batch changes MessageProcessor checked every reference on the surface against the composition constraints for each updateComponents, so a surface sent as n messages took time in proportion to n squared once a catalog declared allowedParents or allowedChildren. It now keeps the references the last check passed and checks a batch against the root, the references from its components and the references into them, falling back to the whole-surface check when the components changed outside the processor, a batch moves a component to another catalog, or the reference fields or constraints change. The composition timing case now passes. --- dart/a2ui_core/CHANGELOG.md | 9 + .../lib/src/core/component_model.dart | 6 + .../lib/src/processing/processor.dart | 198 ++++++++++++++- .../lib/src/validation/component_graph.dart | 177 +++++++++++--- .../test/composition_constraints_test.dart | 228 ++++++++++++++++++ ...ge_processor_scaling_conformance_test.dart | 11 +- 6 files changed, 576 insertions(+), 53 deletions(-) diff --git a/dart/a2ui_core/CHANGELOG.md b/dart/a2ui_core/CHANGELOG.md index 2905584f10..bc11677eef 100644 --- a/dart/a2ui_core/CHANGELOG.md +++ b/dart/a2ui_core/CHANGELOG.md @@ -2,6 +2,15 @@ ## Unreleased +- `MessageProcessor` checks the composition constraints (`allowedParents` + and `allowedChildren`) of an `updateComponents` against the references it + changes: the root, the references from its components, and the references + into them from the rest of the surface. It used to check every reference on + the surface for each message, so a surface sent as n messages took time in + proportion to n squared once a catalog declared constraints. It checks the + whole surface as before when the components changed outside the processor + since its last check, or when the catalogs whose constraints apply changed, + and a message that fails reports the same error as before. - Extracted `ComponentGraphBound` for incremental `SurfaceComponentsModel` component graph checks and optimized its initial height traversal to linear time. diff --git a/dart/a2ui_core/lib/src/core/component_model.dart b/dart/a2ui_core/lib/src/core/component_model.dart index 64a69f6735..fe2ecceabd 100644 --- a/dart/a2ui_core/lib/src/core/component_model.dart +++ b/dart/a2ui_core/lib/src/core/component_model.dart @@ -161,6 +161,12 @@ class SurfaceComponentsModel { /// The components, in insertion order. Iterable get values => _components.values; + /// How many times a component has been added, removed or updated. + /// + /// What a check found about the components holds while this is unchanged. + @internal + int get changeCount => _changes; + /// The distinct catalogs the components name, null for the surface's own, /// in the order the first component naming each was added. @internal diff --git a/dart/a2ui_core/lib/src/processing/processor.dart b/dart/a2ui_core/lib/src/processing/processor.dart index c7ff8c86bb..2bd25ce83b 100644 --- a/dart/a2ui_core/lib/src/processing/processor.dart +++ b/dart/a2ui_core/lib/src/processing/processor.dart @@ -45,11 +45,109 @@ typedef _RoutedMessage = ({ }); /// An `updateComponents` batch that passed every check, ready to apply. +/// +/// `composition` is null when no catalog the surface draws on declares +/// composition constraints. Otherwise it holds the constraints the batch was +/// checked against, and the record of the surface's edges that check read +/// from, or null when it read the whole surface. typedef _CheckedBatch = ({ List> resolved, Map refFields, + ({ + _CompositionRecord? record, + Map rules + })? composition, }); +/// The parent-child edges of a surface's components as the last composition +/// check left them, which passed that check, so that the next check can read +/// only the edges its batch changes. +/// +/// The record holds for the surface's [SurfaceComponentsModel] while its +/// [SurfaceComponentsModel.changeCount] is [changes], and for a check whose +/// reference fields and constraints are those the record was made with. +class _CompositionRecord { + _CompositionRecord(this.rules); + + /// The edges of every component [model] holds, read through [refFields]. + factory _CompositionRecord.of( + SurfaceComponentsModel model, + Map refFields, + Map rules, + ) { + final record = _CompositionRecord(rules); + for (final ComponentModel component in model.all) { + record.setChildren( + component.id, + component.type, + component.properties, + refFields, + ); + } + return record; + } + + /// The constraints the edges passed. + Map rules; + + /// The model's change count once the checked batch was applied. + int changes = -1; + + /// The ids each component references, held or not. + final Map> _children = {}; + + /// The components that reference each id, held or not. + final Map> _parents = {}; + + /// The reference fields each type's references were read through. + final Map _fields = {}; + + /// The components that reference [id]. + Iterable parentsOf(String id) => _parents[id] ?? const {}; + + /// Records the edges of the component [id] of [type], read from [source] + /// through [refFields], in place of the ones it had. + void setChildren( + String id, + String type, + Map source, + Map refFields, + ) { + for (final String child in _children[id] ?? const []) { + _parents[child]?.remove(id); + } + final List children = [ + for (final ComponentReference reference + in componentReferences(source, refFields[type])) + reference.id, + ]; + for (final child in children) { + (_parents[child] ??= {}).add(id); + } + _children[id] = children; + _fields[type] = refFields[type]; + } + + /// Whether a check through [refFields] against [rules] reads the edges the + /// way this record was made. + bool matches( + Map refFields, + Map rules, + ) { + for (final MapEntry(key: String type, value: ComponentRefFields? fields) + in _fields.entries) { + if (!identical(refFields[type], fields)) return false; + } + if (identical(rules, this.rules)) return true; + if (rules.length != this.rules.length) return false; + for (final MapEntry(key: String type, value: CompositionRule rule) + in rules.entries) { + if (this.rules[type] != rule) return false; + } + return true; + } +} + /// The central processor for A2UI messages on renderer side. /// /// It consumes the agent-to-renderer messages @@ -144,6 +242,10 @@ class MessageProcessor { /// use. final Map> _compositionRules = {}; + /// The edges each surface's components had after the last batch whose + /// composition constraints were checked, by the surface's components model. + final Expando<_CompositionRecord> _compositionRecords = Expando(); + /// The RPC layer: it answers the `callRendererFunction` messages this /// processor receives, settles its `agentFunctionResponse`s, and sends the /// `callAgentFunction`s made through [callAgentFunction] or by a surface's @@ -774,16 +876,18 @@ class MessageProcessor { return merged; } - /// The composition constraints of the catalogs [components] draw on, - /// merged the way [_refFieldsFor] merges reference fields. + /// The composition constraints of the surface's default catalog and the + /// catalogs [componentCatalogIds] name, merged the way [_refFieldsFor] + /// merges reference fields: the first catalog in that order wins a type + /// two catalogs both constrain. Map _compositionRulesFor( SurfaceModel surface, - Iterable> components, + Iterable componentCatalogIds, ) { final ids = { if (surface.defaultCatalog case final catalog?) catalog.id, - for (final Map component in components) - if (component['catalogId'] case final String id) id, + for (final String? id in componentCatalogIds) + if (id != null) id, }; final Iterable> involved = ids.map(surface.resolveCatalog); @@ -874,29 +978,105 @@ class MessageProcessor { for (final String? id in catalogIds) if (id != null) id, }.any((String id) => _rulesOf(surface.resolveCatalog(id)).isNotEmpty)) { - return (resolved: resolved, refFields: refFields); + return (resolved: resolved, refFields: refFields, composition: null); } + + // When the edges the last check passed still describe the surface, only + // the edges this batch changes are read. A batch that moves a component + // to another catalog can change which catalogs' constraints apply, and is + // checked against the whole surface. + final _CompositionRecord? record = _compositionRecords[model]; + final bool keepsCatalogs = resolved.every((Map c) { + final ComponentModel? held = switch (c['id']) { + final String id => model.get(id), + _ => null, + }; + return held == null || held.catalog == c['catalogId']; + }); + if (record != null && + record.changes == model.changeCount && + keepsCatalogs) { + // The catalogs of the surface with the batch applied, in the order the + // whole-surface check below merges their constraints. + final Map rules = _compositionRulesFor(surface, [ + for (final Map c in resolved) + c['catalogId'] as String?, + ...model.catalogIds, + ]); + if (record.matches(refFields, rules)) { + try { + checkBatchCompositionConstraints( + resolved, + refFields, + rules, + heldType: (String id) => model.get(id)?.type, + heldParents: record.parentsOf, + rootId: surface.rootId, + surfaceId: surface.id, + ); + return ( + resolved: resolved, + refFields: refFields, + composition: (record: record, rules: rules), + ); + } on A2uiValidationError { + // The whole-surface check below reports the violation it finds + // first, as it would have without the record. + } + } + } + final List> existing = [ for (final ComponentModel c in model.all) if (!resolved.any((Map r) => r['id'] == c.id)) c.toJson(), ]; + final Map rules = _compositionRulesFor(surface, [ + for (final Map c in [...resolved, ...existing]) + c['catalogId'] as String?, + ]); checkCompositionConstraints( resolved, refFields, - _compositionRulesFor(surface, [...resolved, ...existing]), + rules, existing: existing, rootId: surface.rootId, surfaceId: surface.id, ); - return (resolved: resolved, refFields: refFields); + return ( + resolved: resolved, + refFields: refFields, + composition: (record: null, rules: rules), + ); } /// Applies a [batch] that [_checkComponents] accepted. void _applyComponents(SurfaceModel surface, _CheckedBatch batch) { _applyComponentModels(surface, batch); - surface.componentsModel.componentsUpdateApplied(batch.resolved); + final SurfaceComponentsModel model = surface.componentsModel; + model.componentsUpdateApplied(batch.resolved); + if (batch.composition + case ( + record: final _CompositionRecord? checked, + rules: final Map rules, + )) { + final _CompositionRecord record; + if (checked == null) { + record = _CompositionRecord.of(model, batch.refFields, rules); + } else { + record = checked..rules = rules; + for (final Map c in batch.resolved) { + record.setChildren( + c['id']! as String, + c['component']! as String, + c, + batch.refFields, + ); + } + } + _compositionRecords[model] = record..changes = model.changeCount; + } } void _applyComponentModels(SurfaceModel surface, _CheckedBatch batch) { diff --git a/dart/a2ui_core/lib/src/validation/component_graph.dart b/dart/a2ui_core/lib/src/validation/component_graph.dart index 268904bab1..b40bca8a78 100644 --- a/dart/a2ui_core/lib/src/validation/component_graph.dart +++ b/dart/a2ui_core/lib/src/validation/component_graph.dart @@ -109,12 +109,112 @@ void checkCompositionConstraints( case (final String id, final String type)) id: type, }; - String list(List names) => + final checker = _CompositionChecker( + refFields, + rules, + (String id) => types[id], + rootId: rootId, + surfaceId: surfaceId, + ); + for (var index = 0; index < components.length; index++) { + checker.checkComponent(components[index], '/components/$index'); + } + for (final component in existing) { + checker.checkComponent(component, null); + } +} + +/// Checks the parent-child edges a batch of [components] can change against +/// [rules], on a surface whose other edges passed +/// [checkCompositionConstraints] under the same [rules] and have not changed +/// since. +/// +/// Those are the surface's edge into the root when the batch holds it, the +/// edges from each component of the batch, and the edges into each +/// component of the batch from the components outside it. [heldType] gives +/// the type of a component the surface holds, or null for one it does not, +/// and [heldParents] the components the surface holds that reference an id. +/// The rest of the surface is not read, so the check costs what the batch +/// changes rather than the size of the surface. +/// +/// An error for the root or for an edge from the batch is the one +/// [checkCompositionConstraints] reports first for the same surface. An edge +/// into the batch from a component outside it is checked last, so a caller +/// that needs the error the whole-surface check reports first runs that +/// check when this one throws for such an edge. +/// +/// Throws [A2uiValidationError] as [checkCompositionConstraints] does. +void checkBatchCompositionConstraints( + List> components, + Map refFields, + Map rules, { + required String? Function(String id) heldType, + required Iterable Function(String id) heldParents, + String rootId = _rootComponentId, + String? surfaceId, +}) { + if (rules.isEmpty) return; + // A component of the batch replaces the one the surface holds with its id. + final batchTypes = { + for (final component in components) + if (component['id'] case final String id) + id: switch (component['component']) { + final String type => type, + _ => null, + }, + }; + String? typeOf(String id) => + batchTypes.containsKey(id) ? batchTypes[id] : heldType(id); + final checker = _CompositionChecker( + refFields, + rules, + typeOf, + rootId: rootId, + surfaceId: surfaceId, + ); + for (var index = 0; index < components.length; index++) { + checker.checkComponent(components[index], '/components/$index'); + } + for (final MapEntry(key: String id, value: String? type) + in batchTypes.entries) { + if (type == null) continue; + for (final String parent in heldParents(id)) { + if (batchTypes.containsKey(parent)) continue; + final String? parentType = heldType(parent); + if (parentType == null) continue; + checker.checkEdge(parent, parentType, id, type, null); + } + } +} + +/// The checks [checkCompositionConstraints] and +/// [checkBatchCompositionConstraints] run on each component and edge. +class _CompositionChecker { + _CompositionChecker( + this.refFields, + this.rules, + this.typeOf, { + required this.rootId, + required this.surfaceId, + }); + + final Map refFields; + final Map rules; + + /// The type of a component on the surface the check reads, or null when + /// the surface does not hold it. + final String? Function(String id) typeOf; + final String rootId; + final String? surfaceId; + + static String _list(List names) => '[${names.map((name) => "'$name'").join(', ')}]'; - // [pointer] locates [component] in the message, or is null when the - // component was already stored. - void check(Map component, String? pointer) { + /// Checks [component] as the root when it is one, and each edge from it. + /// + /// [pointer] locates [component] in the message, or is null when the + /// component was already stored. + void checkComponent(Map component, String? pointer) { final Object? id = component['id']; final Object? type = component['component']; if (id is! String || type is! String) return; @@ -126,50 +226,59 @@ void checkCompositionConstraints( throw A2uiValidationError( "Component '$id' ($type) cannot be placed under parent " "'$_surfaceParent' ($_surfaceParent). Allowed parents: " - '${list(rootParents)}.', + '${_list(rootParents)}.', code: 'UNALLOWED_PARENT', path: pointer, surfaceId: surfaceId, ); } - final List? allowedChildren = rules[type]?.allowedChildren; for (final ComponentReference reference in _referencesOf( component, refFields, )) { - final String? childType = types[reference.id]; + final String? childType = typeOf(reference.id); if (childType == null) continue; - final String? path = pointer == null - ? null - : '$pointer${referencePointer(reference.field)}'; - if (allowedChildren != null && !allowedChildren.contains(childType)) { - throw A2uiValidationError( - "Container '$id' ($type) cannot contain child '${reference.id}' " - '($childType). Allowed children: ${list(allowedChildren)}.', - code: 'UNALLOWED_CHILD', - path: path, - surfaceId: surfaceId, - ); - } - final List? allowedParents = rules[childType]?.allowedParents; - if (allowedParents != null && !allowedParents.contains(type)) { - throw A2uiValidationError( - "Component '${reference.id}' ($childType) cannot be placed under " - "parent '$id' ($type). Allowed parents: ${list(allowedParents)}.", - code: 'UNALLOWED_PARENT', - path: path, - surfaceId: surfaceId, - ); - } + checkEdge( + id, + type, + reference.id, + childType, + pointer == null ? null : '$pointer${referencePointer(reference.field)}', + ); } } - for (var index = 0; index < components.length; index++) { - check(components[index], '/components/$index'); - } - for (final component in existing) { - check(component, null); + /// Checks the edge from [parentId] to [childId], whose reference is at + /// [path] in the message, or nowhere in it when [path] is null. + void checkEdge( + String parentId, + String parentType, + String childId, + String childType, + String? path, + ) { + final List? allowedChildren = rules[parentType]?.allowedChildren; + if (allowedChildren != null && !allowedChildren.contains(childType)) { + throw A2uiValidationError( + "Container '$parentId' ($parentType) cannot contain child '$childId' " + '($childType). Allowed children: ${_list(allowedChildren)}.', + code: 'UNALLOWED_CHILD', + path: path, + surfaceId: surfaceId, + ); + } + final List? allowedParents = rules[childType]?.allowedParents; + if (allowedParents != null && !allowedParents.contains(parentType)) { + throw A2uiValidationError( + "Component '$childId' ($childType) cannot be placed under " + "parent '$parentId' ($parentType). Allowed parents: " + '${_list(allowedParents)}.', + code: 'UNALLOWED_PARENT', + path: path, + surfaceId: surfaceId, + ); + } } } diff --git a/dart/a2ui_core/test/composition_constraints_test.dart b/dart/a2ui_core/test/composition_constraints_test.dart index 4a2b7cc11c..88cf83c546 100644 --- a/dart/a2ui_core/test/composition_constraints_test.dart +++ b/dart/a2ui_core/test/composition_constraints_test.dart @@ -12,6 +12,8 @@ // See the License for the specific language governing permissions and // limitations under the License. +import 'dart:math'; + import 'package:a2ui_core/a2ui_core.dart'; import 'package:a2ui_core/src/validation/component_graph.dart'; import 'package:a2ui_core/src/validation/component_refs.dart'; @@ -424,4 +426,230 @@ void main() { ); }); }); + + group('MessageProcessor composition constraints, message by message', () { + MessageProcessor processor({ValidationConfig? config}) { + final p = MessageProcessor( + catalogs: [ + Catalog( + id: 'custom', + protocolVersion: A2uiProtocolVersion.v1_0, + components: _catalog().components.values.toList(), + ), + ], + defaultVersion: A2uiProtocolVersion.v1_0, + validationConfig: config, + ); + p.processMessages( + AgentToRendererMessage.parseAll([ + { + 'version': 'v1.0', + 'createSurface': {'surfaceId': 's1', 'catalogId': 'custom'}, + }, + ], protocolVersion: A2uiProtocolVersion.v1_0), + ); + return p; + } + + void update( + MessageProcessor p, + List> components, + ) => + p.processMessages( + AgentToRendererMessage.parseAll([ + { + 'version': 'v1.0', + 'updateComponents': {'surfaceId': 's1', 'components': components}, + }, + ], protocolVersion: A2uiProtocolVersion.v1_0), + ); + + SurfaceComponentsModel components(MessageProcessor p) => + p.model.getSurface('s1')!.componentsModel; + + test('rejects a parent whose new type does not allow a held child', () { + final MessageProcessor p = processor(); + update(p, [ + { + 'id': 'root', + 'component': 'Column', + 'children': ['v1'], + }, + ]); + update(p, [ + {'id': 'v1', 'component': 'Video'}, + ]); + + expect( + () => update(p, [ + { + 'id': 'root', + 'component': 'RestrictedBox', + 'children': ['v1'], + }, + ]), + throwsA( + _violation('UNALLOWED_CHILD', path: '/components/0/children/0'), + ), + ); + expect(components(p).get('root')!.type, 'Column'); + }); + + test('rejects a held child whose new type its parent does not allow', () { + final MessageProcessor p = processor(); + update(p, [ + { + 'id': 'root', + 'component': 'RestrictedBox', + 'children': ['t1'], + }, + ]); + update(p, [ + {'id': 't1', 'component': 'Text'}, + ]); + + expect( + () => update(p, [ + {'id': 't1', 'component': 'Video'}, + ]), + throwsA(_violation('UNALLOWED_CHILD', path: null)), + ); + expect(components(p).get('t1')!.type, 'Text'); + }); + + test('keeps checking after a rejected message', () { + final MessageProcessor p = processor(); + update(p, [ + { + 'id': 'root', + 'component': 'RestrictedBox', + 'children': ['a', 'b'], + }, + ]); + expect( + () => update(p, [ + {'id': 'a', 'component': 'Video'}, + ]), + throwsA(_violation('UNALLOWED_CHILD', path: null)), + ); + update(p, [ + {'id': 'a', 'component': 'Text'}, + ]); + + expect( + () => update(p, [ + {'id': 'b', 'component': 'Video'}, + ]), + throwsA(_violation('UNALLOWED_CHILD', path: null)), + ); + }); + + test('sees a component added to the surface outside the processor', () { + final MessageProcessor p = processor(); + update(p, [ + { + 'id': 'root', + 'component': 'RestrictedBox', + 'children': ['t1', 'v1'], + }, + ]); + update(p, [ + {'id': 't1', 'component': 'Text'}, + ]); + components(p).addComponent(ComponentModel('v1', 'Video', {})); + + expect( + () => update(p, [ + {'id': 't2', 'component': 'Text'}, + ]), + throwsA( + _violation( + 'UNALLOWED_CHILD', + path: null, + message: "Container 'root' (RestrictedBox) cannot contain child " + "'v1' (Video). Allowed children: ['Text', 'Button'].", + ), + ), + ); + }); + + // Each processor is compared with one that checks the whole surface for + // every message, since a component updated outside it makes what it + // recorded about the surface stale. + const ids = ['root', 'a', 'b', 'c', 'd', 'e']; + const types = [ + 'Column', + 'Row', + 'ColumnHeader', + 'RestrictedBox', + 'Banner', + 'Text', + 'Video', + ]; + const containers = {'Column', 'Row', 'RestrictedBox'}; + final Map configs = { + 'no config': null, + 'relaxed config': ValidationConfig.relaxed, + }; + + Map randomComponent(Random random) { + final String type = types[random.nextInt(types.length)]; + return { + 'id': ids[random.nextInt(ids.length)], + 'component': type, + if (containers.contains(type)) + 'children': [ + for (var i = random.nextInt(3); i > 0; i--) + ids[random.nextInt(ids.length)], + ], + }; + } + + Object? outcome(void Function() body) { + try { + body(); + return null; + } on A2uiValidationError catch (e) { + return (e.runtimeType, e.code, e.path, e.message); + } + } + + Map snapshot(MessageProcessor p) => { + for (final ComponentModel c in components(p).all) c.id: c.toJson(), + }; + + for (final MapEntry(key: String name, value: ValidationConfig? config) + in configs.entries) { + test('accepts and rejects what a whole-surface check does ($name)', () { + for (var seed = 0; seed < 50; seed++) { + final random = Random(seed); + final MessageProcessor incremental = processor( + config: config, + ); + final MessageProcessor whole = processor( + config: config, + ); + for (var step = 0; step < 25; step++) { + final Map> batch = { + for (var i = random.nextInt(2) + 1; i > 0; i--) + if (randomComponent(random) case final component) + component['id']! as String: component, + }; + for (final ComponentModel c in components(whole).all) { + c.properties = c.properties; + } + final Object? expected = outcome( + () => update(whole, batch.values.toList()), + ); + expect( + outcome(() => update(incremental, batch.values.toList())), + expected, + reason: 'seed $seed, step $step, batch $batch', + ); + expect(snapshot(incremental), snapshot(whole)); + } + } + }); + } + }); } diff --git a/dart/a2ui_core/test/conformance/message_processor_scaling_conformance_test.dart b/dart/a2ui_core/test/conformance/message_processor_scaling_conformance_test.dart index 72e471980b..dde1212331 100644 --- a/dart/a2ui_core/test/conformance/message_processor_scaling_conformance_test.dart +++ b/dart/a2ui_core/test/conformance/message_processor_scaling_conformance_test.dart @@ -35,20 +35,11 @@ void main() { group('conformance $suite', () { test('suite is not empty', () => expect(cases, isNotEmpty)); for (final testCase in cases) { - final name = testCase['name']! as String; - test(name, () => _runCase(testCase), skip: _knownGaps[name]); + test(testCase['name']! as String, () => _runCase(testCase)); } }); } -/// Cases this package fails, with the reason for each. -const Map _knownGaps = { - 'test_scaling_process_single_component_messages_composition_v10': - 'Once a catalog the surface draws on declares allowedParents or ' - 'allowedChildren, the processor checks the composition constraints ' - 'of every component on the surface for each updateComponents.', -}; - void _runCase(ConformanceTestCase testCase) { expect(testCase['action'], 'measure_growth'); expect(testCase['operation'], 'process_messages');