j-d-carmichael / boats

Beautiful Open Api Template System
MIT License
57 stars 8 forks source link

Discriminator issue #44

Closed TomFreeman closed 3 years ago

TomFreeman commented 3 years ago

Example PR showing https://github.com/johndcarmichael/boats/issues/43 in action.

Very happy to make the fix, but not sure where to start at this point.

j-d-carmichael commented 3 years ago

OK so we're looking to reference a model in both in the "discriminator.mapping" just as is done in the oneOf example.

But as the "discriminator.mapping" doesn't use $ref it is currently making it not possible.. or you have to manually write the reference yourself.

Would that be the correct assessment?

TomFreeman commented 3 years ago

Yes, that sounds right.

If the discriminator.mapping properties could somehow be treated as if they were $ref filenames, then it feels like it would solve the issue. But not sure which part of the puzzle is doing that.

j-d-carmichael commented 3 years ago

So as $ref doesn't work then we just need a replacement.

Did you see if the mixin works?

mapone: {{ mixin('../../some/model.yml') }}

The only issue is if json-schema-ref-parser will automatically recognise the two models are the same or not.

j-d-carmichael commented 3 years ago

If that doesn't work then we could look at introducing a few template function, to replace a relative foreplay with its hash reference.

TomFreeman commented 3 years ago

@johndcarmichael I wasn't sure about the mixin approach. As a discriminator object has to be a Map[string, string] - so I didn't think mixins would be the right approach.

I have tried creating a specific template function.

I'm not 100% happy with it, it's very use-case specific, but making it generic defeated me, and I'm not sure there's any other use-case anyway. Thoughts?

j-d-carmichael commented 3 years ago

@TomFreeman this looks good an happy to merge in, big thanks for the contribution!

Before we merge it in please could you change a few bits:

  1. Your ide or maybe it was prettifier has made changes to the style of the syntax, could you revert them please. We didn't document this anywhere but we opted for the javascript standard styling but with always using trailing semi-colons.
  2. src/nunjucksHelpers/schemaRefGenerator.ts is hardcoded right now to use const schemaFolder = 'components/schemas'; but this would be cleaner if we could inject this as BOATS is also used right now on Openapi2 and 3 projects pus asyncapi.
  3. The getMethodFromFileName and buildIndexFromPath function, could you either nest them into the class, or.. pull them out into 2 files in the utils.

Cheers, John

TomFreeman commented 3 years ago

@TomFreeman this looks good an happy to merge in, big thanks for the contribution!

Before we merge it in please could you change a few bits:

  1. Your ide or maybe it was prettifier has made changes to the style of the syntax, could you revert them please. We didn't document this anywhere but we opted for the javascript standard styling but with always using trailing semi-colons.
  2. src/nunjucksHelpers/schemaRefGenerator.ts is hardcoded right now to use const schemaFolder = 'components/schemas'; but this would be cleaner if we could inject this as BOATS is also used right now on Openapi2 and 3 projects pus asyncapi.
  3. The getMethodFromFileName and buildIndexFromPath function, could you either nest them into the class, or.. pull them out into 2 files in the utils.

Cheers, John

Think I've addressed these now. Although I created an OpenAPI 2.0 version of the helper, I couldn't work out a good test case, as discriminator works differently in OpenAPI 2, similarly for AsyncAPI, although AsyncAPI does use components/schemas

j-d-carmichael commented 3 years ago

@TomFreeman

Awesome! Sorry but 1 more thing :)

Now i see it, we could simplify it a little further/

import schemaRefGenerator from '../schemaRefGenerator';

export default function (fileName: string, componentsPath = 'components/schemas'): string {
  return schemaRefGenerator(
    this.env.globals.currentFilePointer,
    fileName,
    {
      componentsPath
    }
  );
}

You are write with you point that the majority use case will be OA3, but this now allows the majority to be happy and type less :D but also the older projects (like some of mine on OA2 :)) to be able to inject a ref achnor point from the template/yml file.

eg:

type: object
discriminator:
    mapping:
        typeOne: {{ schemaRef("polymorphicmodel_one.yml.njk") }}
        typeTwo: {{ schemaRef("polymorphicmodel_two.yml.njk", "definitions") }}
    propertyName: selector
oneOf:
- $ref: polymorphicmodel_one.yml.njk
- $ref: polymorphicmodel_two.yml.njk
TomFreeman commented 3 years ago

I've taken that onboard, I didn't update the discriminator example, because the files are still under /components/schema, and so the reference comes out very mangled if you're not actually under that path and try and change it.

j-d-carmichael commented 3 years ago

Look great, thanks for the contribution to BOATS!

I will merge this in a release it to npm.

j-d-carmichael commented 3 years ago

Release: 2.9.0