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

feat: Forward route class when it's different from APIRoute #53

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

yuripiratello
Copy link

@yuripiratello yuripiratello commented Feb 13, 2024

Pull Request Checklist

  • Make sure to read the contributor guide here.
  • Make sure you are making a pull request against the main branch.
  • Make sure your pull request title matches the requested format.
  • Make sure to lint, type-check, and run unit and API tests.

Description

This PR enables the route_class to be forwarded to the versionizer API.

Resolve #52 .

@alexschimpf
Copy link
Owner

Looks like tests are failing

@yuripiratello
Copy link
Author

Looks like tests are failing

Hey @alexschimpf !

The tests are failing on main in python versions: 3.9.18, 3.10.13, 3.11.7, and 3.12.1.

It's only working on v3.7.17. Same with this branch/PR.

Did I miss something from the setup steps? I don't think so... the single command required is make install-dev to run the tests.

@alexschimpf
Copy link
Owner

make install-dev will just install python requirements in your current virtualenv. I don't have anything fancy on local to test for all supported python versions. I've only added testing for multiple python versions via Github workflows.

I haven't looked into the errors that deeply, but...
It's possible that the FastAPI versions are different between python versions, which is causing differences in test output. We may have to start dropping support for older python versions to be more in line with FastAPI. For example, FastAPI 0.104.0 dropped support for python 3.7.

@yuripiratello
Copy link
Author

@alexschimpf, could you double-check on your machine if the main branch is passing on tests with a Python version greater than 3.7? Or rerun the last release actions?
If you're looking for an easy way to test multiple python versions I would suggest using pyenv, it's really easy and straightforward to manage it, the steps that I executed were:

  • install any python version
  • create a virtualenv
  • activate
  • make install-dev
  • make run-tests

If you think a newer FastAPI version broke the lib, I can try to solve it, but I tested the lib with my project using Python 3.11, and it worked.

@alexschimpf
Copy link
Owner

alexschimpf commented Feb 22, 2024

@yuripiratello I get the same error as the github workflows show. But I'm running the tests against the main branch, which means the tests were broken before this PR. Not sure why you're not seeing errors? What version of fastapi do you have installed?

After some light testing, I see that the tests fail starting with fastapi==0.109.0. Here is the FastAPI 0.109.0 changelog notes: https://fastapi.tiangolo.com/release-notes/#01090

I don't really have time at the moment to look into this any more deeply. We could simply just pin the fastapi version to <=0.108.0 for now, until we figure out what was changed.

@yuripiratello
Copy link
Author

@alexschimpf, I found the issue: It is the root_path on FastAPI. When you define this attribute, it appends the root_path as a prefix to the openapi_url. I created a new PR to fix that issue.

#54

@alexschimpf
Copy link
Owner

@yuripiratello Can you merge in the latest main branch? That should fix the issue with the tests.

@yuripiratello
Copy link
Author

Done @alexschimpf

@@ -324,6 +324,8 @@ def _add_route_to_router(
version: Tuple[int, int]
) -> None:
kwargs = dict(route.__dict__)
if route.__class__ != APIRoute:
kwargs['route_class_override'] = route.__class__
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couldn't you accomplish the same thing with this:

kwargs['route_class_override'] = type(route)

?

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.

Route class from APIRouter is not getting forward on versionizer
2 participants