modify to use upsert and alchemy style join - #27
Merged
Conversation
☂️ Python Coverage
Overall Coverage
New FilesNo new covered files... Modified Files
|
Contributor
There was a problem hiding this comment.
Pull request overview
Refactors PersonRepository to use SQLite's ON CONFLICT DO UPDATE for true upsert semantics in update_or_create_person, and switches get_person to use joinedload(Person.country) for eager loading. Adds two integration tests covering create-via-PUT and update-via-PUT.
Changes:
update_or_create_personnow usessqlalchemy.dialects.sqlite.insert(...).on_conflict_do_update(...)instead ofsession.merge(person).get_personreplaces a manualjoin(Country, ...)withselect(Person).options(joinedload(Person.country)), and the unusedCountryimport is removed.- New tests
test_upsert_createandtest_upsert_updateverify both branches of the upsert through the PUT endpoint.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| pyfastapi/repositories/person.py | Switches to SQLite INSERT ... ON CONFLICT DO UPDATE for upsert and uses joinedload for Person.country. |
| tests/api_tests/test_persons.py | Adds test_upsert_create and test_upsert_update integration tests for the PUT endpoint. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What?
Modify persisting and loading data for persons
Why?
on loading data: this ensures consistent eager loading and prevents potential N+1 lazy-load issues during serialization
on persisting: upsert would make the insert or update operation atomic
How?
code change
Testing?
add integration tests to cover upsert
AI Summary (optional)
This pull request updates the
PersonRepositoryto use an upsert (update-or-insert) approach for savingPersonrecords and adds corresponding tests to ensure correct behavior. The main changes are the introduction of a database-level upsert using SQLite'sON CONFLICT DO UPDATEand the addition of tests to verify both creation and update via upsert.Repository improvements:
update_or_create_personmethod inPersonRepositoryto use a SQLiteinsertstatement withon_conflict_do_update, enabling true upsert behavior instead of relying on SQLAlchemy'smergemethod. This ensures that if aPersonwith the given ID exists, it is updated; otherwise, a new record is created.get_personmethod to usejoinedloadfor eager loading of the relatedcountry, improving query efficiency and code clarity.Testing improvements:
test_upsert_createto verify that a PUT request for a non-existentPersonID creates a new record as expected.test_upsert_updateto verify that a PUT request for an existingPersonID updates the record correctly.