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.

+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.

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!);

  // ...
}

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;

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.

+1

Please add a test to verify.

WIP, will add it in the next commit.

added

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.

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.

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).

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.

Please don't modify original options.

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.

This looks like an unrelated whitespace change to me, PTAL

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

applied and new test case added.

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

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

Good catch!