PoC: TypeScript types and OpenAPI schema to describe navigational properties (inclusion of related models) #2592

bajtos · 2019-03-15T09:39:49Z

This pull request presents a proof-of-concept implementation demonstrating how to describe navigational properties for inclusion of related models.

See the discussion in #2152 for background information and also examples of different approaches I have researched and abandoned before settling down on what's proposed here.

How to review this pull request:

Start by reading _SPIKE_.md, it contains high-level description of the proposed implementation with code snippets serving as illustrations.
Then review the code. The changes are split into multiple commits, you may find it easier to review the patch in smaller chunks.

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

_SPIKE_.md

examples/todo-list/src/models/todo-list.model.ts

examples/todo-list/src/repositories/todo-list.repository.ts

bajtos · 2019-03-18T11:43:35Z

@raymondfeng thank you for the feedback. I have addressed your comments, is there anything else to discuss? LGTY now?

b-admike · 2019-03-18T14:18:37Z

@bajtos I've taken a peek and the proposal looks great. I will get to review this PR in detail tomorrow.

jannyHou

@bajtos Defining the linked items in a new interface is a reasonable solution for generating the OpenAPI schema to describe the retrieved data with navigational properties, I am good with the design 👍

I haven't reviewed the schema generation related code change, but since the design looks good they should be good too. Will do a more detailed review tomorrow.

A question: who would be the person that add the navigational proper interface + type? User does that or we generate them by cli when a relation is defined?

And a few design thought:

I left a comment about leveraging the inclusion handler in the controller function find.
have you tried rewrite the type of scope in type Inclusion? See my comment regarding the inclusion filter. And feel free to create a new issue to address it.

_SPIKE_.md

jannyHou · 2019-03-21T01:29:51Z

examples/todo-list/src/repositories/todo-list.repository.ts

+
+    // poor-mans inclusion resolver, this should be handled by DefaultCrudRepo
+    // and use `inq` operator to fetch related todos in fewer DB queries
+    if (include && include.length && include[0].relation === 'todos') {


I remember we introduced a inclusion handler in the previous PoC(sorry still looking for the PR), shall we delegate these to the inclusion handler like this PoC ?

I would like us to work incrementally.

In this spike, I focused on how to represent and describe navigational properties. I created this temporary resolver to allow me to test my proposal end-to-end.

I am expecting that we will replace this code with a real resolver once it's implemented. This is out of scope of my spike though.

AFAICT, there is no follow-up story to implement the resolver proposed in #2124, do we need to create one? I can do that as part of this spike if you like.

In this spike, I focused on how to represent and describe navigational properties. I created this temporary resolver to allow me to test my proposal end-to-end.

Fair enough 👍

AFAICT, there is no follow-up story to implement the resolver proposed in #2124

That would be great thanks! No hurry we can create the implementation story after closing this spike.

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

examples/todo-list/src/controllers/todo-list.controller.ts

b-admike · 2019-03-21T06:47:43Z

_SPIKE_.md

+
+To support integration with OpenAPI spec of controllers, where we want to
+reference a shared definition (component schema), we need a slightly different
+schema. Here is an example as produced by `getJsonSchemaRef`:


Can you highlight the difference in the produced schema between the examples below and above?

_SPIKE_.md

b-admike · 2019-03-21T06:54:50Z

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

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

+export interface JsonSchemaOptions {
+  includeRelations?: boolean;
+  visited?: {[key: string]: JSONSchema};


What's visited used for?

In order to handle circular dependencies and don't overflow the stack, we need to know which models have been already visited.

1. get schema for ProductWithLinks 2. get schema for CategoryWithLinks (triggered by `ProductWithLinks#category`) 3. get schema for ProductWithLinks (triggered by `CategoryWithLinks#products`) 4. get schema for CategoryWithLinks (triggered by `ProductWithLinks#category`) 5. etc. until the stack overflows

With options.visited, the step 3 sees that ProductWithLinks has been already visited and emits the reference only.

b-admike

LGTM FWIW 💯 ; I think the proposal is consistent and covers all aspects of the implementation as shown in the PoC (i.e. model definition, json schema generation, controller spec generation). The follow-up stories look good to me as well.

bajtos · 2019-03-21T15:25:18Z

@jannyHou @b-admike thank you for thoughtful comments, I have addressed them in 67d44d5. PTAL again.

A question: who would be the person that add the navigational proper interface + type? User does that or we generate them by cli when a relation is defined?

It will be the user at the beginning, eventually they should be defined by lb4 relation command.

I left a comment about leveraging the inclusion handler in the controller function find.
have you tried rewrite the type of scope in type Inclusion? See my comment regarding the inclusion filter. And feel free to create a new issue to address it.

As far as I understood the scope of the spike #2152, we wanted to figure out how to represent and describe the relational property. I believe we already know how to implement the relation resolver, based on the work done in #2124?

Anyhow, I added a new item to my list of follow-up stories, to implement a proper relation resolver too.

bajtos · 2019-03-22T10:53:06Z

I have converted Inclusion of related models #1352 into an Epic and added the following new stories:

Fix repository-json-schema to handle circular references Fix repository-json-schema to handle circular references #2628
Allow controllers to provide definitions of models referenced in operation spec Allow controllers to provide definitions of models referenced in operation spec #2629
Enhance getJsonSchema to describe navigational properties Enhance getJsonSchema to describe navigational properties #2630
Implement getJsonSchemaRef and getModelSchemaRef helpers Implement getJsonSchemaRef and getModelSchemaRef helpers #2631
Modify Repository find* methods to include navigational properties Modify Repository find* methods to include navigational properties #2632
Add navigational properties to examples/todo-list Add navigational properties to examples/todo-list #2633
Spike: Resolver for inclusion of related models Spike: Resolver for inclusion of related models #2634

bajtos added spike Relations Model relations (has many, etc.) labels Mar 15, 2019

bajtos added this to the March 2019 milestone milestone Mar 15, 2019

bajtos self-assigned this Mar 15, 2019

bajtos requested review from raymondfeng, jannyHou, b-admike and a team March 15, 2019 09:39

bajtos mentioned this pull request Mar 15, 2019

[Spike] Explore the way to resolve inclusion property #2152

Closed

4 tasks

raymondfeng reviewed Mar 15, 2019

View reviewed changes

_SPIKE_.md Outdated Show resolved Hide resolved

raymondfeng reviewed Mar 15, 2019

View reviewed changes

examples/todo-list/src/models/todo-list.model.ts Outdated Show resolved Hide resolved

raymondfeng reviewed Mar 15, 2019

View reviewed changes

examples/todo-list/src/repositories/todo-list.repository.ts Outdated Show resolved Hide resolved

bajtos force-pushed the spike/relation-inclusion-properties-as-iface branch from 4065870 to ec2a6d8 Compare March 18, 2019 11:42

jannyHou reviewed Mar 21, 2019

View reviewed changes

b-admike reviewed Mar 21, 2019

View reviewed changes

b-admike approved these changes Mar 21, 2019

View reviewed changes

bajtos added 7 commits March 21, 2019 16:00

feat: typescript API and example impl for inclusion of related models

43baf2f

feat: generate schema for relation links

2256435

feat: add schemas with relations to controllers

d93dff7

docs: spike notes

e3e6271

fixup! clarify task list

07bfd24

fixup! introduce {Model}WithLink types

bde5635

chore: address review feedback

67d44d5

bajtos force-pushed the spike/relation-inclusion-properties-as-iface branch from ec2a6d8 to 67d44d5 Compare March 21, 2019 15:21

fix: rename Links to Relations

8d49bfa

bajtos mentioned this pull request Mar 22, 2019

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

Closed

3 tasks

This was referenced Mar 22, 2019

Enhance getJsonSchema to describe navigational properties #2630

Closed

Implement getJsonSchemaRef and getModelSchemaRef helpers #2631

Closed

feat: update SPIKE notes with links to issues, add more tests

244f270

This was referenced Mar 22, 2019

Modify Repository find* methods to include navigational properties #2632

Closed

Add navigational properties to examples/todo-list #2633

Closed

Spike: Resolver for inclusion of related models #2634

Closed

feat: add more new stories

b21fb3b

bajtos closed this Mar 22, 2019

This was referenced Mar 26, 2019

Spike: Constrain the OpenAPI schema emitted for a model [DO NOT MERGE] #2646

Closed

Allow controllers to provide definitions of models referenced in operation spec #2629

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PoC: TypeScript types and OpenAPI schema to describe navigational properties (inclusion of related models) #2592

PoC: TypeScript types and OpenAPI schema to describe navigational properties (inclusion of related models) #2592

bajtos commented Mar 15, 2019

bajtos commented Mar 18, 2019

b-admike commented Mar 18, 2019

jannyHou left a comment

jannyHou Mar 21, 2019

bajtos Mar 21, 2019

jannyHou Mar 22, 2019

b-admike Mar 21, 2019

b-admike Mar 21, 2019

bajtos Mar 21, 2019

b-admike left a comment

bajtos commented Mar 21, 2019

bajtos commented Mar 22, 2019 •

edited

Loading

PoC: TypeScript types and OpenAPI schema to describe navigational properties (inclusion of related models) #2592

PoC: TypeScript types and OpenAPI schema to describe navigational properties (inclusion of related models) #2592

Conversation

bajtos commented Mar 15, 2019

Checklist

bajtos commented Mar 18, 2019

b-admike commented Mar 18, 2019

jannyHou left a comment

Choose a reason for hiding this comment

jannyHou Mar 21, 2019

Choose a reason for hiding this comment

bajtos Mar 21, 2019

Choose a reason for hiding this comment

jannyHou Mar 22, 2019

Choose a reason for hiding this comment

b-admike Mar 21, 2019

Choose a reason for hiding this comment

b-admike Mar 21, 2019

Choose a reason for hiding this comment

bajtos Mar 21, 2019

Choose a reason for hiding this comment

b-admike left a comment

Choose a reason for hiding this comment

bajtos commented Mar 21, 2019

bajtos commented Mar 22, 2019 • edited Loading

bajtos commented Mar 22, 2019 •

edited

Loading