Fix nested fields update #604
Conversation
…), add one more test to check nested object with constraint is not duplicated in case of multiple mutation invocation (see issue strawberry-graphql#360)
…function to extract nested object model properly
…ested objects creation), cleanup code, remove unused Issue `comments` field, remove duplicated inherited schema definition
… value if explicitly passed in mutation input data to force create object (workaround for previously added `get_or_create`)
Reviewer's Guide by SourceryThis pull request addresses issues related to nested fields updates and refines the handling of Django's File-Level Changes
Tips
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @EvSpirit - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider refactoring the create() function in resolvers.py to improve readability, possibly by extracting some logic into separate helper functions.
- The new approach for handling nested unique constraints and forcing object creation is good, but please add more documentation to explain this behavior to users of the library.
Here's what I looked at during the review
- 🟡 General issues: 4 issues found
- 🟢 Security: all looks good
- 🟡 Testing: 3 issues found
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
| @@ -299,6 +299,274 @@ def test_input_create_mutation_nested_creation(db, gql_client: GraphQLTestClient | |||
| } | |||
|
|
|||
|
|
|||
| @pytest.mark.django_db(transaction=True) | |||
There was a problem hiding this comment.
suggestion (testing): Missing edge case for empty nested list
Consider adding a test case where the nested list input is empty to ensure the mutation handles it correctly.
|
|
||
|
|
||
| @pytest.mark.django_db(transaction=True) | ||
| def test_input_create_mutation_multiple_level_nested_list_input_creation( |
There was a problem hiding this comment.
issue (testing): Missing test for invalid input
Add a test case to handle invalid input scenarios, such as missing required fields or incorrect data types, to ensure robustness.
| @@ -470,6 +738,55 @@ def test_input_update_mutation(db, gql_client: GraphQLTestClient): | |||
| assert set(issue.tags.all()) == set(expected_tags) | |||
|
|
|||
|
|
|||
| @pytest.mark.django_db(transaction=True) | |||
| def test_input_nested_unique_together_update_mutation( | |||
There was a problem hiding this comment.
suggestion (testing): Test for unique constraint violation
Add a test case to explicitly check for unique constraint violations to ensure the mutation handles such scenarios correctly.
| if v.pk in (None, UNSET): # noqa: PLR6201 | ||
| related_model = get_model_fields(model).get(k).related_model | ||
| v = create(info, related_model, v.data or {}) # noqa: PLW2901 |
There was a problem hiding this comment.
Seems like pk may be either UNSET or None, in both cases it's necessary to create nested object. And to further properly perform full_clean and save we should identify related model (as in test type hierarchy most models use name clashing field names, cleaning could pass somehow, but finally object of different type could be created).
| try: | ||
| # Instead of using `get_or_create` shortcut first use `get()` | ||
| # (which will also raise `DoesNotExist` in case pk `None` value explicitly | ||
| # passed), if no records found - invoke `full_clean()` and `create` | ||
| # right after. The idea is to bypass cleaning first to allow getting | ||
| # an object by e.g. unique fields. In case `full_clean()` invoked before, | ||
| # it would raise unique constraint validation error | ||
| instance = model._default_manager.get(**create_kwargs) | ||
| except model.DoesNotExist: | ||
| # Creating the instance directly via create() without full-clean will | ||
| # raise ugly error messages. To generate user-friendly ones, we want | ||
| # full-clean() to trigger form-validation style error messages. | ||
| full_clean_options = full_clean if isinstance(full_clean, dict) else {} | ||
| dummy_instance.full_clean(**full_clean_options) # type: ignore |
There was a problem hiding this comment.
Instead of using get_or_create shortcut I finally came up with a decision to use get and create separately, but invoke full_clean only after get() is invoked as full_clean may raise unique constraint violation exception (e.g. in use case described in #360 )
| @@ -552,16 +570,33 @@ def update_m2m( | |||
|
|
|||
| use_remove = True | |||
| if isinstance(field, ManyToManyField): | |||
| manager = cast("RelatedManager", getattr(instance, field.attname)) | |||
| remote_field_name = field.attname | |||
| reverse_field_name = field.remote_field.related_name | |||
There was a problem hiding this comment.
Have some doubts here, probably there is any other way to get reverse relationship name.
| @@ -171,6 +181,21 @@ def name_with_priority(self) -> str: | |||
| return f"{self.kind}: {self.priority}" | |||
|
|
|||
|
|
|||
| class Version(NamedModel): | |||
There was a problem hiding this comment.
I tried not to further extend types hierarchy, but this one is required to cover #362 changes
| issues: "RelatedManager[Issue]" | ||
|
|
||
| id = models.BigAutoField( | ||
| verbose_name="ID", | ||
| primary_key=True, | ||
| ) | ||
|
|
||
| name = models.CharField(max_length=255, unique=True) |
There was a problem hiding this comment.
Also related to #362 . Not only unique_together may be the case but also a single unique field.
| @strawberry_django.input(Issue) | ||
| class IssueInputWithMilestones(IssueInput): | ||
| milestone: "MilestoneInputPartial" | ||
|
|
There was a problem hiding this comment.
Workaround not to expose milestone related fields to inherited schema (as related input types were updated to cover nested CUD operations):
strawberry-django/tests/projects/test_schema.py
Lines 21 to 34 in 9025ee5
| # right after. The idea is to bypass cleaning first to allow getting | ||
| # an object by e.g. unique fields. In case `full_clean()` invoked before, | ||
| # it would raise unique constraint validation error | ||
| instance = model._default_manager.get(**create_kwargs) |
There was a problem hiding this comment.
Is this safe? I mean, how can we ensure that create_kwargs will identify an object by unique fields?
Suppose that we have a model that has a name, and it is not unique nor something that should identify it. Then we get create_kwargs = {"name": "Foobar"}. If we already have that object, wouldn't this get that object by mistake instead of creating a new one (which is what we want)?
There was a problem hiding this comment.
Yes, you are completely right, that is my main concern. Unfortunately an object with Foobar name will be returned and no new object created. Django built-inget_or_create shortcut will do exactly the same, I tried to deep-dive into Django code to find any implementation of identifying all unique fields (FK, unique prop/other unique constraints), but it seems that's tricky.
This idea comes from #360, as I tried to unify nested object creation, something breaks here, either #360 fix or new object creation. I shared my thoughts in the current PR description, could you please also have a look?
The idea to identify all unique fields seems promising, but I'm not sure that we can easily cover all possible scenarios, probably there are some DB-specific tricks also. I could not find any built-in implementation for that, Django total_unique_constraints property does not solve that problem.
I see two options here to keep both #360 fix and duplicated objects creation:
- Introduce convention to explicitly add
id=Nonein payload - Identify all unique fields and execute
get()only if unique fields are in the query
Please share your thoughts. I'd personally prefer option 2, but some more research needed, have some doubts here. Option 1 looks a bit weird, but it's bullet-proof.
Probably option 3 is to remove get() and always execute full_clean followed by create, but it would break #360.
There was a problem hiding this comment.
I've checked Django code for unique fields validations, there are a few helpful fragments:
- This one covers
unique_togetherandtotal_unique_constraints
https://github.com/django/django/blob/9cf9c796be8dd53bc3b11355ff39d65c81d7be6d/django/contrib/admin/views/main.py#L473-L484
constraint_field_names = (
*self.lookup_opts.unique_together,
*(
constraint.fields
for constraint in self.lookup_opts.total_unique_constraints
),
)
uniquefield + null check
https://github.com/django/django/blob/9cf9c796be8dd53bc3b11355ff39d65c81d7be6d/django/contrib/admin/views/main.py#L441-L445
total_ordering_fields = {"pk"} | {
field.attname
for field in self.lookup_opts.fields
if field.unique and not field.null
}
So what we can try to do is to extract all unique fields using these three "sources" (unique, unique_together and UniqueConstraint), what do you think? I suppose in case only one of three types is used it should be relatively simple, but in case multiple one are in, that may be tricky.
But I still have doubts, should such a logic be a part of the library? Please share your thoughts.
There was a problem hiding this comment.
Although I do like the "identify the unique sources" idea, I'm worried if that could end up reusing some object where we were supposed to create a new one (and if it was unique, the user would maybe expect the code to break)
I'm thinking about a fourth option: What about expanding key_attr (or creating another field and deprecating it) to tell the list of fields/field combinations that are to be considered "keys"? Just like unique_together = [("foo", "bar"), ("bin",)] for the django model
In that example, someone could only pass "bin" to act like "get_or_create", but ("foo", "bar") would still try to only create, etc.
| result = mutation( | ||
| '{ fruits: updateFruits(data: { types: [{name: "apple"}, {name: "apple"}]}) { id types { id name }}}' | ||
| '{ fruits: updateFruits(data: { types: [{ name: "apple"}, {id: null, name: "apple"}]}) { id types { id name }}}' |
There was a problem hiding this comment.
This is what I was afraid in a previous comment. Should this case be forced to pass id: null? name is not an fk and neither a key that is used to reference the model, so it seems to me that we should not need this change to make this test work
There was a problem hiding this comment.
Yes, unfortunately without explicit id: null duplicated object won't be created as Django ORM get(...) method would return an object found by name.
Provided more details in the previous reply: https://github.com/strawberry-graphql/strawberry-django/pull/604/files#r1696994011, let's continue discussion there if you don't mind
| @@ -1116,21 +1435,21 @@ def test_mutation_full_clean_with_kwargs(db, gql_client: GraphQLTestClient): | |||
|
|
|||
| res = gql_client.query( | |||
| query, | |||
| {"input": {"title": "ABC", "fullCleanOptions": True}}, | |||
| {"input": {"pk": None, "title": "ABC", "fullCleanOptions": True}}, | |||
There was a problem hiding this comment.
In those tests here, what would happen without this change?
There was a problem hiding this comment.
Replied here https://github.com/strawberry-graphql/strawberry-django/pull/604/files#r1697016742, that's a similar case
| @strawberry_django.type(Project) | ||
| class ProjectTypeSubclass(ProjectType): ... | ||
|
|
||
| @strawberry_django.type(Milestone) | ||
| class MilestoneTypeSubclass(MilestoneType): ... | ||
|
|
||
| @strawberry_django.input(Issue) | ||
| class IssueInputSubclass(IssueInput): ... | ||
|
|
||
| @strawberry.type | ||
| class Query: | ||
| project: Optional[ProjectTypeSubclass] = strawberry_django.node() | ||
| milestone: Optional[MilestoneTypeSubclass] = strawberry_django.node() | ||
|
|
||
| @strawberry.type | ||
| class Mutation: | ||
| create_issue: IssueType = mutations.create( | ||
| IssueInputSubclass, | ||
| handle_django_errors=True, | ||
| argument_name="input", | ||
| ) | ||
|
|
||
| schema = strawberry.Schema(query=Query, mutation=Mutation) | ||
| snapshot.snapshot_dir = SNAPSHOTS_DIR | ||
| snapshot.assert_match(str(schema), "schema_with_inheritance.gql") |
There was a problem hiding this comment.
Why did you have to change this?
There was a problem hiding this comment.
Seems that there is a duplicated fragment of code:
strawberry-django/tests/projects/schema_inherited.py
Lines 12 to 39 in 989e6bf
and
strawberry-django/tests/projects/test_schema.py
Lines 22 to 44 in 989e6bf
So I tried to remove duplicated code. Or am I missing something?
There was a problem hiding this comment.
Oh, that schema_inherited.py is not actually used anywhere (maybe it is a left over from some old attempt)
I think the ideal here is to remove that file and have the test "as is", as it is usually better to have everything defined inside the test itself.
I'll send a commit to main removing that file
|
I'd love to see this merged. Happy to help where I can. 😄 |
I'm mostly waiting for the tests to pass and the adjustment to the conflict to review this again. Not sure if @EvSpirit is going to tackle those, but if not, I do appreciate the help to finish this PR and merge it :) |
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
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.