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

Natural vs opaque surrogate keys for defining associations #42

Closed
artem-barmin opened this issue May 8, 2016 · 13 comments
Closed

Natural vs opaque surrogate keys for defining associations #42

artem-barmin opened this issue May 8, 2016 · 13 comments

Comments

@artem-barmin
Copy link
Contributor

For some applications have separate ID and rowId fields increase complexity a lot(I am porting ng-admin to use postgraphql generated endpoint).

As I understand - main reason for using special ID type is to provide global unique object identifier across all entities/tables. This requirement also can be solved by usage of UUID as primary key for tables.

That's why I would like to see parameter(for example : --uuid-based-schema) that will remove rowId and will use ordinary id as key for graphql queries.

I can work on this feature and would like to hear any thoughts/concerns before designing it.

@calebmer
Copy link
Collaborator

calebmer commented May 8, 2016

This doesn't sound like a bad idea. What I'd recommend is detecting if the primary key column has a type of uuid and then switch to using solely the id column. This way we don't need a flag that applies to the entire schema.

A couple of constraints to keep in mind:

  1. There must only be one primary key column.
  2. That column must have the type uuid.
  3. The uuid must be usable alongside the generated ids in the node field.

@artem-barmin
Copy link
Contributor Author

Automatic detection can produce inconsistency in query schema(some entities will work with ID/rowId, some will work with UUID).

I am not sure if we should silently select identification scheme based only on schema. I would otherwise require specific switch to turn on auto-detection mode.

So I would suggest to create special UUID mode with parameter, which will turn on autodetection you suggested. Also extra information will be printed in log(about table-scheme mode).

@artem-barmin
Copy link
Contributor Author

Another question(probably related to topic) : why do we need Node(id:ID) query?

Without it:

  • schema validation passing without errors
  • relations extracted

@calebmer
Copy link
Collaborator

calebmer commented May 8, 2016

The Relay object identification specification requires it to retrieve nodes back.

@calebmer
Copy link
Collaborator

calebmer commented May 8, 2016

I'm not afraid of automatic detection because id is supposed to be opaque and non-human readable. I'm also not too excited about adding the feature behind a flag because it starts to add unnesecary complexity for a feature that isn't insanely common.

Another complexity is the current id format contains information about the table so that it can be reselected according to the Relay specification using the node field, but a UUID does not contain any information about the table it comes from.

@artem-barmin
Copy link
Contributor Author

artem-barmin commented May 8, 2016

About automatic detection - ok, let's do it this way.

node is required by spec, but does it used internaly during queries? Or it's added just for better user experience? For example : https://sandbox.learngraphql.com/ does not provide node query in RootScheme

@artem-barmin
Copy link
Contributor Author

artem-barmin commented May 8, 2016

Why we need node query : http://stackoverflow.com/questions/33399901/in-relay-what-role-do-the-node-interface-and-the-global-id-spec-play.

This interface used during refetching and partial fetching of extra props.

I have better idea how to get rid of rowId:

  1. Currently API provide mix between ID and rowId in different queries. For query by id we should use ID. In list queries for associations we should use rowId values.
  2. During schema generation we know all references, and can automatically transform arguments for queries and mutations.

So mutation :

mutation {
  insertPost(input:{author:1, content: "text"})
}

where 1 - is rowId of author. Should become:

mutation {
  insertPost(input:{author:"ZmlsbXM6MQ==", content: "text"})
}

and author field will be converted to actual database identifier in resolve function. The same principle should be applied to query.

query {listPosts(author:1)} turns into query {listPosts(author:"ZmlsbXM6MQ==")}

That approach should remove inconsistency between ID and rowId, and will allow us to keep node query.

UUID question became not important in case of implementation of such scheme.

@calebmer
Copy link
Collaborator

calebmer commented May 9, 2016

@artem-barim yep, consistency between these two ID schemes is super important. The blocker that I've run into before is compound keys. We can't easily generate a name for that single ID field when a table has two keys (well personAuthorId could work).

A solution using the constraint name is plausible as is explored in #34.

@artem-barmin
Copy link
Contributor Author

For me, main question here : what is the best way to define associations?

If scheme design is based on surrogate keys, we can force user to use opaque ID type for building associations. In this case each composite foreign reference can be replaced by opaque column. ex: foreign key (language::string,state_code::string) can be replaced by parentTable::ID in all mutations and queries.

In case of natural keys user probably wants to build associations using specific values(ex: country code, language code) and forcing him to use opaque identifier may break usage scenarios. So he should be able to use specific column values for building association.

As for composite key naming - constraint name is good option.

@calebmer
Copy link
Collaborator

Yeah, I agree this is a tough decision. Maybe union types are a good space to explore? So for your example a GraphQL ID could be: ID | { language: String, stateCode: String }.

@calebmer
Copy link
Collaborator

Since this issue has evolved into a new conversation could you rename the title or open a more specific issue?

@artem-barmin artem-barmin changed the title UUID as primary key and ID type Natural vs opaque surrogate keys for defining associations May 12, 2016
@artem-barmin
Copy link
Contributor Author

To summarize:

  • keys can be composite, in this case we fold them into opaque ID type, and later talk about them in terms of opaque types
  • check every foreign key constraint
    • if it contains same fields as target table primary key - turn it into separate field with type ID | {all fields from primary key}
    • if it differs from primary key of target table - keep as is
  • primary keys turned into ID, original fields keep as is(except id which is renamed to rowId - but I am not sure, do we need this?)
  • all mutations will contains foreign keys with type ID - which makes associations definition easy. They automaticaly decoded into appropriate column values on the backend.

(sorry: I can't imagine real world example with composite primary key, only table with many-to-many association, but in general algorithm can handle them)

Sample pg schema:

create table person(login text primary key,pass text);
create table role(name text primary key,description text);
create table person_role(person_login text references person(login),role_name text references role(name),primary key(person_login,role_name));

Will produce following graphql schema:

type Person 
{
  id:ID, // base64 from person:login_value
  login:string,
  pass:string
}

type Role 
{
  id: ID, //base64 from role:name_value
  name:string,
  description:string
}

type PersonRole
{
  id: ID,
  personFkey:ID, //assume that reference have name 'personFkey'
  roleFkey:ID,
  personLogin:string, // useless fields, but we still keep them
  roleName:string
}

Mutations will be following

insertPersonRole(input:{personFkey:ID!,roleFkey:ID!,personLogin:string,roleName:string}) // we can define association both with ID's and natural keys, maybe throw exception in case of mismatch
insertRole(input:{login:String,password:String})

Such scheme gives as ability to build associations with opaque ID's, still keeping natural keys available.

@calebmer
Copy link
Collaborator

I’m going to close this as its in the PostGraphQL 2.0 beta 🎉

Start playing with the pre-release, the final release should be out really soon. Tell me what you think! 👍

Specifically, id was renamed to __id allowing rowId to be properly called id. Also you can create, read, update, and delete using the global id (now named __id), the primary key, or any unique constraints. Thanks for your help @artem-barmin 👍

npm install -g postgraphql@next
postgraphql

benjie added a commit that referenced this issue Jan 27, 2020
* Add uppercase enums to reproduce issue

* Fix enum capitalisation issues

* Prefix 0_CONSTANT with _

* Lint

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

No branches or pull requests

3 participants