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: ✨ add multiple primary keys support #31

Merged
merged 16 commits into from
Apr 3, 2024

Conversation

dubusster
Copy link
Contributor

@dubusster dubusster commented Mar 20, 2024

Pull Request Template for FastCRUD

Description

This PR aims to provide multiple primary key handling with the basic crud and the endpoint creator.
This way, for a model such as :

class MultiPkModel(Base):
    __tablename__ = "multi_pk"
    id = Column(Integer, primary_key=True)
    uuid = Column(String(32), primary_key=True)
    name = Column(String, unique=True)

the endpoint creator should gives a path like /multi_pk/get/{id}/{uuid} for the different endpoints (get, update and delete) that need primary keys to interact with the database. The order of path parameter is given by the column order.

Changes

There is some monkey patching of TypeDecorator to also cover custom types.
Although it works, this piece of code seems a bit hacky and might need improvement from SQLModel or even better sqlalchemy.

Tests

Add multiple primary keys tests on create, get and delete crud/endpoints for both sqlmodel and sqlalchemy.

Checklist

  • I have read the CONTRIBUTING document.
  • My code follows the code style of this project.
  • I have added necessary documentation (if appropriate).
  • I have added tests that cover my changes (if applicable).
  • All new and existing tests passed.

Additional Notes

This PR also allows to not be restricted to the id name for primary key 😄

@dubusster dubusster marked this pull request as ready for review March 20, 2024 13:45
@igorbenav igorbenav added the enhancement New feature or request label Mar 20, 2024
@igorbenav
Copy link
Owner

Hey, @dubusster, thanks for another improvement!
I'll take a look at both of them over the weekend.

@igorbenav igorbenav self-requested a review March 21, 2024 00:30
@dubusster
Copy link
Contributor Author

Thanks @igorbenav !
I was struggling for some time with my CRUD in FastAPI and FastCRUD came out of nowhere as what I wanted to achieve. 😄
It's a pleasure to bring some of my stuff in this project. 😉

@dubusster dubusster force-pushed the multi-primary-keys branch 2 times, most recently from 6051ea6 to 129116f Compare March 29, 2024 10:16
@igorbenav
Copy link
Owner

Hey, @dubusster, sorry for the long delay here! I'm having some doubts with Forge, since it's pretty much unmaintained since 2018, do you think it's the best approach?

To be honest, I experimented with it (basically got to something similar to forge) and it seems like in python this always looks bad. My main concern is with Forge being kind of abandoned.

@dubusster
Copy link
Contributor Author

Hey @igorbenav ! Indeed, I used forge as the approach since it was already written but it might be smarter to have some piece of code doing the basics and not using this full unmaintained library. I found out some other approach with basic python here, I'll try to fix something in the following days. 😄

Copy link
Owner

@igorbenav igorbenav left a comment

Choose a reason for hiding this comment

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

Really great pr, @dubusster, thanks! Just a few minor things and I'll merge it.

fastcrud/endpoint/helper.py Outdated Show resolved Hide resolved
fastcrud/endpoint/endpoint_creator.py Outdated Show resolved Hide resolved
Copy link
Contributor Author

@dubusster dubusster left a comment

Choose a reason for hiding this comment

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

Sorry, it's my first time doing a review, I don't know if I had to make commits first or "resolving"...

Anyway, I added your suggestions : some docstring for the decorator and a dedicated function to be used during dict comprehension !

@igorbenav igorbenav self-assigned this Apr 3, 2024
@igorbenav
Copy link
Owner

Nice one, @dubusster, I'll update the docs before the next release!

@igorbenav igorbenav merged commit cab5c58 into igorbenav:main Apr 3, 2024
7 checks passed
@dubusster dubusster deleted the multi-primary-keys branch April 13, 2024 13:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants