DRF Serializer update #326
Conversation
1 similar comment
|
Looks great! I see you decided to go with making the add/update decision based on if there was some sort of id specified. Much more django like. ;) I'm wondering about raising the 404 on update if the instance isn't found - is GraphQL meant to be raising HTTP status codes? From graphql-python/graphene#142 it seems not. |
|
I decided to go with the add/update and leave it up to user if they want to split out. The other way, the implementation would be very much the same but with more enforcement. So I went the flexible route. As for the 404, that is something I am wishing for :-) Graphene eats a lot of errors and returns 200 codes. I see the reasoning for not with the link you posted but I still think that's probably bad behavior to continue executing after a failure. I'll update if needed but one can always dream. |
There was a problem hiding this comment.
Just a few discussion points.
Also, see #228 where the idea of generalizing out to the get_serializer level is proposed. I think get_serializer_kwargs would cover the context case it is after, but I see value in both levels of abstraction.
| @@ -67,9 +90,35 @@ def __init_subclass_with_meta__(cls, serializer_class=None, | |||
| ) | |||
| super(SerializerMutation, cls).__init_subclass_with_meta__(_meta=_meta, input_fields=input_fields, **options) | |||
|
|
|||
| @classmethod | |||
| def resolve_serializer_inputs(cls, root, info, **input): | |||
There was a problem hiding this comment.
What do you think of get_serializer_kwargs for this method? "resolve" and "inputs" both have special meanings in the graphene world.
| return { | ||
| 'instance': instance, | ||
| 'data': input, | ||
| 'partial': cls._meta.partial |
There was a problem hiding this comment.
instance and data definitely make sense here, as those are very dynamic and dependent on the query.
However, I think kwargs like partial (many, context, etc.) should use a different mechanism instead of living on SerializerMutationOptions. If we go with renaming this get_serializer_kwargs, those would be easy enough to add with a call to super on an overridden version.
There is also the compromise of adding a serializer_kwargs option to the SerializerMutationOptions that would be combined with this dict.
There was a problem hiding this comment.
Sounds good, I wasn't too happy with partial there
|
@spockNinja updated with feedback, also changed the operations to |
2 similar comments
|
It looks like the build is breaking due to an issue with py.test. I'll put in a separate PR to get that fixed up, and rebuild this one once it's merged. |
3 similar comments
|
@colanconnon added your lines but getting a weird test failure. I'll see if i can debug, it seems unrelated |
|
@pizzapanther if you rebase from master the CI check will pass :) |
|
@patrick91 rebased |
|
@patrick91 @spockNinja is this PR ready to be merged? |
|
I re-reviewed and everything looks perfect. Sorry for taking so long to merge. |
Currently you can't use serializers for updates only add operations unless you customize
mutate_and_get_payload. This PR adds functionality to support most updates without any customization needed by the user. I also added support for Partial updates.