search

pb33f / libopenapi

libopenapi is a fully featured, high performance OpenAPI 3.1, 3.0 and Swagger parser, library, validator and toolkit for golang applications.
https://pb33f.io/libopenapi/
Other
362 stars 46 forks source link

fix bundler panic when BuildV3Model fail #260

Closed emilien-puget closed 3 months ago

emilien-puget commented 3 months ago

in some cases, BuildV3Model will return a nil first parameter and an error which result in a panic in the next call, this PR aims to fix this behavior.

codecov[bot] commented 3 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 99.63%. Comparing base (94c06b1) to head (0c0c3fb).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #260 +/- ## ======================================= Coverage 99.63% 99.63% ======================================= Files 162 162 Lines 12064 12066 +2 ======================================= + Hits 12020 12022 +2 Misses 44 44 ``` | [Flag](https://app.codecov.io/gh/pb33f/libopenapi/pull/260/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pb33f) | Coverage Δ | | |---|---|---| | [unittests](https://app.codecov.io/gh/pb33f/libopenapi/pull/260/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pb33f) | `99.63% <100.00%> (+<0.01%)` | :arrow_up: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pb33f#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

emilien-puget commented 3 months ago

there is no relevant way to increase the code coverage for this change

daveshanley commented 3 months ago

there is no relevant way to increase the code coverage for this change

What about changing the design so that a failOnError bool can be passed in to decide if it should return on any build errors, or continue going?

The reason why the coverage is failing is because it should not be a 'any failure results in total failure' scenario.

There could be many reasons why specific schemas failed to build or things went wrong deeper in the build tree, that is why there is an error returned. Just because one thing failed, it does not mean the rest of the model is toast. If there is a model, then you are good to go.

Here is where we would allow the consumer to decide if they want to fail at this point (return the error) or continue on.

To continue, would allow the circular reference check to pass (which is why errors may have returned, but the model knows how to handle them when building).

emilien-puget commented 3 months ago

we could return when the model is nil instead of when any error is returned, allowing the consumer to generate a panic is not great

emilien-puget commented 3 months ago

@daveshanley i changed the error handling to be more in sync with the previous behavior, while still being panic free