fix(repository-json-schema): resolve the circular reference #2690

jannyHou · 2019-04-04T19:36:33Z

Description

The solution here is a little bit different than the proposal in the original issue:

Ignore this item. ~~The definitions contains both Category and Product.(This is not true) So that~~

...
properties: {
    category: {
        $ref: '#/definitions/Category',
    },
},
...

~~refers to a valid definition entry.~~

The visited field is ~~an array~~ a set tracks the title of visited items instead of caching the schema. This is because getJsonSchema is called during each iteration of the property schema generation, not after the completed model schema(without definitions) get built.

Checklist

👉 Read and sign the CLA (Contributor License Agreement) 👈

npm test passes on your machine
New tests added or existing tests modified to cover all changes
Code conforms with the style guide
API Documentation in code was updated
Documentation in /docs/site was updated
~~Affected artifact templates in packages/cli were updated~~
~~Affected example projects in examples/* were updated~~

👉 Check out how to submit a PR 👈

raymondfeng · 2019-04-04T20:40:38Z

packages/repository-json-schema/src/build-schema.ts

@@ -14,18 +14,27 @@ import {
 import {JSONSchema6 as JSONSchema} from 'json-schema';
 import {JSON_SCHEMA_KEY} from './keys';

+export interface JsonSchemaOptions {
+  // Track the models/titles that have been visited.
+  visited?: string[];


Let's use Set instead to include visited TypeScript classes.

+1 to use a Set instead of an array.

FYI: In a very near feature, we will be emitting multiple schemas for a single model class:

Vanilla model schema as today

Model schema including relations

Model schema excluding certain properties (id, _rev, etc.)

Using TypeScript classes as the key will not work for that. I think it's better to use Schema title because that has to be unique. See #2653, #2652 and https://github.com/strongloop/loopback-next/pull/2646/files#diff-432476dd2d984e441d8b4e0af39a39be.

@bajtos The set tracks title instead of the class name, see code

Or do you want to store the whole generated schema in the options.visited?
That would not be possible IMO, since generating the referenced definitions happen in the iteration, before all the properties are visited.

The set tracks title instead of the class name

Cool, that should work well 👍

do you want to store the whole generated schema in the options.visited?

IIRC, in my PoC, I was storing the whole generated schema in options.visited to allow me to load it from options.visited and put it to result.definitions.

raymondfeng · 2019-04-04T20:41:58Z

Please run npm run update-package-locks or npm run update-packages to fix package-lock.json files.

bajtos

Great start!

bajtos · 2019-04-05T07:29:08Z

packages/repository-json-schema/src/build-schema.ts

@@ -14,18 +14,27 @@ import {
 import {JSONSchema6 as JSONSchema} from 'json-schema';
 import {JSON_SCHEMA_KEY} from './keys';

+export interface JsonSchemaOptions {
+  // Track the models/titles that have been visited.
+  visited?: string[];


+1 to use a Set instead of an array.

FYI: In a very near feature, we will be emitting multiple schemas for a single model class:

Vanilla model schema as today

Model schema including relations

Model schema excluding certain properties (id, _rev, etc.)

Using TypeScript classes as the key will not work for that. I think it's better to use Schema title because that has to be unique. See #2653, #2652 and https://github.com/strongloop/loopback-next/pull/2646/files#diff-432476dd2d984e441d8b4e0af39a39be.

bajtos · 2019-04-05T07:30:36Z

packages/repository-json-schema/src/build-schema.ts

@@ -148,6 +160,8 @@ export function modelToJsonSchema(ctor: Function): JSONSchema {
  }

  result.title = meta.title || ctor.name;
+  const isVisited =
+    options && options.visited && options.visited.includes(result.title!);


Let's set options to {} by default so that we don't have to check for its existence.

export function modelToJsonSchema( ctor: Function, options: JsonSchemaOptions = {} ): JSONSchema { // ... const isVisited = options.visited && options.visited.includes(result.title!); // ... }

bajtos · 2019-04-05T07:31:33Z

packages/repository-json-schema/src/build-schema.ts

@@ -148,6 +160,8 @@ export function modelToJsonSchema(ctor: Function): JSONSchema {
  }

  result.title = meta.title || ctor.name;
+  const isVisited =
+    options && options.visited && options.visited.includes(result.title!);


Please avoid using ! operator to silence TypeScript checks.

const title = meta.title || ctor.name; const isVisited = options.visited && options.visited.includes(title); result.title = title;

bajtos · 2019-04-05T07:37:41Z

packages/repository-json-schema/src/build-schema.ts

+    // If the model is already visited in the call stack, it implies one or more
+    // of its referenced properties refer back to it.
+
+    if (isVisited) break;


I am confused. Why are we checking isVisited deep inside the loop iterating over model properties?

In the spike 2256435, I was able to return early in the schema generation. Once we know the schema title, we can check whether we have already seen such schema and immediately return.

What am I missing?

The visited field is an array tracks the title of visited items instead of caching the schema. This is because getJsonSchema is called during each iteration of the property schema generation, not after the completed model schema(without definitions) get built.

This should not matter. In our cache (both visited and metadata-based cache used by getJsonSchema), we are storing a schema object by reference. Even if we initially store any empty schema object first with no properties, the code executed by modelToJsonSchema will eventually fill in those properties directly in the cached object.

You are right. My expected JSON schema was wrong, I thought the definitions field from modelToJsonSchema(Category) should contain the schema of Category itself.

I refactored the code to return an empty array when the model is visited, which is still slightly different from your PoC.

In our cache (both visited and metadata-based cache used by getJsonSchema), we are storing a schema object by reference. Even if we initially store any empty schema object first with no properties, the code executed by modelToJsonSchema will eventually fill in those properties directly in the cached object.

Sounds reasonable to me...I am trying the code in PoC while run into the circular error again. I might miss something, need more time to figure it out.

bajtos · 2019-04-05T07:38:02Z

packages/repository-json-schema/src/build-schema.ts

+
+    if (isVisited) break;
+
+    // Use object assign to avoid polluting the original `options`.


+1

Please add a test to verify.

WIP, will add it in the next commit.

bajtos · 2019-04-05T07:40:47Z

packages/repository-json-schema/src/__tests__/integration/build-schema.integration.ts

+      };
+
+      it('handles circular references', () => {
+        const schema = modelToJsonSchema(Category);


Please add a test to verify getJsonSchema too.

I didn't realize getJsonSchema is the entry point of the schema generation, thought it starts with modelToJsonSchema.

Good catch! Tests added.

bajtos · 2019-04-05T07:41:11Z

.vscode/settings.json

@@ -1,5 +1,7 @@
 {
-  "editor.rulers": [80],
+  "editor.rulers": [
+    80


Unrelated change, please revert.

@bajtos The format is changed when auto save. And if you check the other settings, breaking it into multiple the lines is the expected format.

I can try to move the change into a separate commit.

The format is changed when auto save. And if you check the other settings, breaking it into multiple the lines is the expected format.

Fair enough 👍

I can try to move the change into a separate commit.

Yes please. A new PR would be even better, that way we can land this small fix sooner.

It turns out it's just my vscode's weird settings.
The other settings are broken down into multiple lines due to the max characters per line.
Reverting.

bajtos · 2019-04-05T14:07:51Z

packages/repository-json-schema/src/build-schema.ts

+    getJsonSchemaOptions.visited = getJsonSchemaOptions.visited || [];
+    getJsonSchemaOptions.visited.push(result.title!);
+
+    const propSchema = getJsonSchema(referenceType, getJsonSchemaOptions);

    if (propSchema && Object.keys(propSchema).length > 0) {
      result.definitions = result.definitions || {};


One idea: since we are effectively storing the schema of visited models in result.definitions; if we need to access schema of a model that has been already visited, we can perhaps obtain it from those definitions object? Or maybe we should remove options.visited and pass the definitions key-value map in options instead?

One idea: since we are effectively storing the schema of visited models in result.definitions

Hmm, do you mean storing the schema of the current model in the definitions?

Like

{ title: 'Category', properties: { name: { type: 'string' }, }, definitions: { Category: { title: 'Category', properties: { name: { type: 'string' }, }, }, }, }

Hmm, do you mean storing the schema of the current model in the definitions?

No, only schema of models used by properties (i.e. Product when emitting schema for Category).

jannyHou · 2019-04-11T04:33:53Z

@bajtos @raymondfeng Thanks for the review, feedback applied.

@bajtos My implementation is still slightly different than your PoC, see my comment

I may need some time to explore you original approach then compare.

bajtos · 2019-04-11T14:31:14Z

My implementation is still slightly different than your PoC, see my comment

I may need some time to explore you original approach then compare.

Sure, take your time. My spike code is just a prototype, it's fine to end up with a different implementation. As long as it supports the use cases we need to support.

jannyHou · 2019-04-12T14:47:54Z

@bajtos I figured out the difference. Your PoC skipped prompting visited definition to the top level with this line of code, which simplifies everything!

I still didn't get the idea of tracking the visited model with its generated schema like {CategoryModelOnly: ...CategoryModelOnlySchema}. IMO using a set containing all the visited models(titles to be more accurate) is enough, it's light than storing the entire schema in the option. Thought?

bajtos

The implementation looks good at high level, let's improve few implementation details now.

bajtos · 2019-04-15T13:48:56Z

packages/repository-json-schema/src/build-schema.ts

-  result.title = meta.title || ctor.name;
+  const title = meta.title || ctor.name;
+
+  if (options.visited.has(title)) return {};


IIUC, by returning an empty object, you are trying to satisfy the return value type JSONSchema while returning an invalid (or at least useless) value. That's effectively bypassing type checks :(

I think this was the reason why I was keeping the visited schemas in the cache?

Anyhow. Please change this line to return undefined to make it clear that when a model has been already visited, then we are not returning it's schema.

To preserve backwards compatibility, I am proposing the following signature:

export function modelToJsonSchema(ctor: Function): JSONSchema; export function modelToJsonSchema( ctor: Function, options: JsonSchemaOptions = {}, ): JSONSchema | undefined; export function modelToJsonSchema( ctor: Function, options: JsonSchemaOptions = {}, ): JSONSchema | undefined { // the implementation }

Alternatively, and I think this is a better option when it comes to type safety, promote visited to a top-level argument and keep the JsonSchemaOptions as an empty interface for now.

export function modelToJsonSchema( ctor: Function, options: JsonSchemaOptions = {} ): JSONSchema; export function modelToJsonSchema( ctor: Function, options: JsonSchemaOptions, visited: Set<string>; ): JSONSchema | undefined; export function modelToJsonSchema( ctor: Function, options: JsonSchemaOptions = {}, visited?: Set<string>; ): JSONSchema | undefined { // the implementation }

That's effectively bypassing type checks :(
I think this was the reason why I was keeping the visited schemas in the cache?

Oh...that's true!

by returning an empty object, you are trying to satisfy the return value type JSONSchema while returning an invalid (or at least useless) value.

I agree.
And undefined and {} both return useless value which logically isn't as perfect as returning the cache value.

And since the cached schema is just a reference, tracking with a Set won't save space compared with tracking with cache. I switched to the approach in your PoC.

bajtos · 2019-04-15T13:53:31Z

packages/repository-json-schema/src/build-schema.ts

+  ctor: Function,
+  options: JsonSchemaOptions = {},
+): JSONSchema {
+  options.visited = options.visited || new Set<string>();


Please don't modify original options.

Suggested change

options.visited = options.visited || new Set<string>();

if (!options.visited) options = {...options, visited: new Set<string>()};

Please add a test to verify.

Please don't modify original options.

Hmm, any reason?

It's confusing for callers when you modify the options object. The caller may use the same options instance for multiple calls.

For example:

const SCHEMA_OPTIONS = {includeRelations: true}; const productSchema = getJsonSchema(Product, SCHEMA_OPTIONS); const categorySchema = getJsonSchema(Category, SCHEMA_OPTIONS);

👍 got it thank you.

bajtos · 2019-04-16T08:01:28Z

packages/repository-json-schema/src/build-schema.ts

-      // error if itemType/type is not a string or a function
-      resolveType(metaProperty.itemType as string | Function)
+        // error if itemType/type is not a string or a function
+        resolveType(metaProperty.itemType as string | Function)


This looks like an unrelated whitespace change to me, PTAL

bajtos · 2019-04-16T08:02:13Z

packages/repository-json-schema/src/build-schema.ts

+  ctor: Function,
+  options: JsonSchemaOptions = {},
+): JSONSchema {
+  options.visited = options.visited || {};


Please don't modify original options, see the discussion above.

applied and new test case added.

b-admike

LGTM 👍

b-admike · 2019-04-17T21:03:34Z

packages/repository-json-schema/src/__tests__/integration/build-schema.integration.ts

@@ -641,5 +680,55 @@ describe('build-schema', () => {
        },
      });
    });
+    it('does not pollute the JSON schema options', () => {


I wonder if we can treat this as a unit test instead. Feel free to ignore.

nabdelgadir

Just have one minor question, but LGTM 👍

nabdelgadir · 2019-04-18T15:16:15Z

packages/repository-json-schema/src/__tests__/integration/build-schema.integration.ts

+      }
+
+      const JSON_SCHEMA_OPTIONS = {};
+      // tslint:disable-next-line:no-unused


Question, why do you need the const categorySchema = part instead of just calling getJsonSchema(Category, JSON_SCHEMA_OPTIONS);?

Good catch!

raymondfeng · 2019-04-18T17:58:20Z

@jannyHou Do we have a test to make sure such json schemas are converted to OpenAPI spec correctly? For example, using:

components
  - schemas
     - Product

jannyHou · 2019-04-22T14:59:08Z

@raymondfeng Yeah I believe we have it in the openapi-v3 package, see test

jannyHou requested review from bajtos and raymondfeng as code owners April 4, 2019 19:36

jannyHou force-pushed the fix/circular-reference branch from 74edbba to 3b350c4 Compare April 4, 2019 19:40

jannyHou changed the title ~~fix(repository-json-schema): resolve the circular reference~~ [WIP]fix(repository-json-schema): resolve the circular reference Apr 4, 2019

jannyHou force-pushed the fix/circular-reference branch from 3b350c4 to 59447bd Compare April 4, 2019 20:15

jannyHou changed the title ~~[WIP]fix(repository-json-schema): resolve the circular reference~~ fix(repository-json-schema): resolve the circular reference Apr 4, 2019

raymondfeng reviewed Apr 4, 2019

View reviewed changes

bajtos suggested changes Apr 5, 2019

View reviewed changes

bajtos added bug OpenAPI labels Apr 5, 2019

bajtos reviewed Apr 5, 2019

View reviewed changes

jannyHou force-pushed the fix/circular-reference branch 5 times, most recently from 6bedae4 to fdd1cc4 Compare April 11, 2019 04:30

jannyHou mentioned this pull request Apr 11, 2019

unclear documentation about test setup with circular dependency of repositories #2568

Closed

2 tasks

jannyHou force-pushed the fix/circular-reference branch 2 times, most recently from ada0046 to 80ab41d Compare April 11, 2019 21:09

bajtos suggested changes Apr 15, 2019

View reviewed changes

bajtos reviewed Apr 16, 2019

View reviewed changes

jannyHou force-pushed the fix/circular-reference branch 3 times, most recently from d007850 to 20bb8a2 Compare April 16, 2019 19:32

b-admike approved these changes Apr 17, 2019

View reviewed changes

bajtos approved these changes Apr 18, 2019

View reviewed changes

nabdelgadir approved these changes Apr 18, 2019

View reviewed changes

fix(repository-json-schema): resolve the circular reference

acf6da6

jannyHou force-pushed the fix/circular-reference branch from 20bb8a2 to acf6da6 Compare April 22, 2019 15:00

jannyHou merged commit 9b49773 into master Apr 22, 2019

jannyHou mentioned this pull request Apr 22, 2019

Fix repository-json-schema to handle circular references #2628

Closed

3 tasks

jannyHou deleted the fix/circular-reference branch April 25, 2019 14:05


		if (isVisited) break;

		// Use object assign to avoid polluting the original `options`.

	options.visited = options.visited \|\| new Set<string>();
	if (!options.visited) options = {...options, visited: new Set<string>()};

fix(repository-json-schema): resolve the circular reference #2690

fix(repository-json-schema): resolve the circular reference #2690

Conversation

jannyHou commented Apr 4, 2019 • edited Loading

Description

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raymondfeng commented Apr 4, 2019

bajtos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jannyHou Apr 10, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jannyHou commented Apr 11, 2019

bajtos commented Apr 11, 2019

jannyHou commented Apr 12, 2019

bajtos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

b-admike left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nabdelgadir left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raymondfeng commented Apr 18, 2019

jannyHou commented Apr 22, 2019

jannyHou commented Apr 4, 2019 •

edited

Loading

jannyHou Apr 10, 2019 •

edited

Loading