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

Implement support for anonymous models described using anyOf or oneOf… #399

Closed
wants to merge 4 commits into from

Conversation

thetrime
Copy link
Contributor

…. allOf seems OK. Remove TODO. Add tests. Credit Yusei Abe in LICENSE for the core idea of the code, found at https://github.com/diverta/kuroco-sdk/blob/8029698bc6d43b94a9190e7d1f34dad92234b3ed/src/generator/openApi/v3/parser/getModel.ts#L120

This is a bit simple than the approach taken by @yabe-diverta in that it does not handle nested anyOf and allOf (though as far as I can tell, yabe-diverta's code only handles one more level of nesting than mine, rather than arbitrary nesting). The tests all still pass, and I've added two new ones for the anyOf and oneOf cases described.

…. allOf seems OK. Remove TODO. Add tests. Credit Yusei Abe in LICENSE for the base of the code.
@codecov
Copy link

codecov bot commented Oct 28, 2020

Codecov Report

Merging #399 into master will increase coverage by 0.08%.
The diff coverage is 95.23%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #399      +/-   ##
==========================================
+ Coverage   93.39%   93.48%   +0.08%     
==========================================
  Files         108      108              
  Lines        1272     1304      +32     
  Branches      229      245      +16     
==========================================
+ Hits         1188     1219      +31     
- Misses         84       85       +1     
Impacted Files Coverage Δ
src/openApi/v3/parser/getModel.ts 98.05% <95.23%> (-0.31%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 113dd23...fdf1b2b. Read the comment docs.

@lgtm-com
Copy link

lgtm-com bot commented Oct 29, 2020

This pull request introduces 1 alert when merging 3ede3ad into 203b74e - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class

@yabe-diverta
Copy link

thanks for the mention @thetrime .

I think it's not necessary that the modified copyright that includes me.
Because my codes are just patches, they are imperfect.
it may cause some malfunctions in some cases as well.

as a background story,
at the time of launching a new PJ in my team,
they planned to expose it's backend definition as OpenAPI V3,
and i was assigned to make a SDK to handle it easily on front-end (JS/TS) side.

at the first time of first-developing term,
i thought to just use this good tool as a library as one of the dependencies of my SDK.
I just wanted a simple tool that can perse OpenAPI V3 definition to a TS code.
althogh i could not fix the mulfunctions of OneOf... one.
so that i needed to hack and reproduce this repo to override some codes as patches.
i would switch to just add this tool as a dependency of my SDK instead of hacking if the probrem ware fixed.

the backend of my team generates OpenAPI definitions in a very few pattern,
i did a few tests to just fulfill those particular patterns.
i don't guarantee that this patch can be appropriate for general patterns.
I guess it's required to redesign and refine from it's root level to solve this OneOf... one.

Anyway, i don't recommend to merge my patch (this pull request seems to be more sophisticated then mine?).
i would contribute to solve this (if i have much time...).
thanks @thetrime @ferdikoomen (@artem-tim)

@budde377
Copy link
Contributor

budde377 commented Nov 2, 2020

I suspect that the implemented logic could be extended to the allOf case (with intersection instead of union). Which would solve cases like the following:

...
"ModelWithNullableReferenceProperty": {
    "description": "This is a model which property is a reference and nullable",
    "type": "object",
    "properties": {
        "nullableProp": {
            "allOf": [
                {
                    "$ref": "#/components/schemas/ModelWithString"
                },
                {
                     "$ref": "#/components/schemas/ModelThatExtends"
                }
            ],
            "nullable": true
        }
    }
},
...

which currently is rendered as

export interface ModelWithNullableReferenceProperty {
    nullableProp?: any;
}

but with the same logic as above this, could be rendered as

export interface ModelWithNullableReferenceProperty {
    nullableProp?: ModelWithString & ModelThatExtends | null;
}

@thetrime
Copy link
Contributor Author

thetrime commented Nov 5, 2020

thanks for the mention @thetrime .

I think it's not necessary that the modified copyright that includes me.
Because my codes are just patches, they are imperfect.

All code is imperfect :) If you don't want to be mentioned, I can certainly remove it.

it may cause some malfunctions in some cases as well.

Yes, I'm aware this is a bit rough, but it's better than what was available before. If someone wants to refine it even further, then that's fine by me.

as a background story,
at the time of launching a new PJ in my team,
they planned to expose it's backend definition as OpenAPI V3,
and i was assigned to make a SDK to handle it easily on front-end (JS/TS) side.

at the first time of first-developing term,
i thought to just use this good tool as a library as one of the dependencies of my SDK.
I just wanted a simple tool that can perse OpenAPI V3 definition to a TS code.
althogh i could not fix the mulfunctions of OneOf... one.
so that i needed to hack and reproduce this repo to override some codes as patches.
i would switch to just add this tool as a dependency of my SDK instead of hacking if the probrem ware fixed.

Yes - I'm in exactly the same situation.

the backend of my team generates OpenAPI definitions in a very few pattern,
i did a few tests to just fulfill those particular patterns.
i don't guarantee that this patch can be appropriate for general patterns.
I guess it's required to redesign and refine from it's root level to solve this OneOf... one.

It works for my patterns too. Note that my code is even more limited than your patch - it won't handle nested oneOf properly at all

@lgtm-com
Copy link

lgtm-com bot commented Nov 5, 2020

This pull request introduces 1 alert when merging fdf1b2b into fa6fbbc - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class

@budde377
Copy link
Contributor

The PR #419 solves this, but with objects of arbitrary depth and is not limited to interfaces.

@ferdikoomen
Copy link
Owner

@budde377 @thetrime This PR works, but the code is hard to read. However it works, so im tempted to merge it for now and look at this story a bit better in the future.

@ferdikoomen
Copy link
Owner

Closing this in favour of #419

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants