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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for anyOf and oneOf on the jsonToSchemaObject utility #3524

Closed
freakpol opened this issue Aug 8, 2019 · 8 comments · Fixed by #4396
Closed

Add support for anyOf and oneOf on the jsonToSchemaObject utility #3524

freakpol opened this issue Aug 8, 2019 · 8 comments · Fixed by #4396

Comments

@freakpol
Copy link

freakpol commented Aug 8, 2019

Suggestion

Add support for anyOf and oneOf on the jsonToSchemaObject utility

Use Cases

When defining models, sometimes we want to specify that some properties are of one of / any of some types. For this, the property jsonSchema of the property annotation comes handy, but it lack of support for the anyOf and oneOf.

Examples

export class SomeModel extends Model {
  @property({
    type: 'string',
    required: true,
    id: true,
  })
  code: string;

  @property({
    type: 'object',
    required: false,
    jsonSchema: {
      additionalProperties: {
        anyOf: [
          {type: 'string'},
          {type: 'number'},
          {type: 'array', items: { type: 'string'}},
          {type: 'object'},
        ]
      }
    }
  })
  extraProperties?: AnyObject;

}

When loopback generates the openapi specs, that anyOf is totally ignored.

Now, as a potential fix, I've tried adding

    case 'anyOf': {
        result.anyOf = _.map(json.anyOf, item =>
          jsonToSchemaObject(item as JsonSchema),
        );
        break;
      }

As a new case, just below the allOf case (and commenting the corresponding propsToIgnore entry) and all seems to work fine. Validations works when using the openapi explorer and also, the schemas object is generated properly.

Acceptance criteria

TBD - will be filled by the team.

@jannyHou jannyHou self-assigned this Aug 9, 2019
@jannyHou
Copy link
Contributor

jannyHou commented Aug 9, 2019

@freakpol Thank you for the suggestion, good point.
We support converting these spec into the composite TypeScript types, see https://github.com/strongloop/loopback-next/blob/650b387196b59d579e9b0692592af3b466ec0000/packages/cli/generators/openapi/README.md#schemas

But don't support the reverse conversion as you mentioned.

Ideally, the composite types could be inferred from the metadata of parameter's type:

 @model()
    class Test extends Entity {
      @property()
      // The inferred type is `Objecct`, instead of a union of `string | number | boolean`
      composedProp: string | number | boolean;
  
      constructor(data?: Partial<Test>) {
        super(data);
      }
    }

But the reality is the information of composite types are lost in the metadata, which means we cannot generate corresponding schemas for both primitive types and custom types(other models).

Therefore it's on users to

  • provide the entire schema(s) in the anyof/allof/oneof spec, or its $ref value.
  • make sure the spec is identical with the property's type
  • if the value of schema is a reference, make sure the entry of the schema spec is added to components/schemas(or other referred place)

In terms of supporting the feature anyof/allof/oneof , your suggestion looks reasonable to me :)
But I think we need to support custom coercion as well, see my next comment in #3524 (comment)

@jannyHou jannyHou removed their assignment Aug 9, 2019
@jannyHou
Copy link
Contributor

jannyHou commented Aug 9, 2019

And another important thing to consider: for the composite types, how to coerce the data from HTTP request? See the coercion file https://github.com/strongloop/loopback-next/blob/master/packages/rest/src/coercion/coerce-parameter.ts

It's easy for a certain type, but for an union/intersection type, user may need to provide a custom coercion function for it.

@freakpol
Copy link
Author

I'm not really sure if I exposed my point well.

The sample code I've posted in the description of the issue, is actual code.
Now, when loopback generates the openapi spec, that generated documentation does not have the anyOf (in this example) or oneOf sections, because those ones are ignored by jsonToSchemaObject utility.

I was not talking about converting openapi to typescript.

@jannyHou
Copy link
Contributor

@freakpol I think I understand your description correctly, only the first paragraph of my comment is talking about "converting openapi to typescript", the rest are all about the feature you'd like to have.

@freakpol
Copy link
Author

Oh, ok, sorry, I'm not really that expert either on typescript and even less in loopback.
Anyway, I've tested that proposed solution (at least for anyOf) and seems to work fine, it generates the proper openapi docs and also, when using the explorer, the input is validated properly. Note, tho, that I haven't tested with very complex properties.

@bajtos
Copy link
Member

bajtos commented Sep 2, 2019

Anyway, I've tested that proposed solution (at least for anyOf) and seems to work fine, it generates the proper openapi docs and also, when using the explorer, the input is validated properly. Note, tho, that I haven't tested with very complex properties.

@freakpol cool! Would you like to submit your improvement as a pull request? The most important part is to add automated tests to verify your implementation and prevent regressions in the future.

See Submitting a pull request to LoopBack 4 to get started, then we can help you along the way.

@AbhisheKundalia
Copy link

@freakpol can you please share your code for reference if PR is not yet submitted. Thanks in Advance.

@gczobel-f5
Copy link
Contributor

@bajtos @jannyHou Hi, I created a PR to add the missing keyboards. Please, review.

gczobel-f5 added a commit to gczobel-f5/loopback-next that referenced this issue Jan 14, 2020
gczobel-f5 added a commit to gczobel-f5/loopback-next that referenced this issue Jan 14, 2020
raymondfeng pushed a commit that referenced this issue Jan 14, 2020
dougal83 pushed a commit to dougal83/loopback-next that referenced this issue Jan 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants