python-restx / flask-restx

Fork of Flask-RESTPlus: Fully featured framework for fast, easy and documented API development with Flask
https://flask-restx.readthedocs.io/en/latest/
Other
2.16k stars 335 forks source link

Use the referencing package for resolving schema references #560

Closed foarsitter closed 10 months ago

foarsitter commented 1 year ago

For what I found is that the jsonschema validators are compatible with the referencing package resolvers.

This change is at least covered by test_api_payload_strict_verification.

Fixes #553

peter-doggart commented 1 year ago

Sure thing. Will have a review of this later this evening. :)

foarsitter commented 1 year ago

Yesterday everything seemed fine: https://github.com/foarsitter/flask-restx/actions/runs/6237094186/job/16929870907

Today I messed up the branch while on the phone. @hbusul can you see what's the difference?

peter-doggart commented 1 year ago

I think we might run into problems because currently flask-restx supports python 3.7 but the new referencing package does not, just looking at the pipelines that ran previously. Can't check right now, but if python 3.7 is end of life we might need to drop support to make this change.

foarsitter commented 1 year ago

Python 3.7 is indeed EOL since 2023-06-27.

hbusul commented 1 year ago

It is not only referencing but also jsonschema that does not support python 3.7 . With 3.7 it does not even import referencing due to a syntax error. I have nothing against for dropping python 3.7 but I do not know if it should be done with this ticket or it should be announced. What do you think about moving to import statement in try-except and fallback to not supported way but that also gives you time until jsonschema really drops it, maybe to counter that we can fix the version and when we get rid of 3.7 we can unfix the version.

foarsitter commented 1 year ago

Some tests are failing and I'm unable to find a solution.

FAILED tests/test_payload.py::PayloadTest::test_validation_with_inheritance FAILED tests/test_payload.py::PayloadTest::test_validation_on_list

hbusul commented 1 year ago

I'm a bit busy but I will try to look at why those are failing ASAP within this week

codecov[bot] commented 1 year ago

Codecov Report

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

Comparison is base (3ea4ce1) 96.45% compared to head (2715d4a) 96.44%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #560 +/- ## ========================================== - Coverage 96.45% 96.44% -0.01% ========================================== Files 20 20 Lines 2733 2728 -5 ========================================== - Hits 2636 2631 -5 Misses 97 97 ``` | [Files](https://app.codecov.io/gh/python-restx/flask-restx/pull/560?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=python-restx) | Coverage Δ | | |---|---|---| | [flask\_restx/api.py](https://app.codecov.io/gh/python-restx/flask-restx/pull/560?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=python-restx#diff-Zmxhc2tfcmVzdHgvYXBpLnB5) | `96.50% <ø> (-0.06%)` | :arrow_down: | | [flask\_restx/model.py](https://app.codecov.io/gh/python-restx/flask-restx/pull/560?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=python-restx#diff-Zmxhc2tfcmVzdHgvbW9kZWwucHk=) | `96.96% <100.00%> (+0.04%)` | :arrow_up: | | [flask\_restx/resource.py](https://app.codecov.io/gh/python-restx/flask-restx/pull/560?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=python-restx#diff-Zmxhc2tfcmVzdHgvcmVzb3VyY2UucHk=) | `96.07% <100.00%> (ø)` | |

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

foarsitter commented 1 year ago

Did some investigation on the issue today. Seems that we do not need referencing after all. #definitions/Person is a local pointer to within the document. Adding resources to the references registry will not help since that adds global reference (i.e. https://yourapi.com/Person).

Instead of keeping a global registry, I added the definitions to the schema that is going to be validated on the fly. In this way the $ref's can be resolved and we do not need an external resolver.

Note: In order to make the tests succeed I needed to pin flask < 3.0.0. If we will accept this solution that commit has to be removed.

dsrink commented 9 months ago

The last few messages on this PR look strange to me. It seems that this PR was not merged. Could somebody handle this? This issue is bugging me in some of my installations.

foarsitter commented 9 months ago

@dsrink sorry, I deleted the fork. Opened #592 to fix this.