Incremental migration does not configure existing PK column to enable auto-generated values

[aharbis]

Description/Steps to reproduce

From my testing using LoopBack 4 and this connector, the migration performed by npm run migrate does not configure the id (primary key) column to be auto-generated when generated: true is enabled on the property.

Link to reproduction sandbox

I've set up a branch on my loopback-next fork with a recreate. The branch is here:

https://github.com/aharbis/loopback-next/tree/postgres-pk-auto-gen

The recreate uses the todo application. There are two commits on the branch:

aharbis/loopback-next@cdee3cb - Adds the Postgres data source
aharbis/loopback-next@43368cc - Sets generated: true on todo model

Steps to recreate

1. Checkout first commit (that adds the datasource)
2. Install dependencies (npm i)
3. Run migration (npm run migrate)
4. Start app (npm start)
5. Try to create todo with ID, to confirm this works
6. Stop app
7. Checkout second commit (that sets generated: true)
8. Run migration (npm run migrate)
9. Start app (npm start)
10. Try to create todo without ID

Expected result

In step 10, this create should succeed and the ID should be auto-generated.

Actual result

In step 10, this create yields a 500 error:

Unhandled error in POST /todos: 500 error: null value in column "id" violates not-null constraint
    at Connection.parseE (/Users/aharbis/git/loopback-next/examples/todo/node_modules/pg/lib/connection.js:602:11)
    at Connection.parseMessage (/Users/aharbis/git/loopback-next/examples/todo/node_modules/pg/lib/connection.js:399:19)
    at Socket.<anonymous> (/Users/aharbis/git/loopback-next/examples/todo/node_modules/pg/lib/connection.js:121:22)
    at emitOne (events.js:116:13)
    at Socket.emit (events.js:211:7)
    at addChunk (_stream_readable.js:263:12)
    at readableAddChunk (_stream_readable.js:250:11)
    at Socket.Readable.push (_stream_readable.js:208:10)
    at TCP.onread (net.js:601:20)

Additional information

$ node -e 'console.log(process.platform, process.arch, process.versions.node)'
darwin x64 8.12.0

$ npm ls --prod --depth 0 | grep loopback
@loopback/example-todo@1.6.2 /Users/aharbis/git/loopback-next/examples/todo
├── @loopback/boot@1.4.1
├── @loopback/context@1.19.1
├── @loopback/core@1.8.2
├── @loopback/openapi-v3@1.6.2
├── @loopback/openapi-v3-types@1.1.2
├── @loopback/repository@1.7.0
├── @loopback/rest@1.16.0
├── @loopback/rest-explorer@1.2.2
├── @loopback/service-proxy@1.2.2
├── loopback-connector-postgresql@3.6.1
└── loopback-connector-rest@3.4.1

$ psql --version
psql (PostgreSQL) 10.5

[aharbis]

To add to this, I forgot to mention that in order for the auto-generation to be configured for the PK column, I had to run:

npm run migrate -- --rebuild

After doing this, starting the app and creating a todo (without sending an id in my request) yielded a successful result:

{
  "id": 1,
  "title": "todo title",
  "desc": "todo description",
  "isComplete": false
}

[bajtos]

Thank you for reporting this issue, @aharbis.

I think we need to improve PostgreSQL.prototype.getPropertiesToModify to detect the situation where a property becomes autogenerated. At the moment, we check only for datatype and nullability changes:

https://github.com/strongloop/loopback-connector-postgresql/blob/42bf97f5a3008b54b85beda8342a53f01758257c/lib/migration.js#L183-L188

I think we should detect the opposite transition too, from generated: true to generated: false.

Would you like to contribute this fix yourself?

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[aharbis]

Sorry for this going stale. I would be interested in submitting this patch. I will try to find some time to get this done.

[aharbis]

I've been tinkering with this, and I'm a little torn on how to handle this. Technically, moving from a property defined by:

properties = {
  example: {
    type: Number
  }
}

to:

properties = {
  example: {
    type: Number,
    generated: true
  }
}

would yield a change in column type from INTEGER to SERIAL correct? Thus should this be considered in the case of datatypeChanged instead?

I'm certainly fine if we want to keep this as a separate check, but it seems I'd likely need to leverage modifyDatatypeInActual to make the change from INTEGER to SERIAL. In the meantime, I've been working to build a new function to handle the check, autoIncrementChanged.

Side note, in my testing, it seems like searchForPropertyInActual would not yield a result if the propName is id. Thus for my actual test case I added, I used another name for the property to change to generated: true. I'm guessing id properties get special handling? Since this function was not finding a result, we never got into the logic to check if the property should be updated.

What are your thoughts, @bajtos?

[aharbis]

For reference, the I've added a test case (that fails currently) to demonstrate this bug. Implementing the change should allow the test case to pass.

aharbis@d3f16e3

[aharbis]

Thinking a little more on this. The original issue I reported was focused on the behavior with the PRIMARY KEY. I don't think PostgreSQL handles the SERIAL type any different whether it's a PRIMARY KEY or not; however, it does seem that the LoopBack connector has special treatment for id. For example, I mentioned in the above comment the following finding:

Side note, in my testing, it seems like searchForPropertyInActual would not yield a result if the propName is id. Thus for my actual test case I added, I used another name for the property to change to generated: true. I'm guessing id properties get special handling?

So maybe I need to take a small step back and reassess how I'm approaching this issue. I was planning on simply looking for generated: true and changing the type to SERIAL. From a LoopBack connector perspective, maybe I need to understand if generated is a special property only allowed for id (or PK) properties? Would the test case I added (which is setting generated: true on a generic non-PK property) still be a valid test case for what we are trying to accomplish with this issue?

Maybe you can weigh in @jannyHou or @dhmlau?

[bajtos]

@aharbis I am afraid I am not able to help you much. By now, you know much more about this problem than I do 🤷‍♂

Side note, in my testing, it seems like searchForPropertyInActual would not yield a result if the propName is id. Thus for my actual test case I added, I used another name for the property to change to generated: true. I'm guessing id properties get special handling?

So maybe I need to take a small step back and reassess how I'm approaching this issue. I was planning on simply looking for generated: true and changing the type to SERIAL. From a LoopBack connector perspective, maybe I need to understand if generated is a special property only allowed for id (or PK) properties? Would the test case I added (which is setting generated: true on a generic non-PK property) still be a valid test case for what we are trying to accomplish with this issue?

It is entirely possible that the fact that id properties are handled differently than non-id properties is just a quirk of the original connector implementation that we "inherited" from pre-loopback version of juggler.

I think we should find out what design & behavior makes sense for majority of all connectors and then modify both juggler and the connectors to support that desired solution.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.