loopbackio / loopback-next

LoopBack makes it easy to build modern API applications that require complex integrations.
https://loopback.io
Other
4.93k stars 1.06k forks source link

Make `@model` decorator produce JSON Schema #962

Open shimks opened 6 years ago

shimks commented 6 years ago

Thread to discuss on possibly switching the metadata that @model produces to JSON Schema or anything else. Thoughts @raymondfeng @bajtos @strongloop/sq-lb-apex ?

As the name of repository-json-schema suggests, @model and @property are intrinsically linked to @loopback/repository, but we're also using those decorators for creating JSON Schemas when that has nothing to do with the legacy juggler. This could potentially create a dependency cycle (for example, repository-json-schema -> repository -> new juggler (which may potentially leverage) -> repository-json-schema), so the two concepts should be separated. Ideally, JSON Schema being produced by repository-json-schema should be the baseline from which the juggler definition is being created from, on top of any additional metadata given.

I'd propose that we create in a separate package 'private' decorators intended to be used in @model and @property that would solely be used to harvest JSON Schema related metadata, which then can be used 'enhance' our generated juggler definition. This way, future extensions would only need to leverage the key to which JSON Schema metadata is stored under with none of the juggler-exclusive concepts.

Acceptance criteria will be further fleshed out based on the feedback of the current proposal.

Acceptance Criteria

shimks commented 6 years ago

Currently, JSON Schemas are produced in this order: data put into model decorator -> juggler definition -> JSON Schema

The proposal is to switch the order around: data put into model decorator -> JSON Schema -> juggler defintion

The reasoning behind this switch is to allow information passed into the decorator to be available in a standardized and global format.

jannyHou commented 6 years ago

@shimks juggler definition contains all model settings, which includes schema info and other things like relation definitions. So my understanding is conversion "juggler definition -> JSON Schema" doesn't cause losing info, but "JSON Schema -> juggler defintion" does... "JSON Schema" is used for describing "schema" specifically, extending it is feasible while that would against its original purpose I think.

shimks commented 6 years ago

@jannyHou If the concern is about losing juggler information, specific juggler information can be stored at a different key in the metadata space and could be retrieved back when JSON Schemas are converted into a juggler definition.

virkt25 commented 6 years ago

What brought about the requirement for this switch?

shimks commented 6 years ago

From what I remember, the problem was that the juggler def isn't a strict subset of JSON Schema. An example of this was juggler's inability to support union types; separate metadata had to be built in order to create something of union types, and IMO this is tech debt waiting to happen.

But then again if the team doesn't feel this is something we should tackle right now, I'd be ok with not giving this task a high priority in our plan.

b-admike commented 6 years ago

@shimks can you groom the ticket description to have an acceptance criteria as well? @strongloop/lb-next-dev thoughts?

shimks commented 6 years ago

@strongloop/lb-next-dev Original post has been updated with a proposal.

bajtos commented 6 years ago

Since this issue is labeled as non-DP3, I am proposing to put it on back burner until DP3 is done.

@raymondfeng: IIRC, when we proposed to switch from LDL (the juggler definition format) to JSON Schema, you were advocating against using JSON Schema for model definition and keep using our own syntax (maybe a successor to LDL). Could you please remind us of your arguments?

raymondfeng commented 6 years ago
  1. I agree that we should start to refactor some of the code out of @loopback/repository into separate modules. Here are a few candidates:

    • model/property into @loopback/model
    • types into @loopback/types
    • TS declaration of legacy juggler types into loopback-datasource-juggler
    • Datasource (as @loopback/service-proxy also needs to use it)
  2. JSON schema cannot fulfill the duty of LoopBack 4 data modeling and mapping metadata. We have the following perspectives:

    • schema definition (models/types/properties/id/relations) - close to json schema but it does not have the concept of id and relations
    • persistence mapping (table/column/index/...)
    • json mapping (for example, custom property name for json)
    • constraints for validation
stale[bot] commented 3 years ago

This issue has been marked stale because it has not seen activity within six months. If you believe this to be in error, please contact one of the code owners, listed in the CODEOWNERS file at the top-level of this repository. This issue will be closed within 30 days of being stale.

stale[bot] commented 3 years ago

This issue has been marked stale because it has not seen activity within six months. If you believe this to be in error, please contact one of the code owners, listed in the CODEOWNERS file at the top-level of this repository. This issue will be closed within 30 days of being stale.