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

Add feature to customize the item_id parameter name #174

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

nikstuckenbrock
Copy link

As mentioned in #163 a feature request to customize the item_id parameter name in the OpenAPI specification was requested. I implemented it and added some extra tests and documentation for it.

I would be very thankful if you would add the HACKTOBERFEST-ACCEPTED to this pull request.

Hope this helps. Thanks in advance.

@vercel
Copy link

vercel bot commented Oct 26, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated
fastapi-crudrouter ✅ Ready (Inspect) Visit Preview 💬 Add your feedback Jan 29, 2023 at 5:03AM (UTC)

fastapi_crudrouter/core/tortoise.py Outdated Show resolved Hide resolved
tests/test_custom_item_id.py Outdated Show resolved Hide resolved
@vercel
Copy link

vercel bot commented Jan 29, 2023

Someone is attempting to deploy a commit to a Personal Account owned by @awtkns on Vercel.

@awtkns first needs to authorize it.

@awtkns
Copy link
Owner

awtkns commented Jan 29, 2023

Hi @nikstuckenbrock, thanks for this! Sorry about the slow feedback loop, it has been a busy couple months. Great work here! 🚀

A couple notes, by default name of the item_id can probably be extracted from the name of the user's model. This is already being done for routes. Eg: /users/{user_id} or /post/{post_id}. The logic you added here might work well if the user wants even more control.

My one concern is that the item_id on the route (circled in red) is not being change, which may lead to confusion. I wonder if there is a way to modify this in the router?

image

@nikstuckenbrock
Copy link
Author

Hi @awtkns,

I tried my best but couldn't find a way to overwrite this in the router. There is an option using OpenAPI Extensions but this leads to two parameters in the specification (item_id and potato_id). I've tried to overcome this issue using **kwargs instead of a specific parameter named item_id in every single implementation of the CRUDRouter. For example:

def _get_one(self, *args: Any, **kwargs: Any) -> CALLABLE:
        def route(**kwargs) -> SCHEMA:
            for model in self.models:
                if model.id == item_id:  # type: ignore
                    return model

            raise NOT_FOUND

        return route

This leads to a parameter kwargs in the OpenAPI specification.

A different approach could be to modify the specification by replacing the id values as mentioned here. But this would require that the CRUDRouter has access to the FastAPI app instance it is included at. But this is, as far as I can see, not the case at the moment. A possible solution would be to change the way the CRUDRouter is initialized. This example shows how the router could change the app:

app = FastAPI("...")
CRUDRouter(app)

Initialising the CRUDRouter this way there would be access to the app and the parameters could be overwritten.

Hoping for feedback. Let me know what you think. Thanks in advance!

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.

3 participants