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.

Sign up for GitHub

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

Jump to bottom

feat: Add Sqlalchemy factory #342

Merged
merged 14 commits into from
Aug 30, 2023
Merged

feat: Add Sqlalchemy factory #342

merged 14 commits into from
Aug 30, 2023

Conversation

adhtruong
Copy link
Collaborator

Pull Request Checklist

  • New code has 100% test coverage
  • (If applicable) The prose documentation has been updated to reflect the changes introduced by this PR
  • (If applicable) The reference documentation has been updated to reflect the changes introduced by this PR
  • Pre-Commit Checks were ran and passed
  • Tests were ran and passed

Description

  • Initial implementation of SQLAlchemy integration by adding understanding of columns and nullability.
  • Rework existing mapping to allow Python type to be used by default
  • Note that foreign key constraints are not currently met by default as that could be achieved by using subfactories without base factory parsing.
  • Relationships and persistence are currently also ignored.

Close Issue(s)

@adhtruong adhtruong requested a review from a team as a code owner August 26, 2023 13:54
@adhtruong adhtruong force-pushed the sqlalchemy-factory-model-processing branch 2 times, most recently from 388e3de to 74f99fe Compare August 26, 2023 16:05
@Goldziher
Copy link
Contributor

@cofin @provinzkraut and @guacs - i assigned you to this PR since I would like people with more substantial SQLA experience than myself to make the decisions here.

guacs
guacs reviewed Aug 27, 2023
View reviewed changes
Copy link
Member

@guacs guacs left a comment

Choose a reason for hiding this comment

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

I have the same doubts as mentioned in this comment in #278. How does this handle things like relationships and such?

Also, I think it might be nice to have more tests that covers all the types as in these tests for pydantic. At least for the types that have been kept in the get_ssqlalchemy_types.

guacs
guacs reviewed Aug 27, 2023
View reviewed changes
polyfactory/factories/sqlalchemy_factory.py Outdated
types_override = cls.get_sqlalchemy_types()
columns = cls.__model__.__table__.columns
for name, column in columns.items():
annotation: type = type(column.type) if type(column.type) in types_override else column.type.python_type
Copy link
Member

Choose a reason for hiding this comment

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

I think the handling of the type of the items in the list if the type is ARRAY is missing. If I'm not mistaken, column.type.python_type will only return list and not List[str] or List[int] or whatever be the case.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point. I have updated to extract origin for ARRAY SQLAlchemy type here.

Note I think a similar issue exists for tuple type but I wasn't able to add better support for this easily.

@sourcery-ai sourcery-ai bot mentioned this pull request Aug 27, 2023
Closed
@adhtruong
Copy link
Collaborator Author

I have the same doubts as mentioned in this comment in #278. How does this handle things like relationships and such?

Also, I think it might be nice to have more tests that covers all the types as in these tests for pydantic. At least for the types that have been kept in the get_ssqlalchemy_types.

I have the same doubts as mentioned in this comment in #278. How does this handle things like relationships and such?

Also, I think it might be nice to have more tests that covers all the types as in these tests for pydantic. At least for the types that have been kept in the get_ssqlalchemy_types.

I have updated to support relationships optionally by class level configuration with default not parsing these. Default here was chosen to prevent recursion errors.

In regards to association properties and hybrid properties, I do not think need to explicitly set these as will implicitly set by just setting the columns. Happy to add a test to confirm these are ignored.

@adhtruong adhtruong changed the base branch from sqlalchemy-factory to main August 29, 2023 20:32
@adhtruong adhtruong requested a review from a team as a code owner August 29, 2023 20:32
@adhtruong adhtruong changed the title Sqlalchemy factory model processing feat: Add Sqlalchemy factory Aug 30, 2023
@sourcery-ai sourcery-ai bot mentioned this pull request Aug 30, 2023
Closed
@Goldziher Goldziher changed the base branch from main to sqlalchemy-factory August 30, 2023 08:52
@Goldziher
Copy link
Contributor

@adhtruong I rebased the base branch and updated it. Please rebase your PR.

@adhtruong adhtruong force-pushed the sqlalchemy-factory-model-processing branch from cd52ed3 to c67843f Compare August 30, 2023 09:26
@Goldziher Goldziher merged commit ae1e78b into litestar-org:sqlalchemy-factory Aug 30, 2023
15 checks passed
@Goldziher
Copy link
Contributor

@all-contributors add @adhtruong code

@allcontributors allcontributors bot mentioned this pull request Aug 30, 2023
Merged
@allcontributors
Copy link
Contributor

@Goldziher

I've put up a pull request to add @adhtruong! 🎉

adhtruong added a commit to adhtruong/polyfactory that referenced this pull request Sep 17, 2023
@adhtruong
feat: Add Sqlalchemy factory (litestar-org#342)
88aa796 
* feat: Add SQLAlchemyFactory table column parsing

* docs: Add SQLAlchemy reference

* docs: amend link

* doc: amend link

* Revert type updates

* feat: add SQLAlchemy relationship configuration

* feat: improve mapping for SQLAlchemy ARRAY

* feat: SQLAlchemy test hints for 3.8

* feat: SQLAlchemy test hints for 3.8

* feat: SQLAlchemy test hints for 3.8

* feat: SQLAlchemy factory hints for 3.8

* feat: add extra attributes for SQLAlchemyFactory

* feat: resolve SQLAlchemyFactory type issues

* feat: fix rebase errors
guacs pushed a commit that referenced this pull request Sep 18, 2023
@adhtruong @Goldziher
feat: implement SQLA factory (#369)
c76ffc9 
* feat: add sqlalchemy factory (initial)

* feat: Add Sqlalchemy factory (#342)

* feat: Add SQLAlchemyFactory table column parsing

* docs: Add SQLAlchemy reference

* docs: amend link

* doc: amend link

* Revert type updates

* feat: add SQLAlchemy relationship configuration

* feat: improve mapping for SQLAlchemy ARRAY

* feat: SQLAlchemy test hints for 3.8

* feat: SQLAlchemy test hints for 3.8

* feat: SQLAlchemy test hints for 3.8

* feat: SQLAlchemy factory hints for 3.8

* feat: add extra attributes for SQLAlchemyFactory

* feat: resolve SQLAlchemyFactory type issues

* feat: fix rebase errors

* feat: Add SQLA persistence handles, update class attributes

* docs: Add SQLAFactory docs

* test: add missing __init__ files, fix imports, fix rebase errors

* docs: fix SQLA class ref

---------

Co-authored-by: Na'aman Hirschfeld <nhirschfeld@gmail.com>
@adhtruong adhtruong deleted the sqlalchemy-factory-model-processing branch October 18, 2023 15:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@guacs guacs guacs left review comments

@JacobCoffee JacobCoffee Awaiting requested review from JacobCoffee

Assignees

@cofin cofin

@provinzkraut provinzkraut

@guacs guacs

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@adhtruong @Goldziher @guacs @cofin @provinzkraut