Open ramyak-mehra opened 4 years ago
Hey @ramyak-mehra
Instead of getting rid of Relay global id, btw i think its benetifts are open to discussion but it's a bit overkill, i'd recommend a fix on mutation class.
This is the original code on BaseDjangoFormMutation
, base of DjangoModelFormMutation
@classmethod
def get_form_kwargs(cls, root, info, **input):
kwargs = {"data": input}
pk = input.pop("id", None)
if pk:
instance = cls._meta.model._default_manager.get(pk=pk)
kwargs["instance"] = instance
return kwargs
This code tries to fetch your object with Django PK, which i don't see any logic in it. Maybe this base class has other usages that i don't know, but i started to believe it needs to be changed.
def get_form_kwargs(cls, root, info, **input):
kwargs = {"data": input}
global_id = input.pop("id", None)
if global_id:
node_type, pk = from_global_id(global_id)
instance = cls._meta.model._default_manager.get(pk=pk)
kwargs["instance"] = instance
return kwargs
I use this code on all mutations. It converts relay global id to Django's PK first, then tries to fetch to object.
@jkimbo ping.
DjangoModelFormMutation
so basically if I am understanding correctly you are trying to say i should edit the source files of the graphene? sorry if I get it wrong m still new to this technology and also would it affect any other logic ?
Just override it in the mutation class you wrote.
this will just be used in the mutation, that purpose of getting it was solved bt the function
id = from_global_id(id)
what about when I query data and try to get details. this is my schema
class ShopTypeFilter(django_filters.FilterSet):
class Meta:
model = ShopType
fields = '__all__'
filter_overrides = {
models.ImageField :{
'filter_class' : django_filters.CharFilter,
'extra' : lambda f:{
'lookup_expr': 'icontains'
}
}
}
class ShopTypeNode(DjangoObjectType):
class Meta:
model = ShopType
interfaces = (graphene.relay.Node , )
class RelayQuery(graphene.ObjectType):
node = node = graphene.relay.Node.Field()
all_shops = DjangoFilterConnectionField(ShopNode , filterset_class=ShopFilter)
shop = graphene.relay.Node.Field(ShopNode)
i need to return the pk here also, i heard something about creating a customnode but its confusing and there is no proper documentation for it
I think there 2 issues here which are causing confusion:
DjangoFilterConnectionField
returns Relay IDs by defaultThe documentation doesn't do a good job of differentiating what is a relay only concept and what is not the code often defaults to assumption that you are using relay which is often not the case. The connection pattern (edges, nodes etc) is a relay concept that has been adopted by other graphql client libraries because it allows efficient filtering and pagination of long lists. The Relay Global ID is specific to Relay only and (as far as I know) hasn't been adopted by any other graphql client library and is often very confusing to people who are not using Relay.
The fact that Graphene-Django's API defaults to relay in some places and not others (this issue being a good example) is not great and we should move to default to non relay specific APIs with relay being support being an option.
In this particular use case the support is there to not return Relay IDs from DjangoFilterConnectionField
. You can do it by changing the ShopTypeNode
type to not implement the relay.Node
interface but instead mark it as a plain connection:
class ShopTypeNode(DjangoObjectType):
class Meta:
model = ShopType
use_connection = True
This option is not documented and IMO is not very intuitive. I find it confusing that you would "enable the connection" on the ObjectType and I think it makes more sense for you to define a wrapper Connection type like you would do it in graphene: https://docs.graphene-python.org/en/latest/relay/connection/ . For example:
class ShopTypeNode(DjangoObjectType):
class Meta:
model = ShopType
class ShopTypeConnection(relay.Connection):
class Meta:
node = ShopTypeNode
class Query(ObjectType):
all_shops = DjangoFilterConnectionField(ShopTypeConnection, filterset_class=ShopFilter)
It's more verbose but I think it makes a lot more sense. Feedback welcome.
Additionally I think we should try and add filter support to the plain DjangoListField
because I don't think you should have to use the connection pattern if you don't want to.
DjangoModelFormMutation
Again, because the Graphene-Django API is inconsistent it's quite easy to assume that the mutation classes would accept Relay IDs because that is what is being returned by the connection fields. However that is not the case as @ulgens points out. In my opinion it would be better to make relay specific versions of the mutation classes (or at least document how to accept Relay IDs) rather than try and figure out if the ID is a Relay ID or not. Not sure exactly what the API should look like. @ulgens any ideas?
@jkimbo the first solution DjangoConnectionField DjangoConnectionField only accepts DjangoObjectType types
class UpdateShop(graphene.relay.ClientIDMutation):
shop = graphene.Field(ShopNode)
class Input:
id = graphene.String()
name = graphene.String()
about = graphene.String()
owner = graphene.String()
phone_no = graphene.String()
time_open = graphene.types.datetime.Time()
delivery = graphene.Boolean()
in_campus = graphene.Boolean()
stars = graphene.Float()
time_close = graphene.types.datetime.Time()
paytm_id = graphene.String()
location = graphene.String()
@classmethod
def mutate_and_get_payload(cls,root, info, **input):
id = input.get('id')
id = from_global_id(id)
id = id[1]
name = input.get('name')
about = input.get('about')
delivery = input.get('delivery')
in_campus = input.get('in_campus')
type = input.get('type')
stars = input.get('stars')
owner = input.get('owner')
phone_no = input.get('phone_no')
time_open = input.get('time_open')
time_close = input.get('time_close')
paytm_id = input.get('paytm_id')
location = input.get('location')
shop = Shop.objects.get(pk=id)
if name:
shop.name = name
if about:
shop.about = about
if delivery:
shop.delivery = delivery
if in_campus:
shop.in_campus = in_campus
if stars:
shop.stars = stars
if type:
shop.type = type
if owner:
shop.owner = owner
if phone_no:
shop.phone_no = phone_no
if time_open:
shop.time_open = time_open
if time_close:
shop.time_close = time_close
if paytm_id:
shop.paytm_id = paytm_id
if location:
shop.location = location
shop.save()
return UpdateShop(shop=shop)
This is my current code for updating a item. I know it's not the perfect but see at the starting of the mutateget payload method. This works, but I am not happy with the implementation
@jkimbo What about creating a new issue to discuss relay - non-relay stuff? I think it's doable and i have some ideas, but not sure when / where to implement them. v3 is a good candidate, but it's too late for that version imo, maybe with 3.1.
We had to subclass SerializerMutation to use the client mutation id. It would be nice if that was an option on serializer mutation. I would gladly share our implementation if anyone is interested.
Used from_global_id only it added one step but I don't thik so it would be that bad
@jottenlips would love to see your implementation also
This is our implementation. credit to @kevinharvey
from collections import OrderedDict
import graphene
from graphene_django.types import ErrorType
from graphene.types.objecttype import yank_fields_from_attrs
from graphene.types import Field, InputField
from graphene_django.registry import get_global_registry
from graphene_django.rest_framework.mutation import SerializerMutation, SerializerMutationOptions
from graphene_django.rest_framework.serializer_converter import (
get_graphene_type_from_serializer_field,
convert_serializer_to_input_type
)
from graphql_relay.node.node import from_global_id
from rest_framework import serializers
from django.shortcuts import get_object_or_404
def fohboh_convert_serializer_field(field, is_input=True, convert_choices_to_enum=True):
"""
Converts a django rest frameworks field to a graphql field
and marks the field as required if we are creating an input type
and the field itself is required
"""
if isinstance(field, serializers.ChoiceField) and not convert_choices_to_enum:
graphql_type = graphene.String
elif field.label == 'ID' and is_input:
graphql_type = graphene.ID
elif isinstance(field, serializers.PrimaryKeyRelatedField):
graphql_type = graphene.ID
else:
graphql_type = get_graphene_type_from_serializer_field(field)
args = []
kwargs = {"description": field.help_text,
"required": is_input and field.required}
# if it is a tuple or a list it means that we are returning
# the graphql type and the child type
if isinstance(graphql_type, (list, tuple)):
kwargs["of_type"] = graphql_type[1]
graphql_type = graphql_type[0]
if isinstance(field, serializers.ModelSerializer):
if is_input:
graphql_type = convert_serializer_to_input_type(field.__class__)
else:
global_registry = get_global_registry()
field_model = field.Meta.model
args = [global_registry.get_type_for_model(field_model)]
elif isinstance(field, serializers.ListSerializer):
field = field.child
if is_input:
kwargs["of_type"] = convert_serializer_to_input_type(
field.__class__)
else:
del kwargs["of_type"]
global_registry = get_global_registry()
field_model = field.Meta.model
args = [global_registry.get_type_for_model(field_model)]
return graphql_type(*args, **kwargs)
def fohboh_fields_for_serializer(
serializer,
only_fields,
exclude_fields,
is_input=False,
convert_choices_to_enum=True,
):
fields = OrderedDict()
for name, field in serializer.fields.items():
is_not_in_only = only_fields and name not in only_fields
is_excluded = any(
[
name in exclude_fields,
field.write_only
and not is_input, # don't show write_only fields in Query
field.read_only and is_input # don't show read_only fields in Input
# ALLOW FOR ID-BASED PARTIAL UPDATE
and name not in ['id', 'pk'],
]
)
if is_not_in_only or is_excluded:
continue
fields[name] = fohboh_convert_serializer_field(
field, is_input=is_input, convert_choices_to_enum=convert_choices_to_enum
)
return fields
class FohbohSerializerMutation(SerializerMutation):
"""
Copied almost entirely from graphen_django.rest_framework.mutations, with the fix described
in https://github.com/graphql-python/graphene-django/issues/906
"""
class Meta:
abstract = True
errors = graphene.List(
ErrorType, description="May contain more than one error for same field."
)
global_id_fields = []
@classmethod
def __init_subclass_with_meta__(
cls,
lookup_field=None,
serializer_class=None,
model_class=None,
model_operations=("create", "update"),
only_fields=(),
exclude_fields=(),
convert_choices_to_enum=True,
**options
):
if not serializer_class:
raise Exception(
"serializer_class is required for the SerializerMutation")
if "update" not in model_operations and "create" not in model_operations:
raise Exception(
'model_operations must contain "create" and/or "update"')
serializer = serializer_class()
if model_class is None:
serializer_meta = getattr(serializer_class, "Meta", None)
if serializer_meta:
model_class = getattr(serializer_meta, "model", None)
if lookup_field is None and model_class:
lookup_field = model_class._meta.pk.name
input_fields = fohboh_fields_for_serializer(
serializer,
only_fields,
exclude_fields,
is_input=True,
convert_choices_to_enum=convert_choices_to_enum,
)
output_fields = fohboh_fields_for_serializer(
serializer,
only_fields,
exclude_fields,
is_input=False,
convert_choices_to_enum=convert_choices_to_enum,
)
_meta = SerializerMutationOptions(cls)
_meta.lookup_field = lookup_field
_meta.model_operations = model_operations
_meta.serializer_class = serializer_class
_meta.model_class = model_class
_meta.fields = yank_fields_from_attrs(output_fields, _as=Field)
input_fields = yank_fields_from_attrs(input_fields, _as=InputField)
super(SerializerMutation, cls).__init_subclass_with_meta__(
_meta=_meta, input_fields=input_fields, **options
)
@classmethod
def get_serializer_kwargs(cls, root, info, **input):
"""
Customized to allow update using the object's global ID
"""
lookup_field = cls._meta.lookup_field
model_class = cls._meta.model_class
for field_name, field_input in input.items():
if field_name in cls.global_id_fields:
_, row_id = from_global_id(field_input)
input[field_name] = int(row_id)
if model_class:
if "update" in cls._meta.model_operations and lookup_field in input:
lookup_kwargs = {
'klass': model_class,
lookup_field: input[lookup_field]
}
if lookup_field in ['id', 'pk'] and type(input[lookup_field]) == type(''):
_, object_pk = from_global_id(input[lookup_field])
lookup_kwargs[lookup_field] = object_pk
instance = get_object_or_404(**lookup_kwargs)
partial = True
elif "create" in cls._meta.model_operations:
instance = None
partial = False
else:
raise Exception(
'Invalid update operation. Input parameter "{}" required.'.format(
lookup_field
)
)
return {
"instance": instance,
"data": input,
"context": {"request": info.context},
"partial": partial,
}
return {"data": input, "context": {"request": info.context}}
@classmethod
def mutate_and_get_payload(cls, root, info, **input):
kwargs = cls.get_serializer_kwargs(root, info, **input)
serializer = cls._meta.serializer_class(**kwargs)
if serializer.is_valid():
return cls.perform_mutate(serializer, info)
else:
errors = ErrorType.from_errors(serializer.errors)
return cls(errors=errors)
@classmethod
def perform_mutate(cls, serializer, info):
obj = serializer.save()
kwargs = {}
for f, field in serializer.fields.items():
if not field.write_only:
if isinstance(field, serializers.SerializerMethodField):
kwargs[f] = field.to_representation(obj)
else:
kwargs[f] = field.get_attribute(obj)
kwargs[cls.output_field] = obj
return cls(errors=None, **kwargs)
Would it make sense to allow global ids to be optional? Could we define the behavior as a DjangoObjectType meta var?
Would it make sense to allow global ids to be optional? Could we define the behavior as a DjangoObjectType meta var?
I guess you could make a custom node and - have your configuration there
I have actually just submitted 2 PRs (for v2 and v3) for that on graphene: https://github.com/graphql-python/graphene/issues/1276
This is my implementation
# relay.py
from functools import partial
from graphene.types import Interface, Field
from graphene.types.interface import InterfaceOptions
from graphene.relay.node import GlobalID
from graphene.relay.id_type import BaseGlobalIDType, SimpleGlobalIDType
from graphene.types.utils import get_type
class NodeField(Field):
def __init__(self, node, type_=False, **kwargs):
assert issubclass(node, Node), "NodeField can only operate in Nodes"
self.node_type = node
self.field_type = type_
global_id_type = node._meta.global_id_type
super(NodeField, self).__init__(
# If we don't specify a type, the field type will be the node interface
type_ or node,
print(type_, node),
id=global_id_type.graphene_type(
required=True, description="The ID of the object"
),
**kwargs,
)
def wrap_resolve(self, parent_resolver):
return partial(self.node_type.node_resolver, get_type(self.field_type))
class AbstractNode(Interface):
class Meta:
abstract = True
@classmethod
def __init_subclass_with_meta__(cls, global_id_type=SimpleGlobalIDType, **options):
assert issubclass(
global_id_type, BaseGlobalIDType
), "Custom ID type need to be implemented as a subclass of BaseGlobalIDType."
_meta = InterfaceOptions(cls)
_meta.global_id_type = global_id_type
_meta.fields = {
"id": GlobalID(
cls, global_id_type=global_id_type, description="The ID of the object"
)
}
super(AbstractNode, cls).__init_subclass_with_meta__(_meta=_meta, **options)
@classmethod
def resolve_global_id(cls, info, global_id):
return cls._meta.global_id_type.resolve_global_id(info, global_id)
class Node(AbstractNode):
"""An object with an ID"""
@classmethod
def Field(cls, *args, **kwargs): # noqa: N802
return NodeField(cls, *args, **kwargs)
@classmethod
def node_resolver(cls, only_type, root, info, id):
return cls.get_node_from_global_id(info, id, only_type=only_type)
@classmethod
def get_node_from_global_id(cls, info, global_id, only_type=None):
_type, _id = cls.resolve_global_id(info, global_id)
graphene_type = info.schema.get_type(_type)
if graphene_type is None:
raise Exception(f'Relay Node "{_type}" not found in schema')
graphene_type = graphene_type.graphene_type
if only_type:
assert (
graphene_type == only_type
), f"Must receive a {only_type._meta.name} id."
# We make sure the ObjectType implements the "Node" interface
if cls not in graphene_type._meta.interfaces:
raise Exception(
f'ObjectType "{_type}" does not implement the "{cls}" interface.'
)
get_node = getattr(graphene_type, "get_node", None)
if get_node:
return get_node(info, _id)
@classmethod
def to_global_id(cls, type_, id):
return cls._meta.global_id_type.to_global_id(type_, id)
# schema.py
from .relay import Node
class ModelNode(DjangoObjectType):
class Meta:
model = Model
interfaces = (Node)
filter_fields = {
...
}
use_connection = True
So basically I am trying to query through the objects using relay. when I fetch the id it returns the relay global id.
output
it works just fine , even filtering and stuff. But the problem occurs in mutation i cant pass the relay id while updating or referencing an entry in django. I want the object id there.
A workaround I tried Use this function
its in graphql_relay.node.node but the problem is it increases a lot of my code and everywhere I want i have to use this and I have a lot of code to manage already.
*Is there any way to globally replace relay global id with model id,(i am using Django's UUID field) keeping two ids is redundant code