feat(repository-json-schema): add in top-level metadata for json schema #907

shimks · 2018-01-24T21:31:57Z

added decorators to provide top-level metadata intended to be used for JSON Schema generation
can now take in:
- title (model)
- description (model)
- required (property)
- ~~oneOf (property)~~
point of interest: Definitions of PropertyType and ModelDefinitionSyntax have changed. Please take a look at how their new definitions would affect the juggler implementation
connected to [Schema] TypeScript to JSON Schema #786

Checklist

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

raymondfeng · 2018-01-25T23:22:22Z

@shimks I have two questions:

Should we align the decorator names more with TypeScript or JSON schema terms? For example, oneOf vs. unionType?
Do we prefer to have many fine-grained decorators or only a few taking metadata as an object?

bajtos · 2018-01-26T08:54:00Z

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

+    }
+  };
+
+  if (meta.title) {


The title property does not make sense to me. IMO, the title should be always set to the model name.

See e.g. http://json-schema.org/example1.html#starting-the-schema

{ "$schema": "http://json-schema.org/draft-06/schema#", "title": "Product", "description": "A product from Acme's catalog", "type": "object" }

I am imagining such schema would be created from the following code:

@model({ description: 'A product from Acme\'s catalog', }) class Product { }

I don't see the harm in allowing it to be overridden, though I'd expect that to take the form of result.title = meta.title || ctor.prototype.name; or something similar

Fair enough. Even though it's not required for MVP, it looks like a small addition with big benefits.

@shimks Please make sure there is a unit-test covering all different combinations (title not provided - model name used, title provided - model name is ignored) and also the documentation mentions this behavior.

bajtos · 2018-01-26T08:57:57Z

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

+      if (propMeta.validationKey === 'oneOf') {
+        let property: JsonDefinition = (schema.properties[prop] =
+          schema.properties[prop] || {});
+        if (!property.oneOf) {


AFAIK, juggler does not support oneOf constraints. Juggler allows clients to store values violating this constraint and as a result, the REST API will return responses that are not conforming to the OpenAPI spec.

I am proposing to leave oneOf out of this pull request. First of all, it's out of MVP scope. Secondly, we should implement support for oneOf in juggler first, and only then expose it via decorators.

Please correct me if my understanding is wrong and juggler does support oneOf constraint.

Interestingly enough ,the acceptance criteria for #786 say

For @model:
Add top-level specific metadata to the entry for this type:
(...)
oneOf (it's a union type!)

I find this confusing - oneOf is related to @property, not @model decorator, AFAIK juggler does not support oneOf and I don't understand why is this required for our MVP scope? AFAICT, https://github.com/strongloop/loopback-next/tree/master/packages/example-getting-started does not use oneOf at all.

@kjdelisle could you please clarify?

I'm relatively sure that oneOf is not supported juggler as well. Since support of oneOf was an MVP goal I've put in support for it and coded under the assumption that not every properties that get put into @model decorator had to be juggler related.

I think this just goes to show that we need to extract out the decorators into @loopback/model package of some sort to just have a general container for information that juggler and json-schema can use. This will require a refactor and I'm not sure when it should be done, but I think it'll save us a lot of headaches in the future.

@bajtos

The metadata we're producing in JSON Schema form is not the literal metadata that will be consumed by the Juggler, IMO. The primary purpose of making this JSON Schema is that it's the most descriptive form of metadata available to us after compilation, and consumers can freely use whichever parts of it they'd like and ignore others. The legacy juggler bridge does not need to support oneOf, but I think it would be worth it if the new Juggler can handle these types, since it would more neatly allow for integration with the newer OpenAPI spec.

We could just create the metadata we want for the current Juggler DSL directly, but I think that would be a mistake because it would limit our understanding of what could be compatible in the future. Using JSON Schema as the common ground comes at the cost of a slightly slower startup time, but gives us compile-time and run-time ways of seeing what we can transform representations into. (EDIT: to clarify the slower startup time, I meant that we would generate the JSON schema and the Juggler schema afterwards)

This means that extension developers can:

Work against an existing standard for developing their own representations for plugins, rather than conforming to our homegrown format

Use those new formats with the Juggler by converting them, and knowing they can do so by testing that their metadata readily converts to and from JSON Schema

Ideally, the new Juggler DSL would be a strict superset of JSON Schema, and would happily implement all of its types and representations.
cc @raymondfeng

@kjdelisle I see your point about using JSON Schema as the common ground. I have raised this point in the past, but @raymondfeng did not agree IIRC. If we decide to pursue this direction, then we must make it easy to use and tell the user when they are doing something that does not work (like using oneOf on a model that's persisted by juggler that does not support oneOf validation).

I also don't see why should oneOf be part of our MVP? Remember, we defined the MVP scope as whatever is needed to allow LoopBack4 developers to easily build our getting-started application. AFAICT, this application does not need oneOf, therefore oneOf is not part of our MVP.

I consider this as a scope creep, I really wish we could do better in keeping focus on what's absolutely required by MVP. I'm sure it will help us to deliver more of what we committed to in our monthly milestones.

I also don't see why should oneOf be part of our MVP? Remember, we defined the MVP scope as whatever is needed to allow LoopBack4 developers to easily build our getting-started application. AFAICT, this application does not need oneOf, therefore oneOf is not part of our MVP.

I think the task wasn't sufficiently groomed in that it didn't expound upon the expected support for properties and their metadata, so in that respect we definitely misstepped.

All that being said, I'm not certain what we gain by abandoning that code right here and now since it still would be something we'd want to add support for as a part of supporting JSON Schema.

I see your point about using JSON Schema as the common ground. I have raised this point in the past, but @raymondfeng did not agree IIRC. If we decide to pursue this direction, then we must make it easy to use and tell the user when they are doing something that does not work (like using oneOf on a model that's persisted by juggler that does not support oneOf validation).

Okay, I re-read this a few times and I think I get what you're saying now; the fact that the legacy-juggler-bridge doesn't already have any facilities in place for processing this metadata means that it would at best, do nothing and at worst, break things for anyone making use of the LJB, at least until we add logic to ignore/handle it (which is definitely out of scope for this PR, IMO).

My problem with this patch is that it creates the impression that people can use oneOf restriction when defining their (database) models. When they start using it, they will discover their oneOf validation is actually not performed. In the best case, they will do some debugging to find out that oneOf is not supported by juggler/database connectors. My concern is that most people won't do that and they will instead go directly to us, asking what's wrong. This is sort of similar to basePath situation which is confusing to LB users.

All that being said, I'm not certain what we gain by abandoning that code right here and now since it still would be something we'd want to add support for as a part of supporting JSON Schema.

Makes sense.

I am asking to add a bit of code to our legacy juggler that will throw an exception when somebody tries to add oneOf validation to a model property backed by juggler. So that potential users are immediately notified that they are trying to use a feature that's not supported yet.

bajtos · 2018-01-26T09:00:14Z

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

+    schema: JsonDefinition,
+    prop: string,
+    propCtor: Function | string,
+  ) => {


Please extract defineSchemaProperty to a regular top-level function. Right now, the diff contains too many whitespace changes which makes it impractical to review. Also modelToJsonSchema is becoming too long in the terms of lines of code, we should start refactoring bits of code into standalone functions defined outside of modelToJsonSchema.

bajtos · 2018-01-26T09:02:23Z

packages/repository/src/decorators/model.ts

@@ -81,14 +81,28 @@ export function property(definition?: Partial<PropertyDefinition>) {
 export namespace property {
  export const ERR_PROP_NOT_ARRAY =
    '@property.array can only decorate array properties!';
+  export const ERR_NO_ARGS = 'Should have at least one argument';
+  export const ERR_PROP_IS_ARRAY = 'Cannot use @property.union on arrays';


The code (API) is using oneOf, this error message says union. Please make it consistent. (I think this is just an oversight on your part?)

bajtos · 2018-01-26T09:04:20Z

packages/repository/src/decorators/model.ts

@@ -81,14 +81,28 @@ export function property(definition?: Partial<PropertyDefinition>) {
 export namespace property {
  export const ERR_PROP_NOT_ARRAY =
    '@property.array can only decorate array properties!';
+  export const ERR_NO_ARGS = 'Should have at least one argument';


This error message is too generic which may make it difficult for users to understand what went wrong.

Proposed text:

@property.oneOf requires at least one argument.

oneOf with a single item does not make much sense to me - people should use Foo instead of oneOf: [Foo]. I would personally enforce at least two arguments.

Actually, the JSON Schema specification says that one item is fine:

An instance validates successfully against this keyword if it validates successfully against exactly one schema defined by this keyword's value.

Source

shimks · 2018-01-26T15:27:06Z

@raymondfeng

I've decided to go for the JSON Schema term for now since the juggler has no use for it and the naming scheme would tell the user that the decorator is intended for JSON Schema generation. On the other hand, if we intended the decorators to be named after a general LoopBack model and not strictly juggler or JSON Schema concepts, I think it's also reasonable to name it using TypeScript term. Do you have a strong opinion for either of the cases?
I've gone with the approach of having multiple decorators since manually giving the necessary information to just the basic decorator is not too user friendly. Compare:

@property({
  array: true,
  types: [Foo, Bar]
})

with this:

@property.oneOf.array(Foo, Bar)

But it's also bad for us to give users separate decorators for each little cases, so I can see the arguments for both cases here.

kjdelisle · 2018-01-24T22:15:27Z

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

+    }
+
+    if (propMeta.validationKey) {
+      if (propMeta.validationKey === 'oneOf') {


I think everything in this if statement should be extracted into its own function, to help with readability.

kjdelisle · 2018-01-28T19:57:17Z

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

+      if (propMeta.validationKey === 'oneOf') {
+        let property: JsonDefinition = (schema.properties[prop] =
+          schema.properties[prop] || {});
+        if (!property.oneOf) {


@bajtos

The metadata we're producing in JSON Schema form is not the literal metadata that will be consumed by the Juggler, IMO. The primary purpose of making this JSON Schema is that it's the most descriptive form of metadata available to us after compilation, and consumers can freely use whichever parts of it they'd like and ignore others. The legacy juggler bridge does not need to support oneOf, but I think it would be worth it if the new Juggler can handle these types, since it would more neatly allow for integration with the newer OpenAPI spec.

We could just create the metadata we want for the current Juggler DSL directly, but I think that would be a mistake because it would limit our understanding of what could be compatible in the future. Using JSON Schema as the common ground comes at the cost of a slightly slower startup time, but gives us compile-time and run-time ways of seeing what we can transform representations into. (EDIT: to clarify the slower startup time, I meant that we would generate the JSON schema and the Juggler schema afterwards)

This means that extension developers can:

Work against an existing standard for developing their own representations for plugins, rather than conforming to our homegrown format

Use those new formats with the Juggler by converting them, and knowing they can do so by testing that their metadata readily converts to and from JSON Schema

Ideally, the new Juggler DSL would be a strict superset of JSON Schema, and would happily implement all of its types and representations.
cc @raymondfeng

kjdelisle · 2018-01-28T20:11:04Z

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

+    }
+  };
+
+  if (meta.title) {


I don't see the harm in allowing it to be overridden, though I'd expect that to take the form of result.title = meta.title || ctor.prototype.name; or something similar

kjdelisle · 2018-01-28T20:26:30Z

packages/repository/src/decorators/model.ts

@@ -81,14 +81,28 @@ export function property(definition?: Partial<PropertyDefinition>) {
 export namespace property {
  export const ERR_PROP_NOT_ARRAY =
    '@property.array can only decorate array properties!';
+  export const ERR_NO_ARGS = 'Should have at least one argument';


Actually, the JSON Schema specification says that one item is fine:

An instance validates successfully against this keyword if it validates successfully against exactly one schema defined by this keyword's value.

Source

bajtos · 2018-01-29T14:01:52Z

@shimks in relation to your last comment, could you please add tests showing/verifying how to specify these different types?

Either a string or a list of strings (oneOf containing an array option). Expressed in TypeScript: string | string[].
An array of items where each item is either a number or a string (an array containing items restricted via oneOf). Expressed in TypeScript: (number | string)[].
Either a string, or a number, or an array of items where each item is either a number or a string (an array of oneOf of array): (number | string | (number | string)[]).

shimks · 2018-01-30T17:55:01Z

@bajtos @kjdelisle I ended up getting rid of oneOf support. There were two main reasons: one was that I did not like the way I implemented it (using type as an array) since it ends up breaking repository stuff if it was to be used. This would require a refactor, and that seems out of scope now with @bajtos 's concern in mind. The second is that since decoupling of model from repository is in the talks (or at least in the minds of a couple of people), I don't think it make sense to write code that is going to be heavily refactored in the future anyway.

kjdelisle · 2018-01-30T18:02:08Z

I'm okay with this. We can revisit the more advanced JSON schema features later.

kjdelisle

I'm satisfied. Anything minor can be addressed in subsequent PRs.

Comments addressed as originally discussed (oneOf has been removed)

b-admike

LGTM.

shimks requested review from bajtos, kjdelisle and raymondfeng as code owners January 24, 2018 21:31

shimks added this to the Sprint 54 milestone Jan 24, 2018

bajtos previously requested changes Jan 26, 2018

View reviewed changes

kjdelisle reviewed Jan 28, 2018

View reviewed changes

shimks added 2 commits January 30, 2018 12:07

feat(repository-json-schema): add in top-level metadata for json schema

05f1b5b

feat(repository-json-schema): apply feedback

eeb1cc0

shimks force-pushed the json-schema-metadata branch from 7dc92eb to eeb1cc0 Compare January 30, 2018 17:49

kjdelisle modified the milestones: Sprint 54, February 2018 Jan 30, 2018

kjdelisle assigned shimks Jan 30, 2018

kjdelisle approved these changes Jan 30, 2018

View reviewed changes

kjdelisle requested review from jannyHou and b-admike January 30, 2018 18:21

b-admike approved these changes Jan 30, 2018

View reviewed changes

shimks merged commit fe59e6b into master Jan 30, 2018

shimks deleted the json-schema-metadata branch January 30, 2018 18:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(repository-json-schema): add in top-level metadata for json schema #907

feat(repository-json-schema): add in top-level metadata for json schema #907

shimks commented Jan 24, 2018 •

edited

Loading

raymondfeng commented Jan 25, 2018

bajtos Jan 26, 2018

kjdelisle Jan 28, 2018

bajtos Jan 29, 2018

bajtos Jan 26, 2018

bajtos Jan 26, 2018

shimks Jan 26, 2018

kjdelisle Jan 28, 2018

bajtos Jan 29, 2018

kjdelisle Jan 29, 2018

kjdelisle Jan 29, 2018

bajtos Jan 30, 2018

bajtos Jan 26, 2018

bajtos Jan 26, 2018

bajtos Jan 26, 2018

kjdelisle Jan 28, 2018

shimks commented Jan 26, 2018 •

edited

Loading

kjdelisle Jan 24, 2018

kjdelisle Jan 28, 2018

kjdelisle Jan 28, 2018

kjdelisle Jan 28, 2018

bajtos commented Jan 29, 2018

shimks commented Jan 30, 2018

kjdelisle commented Jan 30, 2018

kjdelisle left a comment

b-admike left a comment

feat(repository-json-schema): add in top-level metadata for json schema #907

feat(repository-json-schema): add in top-level metadata for json schema #907

Conversation

shimks commented Jan 24, 2018 • edited Loading

Checklist

raymondfeng commented Jan 25, 2018

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

shimks commented Jan 26, 2018 • edited Loading

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

bajtos commented Jan 29, 2018

shimks commented Jan 30, 2018

kjdelisle commented Jan 30, 2018

kjdelisle left a comment

Choose a reason for hiding this comment

b-admike left a comment

Choose a reason for hiding this comment

shimks commented Jan 24, 2018 •

edited

Loading

shimks commented Jan 26, 2018 •

edited

Loading