feat(json-schema): add modules to convert TS models to JSON Schema #860

shimks · 2018-01-12T15:21:08Z

initial implementation of TS model to JSON schema: leverages @model and @property in @loopback/repository to create valid json-schema from the metadata and then store in them global metadata space
uses type inference from parameter level @param to access the metadata
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

bajtos · 2018-01-12T15:42:16Z

@shimks Have you considered using our existing code in loopback-swagger (see lib/specgen/model-helper.js instead of writing the conversion from scratch? The are several edge cases that require special treatment, this has been all solved in loopback-swagger already.

On the other hand, your current approach may be better in the longer term, especially if/when we start making changes in our new model/property definition format that will diverge from LDL used by the current juggler.

bajtos · 2018-01-12T15:49:42Z

packages/json-schema/test/integration/build-schema.test.ts

+              $ref: '#definitions/Bar',
+            },
+          },
+        },


I am concerned that when this test fails, the error message will be difficult to understand, because the expected and actual objects are so large!

Please split this test into several smaller tests, where each test is checking one of the types.

it('properly converts "string" properties', () => { @model() class TestModel { @property() str: string; } const jsonDef = modelToJSONDef(TestModel); expect(jsonDef.properties).to.deepEqual({ str: { type: 'string', }, }); });

It is also important to make these tests resilient to future changes. For example, when modelToJSONDef starts emitting more top-level fields beyond properties, then we don't want to have to update all tests in this file with the same default values for these new properties. My example code above is already addressing that problem by verifying only the contents of jsonDef.properties. There should be a dedicated test to verify the overall shape of jsonDef.

See also "Isolation of failures" and "Resilience" in http://loopback.io/doc/en/lb4/Defining-your-testing-strategy.html:

Isolation of failures: The test suite should make it easy to isolate the source of test failures. To fix a failing test, developers need to find the specific place that does not work as expected. When the project contains thousands of lines and the test failure can be caused by any part of the system, then finding the bug is very difficult, time consuming and demotivating.

Resilience: The test implementation should be robust and resilient to changes in the tested code. As the project grows and matures, you may need to change existing behavior. With a brittle test suite, each change may break dozens of tests, for example when you have many end-to-end/UI tests that rely on specific UI layout. This makes change prohibitively expensive, up to a point where you may start questioning the value of such test suite.
References:

bajtos · 2018-01-12T15:50:26Z

packages/json-schema/test/integration/build-schema.test.ts

+            type: 'string',
+          },
+        });
+        expect(toJSONProperty(propMeta['arrBar'])).to.deepEqual({


One (logical) assert per test please. There should be two tests here - one for string arrays, another for Bar arrays.

bajtos · 2018-01-12T15:53:20Z

packages/openapi-spec/src/openapi-spec-v2.ts

      version: '1.0.0',
    },
    paths: {},
+    definitions: {},


The property definitions is optional according to Swagger Spec. Please leave it out from the empty API spec. (It will save you updates of so many tests below.)

bajtos · 2018-01-12T15:55:00Z

packages/openapi-v2/test/unit/controller-spec/controller-decorators.test.ts

          },
        },
      },
+      definitions: {},


Please revert these changes - see https://github.com/strongloop/loopback-next/pull/860/files#r161257117

Alternatively, rework the test suite in this file to be more robust:

modify the tests where you had to add an empty definitions property in such way that they don't depend on exact shape of the spec object (e.g. assert only the value of paths property)

ensure there is a single test verifying the top-level properties of the object returned by getControllerSpec

bajtos · 2018-01-12T15:57:21Z

...es/openapi-v2/test/unit/controller-spec/controller-decorators/param-decorators/param.test.ts

+              },
+            },
+          },
+          MySoul: {


I think this part is redundant - we have already verified elsewhere that "simple" models are correctly converted to JSON schema. Please simplify this test so that it asserts only the important/relevant facts.

For example:

expect(Object.keys(spec.definitions)).to.deepEqual(['MyBody', 'MySoul']); expect(spec.definitions.MyBody).to.deepEqual({ // ... });

bajtos · 2018-01-12T15:58:20Z

...es/openapi-v2/test/unit/controller-spec/controller-decorators/param-decorators/param.test.ts

+      const spec = getControllerSpec(MyController);
+      expect(spec).to.containEql(expectedDefs);
+    });
+    it('does not infer definition if no class metadata is present', () => {


Please add an empty line before it to serve as a visual delimiter.

bajtos · 2018-01-12T15:59:55Z

packages/rest/src/rest-server.ts

    // accidentally modifying our internal routing data
    spec.paths = cloneDeep(this.httpHandler.describeApiPaths());
-
+    spec.definitions = this.httpHandler.getApiDefinitions();


Should we apply cloneDeep here too?

~~I thought that would make sense when I wrote the code there, but it seems like cloneDeep is currently typed for just PathsObject. Maybe I'll add in a similar function for DefinitionsObject.~~ Nevermind, I'll apply cloneDeep there

shimks · 2018-01-12T16:14:56Z

@bajtos I should've known that the work was done for me haha. Personally, I'd be ok with using loopback-swagger, but you bring up a good point about how this approach might be better long term. I'll leave the discussion to anyone that wants to make a strong argument for either of the cases.

raymondfeng · 2018-01-12T16:35:09Z

packages/json-schema/README.md

@@ -0,0 +1,89 @@
+# @loopback/openapi-spec-builder


Fix the module name

raymondfeng · 2018-01-12T16:35:43Z

packages/json-schema/package.json

+{
+  "name": "@loopback/json-schema",
+  "version": "4.0.0-alpha.1",
+  "description": "TBD",


raymondfeng · 2018-01-12T16:37:16Z

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

+}
+
+// tslint:disable-next-line:no-any
+export function toJSONProperty(propMeta: any): JSONDefinition {


The arg type should be PropertyDefinition or something similar.

kjdelisle

I like a lot of what I see, but there are some strange bits:

The README is for json-schema, not OpenAPI and should be rewritten accordingly
Commented code needs to go
Missing TSDocs on the new functions, classes and types (these are a must)
Keywords in the README should not include references to OpenAPI/Swagger

Ultimately, this package gives us JSON schema that we can leverage across a variety of other protocols and approaches; it shouldn't be tied to OpenAPI or Swagger in any direct sense. Making use of those functions to simplify our OAI/Swagger story is one thing, but no one should need to deal with them to leverage your new module for other purposes.

kjdelisle · 2018-01-12T15:46:26Z

packages/json-schema/README.md

@@ -0,0 +1,89 @@
+# @loopback/openapi-spec-builder


The json-schema package should be about generating JSON schema from models, and not have anything to do with the OpenAPI spec builder. Did you copy-paste this README as a placeholder?

Yup, the README is not ready to be reviewed at all as in I haven't touched it after I've copy-pasted.

kjdelisle · 2018-01-12T15:47:01Z

packages/json-schema/package.json

+  "version": "4.0.0-alpha.1",
+  "description": "TBD",
+  "engines": {
+    "node": ">=6"


Node 8 or greater, please. "node": ">=8"

FWIW, if this new module supports only Node.js 8+, then we cannot start depending on it anywhere in the existing code base until we drop support for Node.js 6.x via #611.

kjdelisle · 2018-01-12T15:49:06Z

packages/json-schema/package.json

+  "license": "MIT",
+  "keywords": [
+    "Swagger",
+    "OpenAPI Spec",


Neither Swagger nor OpenAPI Spec should be keywords in this package.

kjdelisle · 2018-01-12T15:49:39Z

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

+// no metadata for: union, optional, nested array, any, enum, string literal,
+// anonymous types, inherited properties
+export function modelToJSONDef(ctor: Function): JSONDefinition {
+  // if (!MetadataInspector.getClassMetadata(MODEL_KEY, ctor)) {


Remove commented code

kjdelisle · 2018-01-12T15:49:59Z

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

+  // }
+  // tslint:disable-next-line:no-any
+  const meta = ModelMetadataHelper.getModelMetadata(ctor) as any;
+  // if (Object.keys(meta.properties).length === 0) {


Remove commented code

kjdelisle

I'm much happier with this version. Should still wait for @bajtos and @raymondfeng

jannyHou

cc @kjdelisle
~~Do we need a new package for this functionality? I think we already agree to put all the spec generator functions in package openapi-v2 right?~~

Sorry never mind I thought this PR converts it to openapi spec.

virkt25

Just some nitpicks

virkt25 · 2018-01-12T20:49:31Z

packages/json-schema/LICENSE

@@ -0,0 +1,25 @@
+Copyright (c) IBM Corp. 2018. All Rights Reserved.
+Node module: @loopback/openapi-spec-builder


Incorrect module name.

virkt25 · 2018-01-12T20:52:25Z

packages/json-schema/index.d.ts

@@ -0,0 +1,6 @@
+// Copyright IBM Corp. 2018. All Rights Reserved.
+// Node module: @loopback/openapi-spec-builder


Update module name

virkt25 · 2018-01-12T20:52:34Z

packages/json-schema/index.js

@@ -0,0 +1,9 @@
+// Copyright IBM Corp. 2018. All Rights Reserved.
+// Node module: @loopback/testlab


Update module name

virkt25 · 2018-01-12T20:52:51Z

packages/json-schema/index.ts

@@ -0,0 +1,7 @@
+// Copyright IBM Corp. 2013,2018. All Rights Reserved.
+// Node module: @loopback/openapi-spec-builder


update module name

virkt25 · 2018-01-12T20:53:51Z

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

@@ -0,0 +1,74 @@
+// Copyright IBM Corp. 2013,2018. All Rights Reserved.
+// Node module: @loopback/openapi-spec-builder


update module name

virkt25 · 2018-01-12T20:55:00Z

packages/json-schema/src/index.ts

@@ -0,0 +1,6 @@
+// Copyright IBM Corp. 2013,2018. All Rights Reserved.
+// Node module: @loopback/openapi-spec


update module name

virkt25 · 2018-01-12T20:55:11Z

packages/json-schema/test/integration/build-schema.test.ts

@@ -0,0 +1,215 @@
+// Copyright IBM Corp. 2013,2018. All Rights Reserved.
+// Node module: @loopback/openapi-spec-builder


update module name

virkt25 · 2018-01-12T20:58:10Z

packages/json-schema/test/integration/build-schema.test.ts

+import {expect} from '@loopback/testlab';
+
+describe('build-schema', () => {
+  context('exception', () => {


context is an alias for describe. AFAIK we just use nested describe statements. @bajtos can comment on the pattern we should be using.

I think the usage of contex is correct here, see http://loopback.io/doc/en/contrib/style-guide.html#test-block-naming

In this particular case, I feel this particular nesting block is redundant and the test it('errors out when "@property.array" is not used on an array' should be moved into describe('modelToJsonDef') block below, because modelToJsonDef is the function executed by this test.

virkt25 · 2018-01-12T20:59:43Z

packages/json-schema/test/integration/build-schema.test.ts

+      class BadArray {
+        @property() badArr: string[];
+      }
+      expect(() => {


What is the purpose of the anonymous function? Is there a reason this can't be expect(modelToJsonDef(BadArray)).to.throw(/type is defined as an array/);?

Apparently I need to enclose whatever that I expect to throw an error in an anonymous function. Does ShouldJS have an alternative where I we wouldn't need to use the anonymous function?

I used expect(function()).to.be.rejectedWith() ... where rejectedWith takes the same arguments as throw.

it doesn't work :(, the error just gets thrown at my face instead of being caught

Oh ok. I’m fine with what you had. I was just curious. I think rejectedwith is for promises 😅

virkt25 · 2018-01-12T21:02:38Z

packages/json-schema/test/integration/build-schema.test.ts

+        @property() nul: null;
+        @property() undef: undefined;
+      }
+      expect(modelToJsonDef(TestModel).properties).to.not.containEql({


I think it might be better to store the results of the function call and then do the checks.

const jsonDef = modelToJsonDef(TestModel); expect(jsonDef.properties).to.no.containEql({nul: {type: 'null'}}); expect(modelToJsonDef(TestModel).properties).to.not.containEql({undef: {type: 'undefined'}});

bajtos

The new version looks much better! I have few more comments to consider and discuss.

bajtos · 2018-01-15T13:16:48Z

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

+ * Currently, objects of type JSONDefinition can also be cast as a SchemaObject,
+ * a property of OpenAPI-v2's specification
+ */
+export type JsonDefinition = {


I don't mind maintaining our own copy of TypeScript interface/type definition for JSON Schema, but it still makes me wonder - isn't there any npm package we could leverage for that?

bajtos · 2018-01-15T13:18:02Z

packages/json-schema/README.md

+
+## Overview
+
+This package provides modules to easily convert LoopBack4 models that have been decorated with `@model` and `@property` to a matching JSON Schema Definition.


Since @model and @property decorators are coming from @loopback/repository package and are technically optional, would it make sense to rename @loopback/json-schema to something like @loopback/repository-json-schema or @loopback/model-json-schema?

I think it makes sense considering the package is somewhat of an extension to the functionalities that @model and @property are going to provide. I changed the name to @loopback/repository-json-schema.

bajtos · 2018-01-15T13:20:12Z

packages/json-schema/test/integration/build-schema.test.ts

+import {expect} from '@loopback/testlab';
+
+describe('build-schema', () => {
+  context('exception', () => {


I think the usage of contex is correct here, see http://loopback.io/doc/en/contrib/style-guide.html#test-block-naming

In this particular case, I feel this particular nesting block is redundant and the test it('errors out when "@property.array" is not used on an array' should be moved into describe('modelToJsonDef') block below, because modelToJsonDef is the function executed by this test.

bajtos · 2018-01-15T13:26:14Z

packages/json-schema/test/integration/build-schema.test.ts

+      }
+      const jsonDef = modelToJsonDef(TestModel);
+      expect(jsonDef.properties).to.not.containEql({
+        nul: {type: 'null'},


I find this check weird and possible brittle. In theory, there is an infinite number of ways how to convert a null property into JSON schema, almost all of them invalid, and here we are ruling out only one of the invalid options.

For example, the following output would pass this test:

{ properties: { nul: { anyOf: [{type: 'null'}] } } }

I think you wanted to assert that jsonDef.properties does not contain any entry for the nul property, right?

expect(jsonDef.properties).to.not.have.key('nul');

The same comment applies to next check for undef below.

bajtos · 2018-01-15T13:27:06Z

packages/json-schema/test/integration/build-schema.test.ts

+      }).to.throw(/type is defined as an array/);
+    });
+  });
+  describe('modelToJSON', () => {


Please add an empty lines between describe blocks and also between it blocks. They server as a visual delimiter and make it easier to spot where on test (case/suite) ends and the next one starts.

bajtos · 2018-01-15T13:34:41Z

packages/json-schema/test/integration/build-schema.test.ts

+        propMeta = meta.properties;
+      });
+      it('converts primitively typed property correctly', () => {
+        expect(toJsonProperty(propMeta['str'])).to.deepEqual({


propMeta.str

The same comment applies to most (if not all) similar tests below.

bajtos · 2018-01-15T13:35:30Z

packages/openapi-v2/src/controller-spec.ts

+
+    for (const p of paramTypes) {
+      if (
+        !includes([String, Number, Boolean, Array, Object], p) &&


Please extract the multi-line condition into a named variable, see http://loopback.io/doc/en/contrib/style-guide.html#indentation-of-multi-line-expressions-in-if

bajtos · 2018-01-15T13:37:19Z

packages/rest/test/unit/rest-server/rest-server.open-api-spec.test.ts

      basePath: '/api',
      paths: {},
      'x-foo': 'bar',
+      definitions: {},


Maybe I am too stubborn about this, but cannot we remove definitions property from the api spec reported by the server when there were not definitions specified?

See my two comments above for an inspiration.

Nope, you're not being stubborn. I just forgot to take those out :)

bajtos · 2018-01-15T13:37:52Z

packages/rest/src/http-handler.ts


 export class HttpHandler {
  protected _routes: RoutingTable = new RoutingTable();
+  protected _apiDefinitions: DefinitionsObject = {};


I am proposing to change the default value to undefined here.

bajtos · 2018-01-15T13:38:11Z

packages/rest/src/http-handler.ts

  }

+  registerApiDefinitions(defs: DefinitionsObject) {
+    Object.assign(this._apiDefinitions, defs);


this._apiDefinitions = Object.assign({}, this._apiDefinitions, defs);

bajtos

I don't think it's a good idea to leave support for Node.js 6.x in the new package as part of this pull request. It affects more than just engines field in package.json, see my comments below.

Also if this new module supports only Node.js 8+, then I think we cannot start depending on it anywhere in the existing code base until we drop support for Node.js 6.x there too (via #611), otherwise CI builds will start failing. (They are not failing yet because we are still transpiling for Node.js 6.x now.)

bajtos · 2018-01-15T13:41:28Z

packages/json-schema/package.json

+    "build": "npm run build:dist && npm run build:dist6",
+    "build:current": "lb-tsc",
+    "build:dist": "lb-tsc es2017",
+    "build:dist6": "lb-tsc es2015",


If we are supporting Node.js >=8, then there is no need for build:dist6.

bajtos · 2018-01-15T13:41:51Z

packages/json-schema/package.json

+    "clean": "lb-clean loopback-json-schema*.tgz dist dist6 package api-docs",
+    "prepare": "npm run build && npm run build:apidocs",
+    "pretest": "npm run build:current",
+    "test": "lb-dist mocha --opts ../../test/mocha.opts 'DIST/test/unit/**/*.js' 'DIST/test/integration/**/*.js' 'DIST/test/acceptance/**/*.js'",


If we are supporting Node.js >=8, then there is no need for lb-dist, because there is only a single dist folder.

bajtos · 2018-01-15T13:42:27Z

packages/json-schema/index.js

+const nodeMajorVersion = +process.versions.node.split('.')[0];
+module.exports = nodeMajorVersion >= 7 ?
+  require('./dist/src') :
+  require('./dist6/src');


If we are supporting Node.js >=8, then there should be no dist6 and we should always forward to ./dist/src.

b-admike

LGTM once comments have been addressed.

bajtos

Almost there!

bajtos · 2018-01-16T14:46:12Z

packages/repository-json-schema/docs.json

+  "codeSectionDepth": 4,
+  "assets": {
+    "/": "/docs",
+    "/docs": "/docs"


Since we don't have any API docs asset yet, and /docs does not even exist, I am proposing to remove the "assets" property completely from this file.

bajtos · 2018-01-16T14:46:45Z

packages/repository-json-schema/docs.json

@@ -0,0 +1,8 @@
+{
+  "content": ["index.ts", "src/index.ts", "src/build-schema.ts"],


We usually put each content file on its own line, to make future patches smaller.

{ "content": [ "index.ts", "etc." ], "etc." }

bajtos · 2018-01-16T14:47:55Z

packages/repository-json-schema/package.json

+    "clean": "lb-clean loopback-repository-json-schema*.tgz dist package api-docs",
+    "prepare": "npm run build && npm run build:apidocs",
+    "pretest": "npm run build:current",
+    "test": "mocha --opts ../../test/mocha.opts 'DIST/test/unit/**/*.js' 'DIST/test/integration/**/*.js' 'DIST/test/acceptance/**/*.js'",


❗️ DIST directory does not exist, this command will become a no-op on case-sensitive file systems (typically on Linux). (Both MacOS and Windows are case-insensitive AFAIK).

Please use dist instead of DIST.

bajtos · 2018-01-16T14:49:32Z

packages/repository-json-schema/index.ts

@@ -0,0 +1,7 @@
+// Copyright IBM Corp. 2013,2018. All Rights Reserved.


FWIW, the copyright years should be just 2018 in new files.

bajtos · 2018-01-16T14:49:45Z

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

@@ -0,0 +1,71 @@
+// Copyright IBM Corp. 2013,2018. All Rights Reserved.


Ditto, the copyright year is just 2018.

bajtos · 2018-01-16T14:49:49Z

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

@@ -0,0 +1,6 @@
+// Copyright IBM Corp. 2013,2018. All Rights Reserved.


Ditto, the copyright year is just 2018.

bajtos · 2018-01-16T14:49:51Z

packages/repository-json-schema/test/integration/build-schema.test.ts

@@ -0,0 +1,232 @@
+// Copyright IBM Corp. 2013,2018. All Rights Reserved.


Ditto, the copyright year is just 2018.

bajtos · 2018-01-16T15:05:52Z

@shimks I don't think it was a good idea to drop support for Node.js 6.x, as I explained in one of my comments. As you can see for yourself, CI builds of your pull request are failing on Node.js 6.x platform now. IMO, removing support for Node.js 6.x from existing packages is out of scope of this pull request, such changes should be made it a new dedicated pull request.

bajtos · 2018-01-16T15:06:57Z

coverage/coveralls — Coverage decreased (-0.02%) to 96.428%

@shimks please check the coverage report, find the new code that's not covered by our tests and verify that we are not missing important test cases.

bajtos · 2018-01-18T14:03:04Z

The last commit allows for json-schemas to be generated as the decorators are executed and then stored into global metadata space.

I don't understand why is it needed to generate json-schemas at the time when the decorators are executed. Could you please explain the reasoning behind this decision?

IMO, it's ok for consumers of json-schema to call a method from your new json-schema package whenever they need to obtain json-schema description of a model class. If we want to cache those descriptions, then those getter methods are free to add a caching layer as their implementation detail.

What am I missing?

shimks · 2018-01-18T15:07:46Z

I thought it'd be cleaner to generate the json-schemas before any of a LoopBack app run so that we don't have to keep a track of when the schemas are initially generated. But, json-schemas aren't that costly to generate every time they're needed, so maybe it's fine to leave it at previous implementation + caching. @kjdelisle Any thoughts?

kjdelisle · 2018-01-18T17:44:39Z

I think the reason to generate the schema at decoration time is to ensure that a variety of metadata consumers can rely on the existence of those definitions during application configuration and startup.

It's beginning to seem like the concept of a model should be its own package, perhaps under @loopback/model?

shimks · 2018-01-18T17:50:56Z

^^^ This was one thing that I thought of while thinking about ways to resolve the dependency cycle. Just looking at things from a top-level, I think it makes sense to decouple these model decorators from repository concepts so that stuff from json-schema can use the decorators without having to worry about dependency cycles

bajtos · 2018-01-19T09:46:32Z

I think the reason to generate the schema at decoration time is to ensure that a variety of metadata consumers can rely on the existence of those definitions during application configuration and startup.

Playing devil's advocate here: what if ConsumerA (a 3rd-party extension) wants to get JSON Schema version draft-07 , but ConsumerB (another 3rd-party extension) wants to get JSON Schema version 1.0 (a hypothetical future), and these two versions have important incompatible differences?

Wouldn't it be better if each metadata consumer was explicit about what kind of schema they are expecting?

Under the hood, the functions building metadata can cache (memoize) the values if needed, but that should be an implementation detail.

I am also worried that we are starting the path towards adding too much responsibility to our Model classes. Consider Single Responsibility Principle:

A class should have only one reason to change

In your proposal, the model has to change when either of the LDL definition or the JSON Schema version changes.

Just looking at things from a top-level, I think it makes sense to decouple these model decorators from repository concepts so that stuff from json-schema can use the decorators without having to worry about dependency cycles

This is a good point! A similar idea has been already proposed long time ago when we were discussing juggler cleanup/refactoring, looks like it's a very valid one!

I have one concern though:

@model and @property decorators are tightly coupled with LDL (docs1, docs2)
The repository implementation is tightly coupled with LDL in two ways:
1. The underlying juggler must understand the LDL version/flavor used by Repository to define backing PersistedModel classes
2. Repository relies on model definitions created by the decorators to obtain metadata needed by juggler.

If we want to truly decouple models from repositories, then I think we may need to decouple model definition syntax used by @model and @property from the model definition syntax used by Juggler. Having two different and yet similar spec for describing models and properties sounds like too much of effort with too little benefits to me. Maybe what we need is to decouple LDL from both juggler and model decorators, change it from an internal almost implicit spec format into a proper standalone spec format similar to OpenAPI, JSON Schema, etc. This will allow us to start versioning the spec language, which will us to verify that model decorators (LDL producers) are using a LDL version understood by the repository package (an LDL consumer), and potentially implement converters to allow a single repository version to support multiple LDL versions.

@model
Product {}
// sets Product.definition.version to '1.0'

const repo = new DefaultCrudRepository<Product>(Product, db)
// asserts that Product.definition.version is compatible with juggler.ldlVersion
// alternatively upgrade the spec from 1.0 to the version required by juggler
// before passing it to PeristedModel

Even with this design, and after moving model decorators to its own package, I still think it's better to keep JSON-schema generators decoupled from our models. Maybe I am missing an important point here? I admit I am biased by the successful experience we had with loopback-swagger, which provides methods converting LDL to Swagger in a decoupled way.

In order to keep this pull request moving forward, I am proposing to leave the integration with Models out of scope of this pull request. Let's start with implementing a @loopback/respository-json-schema package that can be used on its own, land the pull request, and then discuss tighter integration in a follow-up pull request.

Thoughts?

shimks · 2018-01-19T14:51:39Z

My main concern was the memoization of JSON schemas but it's true that the caching can be done as a part of schema generation. I'll reverse the last commit to keep the PR moving forward like you've said and add in a bit for memoization during JSON schema generations.

Do you think we should create an issue for the discussion on decoupling of model decorators (@model/@property) from LDL definition?

bajtos

I am short of time but don't want to block your progress. I quickly scanned the changes and didn't found any major/blocking issues.

Feel free to dismiss my review after you address my comments below and then get somebody else from the team to review and approve this patch.

bajtos · 2018-01-19T16:13:52Z

packages/openapi-v2/src/controller-spec.ts

+      }
+      case 'allOf': {
+        const collector: SchemaObject[] = [];
+        for (const item of def.allOf) {


emptySchemaObj.allOf = def.allOf.map(item => jsonToSchemaObject(item));

I think emptySchemaObj is not a very good name because emptySchemaObj is no longer empty after this line. Perhaps target or result would be better one?

bajtos · 2018-01-19T16:14:21Z

packages/openapi-v2/src/controller-spec.ts

+      case 'properties': {
+        const properties: {[key: string]: JsonDefinition} = def.properties;
+        const collector: MapObject<SchemaObject> = {};
+        for (const item in properties) {


Ditto, please use Array.prototype.map or lodash's variant for mapping object properties.

bajtos · 2018-01-19T16:15:12Z

packages/openapi-v2/src/controller-spec.ts

+    }
+  }
+
+  return <SchemaObject>emptySchemaObj;


Is this explicit cast necessary? Can we remove it?

return emptySchemaObj;

bajtos · 2018-01-19T16:17:28Z

packages/openapi-v2/test/unit/controller-spec/json-to-schema-object.test.ts

+    property: Object,
+    expected: Object,
+  ) {
+    it(name, () => {


Could you please move it(name part out of this helper function? It makes it easier to find and debug individual test cases, e.g. via it.skip.

it('additionalProperties', testPropertyConversion(additionalDef, expectedAdditional));

Also the test name "additionalProperties" does not read well ("it additionalProperties" is not an English sentence). Please fix.

bajtos · 2018-01-19T16:17:48Z

packages/repository-json-schema/.gitignore

@@ -0,0 +1,3 @@
+*.tgz
+dist*
+package


I believe we are using a shared top-level .gitignore file, please remove this package-specific one.

bajtos · 2018-01-19T16:18:34Z

packages/repository-json-schema/README.md

+  @property() name: string;
+}
+
+const jsonDef = getJsonDef(MyModel);


Since we are producing JSON Schema, I am proposing to rename getJsonDef to getJsonSchema, and jsonDef to jsonSchema or simply schema.

bajtos · 2018-01-19T16:19:30Z

packages/repository-json-schema/package.json

+    "clean": "lb-clean loopback-json-schema*.tgz dist dist6 package api-docs",
+    "prepare": "npm run build && npm run build:apidocs",
+    "pretest": "npm run build:current",
+    "test": "lb-dist mocha --opts ../../test/mocha.opts 'dist/test/unit/**/*.js' 'dist/test/integration/**/*.js' 'dist/test/acceptance/**/*.js'",


mocha.opts was moved to a different location (see #876), please rebase your PR and update this line accordingly.

bajtos · 2018-01-19T16:19:50Z

packages/repository-json-schema/package.json

+    "prepare": "npm run build && npm run build:apidocs",
+    "pretest": "npm run build:current",
+    "test": "lb-dist mocha --opts ../../test/mocha.opts 'dist/test/unit/**/*.js' 'dist/test/integration/**/*.js' 'dist/test/acceptance/**/*.js'",
+    "verify": "npm pack && tar xf loopback-jsons-schema*.tgz && tree package && npm run clean"


Typo: -jsons-

bajtos · 2018-01-19T16:20:25Z

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

+  properties?: {[property: string]: JsonDefinition};
+}
+
+export function getJsonDef(ctor: Function): JsonDefinition {


I am proposing getJsonSchema.

bajtos · 2018-01-19T16:20:33Z

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

+ * reflection API
+ * @param ctor Constructor of class to convert from
+ */
+export function modelToJsonDef(ctor: Function): JsonDefinition {


Similarly here, modelToJsonSchema?

kjdelisle

Wow. 💯
Top-shelf PR, love it.
One teensy, tiny nit and LGTM

kjdelisle · 2018-01-22T20:43:04Z

packages/repository-json-schema/test/integration/build-schema.test.ts

+    it('errors out when explicit type decoration is not primitive', () => {
+      @model()
+      class TestModel {
+        @property({type: 'not primitive'})


Can you remove the space in this type string? Technically, it wouldn't be a valid class/variable reference as a result, so it doesn't quite prove what's being asserted in the it statement.
type: 'NotPrimitive'

I reviewed, as per previous comment.

shimks · 2018-01-22T21:53:48Z

@slnode test please

…N Schema

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

shimks added the needs-priority label Jan 12, 2018

shimks force-pushed the swagger-model branch from 8500368 to 7d4f1b1 Compare January 12, 2018 15:30

bajtos reviewed Jan 12, 2018

View reviewed changes

bajtos suggested changes Jan 12, 2018

View reviewed changes

raymondfeng reviewed Jan 12, 2018

View reviewed changes

packages/json-schema/README.md Outdated

@@ -0,0 +1,89 @@

# @loopback/openapi-spec-builder

Copy link

Contributor

raymondfeng Jan 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix the module name

raymondfeng reviewed Jan 12, 2018

View reviewed changes

kjdelisle suggested changes Jan 12, 2018

View reviewed changes

shimks force-pushed the swagger-model branch from 7d4f1b1 to ff834a9 Compare January 12, 2018 19:37

shimks changed the title ~~WIP: convert TS models to JSON Schema~~ feat(json-schema): add modules to convert TS models to JSON Schema Jan 12, 2018

kjdelisle approved these changes Jan 12, 2018

View reviewed changes

jannyHou reviewed Jan 12, 2018

View reviewed changes

shimks force-pushed the swagger-model branch from ff834a9 to 627ee79 Compare January 12, 2018 20:38

virkt25 previously requested changes Jan 12, 2018

View reviewed changes

bajtos suggested changes Jan 15, 2018

View reviewed changes

bajtos reviewed Jan 15, 2018

View reviewed changes

b-admike approved these changes Jan 15, 2018

View reviewed changes

shimks force-pushed the swagger-model branch from 66dd12b to 65bb24f Compare January 15, 2018 20:02

bajtos suggested changes Jan 16, 2018

View reviewed changes

shimks added this to the Sprint 53 milestone Jan 18, 2018

shimks force-pushed the swagger-model branch 2 times, most recently from b251b9c to 39d276a Compare January 19, 2018 15:33

bajtos previously requested changes Jan 19, 2018

View reviewed changes

shimks force-pushed the swagger-model branch 2 times, most recently from 9bc8666 to 23731d8 Compare January 22, 2018 19:59

kjdelisle approved these changes Jan 22, 2018

View reviewed changes

shimks force-pushed the swagger-model branch 3 times, most recently from 19b356d to 55c5ed8 Compare January 22, 2018 21:26

shimks added 6 commits January 23, 2018 09:51

feat(repository-json-schema): add modules to convert TS models to JSO…

5b5af77

…N Schema

fix(repository-json-schema): fix license headers

1787a41

fix(repository-json-schema): apply feedback round 2

890344e

fix(repository-json-schema): add support for node.js 6

a1dec24

feat(repository-json-schema): add json-schema caching

39aab05

feat(openapi-v2): add full model definition gen

f630a20

shimks force-pushed the swagger-model branch from 55c5ed8 to f630a20 Compare January 23, 2018 14:51

shimks merged commit 81a87f6 into master Jan 23, 2018

shimks removed the needs-priority label Jan 23, 2018

shimks deleted the swagger-model branch January 23, 2018 15:20

bajtos mentioned this pull request Feb 15, 2018

[prototype]Upgrade Swagger 2 to OpenAPI 3 #916

Closed

6 tasks

		@@ -0,0 +1,25 @@
		Copyright (c) IBM Corp. 2018. All Rights Reserved.
		Node module: @loopback/openapi-spec-builder

		@@ -0,0 +1,6 @@
		// Copyright IBM Corp. 2018. All Rights Reserved.
		// Node module: @loopback/openapi-spec-builder

		@@ -0,0 +1,9 @@
		// Copyright IBM Corp. 2018. All Rights Reserved.
		// Node module: @loopback/testlab

		@@ -0,0 +1,7 @@
		// Copyright IBM Corp. 2013,2018. All Rights Reserved.
		// Node module: @loopback/openapi-spec-builder

		@@ -0,0 +1,74 @@
		// Copyright IBM Corp. 2013,2018. All Rights Reserved.
		// Node module: @loopback/openapi-spec-builder

		@@ -0,0 +1,6 @@
		// Copyright IBM Corp. 2013,2018. All Rights Reserved.
		// Node module: @loopback/openapi-spec

		@@ -0,0 +1,215 @@
		// Copyright IBM Corp. 2013,2018. All Rights Reserved.
		// Node module: @loopback/openapi-spec-builder


		## Overview

		This package provides modules to easily convert LoopBack4 models that have been decorated with `@model` and `@property` to a matching JSON Schema Definition.

		@@ -0,0 +1,8 @@
		{
		"content": ["index.ts", "src/index.ts", "src/build-schema.ts"],

		@@ -0,0 +1,71 @@
		// Copyright IBM Corp. 2013,2018. All Rights Reserved.

		@@ -0,0 +1,232 @@
		// Copyright IBM Corp. 2013,2018. All Rights Reserved.

@@ @@ -0,0 +1,3 @@ @@
+              *.tgz
+              dist*
+              package

feat(json-schema): add modules to convert TS models to JSON Schema #860

feat(json-schema): add modules to convert TS models to JSON Schema #860

Uh oh!

Conversation

shimks commented Jan 12, 2018 • edited by kjdelisle Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

bajtos commented Jan 12, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shimks Jan 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shimks commented Jan 12, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kjdelisle left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kjdelisle left a comment

Choose a reason for hiding this comment

Uh oh!

jannyHou left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

virkt25 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

shimks commented Jan 12, 2018 •

edited by kjdelisle

Loading

shimks Jan 12, 2018 •

edited

Loading

jannyHou left a comment •

edited

Loading

shimks Jan 12, 2018 •

edited

Loading

shimks Jan 15, 2018 •

edited

Loading