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

openapi2: fix un/marshalling discriminator field #1011

Merged
merged 7 commits into from
Oct 1, 2024

Conversation

reversearrow
Copy link
Contributor

This PR addresses issue #1010. It allows discriminator field to be of any type and case it based on the OpenAPI version.

@reversearrow reversearrow marked this pull request as draft September 17, 2024 15:58
@reversearrow reversearrow marked this pull request as ready for review September 17, 2024 16:07
@reversearrow reversearrow marked this pull request as draft September 17, 2024 16:07
@reversearrow reversearrow marked this pull request as ready for review September 17, 2024 16:37
@reversearrow
Copy link
Contributor Author

@fenollp can please review this PR? Thanks

@fenollp
Copy link
Collaborator

fenollp commented Sep 19, 2024

Hi! Thanks for tackling this.

The ideal solution to me is to define a different Discriminator type in v2 and leave the current definition as is in v3. I. E. to have two different types and avoid casting.
Do you think you'd be able to update your PR with this in mind?
Thanks

@fenollp fenollp linked an issue Sep 19, 2024 that may be closed by this pull request
@reversearrow reversearrow force-pushed the fix/issue_1010 branch 2 times, most recently from 34c2229 to b0f93cc Compare September 23, 2024 00:10
@reversearrow
Copy link
Contributor Author

reversearrow commented Sep 23, 2024

Hi! Thanks for tackling this.

The ideal solution to me is to define a different Discriminator type in v2 and leave the current definition as is in v3. I. E. to have two different types and avoid casting. Do you think you'd be able to update your PR with this in mind? Thanks

Hi @fenollp, thank you for your feedback. I agree with your suggestion. After looking into the proposed approach, it seemed like it would involve significant effort to refactor. I’ve updated the PR with a simpler approach than before to address the bug. Please review and let me know what you think. I can look into implementing the suggested approach without any strict time constraints in a different PR if its still required after this PR.

@reversearrow reversearrow marked this pull request as draft October 1, 2024 17:37
@reversearrow reversearrow marked this pull request as ready for review October 1, 2024 18:56
@reversearrow
Copy link
Contributor Author

@fenollp I have create a new Schema Object for the OpenAPI V2. Please review and let me know.

@fenollp fenollp changed the title fix: issue unmarshalling when discriminator field is set in openapi2.0 openapi2: fix un/marshalling discriminator field Oct 1, 2024
@fenollp fenollp merged commit 1eeb41c into getkin:master Oct 1, 2024
5 checks passed
@reversearrow reversearrow deleted the fix/issue_1010 branch October 1, 2024 21:25
fenollp added a commit that referenced this pull request Oct 21, 2024
* add origin - step 1

* delete origin from content

* point to Origin

* make include origin configurable

* generic unmarshalStringMap

* add origin to more components

* revert comments

* use const originKey

* comment to Scopes

* test more specs

* Fix Discriminator

* update docs

* test on a decent set of dedicated specs

* remove trainling spaces

* add comments

* remove trailing whitespace

* update docs

* dedicated tests

* update docs

* remove whitespace

* rm empty line

* rm last newline

* add tests

* rm unused test files

* update deps

* fix paths test

* test components/security

* fix LF

* include origin in unmarshal maps

* move component unmarshallers to respective files

* fix test (json-schema 301)

* Add github.com/pb33f/libopenapi (#1004)

* Add github.com/pb33f/libopenapi

it looks like a reasonable alternative

* Update README.md

---------

Co-authored-by: Pierre Fenoll <[email protected]>

* Introduce an option to override the regex implementation (#1006)

* make form required field order deterministic (#1008)

* openapi2: fix un/marshalling discriminator field (#1011)

* fix: issue unmarshalling when discriminator field is set in openapi2.0

* revert original approach

* update with different approach

* Revert "update with different approach"

This reverts commit 2db2b39.

* v2 schema with discriminator field set as string

* update ref link and comment

* run docs.sh

* README: add Fuego to dependents (#1017)

* openapi3: skip a test in CI to avoid 403s from some remote server (#1019)

Signed-off-by: Pierre Fenoll <[email protected]>

* revert fix test (json-schema 301)

* openapi3: introduce StringMap type to enable unmarshalling of maps with Origin (#1018)

* add origin to more components

* Fix Discriminator

* update docs

* update doccs

* merge with stringmap

* remove unused Scopes

---------

Signed-off-by: Pierre Fenoll <[email protected]>
Co-authored-by: Jille Timmermans <[email protected]>
Co-authored-by: Pierre Fenoll <[email protected]>
Co-authored-by: Alexander Bakker <[email protected]>
Co-authored-by: Justin Sherrill <[email protected]>
Co-authored-by: Jay Shah <[email protected]>
Co-authored-by: Ewen Quimerc'h <[email protected]>
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.

OpenAPI 2.0: Unable to Unmarshal Spec with Discriminator Field Set
2 participants