Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 10 commits into from

Conversation

Projects
None yet
4 participants
@bajtos
Copy link
Member

bajtos commented Mar 15, 2019

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

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

@bajtos bajtos self-assigned this Mar 15, 2019

@bajtos bajtos requested review from raymondfeng, jannyHou, b-admike and strongloop/loopback-next Mar 15, 2019

@bajtos bajtos referenced this pull request Mar 15, 2019

Closed

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

0 of 4 tasks complete
Show resolved Hide resolved _SPIKE_.md Outdated

@bajtos bajtos force-pushed the spike/relation-inclusion-properties-as-iface branch from 4065870 to ec2a6d8 Mar 18, 2019

@bajtos

This comment has been minimized.

Copy link
Member Author

bajtos commented Mar 18, 2019

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

@b-admike

This comment has been minimized.

Copy link
Member

b-admike commented Mar 18, 2019

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

@jannyHou
Copy link
Contributor

jannyHou left a comment

@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.
Show resolved Hide resolved _SPIKE_.md Outdated
Show resolved Hide resolved _SPIKE_.md Outdated
Show resolved Hide resolved _SPIKE_.md
Show resolved Hide resolved _SPIKE_.md Outdated
Show resolved Hide resolved _SPIKE_.md
Show resolved Hide resolved _SPIKE_.md Outdated

// 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') {

This comment has been minimized.

Copy link
@jannyHou

jannyHou Mar 21, 2019

Contributor

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 ?

This comment has been minimized.

Copy link
@bajtos

bajtos Mar 21, 2019

Author Member

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.

This comment has been minimized.

Copy link
@jannyHou

jannyHou Mar 22, 2019

Contributor

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.

Show resolved Hide resolved packages/repository-json-schema/src/build-schema.ts
Show resolved Hide resolved examples/todo-list/src/controllers/todo-list.controller.ts

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`:

This comment has been minimized.

Copy link
@b-admike

b-admike Mar 21, 2019

Member

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

Show resolved Hide resolved _SPIKE_.md Outdated
Show resolved Hide resolved _SPIKE_.md Outdated
Show resolved Hide resolved _SPIKE_.md Outdated
@@ -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};

This comment has been minimized.

Copy link
@b-admike

b-admike Mar 21, 2019

Member

What's visited used for?

This comment has been minimized.

Copy link
@bajtos

bajtos Mar 21, 2019

Author Member

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
Copy link
Member

b-admike left a comment

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 added some commits Mar 14, 2019

@bajtos bajtos force-pushed the spike/relation-inclusion-properties-as-iface branch from ec2a6d8 to 67d44d5 Mar 21, 2019

@bajtos

This comment has been minimized.

Copy link
Member Author

bajtos commented Mar 21, 2019

@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 bajtos referenced this pull request Mar 22, 2019

Open

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

0 of 3 tasks complete
@bajtos

This comment has been minimized.

Copy link
Member Author

bajtos commented Mar 22, 2019

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 #2628
  • Allow controllers to provide definitions of models referenced in operation spec #2629
  • Enhance getJsonSchema to describe navigational properties #2630
  • Implement getJsonSchemaRef and getModelSchemaRef helpers #2631
  • Modify Repository find* methods to include navigational properties #2632
  • Add navigational properties to examples/todo-list #2633
  • Spike: Resolver for inclusion of related models #2634
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can鈥檛 perform that action at this time.