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

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';

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());
   });
 });

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;
 }

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<string> = 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;
     }

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<inline Address {number, street, city}>
       `)).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']);
   });
 });

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,15 +357,15 @@ 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}
         `);
       assert.throws(
           () => resolveFieldPathType(['inlined', 'missing'], type),
           `Schema 'Foo {name: Text}' does not contain field 'missing'.`);
-    });
+    }));
   });
 
   describe('ordered lists', () => {
@@ -376,31 +377,31 @@ 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}>
       `);
       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<inline Bar {inner: Number}>
       `);
       resolveFieldPathType(['list'], type);
       resolveFieldPathType(['list', 'inner'], type);
-    });
+    }));
   });
 });

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(`

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.

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,