Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Global identification #946

Open
glen-84 opened this issue May 15, 2022 · 10 comments
Open

Global identification #946

glen-84 opened this issue May 15, 2022 · 10 comments

Comments

@glen-84
Copy link

glen-84 commented May 15, 2022

As discussed in #232, __id doesn't really make sense if it actually returns the global ID, since double-underscore fields are usually used for type information. But what if it did return type information (the name of the global ID field), and that field was defined using a directive?

The original idea comes from @leebyron and @calebmer here.

Example

Specifying a global identifier field

type Person {
    id: ID! # Entity/local ID.
    myGlobalId: ID! @globalIdField # This could instead be added to a `Node` interface, that `Person` implements.
    name: String
}

Selection

{
    person {
        __globalIdField
        myGlobalId
        id
        name
    }
}
{
    "data": {
        "person": {
            "__globalIdField": "myGlobalId",
            "myGlobalId": "person-123",
            "id": "123",
            "name": "Jane Doe",
        }
    }
}

Introspection

{
    __type(name: "Person") {
      name
      globalIdField
    }
}
{
    "data": {
        "__type": {
            "name": "Person",
            "globalIdField": "myGlobalId"
        }
    }
}

Something like @idField could perhaps also be added for non-global IDs. Built-in directives may need some form of namespacing – not sure if something like @__idField would look good, but there may be other alternatives like @@idField).


Could something like this work?

@rivantsov
Copy link
Contributor

rivantsov commented May 15, 2022

I believe this stuff belongs to Relay or some other spec, not GraphQL.

In real world models in general, having ID is not a universal thing, that each object has. There might be no id field/prop/concept at all, a type/table might have composite key (id composed of multiple fields), and Id field type might vary quite a lot (int? UUID? string), and level of uniqueness might vary as well - nothing to standardize. So ID is not that universal and common, to be standardized in this spec.

If existence of ID was necessary for some other feature in spec - that would give it some reason to be (like I guess in Relay); but that's not the situation here.

@glen-84
Copy link
Author

glen-84 commented May 15, 2022

I believe this stuff belongs to Relay or some other spec, not GraphQL.

There's no way to access directive metadata from the client (#300).

@benjie
Copy link
Member

benjie commented May 16, 2022

@glen-84 My main concern with this is that you'd need to query __globalIdField to know which field it is that you then have to query - so it's not guaranteed that the relevant field will have been queried. What if, instead, the __id field effectively "delegated" to the underlying identifier field (which we should require is of type ID, and thus effectively a string)?

Either way, we still need to solve Lee's nullability concerns which seem valid, though I wonder if they're actually that big of a deal in practice... seems like if __id comes back null then that node simply doesn't have an identity / isn't cacheable.

@glen-84
Copy link
Author

glen-84 commented May 17, 2022

My main concern with this is that you'd need to query __globalIdField to know which field it is that you then have to query

That's true. If it were only on the schema, it might be easier, since you'd only have to check that once. If there's only one globalIdField for the whole schema, then it's also easier to know ahead of time (without introspection).

schema @options(globalIdField: "myGlobalId") {
    // ...
}

(@options might be named differently, and/or be namespaced)

Could a single option like this work? Are there any implications WRT stitching or federation?

What if, instead, the __id field effectively "delegated" to the underlying identifier field

Ultimately, it would be returning non-type-related data via a reserved field name, which I think needed to be avoided.

Either way, we still need to solve Lee's nullability concerns which seem valid

I didn't really understand the issue with making it nullable. There is surely no flawed assumption if the field is typed as nullable?

@michaelstaib
Copy link
Member

@benjie wasn't there a new push on object identity from the relay team. I remember that Joseph was mentioning this. I could be wrong :)

@benjie
Copy link
Member

benjie commented May 17, 2022

@michaelstaib I think there's been some gentle pushes towards something like this from a lot of corners for quite a while; but yes I think Relay may be keen - having the id field be in the GraphQL Global Object Identification spec certainly makes adoption harder if you already have fields in your schema called "id"!

@glen-84

Ultimately, it would be returning non-type-related data via a reserved field name, which I think needed to be avoided.

I don't think that's a hard rule. __ fields are reserved for GraphQL spec usage, they don't have to be introspection... just at the moment that's all they're used for.

I didn't really understand the issue with making it nullable. There is surely no flawed assumption if the field is typed as nullable?

Think of it like this:

type User implements Node {
  __typename: String!
  __id: ID
  id: ID!
}

It's obvious it has a __typename, and obvious it has an id - both are non-nullable. But __id must be nullable (because all types share it, and not all implement identification) and yet our tooling should know that User will always have an __id.

@glen-84
Copy link
Author

glen-84 commented May 17, 2022

Think of it like this:

I see. With my idea above, the use of @options(globalIdField: "myGlobalId") on the schema could perhaps mandate a non-nullable myGlobalId field on all types within the schema, to avoid this issue?

@benjie
Copy link
Member

benjie commented May 17, 2022

It could; but we wouldn’t want that. E.g. PageInfo should not have an ID, edges should not have IDs, mutation payloads should not have IDs, etc.

@glen-84
Copy link
Author

glen-84 commented May 17, 2022

First-class nodes?

# Opt in.
schema @options(nodeInterface: "Node") { ... }

interface Node {
    # First/only ID field is the global ID, or use a directive, or a schema option.
    myGlobalId: ID!
}

@benjie
Copy link
Member

benjie commented May 18, 2022

Looks like that would benefit from #300

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants