fix(graphql-migrations): set primary keys when a column has @id

[machi1990]

Fixes #1397

Opening as draft to get early feedback but from what I am seeing the @id or its predecessor @db.primary never really worked before.

Here is what I mean, apart from the primary field not being set, which should be fixed by this patch, another issue is when you have a relation like the one we have in https://github.com/aerogear/graphback/blob/24894c385f1ad846312c9382dae5468f98dc1f73/templates/ts-apollo-postgres-backend/model/datamodel.graphql where the migration will fail with the below error:

Foreign field id on type Note not found on field note.
Foreign field id on type Note not found on field note.
(node:136301) UnhandledPromiseRejectionWarning: TypeError: Cannot read property 'name' of undefined
    at /home/manyanda/Documents/Projects/aerogear/graphback/packages/graphql-migrations/dist/abstract/generateAbstractDatabase.js:389:80
    at Array.find (<anonymous>)
    at AbstractDatabaseBuilder.hasColumn (/home/manyanda/Documents/Projects/aerogear/graphback/packages/graphql-migrations/dist/abstract/generateAbstractDatabase.js:389:38)
    at AbstractDatabaseBuilder.createOneToManyRelationship (/home/manyanda/Documents/Projects/aerogear/graphback/packages/graphql-migrations/dist/abstract/generateAbstractDatabase.js:368:19)
    at AbstractDatabaseBuilder.build (/home/manyanda/Documents/Projects/aerogear/graphback/packages/graphql-migrations/dist/abstract/generateAbstractDatabase.js:71:18)
    at Object.<anonymous> (/home/manyanda/Documents/Projects/aerogear/graphback/packages/graphql-migrations/dist/abstract/generateAbstractDatabase.js:40:24)

as it still expects the id to be a primary field. I'd consider that a separate issue and I can investigate further once we are okay with this patch.

/cc @craicoverflow

[wtrocki]

I think our initial design was that PK was never actually applied. When using primary - confusing name - you got sequence for id and if it something else you should get unique only. This is due to the format of the data. You can set primary/id on username and this primary is not considered and primary key - it is more like I'd that will be used for queries.

Also enda question nailed the root of the problem. Incementability is not exacly going to work with any type - especially not numeric one like email etc.

[machi1990]

Hi @wtrocki thanks for chimming in. Really good points you raised here.

... You can set primary/id on username and this primary is not considered and primary key - it is more like I'd that will be used for queries.

So you are saying that we basically should not be setting primary key constraint? What should be done when the @id is used in a field other than a scalar ID?

Also enda question nailed the root of the problem. Incementability is not exacly going to work with any type - especially not numeric one like email etc.

Wel, that's it in this patch I am not increment any other type but only the ID scalar type, other types will be considered primary keys but not incrementable - see this part of the code

graphback/packages/graphql-migrations/src/abstract/generateAbstractDatabase.ts

Line 409 in 845d890

const autoIncrementable = isScalarType(fieldType) && ID_TYPE.default(field.name, fieldType.name);

The only effect that the @id has is to only add the primary key constraint - something this test should show

graphback/packages/graphback-runtime-knex/tests/service/DefaultCRUDServiceTest.ts

Line 134 in 845d890

test('insertion of User with same custom ID field more than once should fail', async () => {

which I believe fails if we do not have such info.

[wtrocki]

ok. so you have it figured out and fix seems to addressing main challenges. Lets have great evening and relax. I will verify this for you tommorow

[machi1990]

ok. so you have it figured out and fix seems to addressing main challenges. Lets have great evening and relax. I will verify this for you tommorow

Yes, this is what I have when I ran the migration with for the model below:

model

""" @model """
type Note {
  """
  @id
  """
  title: String!
  """
  @db.foreign: 'title'
  @oneToMany field: 'note'
  """
  comments: [Comment]!
}

""" @model """
type Comment {
  """
  @id
  """
  text: String
  description: String
  """
  @db.foreign: 'title'
  @ManyToOne field: 'comments'
  """
  note: Note
}

database

users=# \d note
             Table "public.note"
 Column |          Type          | Modifiers 
--------+------------------------+-----------
 title  | character varying(255) | not null
Indexes:
    "note_pkey" PRIMARY KEY, btree (title)
Referenced by:
    TABLE "comment" CONSTRAINT "comment_noteid_foreign" FOREIGN KEY ("noteId") REFERENCES note(title)

users=# \d comment
              Table "public.comment"
   Column    |          Type          | Modifiers 
-------------+------------------------+-----------
 text        | character varying(255) | not null
 description | character varying(255) | 
 noteId      | character varying(255) | 
Indexes:
    "comment_pkey" PRIMARY KEY, btree (text)
Foreign-key constraints:
    "comment_noteid_foreign" FOREIGN KEY ("noteId") REFERENCES note(title)

Now, going back to the PR description:

Here is what I mean, apart from the primary field not being set, which should be fixed by this patch, another issue is when you have a relation like the one we have in https://github.com/aerogear/graphback/blob/24894c385f1ad846312c9382dae5468f98dc1f73/templates/ts-apollo-postgres-backend/model/datamodel.graphql where the migration will fail with the below error:

Foreign field id on type Note not found on field note.
Foreign field id on type Note not found on field note.

The error described here is that we cannot automatically infer a primary key because we always seem to be defaulting to id - https://github.com/aerogear/graphback/blob/master/packages/graphql-migrations/src/abstract/generateAbstractDatabase.ts#L245

So have to explicitly specify foreign key using the @db.foreign as can be seen from the model above, which is fine but I was thinking maybe we should go further (in a followup PR) and infer this stuff since we have all the info to do so.

[craicoverflow]

I've done some quick verification checks.

Graphback allows you to set id: ID and set a different custom ID field with @id. However graphql-migrations tries to set multiple PKs:

"""
@model
"""
type User {
  id: ID!
  """
  @id
  """
  email: String
}

The model above (which would be acceptable in Graphback, throws the following error in migrations:

UnhandledPromiseRejectionWarning: error: alter table "public"."user" add constraint "user_pkey" primary key ("email2") - multiple primary keys for table "user" are not allowed

Perhaps graphql-migrations should read getPrimaryKey from @graphback/core, it is already a dependency.

There appears to be a problem when trying to add new columns after first migration.

"""
@model
"""
type User {
  """
  @id
  """
  email2: String
  email3: String
}

The above model performs a migration as expected.

users=# \d user
             Table "public.user"
 Column |          Type          | Modifiers 
--------+------------------------+-----------
 email2 | character varying(255) | not null
 email3 | character varying(255) | 
Indexes:
    "user_pkey" PRIMARY KEY, btree (email2)

However an error occurs when I added a new regular field email3: String and tried to run another migration.

UnhandledPromiseRejectionWarning: error: alter table "public"."user" alter column "email2" drop not null - column "email2" is in a primary key

For some reason it tries to drop email2, even though I have not made any change to it.

[machi1990]

I've done some quick verification checks.

Graphback allows you to set id: ID and set a different custom ID field with @id. However graphql-migrations tries to set multiple PKs:

"""
@model
"""
type User {
  id: ID!
  """
  @id
  """
  email: String
}

The model above (which would be acceptable in Graphback, throws the following error in migrations:

UnhandledPromiseRejectionWarning: error: alter table "public"."user" add constraint "user_pkey" primary key ("email2") - multiple primary keys for table "user" are not allowed

Perhaps graphql-migrations should read getPrimaryKey from @graphback/core, it is already a dependency.

Very good point.

There appears to be a problem when trying to add new columns after first migration.

"""
@model
"""
type User {
  """
  @id
  """
  email2: String
  email3: String
}

The above model performs a migration as expected.

users=# \d user
             Table "public.user"
 Column |          Type          | Modifiers 
--------+------------------------+-----------
 email2 | character varying(255) | not null
 email3 | character varying(255) | 
Indexes:
    "user_pkey" PRIMARY KEY, btree (email2)

However an error occurs when I added a new regular field email3: String and tried to run another migration.

UnhandledPromiseRejectionWarning: error: alter table "public"."user" alter column "email2" drop not null - column "email2" is in a primary key

For some reason it tries to drop email2, even though I have not made any change to it.

Yeap, I have encountered the issue too. I suppose the change I have added in the computeDiff are not enough, I am looking at it now.

[craicoverflow]

So have to explicitly specify foreign key using the @db.foreign as can be seen from the model above, which is fine but I was thinking maybe we should go further (in a followup PR) and infer this stuff since we have all the info to do so.

Fully agreed. Feedback suggests fewer annotations needed the better (and rightly so)

[machi1990]

So have to explicitly specify foreign key using the @db.foreign as can be seen from the model above, which is fine but I was thinking maybe we should go further (in a followup PR) and infer this stuff since we have all the info to do so.

Fully agreed. Feedback suggests fewer annotations needed the better (and rightly so)

Thanks for the linked issue for this. Totally agree.

[craicoverflow]

Checking this out locally now.

[craicoverflow]

It appears that changing the field annotated with """@id""" after the first migration does not update the database primary key. Do you see this too @machi1990?

error: alter table "public"."user" alter column "email" drop not null - column "email" is in a primary key

I don't know if this is fixable, as long as the error is thrown though it is probably fine. Perhaps we should add this as note to website documentation?

[machi1990]

It appears that changing the field annotated with """@id""" after the first migration does not update the database primary key. Do you see this too @machi1990?

error: alter table "public"."user" alter column "email" drop not null - column "email" is in a primary key

Ah strange as this should have been addressed by https://github.com/aerogear/graphback/pull/1421/files#diff-4972a24a7bb3dda74ca34232ed6243aeR304

I don't know if this is fixable, as long as the error is thrown though it is probably fine. Perhaps we should add this as note to website documentation?

From the test I did, looks like the code I shared above fixed it Might have been a false positive. Let me keep on looking and if not fixable I can add a note in the documentation as suggested.

[machi1990]

@craicoverflow I have pushed new changes hoping to fix the drop no null issue on a previous primary key column. Can you re-review please?

[craicoverflow]

One small comment here https://github.com/aerogear/graphback/pull/1421/files#r440061816

Other than that, this is great!