sourcery-ai[bot]
commented
4 months ago Reviewer's Guide by Sourcery
This pull request addresses issues related to nested fields updates and refines the handling of Django's get_or_create shortcut. The changes include bug fixes, enhancements, and code refactoring. The main approach involves using get before create to avoid unique constraint violations and introducing a convention to force object creation when a None primary key is explicitly passed. Additionally, the PR includes new tests, schema updates, and minor code cleanups.
File-Level Changes
| Files | Changes |
|---|---|
tests/test_input_mutations.pytests/projects/schema.pytests/projects/test_schema.pytests/projects/models.pytests/mutations/test_mutations.pytests/projects/schema_inherited.pytests/types.py |
Added new test cases, updated existing tests, and modified schema and model definitions to support new nested relations and input types. |
strawberry_django/mutations/resolvers.pystrawberry_django/mutations/mutations.pystrawberry_django/mutations/types.pystrawberry_django/mutations/fields.py |
Refactored mutation resolver functions to handle None and UNSET primary key values, introduced full_clean options, and improved nested object creation handling. |
axieum
bellini666
Description
This PR addresses issues described in #603. Kindly ask you to review the code and share your ideas.
While working on these two major bugs I also faced another issue related to previously added Django
get_or_createshortcut (see PR #362). The current implementation seems a bit inconsistent for me becauseget_or_createis used only in m2m relations, more than that, this shortcut relies not only on unique fields. So it definitely fixes the issue mentioned in #360 , but at the same time there may be an opposite situation when duplicated records won't be created. First I thought to identify all unique fields and constraints (e.g. using Djangototal_unique_constraints) to further understand ifgetshould be used before creating an object. But it seems there are quite many caveats. So what I'm suggesting is to have the following convention: if an object input contains explicitly passedNone/nullpk value (notUNSET), it would indicate an intention to force create an object. If omitted,get_or_createapproach will be applied (actually, it's a bit different under the hood, I refactoredfull_cleanusage to be invoked only afterget(), otherwise,full_cleanmay trigger unique constraint violation).Personally I think that it should not be hardcoded somewhere in the lib and this approach looks only as a workaround for me, probably it's a good idea to further extend input types or probably integrate forms to precisely control fields cleaning and saving, that's for further discussion and may be a a good feature request (as while working on the issue I noticed that
full_cleanoptions usability may be limited in case of nested relations).It the current idea looks good, I think it would be better to update docs respectively.
Other than proposed fixes for the issues mentioned above, there is some minor code refactoring and cleanup:
full_cleanoptions exposed toDjangoMutationCUDPlease let me know if it is better to move out such changes to another PR.
Types of Changes
Issues Fixed or Closed by This PR
603
Checklist
Summary by Sourcery
This pull request fixes issues with nested fields update, refines the
full_cleanprocess, and enhances the handling ofget_or_createfor better consistency. It also includes new tests to cover these changes and removes redundant test code.full_cleanusage to be invoked only afterget()to prevent unique constraint violations.full_cleanoptions toDjangoMutationCUDfor better control over validation.get_or_createto support explicitNone/nullpk values for forced object creation.