From cd3d60ab9c36bcb33c126d18d5ba48680818366b Mon Sep 17 00:00:00 2001 From: Joshua Pratt Date: Thu, 5 Nov 2020 21:54:01 -0800 Subject: [PATCH] Throw error on recursive schemas (#6485) Adds flag to allow testing of recursive schema support and throws error on detection of recursive schemas Fixes b/172029133 Closes #6485 PiperOrigin-RevId: 340986619 --- .../tests/arc-stores-fetcher-test.ts | 3 +- .../tests/find-hosted-particle-test.ts | 3 + src/runtime/flags.ts | 2 + src/runtime/manifest.ts | 38 ++++++++- src/runtime/policy/tests/policy-test.ts | 7 ++ src/runtime/tests/field-path-test.ts | 17 ++-- src/runtime/tests/manifest-test.ts | 34 +++++++- src/types/internal/schema.ts | 9 ++- src/types/lib-types.ts | 3 +- src/types/tests/schema-test.ts | 78 ++++++++++++++++++- src/utils/internal/misc.ts | 11 ++- 11 files changed, 183 insertions(+), 22 deletions(-) diff --git a/src/devtools-connector/tests/arc-stores-fetcher-test.ts b/src/devtools-connector/tests/arc-stores-fetcher-test.ts index 6509864518c..91c8709c41c 100644 --- a/src/devtools-connector/tests/arc-stores-fetcher-test.ts +++ b/src/devtools-connector/tests/arc-stores-fetcher-test.ts @@ -18,6 +18,7 @@ import {SingletonType} from '../../types/lib-types.js'; import {storageKeyPrefixForTest} from '../../runtime/testing/handle-for-test.js'; import {Entity} from '../../runtime/entity.js'; import {ActiveSingletonEntityStore, handleForStoreInfo} from '../../runtime/storage/storage.js'; +import {deleteFieldRecursively} from '../../utils/lib-utils.js'; describe('ArcStoresFetcher', () => { before(() => DevtoolsForTests.ensureStub()); @@ -50,7 +51,7 @@ describe('ArcStoresFetcher', () => { // Location in the schema file is stored in the type and used by some tools. // We don't assert on it in this test. - delete results[0].messageBody.arcStores[0].type.innerType.entitySchema.fields.value.location; + deleteFieldRecursively(results, 'location'); const sessionId = arc.idGenerator.currentSessionIdForTesting; const entityId = '!' + sessionId + ':fooStoreId:2'; diff --git a/src/planning/strategies/tests/find-hosted-particle-test.ts b/src/planning/strategies/tests/find-hosted-particle-test.ts index 5af495ff29e..55ba3683bfd 100644 --- a/src/planning/strategies/tests/find-hosted-particle-test.ts +++ b/src/planning/strategies/tests/find-hosted-particle-test.ts @@ -20,6 +20,7 @@ import {ArcId} from '../../../runtime/id.js'; import {handleForStoreInfo} from '../../../runtime/storage/storage.js'; import {Runtime} from '../../../runtime/runtime.js'; import {StoreInfo} from '../../../runtime/storage/store-info.js'; +import {deleteFieldRecursively} from '../../../utils/lib-utils.js'; async function runStrategy(manifestStr) { const manifest = await Manifest.parse(manifestStr); @@ -183,6 +184,8 @@ describe('FindHostedParticle', () => { assert.isNotNull(particleSpec['id'], 'particleSpec stored in handle should have an id'); delete particleSpec['id']; await arc.idle; + deleteFieldRecursively(manifest.findParticleByName('TestParticle'), 'location'); + deleteFieldRecursively(particleSpec, 'location'); assert.deepEqual(manifest.findParticleByName('TestParticle').toLiteral(), particleSpec.toLiteral()); }); }); diff --git a/src/runtime/flags.ts b/src/runtime/flags.ts index 7b0d1ae379b..ba7757ed3b2 100644 --- a/src/runtime/flags.ts +++ b/src/runtime/flags.ts @@ -24,6 +24,8 @@ class FlagDefaults { static useSlandles = false; static defaultToSlandles = false; static fieldRefinementsAllowed = false; + // Enables support for recursive & co-recursive schemas. See b/156427820 + static recursiveSchemasAllowed = false; // TODO(#4843): temporary avoid using reference-mode-store in tests. static defaultReferenceMode = false; } diff --git a/src/runtime/manifest.ts b/src/runtime/manifest.ts index 4d60e533bae..5ba2f580df2 100644 --- a/src/runtime/manifest.ts +++ b/src/runtime/manifest.ts @@ -11,6 +11,7 @@ import {parse} from '../gen/runtime/manifest-parser.js'; import {assert} from '../platform/assert-web.js'; import {digest} from '../platform/digest-web.js'; +import {Flags} from './flags.js'; import {Id, IdGenerator} from './id.js'; import {MuxType, Schema, FieldType, Refinement, BigCollectionType, CollectionType, EntityType, InterfaceInfo, InterfaceType, ReferenceType, SlotType, Type, TypeVariable, SingletonType, TupleType, @@ -38,7 +39,6 @@ import {canonicalManifest} from './canonical-manifest.js'; import {Policy} from './policy/policy.js'; import {resolveFieldPathType} from './field-path.js'; import {StoreInfo, StoreClaims} from './storage/store-info.js'; -import {CRDTTypeRecord} from '../crdt/lib-crdt.js'; export enum ErrorSeverity { Error = 'error', @@ -548,6 +548,8 @@ ${e.message} await processItems('store', item => Manifest._processStore(manifest, item, loader, memoryProvider)); await processItems('policy', item => Manifest._processPolicy(manifest, item)); await processItems('recipe', item => Manifest._processRecipe(manifest, item)); + + Manifest._checkValidityOfRecursiveSchemas(manifest); } catch (e) { dumpErrors(manifest); throw processError(e, false); @@ -715,6 +717,35 @@ ${e.message} visitor.traverse(items); } + private static _checkValidityOfRecursiveSchemas(manifest: Manifest) { + if (Flags.recursiveSchemasAllowed) { + return; // No further checking needed + } + for (const schema of Object.values(manifest.schemas)) { + const referenced: Set = new Set(); + const visit = (schema: Schema) => { + // visit fields + for (const field of Object.values(schema.fields)) { + const entityType = field.getEntityType(); + if (entityType == null) { + // ignore Primitive types + continue; + } + const fieldType = entityType.getEntitySchema(); + if (referenced.has(fieldType.name)) { + return; // already visited + } + referenced.add(fieldType.name); + visit(fieldType); + } + }; + visit(schema); + if (referenced.has(schema.name)) { + throw new ManifestError(schema.location, `Recursive schemas are unsuported, unstable support can be enabled via the 'recursiveSchemasAllowed' flag: ${schema.name}`); + } + } + } + private static _discoverSchema(manifest: Manifest, schemaItem) { const names = [...schemaItem.names]; const name = schemaItem.alias || names[0]; @@ -723,7 +754,9 @@ ${e.message} schemaItem.location, `Schema defined without name or alias`); } - manifest._schemas[name] = new Schema(names, {}, {}); + const schema = new Schema(names, {}, {}); + schema.location = schemaItem.location; + manifest._schemas[name] = schema; } private static _processSchema(manifest: Manifest, schemaItem) { @@ -787,6 +820,7 @@ ${e.message} const annotations: AnnotationRef[] = Manifest._buildAnnotationRefs(manifest, schemaItem.annotationRefs); manifest._schemas[name] = schema; const updatedSchema = new Schema(names, fields, {description, annotations}); + updatedSchema.location = schemaItem.location; if (schemaItem.alias) { updatedSchema.isAlias = true; } diff --git a/src/runtime/policy/tests/policy-test.ts b/src/runtime/policy/tests/policy-test.ts index 8d6aa6e900f..36b44f9716b 100644 --- a/src/runtime/policy/tests/policy-test.ts +++ b/src/runtime/policy/tests/policy-test.ts @@ -15,6 +15,7 @@ import {assertThrowsAsync} from '../../../testing/test-util.js'; import {mapToDictionary} from '../../../utils/lib-utils.js'; import {TtlUnits, Persistence, Encryption, Capabilities, Ttl} from '../../capabilities.js'; import {IngressValidation} from '../ingress-validation.js'; +import {deleteFieldRecursively} from '../../../utils/lib-utils.js'; const customAnnotation = ` annotation custom @@ -400,6 +401,8 @@ policy MyPolicy { address: &Address {number, street, city} otherAddresses: [&Address {city, country}] `)).schemas; + deleteFieldRecursively(schema, 'location'); + deleteFieldRecursively(expectedSchemas, 'location'); assert.deepEqual(schema, expectedSchemas['Person']); }); @@ -448,6 +451,8 @@ policy MyPolicy { name: inline Name {last: Text} addresses: List `)).schemas; + deleteFieldRecursively(schema, 'location'); + deleteFieldRecursively(expectedSchemas['Person'], 'location'); assert.deepEqual(schema, expectedSchemas['Person']); }); @@ -504,6 +509,8 @@ policy MyPolicy { address: &Address {number, street, city} otherAddresses: [&Address {city, country}] `)).schemas; + deleteFieldRecursively(maxReadSchema, 'location', {replaceWithNulls: true}); + deleteFieldRecursively(expectedSchemas['Person'], 'location', {replaceWithNulls: true}); assert.deepEqual(maxReadSchema, expectedSchemas['Person']); }); }); diff --git a/src/runtime/tests/field-path-test.ts b/src/runtime/tests/field-path-test.ts index 544a89e129d..afd1c5b0a5d 100644 --- a/src/runtime/tests/field-path-test.ts +++ b/src/runtime/tests/field-path-test.ts @@ -13,6 +13,7 @@ import {EntityType, SingletonType, CollectionType, TypeVariable, TupleType} from import {Manifest} from '../manifest.js'; import {assert} from '../../platform/chai-web.js'; import {deleteFieldRecursively} from '../../utils/lib-utils.js'; +import {Flags} from '../flags.js'; async function parseTypeFromSchema(manifestStr: string) { const manifest = await Manifest.parse(manifestStr); @@ -356,7 +357,7 @@ describe('field path validation', () => { resolveFieldPathType(['inlined', 'name'], type); }); - it('rejects missing fields nested inside inline schemas', async () => { + it('rejects missing fields nested inside inline schemas', Flags.withFlags({recursiveSchemasAllowed: true}, async () => { const type = await parseTypeFromSchema(` schema Bar inlined: inline Foo {name: Text} @@ -364,7 +365,7 @@ describe('field path validation', () => { assert.throws( () => resolveFieldPathType(['inlined', 'missing'], type), `Schema 'Foo {name: Text}' does not contain field 'missing'.`); - }); + })); }); describe('ordered lists', () => { @@ -376,15 +377,15 @@ describe('field path validation', () => { resolveFieldPathType(['list'], type); }); - it('can refer to fields nested inside ordered lists', async () => { + it('can refer to fields nested inside ordered lists', Flags.withFlags({recursiveSchemasAllowed: true}, async () => { const type = await parseTypeFromSchema(` schema Bar list: List<&Bar {inner: Number}> `); resolveFieldPathType(['list', 'inner'], type); - }); + })); - it('rejects missing fields nested inside ordered lists', async () => { + it('rejects missing fields nested inside ordered lists', Flags.withFlags({recursiveSchemasAllowed: true}, async () => { const type = await parseTypeFromSchema(` schema Bar list: List<&Bar {inner: Number}> @@ -392,15 +393,15 @@ describe('field path validation', () => { assert.throws( () => resolveFieldPathType(['list', 'missing'], type), `Schema 'Bar {inner: Number}' does not contain field 'missing'.`); - }); + })); - it('works with inlined schemas inside ordered lists', async () => { + it('works with inlined schemas inside ordered lists', Flags.withFlags({recursiveSchemasAllowed: true}, async () => { const type = await parseTypeFromSchema(` schema Bar list: List `); resolveFieldPathType(['list'], type); resolveFieldPathType(['list', 'inner'], type); - }); + })); }); }); diff --git a/src/runtime/tests/manifest-test.ts b/src/runtime/tests/manifest-test.ts index 4749b41b129..f23ddd8c055 100644 --- a/src/runtime/tests/manifest-test.ts +++ b/src/runtime/tests/manifest-test.ts @@ -36,6 +36,7 @@ import {ActiveCollectionEntityStore, handleForStoreInfo, CollectionEntityType} f import {Ttl} from '../capabilities.js'; import {StoreInfo} from '../storage/store-info.js'; import {StorageServiceImpl} from '../storage/storage-service.js'; +import {deleteFieldRecursively} from '../../utils/lib-utils.js'; function verifyPrimitiveType(field, type) { assert(field instanceof PrimitiveField, `Got ${field.constructor.name}, but expected a primitive field.`); @@ -645,8 +646,11 @@ ${particleStr1} thing: reads Thing` }); const manifest = await Manifest.load('./a', loader, {registry, memoryProvider}); - assert.isTrue(manifest.recipes[0].particles[0].spec.equals( - (await registry['./b']).findParticleByName('ParticleB')) + const particleLit = (await registry['./b']).findParticleByName('ParticleB').toLiteral(); + deleteFieldRecursively(particleLit, 'location'); + assert.deepEqual( + manifest.recipes[0].particles[0].spec.toLiteral(), + particleLit ); }); it('can parse a schema extending a schema in another manifest', async () => { @@ -4311,7 +4315,8 @@ Only type variables may have '*' fields. const bazReference = foo.fields['baz'].getEntityType().getEntitySchema(); assert.equal(bazReference.fields['t'].getType(), 'Text'); }); - it('handles recursive schemas declarations', async () => { + it('catches unsupported recursive schemas declarations', Flags.withFlags({recursiveSchemasAllowed: false}, async () => { + assertThrowsAsync(async () => { const manifest = await parseManifest(` schema Baz t: Text @@ -4330,7 +4335,28 @@ Only type variables may have '*' fields. const bazReference = foo.fields['baz'].getEntityType().getEntitySchema(); assert.equal(bazReference.fields['t'].getType(), 'Text'); - }); + }); + })); + it('handles recursive schemas declarations', Flags.withFlags({recursiveSchemasAllowed: true}, async () => { + const manifest = await parseManifest(` + schema Baz + t: Text + bar: &Bar + schema Foo + bar: &Bar + baz: &Baz + schema Bar + n: Number + baz: &Baz + `); + const foo = manifest.schemas['Foo']; + + const barReference = foo.fields['bar'].getEntityType().getEntitySchema(); + assert.equal(barReference.fields['n'].getType(), 'Number'); + + const bazReference = foo.fields['baz'].getEntityType().getEntitySchema(); + assert.equal(bazReference.fields['t'].getType(), 'Text'); + })); }); it('warns about using external schemas', async () => { const manifest = await parseManifest(` diff --git a/src/types/internal/schema.ts b/src/types/internal/schema.ts index fff2a0a2c72..23a8a4cdb66 100644 --- a/src/types/internal/schema.ts +++ b/src/types/internal/schema.ts @@ -13,7 +13,7 @@ import {CollectionField, FieldType, InlineField, NestedField, OrderedListField, import {Flags} from '../../runtime/flags.js'; import {mergeMapInto} from '../../utils/lib-utils.js'; import {AnnotationRef} from '../../runtime/arcs-types/annotation.js'; -import {Primitive} from '../../runtime/manifest-ast-types/manifest-ast-nodes.js'; +import {Primitive, SourceLocation} from '../../runtime/manifest-ast-types/manifest-ast-nodes.js'; import {Dictionary, IndentingStringBuilder} from '../../utils/lib-utils.js'; import {CRDTEntity, SingletonEntityModel, CollectionEntityModel, Referenceable, CRDTCollection, CRDTSingleton} from '../../crdt/lib-crdt.js'; @@ -33,6 +33,7 @@ export class Schema { isAlias: boolean; hashStr: string = null; _annotations: AnnotationRef[]; + location?: SourceLocation = null; // The implementation of fromLiteral creates a cyclic dependency, so it is // separated out. This variable serves the purpose of an abstract static. static fromLiteral: SchemaMethod = null; @@ -114,13 +115,17 @@ export class Schema { fields[key] = this.fields[key].toLiteral(); } - return { + const lit = { names: this.names, fields, description: this.description, refinement: this.refinement && this.refinement.toLiteral(), annotations: this.annotations }; + if (this.location !== null) { + lit['location'] = this.location; + } + return lit; } // TODO(cypher1): This should only be an ident used in manifest parsing. diff --git a/src/types/lib-types.ts b/src/types/lib-types.ts index 689f5de224f..baa2a2874ea 100644 --- a/src/types/lib-types.ts +++ b/src/types/lib-types.ts @@ -14,7 +14,8 @@ export {Type, TypeLiteral, CountType, EntityType, SingletonType, CollectionType, TypeVarReference, InterfaceInfoLiteral, MatchResult} from './internal/type.js'; export {Schema} from './internal/schema.js'; -export {FieldType, PrimitiveField, OrderedListField, ReferenceField, InlineField} from './internal/schema-field.js'; +export {FieldType, PrimitiveField, ReferenceField, InlineField, CollectionField, OrderedListField, + } from './internal/schema-field.js'; export {Refinement, RefinementExpressionLiteral, RefinementExpressionVisitor, BinaryExpression, UnaryExpression, FieldNamePrimitive, QueryArgumentPrimitive, BuiltIn, NumberPrimitive, diff --git a/src/types/tests/schema-test.ts b/src/types/tests/schema-test.ts index 1102fa60925..9425d3f1d65 100644 --- a/src/types/tests/schema-test.ts +++ b/src/types/tests/schema-test.ts @@ -12,14 +12,17 @@ // tslint:disable: variable-name // tslint:disable: no-unused-expression -import {EntityType, ReferenceType, Schema, PrimitiveField} from '../lib-types.js'; +import {EntityType, ReferenceType, Schema, PrimitiveField, ReferenceField, InlineField, + CollectionField} from '../lib-types.js'; import {assert} from '../../platform/chai-web.js'; +import {assertThrowsAsync} from '../../testing/test-util.js'; import {Manifest} from '../../runtime/manifest.js'; import {Reference} from '../../runtime/reference.js'; import {Loader} from '../../platform/loader.js'; import {Entity} from '../../runtime/entity.js'; import {ConCap} from '../../testing/test-util.js'; import {Flags} from '../../runtime/flags.js'; +import {deleteFieldRecursively} from '../../utils/lib-utils.js'; function getSchemaFromManifest(manifest: Manifest, handleName: string, particleIndex: number = 0): Schema { return manifest.particles[particleIndex].handleConnectionMap.get(handleName).type.getEntitySchema(); @@ -363,6 +366,9 @@ describe('schema', () => { const Thing = manifest.findSchemaByName('Thing'); const Product = manifest.findSchemaByName('Product'); + deleteFieldRecursively(Product, 'location', {replaceWithNulls: true}); + deleteFieldRecursively(Thing, 'location', {replaceWithNulls: true}); + assert.deepEqual(Schema.intersect(Product, Thing), Thing); assert.deepEqual(Schema.intersect(Thing, Product), Thing); }); @@ -904,4 +910,74 @@ describe('schema', () => { // These should not be equal! assert.notEqual(await manifest1.schemas['Outer'].hash(), await manifest2.schemas['Outer'].hash()); }); + describe('recursive schemas', async () => { + it('handles recursive Schemas syntax', Flags.withFlags({recursiveSchemasAllowed: true}, async () => { + const manifest = await Manifest.parse(` + schema GraphNode + name: Text + neighbors: [&GraphNode]`); + + const schema = manifest.schemas['GraphNode']; + assert.deepEqual(schema.names, ['GraphNode']); + assert.hasAllKeys(schema.fields, ['name', 'neighbors']); + assert.strictEqual(schema.fields.name.getType(), 'Text'); + const neighbors = schema.fields.neighbors as CollectionField; + assert.strictEqual(neighbors.kind, 'schema-collection'); + const ref = neighbors.schema as ReferenceField; + assert.strictEqual(ref.kind, 'schema-reference'); + const inline = ref.schema as InlineField; + assert.strictEqual(inline.kind, 'schema-inline'); + const model = inline.model; + const recursiveSchema = model.entitySchema; + assert.deepEqual(recursiveSchema.names, ['GraphNode']); + assert.hasAllKeys(recursiveSchema.fields, ['name', 'neighbors']); + assert.strictEqual(recursiveSchema.fields.name.getType(), 'Text'); + })); + it('catches disallowed recursive Schemas syntax', Flags.withFlags({recursiveSchemasAllowed: false}, async () => { + assertThrowsAsync(async () => { + await Manifest.parse(` + schema GraphNode + name: Text + neighbors: [&GraphNode]`); + }, `Recursive schemas are unsuported, unstable support can be enabled via the 'recursiveSchemasAllowed' flag: GraphNode`); + })); + it('catches disallowed co-recursive (2 steps) Schemas syntax', Flags.withFlags({recursiveSchemasAllowed: false}, async () => { + assertThrowsAsync(async () => { + await Manifest.parse(` + schema Edge + name: Text + from: &Node + to: &Node + schema Node + name: Text + edges: [&Edge]`); + }, /Recursive schemas are unsuported, unstable support can be enabled via the 'recursiveSchemasAllowed' flag: (Node|Edge)/); + })); + it('catches disallowed co-recursive (3 steps) Schemas syntax', Flags.withFlags({recursiveSchemasAllowed: false}, async () => { + assertThrowsAsync(async () => { + await Manifest.parse(` + schema Edges + edges: [&Edge] + schema Edge + name: Text + from: &Node + to: &Node + schema Node + name: Text + edges: &Edges`); + }, /Recursive schemas are unsuported, unstable support can be enabled via the 'recursiveSchemasAllowed' flag: (Node|Edge|Edges)/); + })); + it('catches disallowed co-recursive inline (2 steps) Schemas syntax', Flags.withFlags({recursiveSchemasAllowed: false}, async () => { + assertThrowsAsync(async () => { + await Manifest.parse(` + schema Edge + name: Text + from: inline Node + to: inline Node + schema Node + name: Text + edges: [&Edge]`); + }, /Recursive schemas are unsuported, unstable support can be enabled via the 'recursiveSchemasAllowed' flag: (Node|Edge)/); + })); + }); }); diff --git a/src/utils/internal/misc.ts b/src/utils/internal/misc.ts index 245d2c2f241..036e24d5dab 100644 --- a/src/utils/internal/misc.ts +++ b/src/utils/internal/misc.ts @@ -109,14 +109,19 @@ export function mapToDictionary(map: Map): Dictionary { /** Recursively delete all fields with the given name. */ // tslint:disable-next-line: no-any -export function deleteFieldRecursively(node: any, field: string) { +export function deleteFieldRecursively(node: any, field: string, options?: {replaceWithNulls?: boolean}) { + options = options || {}; if (node == null || typeof node !== 'object') { return; } if (field in node) { - delete node[field]; + if (options.replaceWithNulls) { + node[field] = null; + } else { + delete node[field]; + } } for (const value of Object.values(node)) { - deleteFieldRecursively(value, field); + deleteFieldRecursively(value, field, options); } }