Rename __id to nodeId

[calebmer]

In the beginning of PostGraphQL globally unique identifiers used the field name id. This was undesirable for a very obvious reason. Most peoples’ Postgres schemas had a column named id! So we would rename Postgres id columns to rowId. While this worked and allowed PostGraphQL schemas to support Relay it was suboptimal for many users.

So I opened facebook/relay#1061 on the Relay repository asking for a loosening of the object identification spec. There was a lot of interest in this issue and eventually, a Relay core contributor mentioned that internally there were people at Facebook talking about a proposal which would add a __id field with the same semantics as the id field in Relay 1. Standardizing around a __id field would have many benefits. The first benefit to PostGraphQL would be we wouldn’t have to rename id to rowId. The more exciting benefit (to me) is that by standardizing an object identification mechanism in GraphQL then client tools and server implementors would have common ground to work off of in building advanced data caching, consistency, and fetching tools.

After some encouragement from Facebook employees, one of the core developers of Apollo Client opened RFC graphql/graphql-spec#232 against the GraphQL specification. PostGraphQL and other GraphQL developers pre-emptively adopted the specification after hearing it had support from Facebook from both the GraphQL team and the Relay team.

However, as time went on it became apparent that the RFC wasn’t a slam dunk like everyone outside of Facebook thought and eventually the RFC was closed with the decision that more discussion was necessary.

Getting the RFC rejected wouldn’t really be an issue. The community could still standardize around __id even if it was not in the spec, and even if the RFC wasn’t going to get merged the name __id was still appropriate for PostGraphQL and its users. What was really frustrating was a combination of graphql/graphql-spec#244 and graphql/graphql-js#600 which completely banned the use of __ for non-introspection purposes. The change makes sense as it preserves that space for future use by the GraphQL specification, but it was disappointing from a community perspective because now the APIs of progressive GraphQL developers were now considered invalid by the spec. PostGraphQL APIs are one such example 😖

Unfortunately, by adopting __id in PostGraphQL it did not make the inclusion of __id in the GraphQL spec more likely which, in my opinion, is unfortunate for the entire GraphQL community and not just PostGraphQL as we have reverted to the point where no one can agree on how to properly identify data in their APIs.

---

This PR removes the use of __id from PostGraphQL and chooses to use nodeId instead. The previous behavior may, of course, be obtained using the --classic-ids flag. This makes sense as node is already a reserved name so calling the field nodeId is a logical extension of that reservation. In the future we may want to consider the “Node” nomenclature entirely, perhaps using _Node, _Identifiable, or _Cacheable for the interface name and _nodeId, _cacheKey, or something else for the field name. For now, this change makes the most sense for PostGraphQL, and we will wait to see where the GraphQL community moves on identification before making another big change like this.

This is, of course, a breaking change and will be released in 3.x.

[ferdinandsalis]

Looks good to me.

[benjie]

I've changed the base branch from 'master' to 'next' to defend against premature merging of this (since it's v3).

[benjie]

Another +1 and a sharing of frustrations

👍 😖

[martinheidegger]

I understand the frustrations but I think this might be the wrong approach. The problem of id was that it was a primary identifier for a table that works for postgres queries but the GraphQL approach does required global unique id. With nodeId (and __id for that matter) the Problem is just shifted until another someone has an own nodeId or __id table. It would be possible to take the a different approach: using the name of the private-key PostgraphQL could replace the type with UUID that can be used with any reference, it would turn the API from something like:

someType(__id: ID!) {
    primKey: int,
    ___id: ID!
}

to

someType(primKey: ID!) {
   primKey: ID!
}

No conflicts no, future problems and significantly more beautiful

[calebmer]

@martinheidegger I definitely agree that it is much more beautiful, but an issue appears when a user has a compound primary key. Like this table. Compound primary keys turn out to be a common case for “edge” tables if you will. Like post_like or person_follower.

There is also the issue of how to convert the types in related tables. So does a personID column become of type ID!? How then do we convert types in compound keys?

Also, there was a point in PostGraphQL’s history where we did not have fields like postById(id: Int!) and instead only had the field post(id: ID!). The same went for mutations. A couple of issues were open requesting a way to select using the integer row id of a type, and a similar thing happened for mutations.

The way I look at it, PostGraphQL should put very few restrictions on what you can do with your Postgres database through the GraphQL interface. This gives the developer maximum power over their data.

That said, I am definitely open to discussing what a more constrained and opinionated interface would look like, and the methodologies to allow users to create that tighter interface.

This is part of my vision for PostGraphQL which reflects itself in how the code is currently architected. We should be able to support multiple different GraphQL API designs simply by cloning the code in the ./src/graphql folder and making some opinionated changes. If you want to start talking about the designs for a module system that would make this possible, I would definitely be interested 👍

[martinheidegger]

Compound keys are indeed an issue (that my Postgraphql-beginner-brain didn't think of) and after the consideration I came to understand this whole issue runs into philosophical issues between GraphQL and SQL:

SQL: Keys (and compound keys) are human readable identifiers, unique to a table, that can help to make more sense of a content and are possible to be changed for a row.
GraphQL: IDs are immutable and global and contain no information except for identifying a row.

Naturally this results into issues such as:

requesting a way to select using the integer row id of a type

As I can understand there are two groups of people interesting in using postgraphql

• People that think PostgraphQL is a way to get graphql.
  Those people put the usability of graphql at the top. This includes relay interoperability and global access, etc.
• People that think PostgraphQL is a way to deal with postgres.
  Those people have or want postgres databases and want to use as many features of it as possible. They do not really care about complete graphql interoperability but rather about being able to use and take care of all their data.

I am not sure it is possible to support both groups equally in usability and at some point (the point of this PR) one path becomes more important.

As such I don't think changing __id to nodeId doesn't really fix the problem and is only cosmetic and also: Some might be needing support for __id as they have been using PostgraphQL with this option: So: changing the option classic-ids to ids=classic or ids=__ might be more sensible.

but looking at it from a different angle:

What if not every type generated by PostgraphQL actually implements Node?

Not every table can be addressed by an unique id and attempting to do so seems to ultimately always result in hacks. But Postgres supports UUID. Which means that if a user wants to support "full graphql" she needs to actually use a functional UUID. And unless that is given, Node will not be implemented in the type. If by other means immutability and uniqueness can be guaranteed then Node could be implemented as well (I.e. a primary key that is autoincrementing)

Doesn't that sound like a cleaner way to go?

[benjie]

To be clear: using __id does not work with the latest graphql-js; so we had to change it to something. We've elected for that thing to be nodeId since we need to get a fix out quickly or start lagging behind graphql-js which is undesirable.

---

@calebmer I use Alloy's Relay fork to get support for __id in Relay; do you know if they intend to move to something like nodeId like us? AFAICT only the __id branch exists, and there's no issues through which to ask the question.

[benjie]

I don't think that UUID in itself is sufficient because given a UUID we don't know to which table/collection it belongs - we'd need a lookup table or similar to figure it out. Further you also need IDs on the edges, so those would have to be derived from the UUIDs making them quite long (as would compound keys).

[martinheidegger]

we'd need a lookup table or similar to figure it out.

Simply wrap the UUID+table (base64 obfuscated?) like it is done atm. when interaction through graphql?!

... as would compound keys)

I am not sure compound keys could necessarily qualify as general purpose Id's right? Having a compound key with a UUID seems like using UUID wrong.

[calebmer]

@martinheidegger

As such I don't think changing __id to nodeId doesn't really fix the problem and is only cosmetic

I wish it was only cosmetic 😣. As @benjie pointed out, the specification now reads:

“All types and directives defined within a schema must not have a name which begins with "__" (two underscores), as this is used exclusively by GraphQL’s introspection system.”

(source)

This was not the case when we chose to use the name __id, so now this PR is to fix our spec compliance. I’m definitely interested in discussing some of the points you raise, but maybe this isn’t the right place?

---

@benjie

Do you know if they intend to move to something like nodeId like us?

I don’t think they ever intended to switch to __id (unless the RFC got merged), so I’d guess that’s a no on this one too 😣

They closed my initial issue which confirms this for me facebook/relay#1061

[martinheidegger]

Note that __id became a warning: graphql/graphql-js#692

[martinheidegger]

Alright, I am sorry if this was the wrong place, considering that it is merely a warning at the moment I would keep the code as-is and fix it "thoroughly" in a follow-up PR that tries to avoid both the incompatibility with Postgres and with the GraphQL API.

[calebmer]

Ok, cool. I’ll merge this soon with #328 and release PostGraphQL 3.0.