Add support for custom global ID in v2 (Issue #1276)

[tcleonard]

Solution for issue #1276 for graphene v2

[jkimbo]

@tcleonard thank you for the PR but I'm not sure I fully understand why this change is necessary? Why can't you created a custom Node type to achieve the same result?

[jkimbo]

I don't think this will work if the return type is an interface or a union. I think it will return the interface/union type rather than the underling result type.

[tcleonard]

you are probably correct... I will add a unit test for that and try to figure out a solution.
Let me know if you have an idea to suggest

[tcleonard]

You can't have a Union be a node so I don't think the union is really something we need to consider (unless I am missing your point), I'm looking into interfaces

[tcleonard]

Actually same comment about interfaces... I don't really see why an interface would be a Node... it's the object type that you would derive from the interface that you would make a node field.
Did you have a specific case in mind? Cause I can't write a unit test that makes sense to test that...

[tcleonard]

@tcleonard thank you for the PR but I'm not sure I fully understand why this change is necessary? Why can't you created a custom Node type to achieve the same result?

You can't simply create a custom node without completely re-implementing everything because the type ID is hard-coded in a few places (typically GlobalID class forces the type ID.. so even if you override it in your Node you actually get some consistency issues because you can't control what GlobalID does).

If you don't change the underlying graphene type (like in SimpleGlobalIDType for example) then I agree that it's easy to do it by simply having a custom node. But in the example of the UUID type of id then it's more complicated... (same thing for the example of an Int type in the unit test)

[jkimbo]

@tcleonard ok I see your point but I think this would be better solved by allowing you to specify what the id field should be rather than extending the GlobalID type. It could look something like this:

def test_custom_id_field():
    class CustomIntNode(Node):
        class Meta:
            name = "Node"
            id_type = Int(required=True, description="The ID of the object")

        @staticmethod
        def to_global_id(type_, id):
            return id

        @staticmethod
        def get_node_from_global_id(info, id, only_type=None):
            return User(id=1, name="User 1",)

    class User(ObjectType):
        class Meta:
            interfaces = [CustomIntNode]

        name = String(description="The full name of the user")

    class RootQuery(ObjectType):
        node = CustomIntNode.Field(id_type=Int(required=True))

    schema = Schema(query=RootQuery, types=[User])

    assert str(schema) == dedent(
        '''
        schema {
          query: RootQuery
        }

        type User implements Node {
          """The ID of the object"""
          id: Int!

          """The full name of the user"""
          name: String
        }

        interface Node {
          """The ID of the object"""
          id: Int!
        }

        type RootQuery {
          node(id: Int!): Node
        }
        '''
    )

    query = """
      {
        node(id: 1) {
          id
        }
      }
    """
    result = schema.execute(query)
    assert not result.errors
    assert result.data == {"node": {"id": 1}}

(id_type is maybe not the best name for the option)

Does that make sense?

BTW there are some examples of creating a custom Node type here: https://github.com/graphql-python/graphene/blob/master/graphene/relay/tests/test_node_custom.py

[tcleonard]

This is basically what I have done (as you can see in the unit test TestCustomGlobalID that I added)... except that I provided an interface to implement that (and also implemented a few common patterns)

Why I did it that way is because, in the current implementation of graphene there is a strong assumption not only on the ID type but its content as well.

Indeed, if I simply declare a custom node:

    class CustomIntNode(Node):
        class Meta:
            name = "Node"
            id_type = Int(required=True, description="The ID of the object")

It just doesn't work... because the implementation of the to/from global ID methods are also working on the assumption (implicitly) that your ID is of ID! type and contains the type of the id in base64.

So that was the idea behind my approach to actually provide a way to offer this option but with the framework around it that tells you what you need to implement (so not just the id type but also the 2 methods needed to make that new ID type work) aka this interface:

class BaseGlobalIDType:
    """
    Base class that define the required attributes/method for a type.
    """

    graphene_type = ID  # type: Type[BaseType]

    @classmethod
    def resolve_global_id(cls, info, global_id):
        # return _type, _id
        raise NotImplementedError

    @classmethod
    def to_global_id(cls, _type, _id):
        # return _id
        raise NotImplementedError

Because I find it quite disturbing to offer an option like you suggest that doesn't tell you that if you just use it as is... it does not work and you have to know somehow what else you need to do.
While in my set up, if you only set the graphene_type you get some NotImplementedError which will explicitly tell you what you are missing.

Do you see my point?

[apelliciari]

Hi @tcleonard @jkimbo this is a very interesting change, if we can use our IDs instead of Relays.
May we think of releasing it if you are fine with the implementation?
Unfortunately I cannot contribute to the discussion because I don't grasp the internals very well but this is an important change, so let's push it asap, thanks!

[tcleonard]

Any update on this?... seeing how many questions are asked on stack overflow and in issues comments I am wondering if I could get some feedback. Happy to do some changes but at that point this PR is just going stale...

[tcleonard]

@jkimbo is there any chance to get some feedback on that? or could you point it towards an active maintainer?