fix: allow string type attribute to be auto-generated in Postgres #404

agnes512 · 2019-12-10T19:48:11Z

please review it with whitespace hided

I went through several related issues:

loopbackio/loopback-next#2398 is partially fixed in loopbackio/loopback-datasource-juggler#1783.

@model()
export class Customer extends Entity {
  @property({
    type: 'string',
    id: true,
    useDefaultIdType: false,
    generated: true,
  })
  custid: string;

Issue

The flag useDefaultIdType: false is supposed to allow to use custom type of auto-generated id property. But it only allows the default type, and it won't generated the custom id type itself as different DBs have different syntax/setup.

For example, Postgres and MySQL default identity type is INT. And they can be auto-generated by SERIAL and AUTO_INCREAMENT respectively.

The model definition above won't work on Postgres and MySQL because these two connectors only support to auto-generated the default id type ( integer in this case).

From #3602 what suggests and real world usage, I am proposing to add uuid type to data type, and also add support for auto-generated string type id to Postgres connector:

Proposal

Similar to mongoDB, when property type is uuid and auto-generated, by default, use uuid-ossp extension and uuid_generate_v4() function to allow auto-generated string.

  @property({
    id: true,
    type: 'string',
    generated: true,
    useDefaultIdType: false, // this is needed if this property is id
    postgresql: {
      dataType: 'uuid',
    },
  })
  id: String;

~~when id type is string and auto-generated, treat it as uuid. ( Should we?)~~

If user would like use other extension & function, they can do:

  @property({
    id: true,
    type: 'string',
    generated: true,
    useDefaultIdType: false, // this is needed if this property is id
    postgresql: {
      dataType: 'uuid',
      defaultFn: 'example_function',
      extension: 'my_extension'
    },
  })
  id: String;

However, it is user's responsibility to provide valid extension and function.

If user would like to use other type as id type and make it auto-generated, should warn them that LoopBack doesn't support to auto-generate such type for Postgres. They need to do ALTER TABLE to set up the table manually.
Document the above setup rules, options, and limitations in both LB3 and LB4

If the above is okay, will fix MySQL as well.

Checklist

👉 Read and sign the CLA (Contributor License Agreement) 👈

npm test passes on your machine
New tests added or existing tests modified to cover all changes
Code conforms with the style guide
Commit messages are following our guidelines

lib/migration.js

bajtos · 2019-12-12T08:24:13Z

This is tricky issue, thank you @agnes512 for digging deep into this problem space.

IIUC, there are two aspects we need to address:

How to represent the database-generated UUID values inside LoopBack: how to define such model property, what JavaScript type it uses at runtime, how is it described in OpenAPI spec.
How to auto-migrate models using database-generated UUID value as the primary key.

Also IIUC, this issue is specific to SQL databases that typically use a numeric primary key when the value is generated by the database. For example, database-generated UUID primary keys are the default option in MongoDB.

While I like the idea of introducing a new data type uuid (see the bottom of my comment), I am reluctant to use that as the solution here, because such change affects many consumers of LoopBack definition language. Besides updating SQL connectors, we also need to update NoSQL connectors (including those we don't maintain ourselves), code converting LoopBack models to Swagger/OpenAPI, etc.

I find this situation similar to ObjectID issues we are facing in MongoDB - we have a string-like value that must be treated differently by the connector.

I prefer to find a solution where the property type stays as string and the UUID aspect is described via connector-specific metadata (if needed). For example:

@model()
export class Customer extends Entity {
  @property({
    type: 'string',
    id: true,
    useDefaultIdType: false,
    postgresql: {
      defaultFn: 'gen_random_uuid()'
    }
  })
  custid: string;
}

Or perhaps we can enhance generated field to support non-boolean values:

generated: false (or generated not present): auto-generation is disabled
generated: true: the default database mechanism is used
generated: 'uuid': values are generated as UUID values using database-specific features

To be honest, I feel both options described above are adding too much complexity and not enough benefits for the majority of our users. Let's try to find something simpler.

I like the following items in your original proposal:

When id type is string and auto-generated, treat it as uuid.

If user would like to use other type as id type and make it auto-generated, should throw an error and suggest them to remove generated: true, then do ALTER TABLE to set up the table manually.

When id is auto-generated and the type is not db default type, alert that user should modify the table to enable the auto generation themselves.

I would like to propose a slightly different solution building on your ideas:

When id type is string and auto-generated, treat it as uuid (using database-provided mechanism).
When id is auto-generated and we don't know how to auto-generate the value (e.g. because the database does not support auto-generated values for this particular data type), then alert that user should modify the table to enable the auto generation themselves.

In other words, if the user defines an primary key as auto-generated, then LoopBack should use the most sensible option supported by the database (i.e. gen_random_uuid() for PostgreSQL) or alert the user when there is no such option available/implemented by the connector.

IMO, the migration should not abort the process by throwing an error, I am proposing to print a warning instead and continue with the migration process. I think that even if don't configure all auto-generation rules in the database, it is still very useful to have the rest of the schema created by the LoopBack migration tool. (We also don't want to abort migration in the middle, when some of the tables were already migrated/updated but some haven't.)

We can also apply these new rules to connectors that don't support auto-generated numeric values (MongoDB and Cloudant), see e.g. loopbackio/loopback-next#2040. Because schema migration is usually not executed for NoSQL databases, this would require the check to be performed at runtime. To keep things simple, it may be best to leave this part out of scope of this pull request, just keep it in mind.

Circling back to the proposal to add uuid type. I think it's a good idea to eventually implement uuid type as a first-class data type supported by all layers of LoopBack, from database persistence to JSON Schema validation in the REST API layer. This will make it easier to use ObjectID type in MongoDB, define JSON Schema validations to enforce uuid formatting of the string values, etc. There is a also a TC39 proposal to add uuid support to JavaScript standard library, see https://github.com/tc39/proposal-uuid, see especially the FAQ section explaining why most application should always use v4 UUID algorithm only.

I am proposing to open a new feature request in loopback-next to implement uuid data type. Maybe a new Epic and a spike story to kick-off the work with the necessary research?

agnes512 · 2019-12-13T13:55:45Z

@bajtos thank you for elaborating all your concern!

because such change affects many consumers of LoopBack definition language. Besides updating SQL connectors, we also need to update NoSQL connectors (including those we don't maintain ourselves), code converting LoopBack models to Swagger/OpenAPI, etc.

You're right, I didn't consider this part. Will just use the existing types.

Or perhaps we can enhance generated field to support non-boolean values:

- `generated: false` (or `generated ` not present): auto-generation is disabled
- `generated: true`: the default database mechanism is used
- `generated: 'uuid'`: values are generated as UUID values using database-specific features

I was thinking to have something similar to mongoDB:

    postgresql: {
      dataType: 'uuid'
    }

so when generated && postgresql.dataType === 'uuid' -> use uuid_generate_v4() as default function.

Any preferences?

bajtos · 2019-12-13T13:59:40Z

was thinking to have something similar to mongoDB:

    postgresql: {
      dataType: 'uuid'
    }

That's a decent solution too!

Any preferences?

Personally, I would try to keep this simple and easy to use for our users. If their model/property definition says that the PK/id property should be an auto-generated string, then I would like the framework to fulfill that wish in the most sensible way. If we don't know how to do that or there isn't any sensible default, then notify the user about the problem.

agnes512 · 2019-12-14T00:15:11Z

@slnode test please

agnes512 · 2019-12-14T03:36:30Z

@slnode test please

agnes512 · 2019-12-14T03:39:30Z

@slnode test please

agnes512 · 2019-12-14T03:51:48Z

@slnode test please

agnes512 · 2019-12-14T13:52:36Z

@slnode test please

agnes512 · 2019-12-15T13:21:02Z

@slnode test please

emonddr · 2019-12-16T15:21:41Z

Good discussion.

@agnes512 , this seems like a good feature. Great work!

agnes512 · 2019-12-16T15:42:43Z

@slnode test please

nabdelgadir

Minor nitpicks and some questions

lib/migration.js

test/postgresql.migration.test.js

jannyHou · 2019-12-17T15:58:50Z

lib/migration.js

@@ -295,12 +296,33 @@ function mixinMigration(PostgreSQL) {
    const self = this;
    const modelDef = this.getModelDefinition(model);
    const prop = modelDef.properties[propName];
+    let result = self.columnDataType(model, propName);


~~The variable name here is a bit misleading, maybe use columnType instead of result?~~

sorry never mind, I just realized the code is in a function called buildColumnDefinition, so result is better.

jannyHou · 2019-12-17T16:10:19Z

lib/migration.js

+      if (result === 'INTEGER') {
+        return 'SERIAL';
+      } else if (postgType === 'UUID') {
+        if (postgSettings && postgSettings.defaultFn && postgSettings.extension) {


Hmm, how do we deal with the extension? Is it a function or just a boolean value?

We need to 'install' the extension to use its functions. e.g, to use foo.bar(), we need to have foo installed first. In here extension is just a name that will be used in postgres query of creating table later.

jannyHou

👏 Nice feature! Left a few comments.

jannyHou · 2019-12-17T17:13:17Z

lib/migration.js

@@ -481,7 +524,7 @@ function mixinMigration(PostgreSQL) {
      default:
      case 'String':
      case 'JSON':
-        return 'TEXT';
+      case 'Uuid':


Hmm, what's the difference between Uuid and UUID?

I am following the pattern with other type such as String, Number. But I think the case doesn't matter. I can change it to uuid.
As for the UUID below, it's modified to upper case so I use UUID there.

oh I see, the code comparison gave me an impression that they are two cases in the same function, while actually not lol. 👍

jannyHou · 2019-12-17T17:45:40Z

test/postgresql.migration.test.js

+      assert.deepEqual(cols, {
+        defaultcode:
+        {column_name: 'defaultcode',
+          column_default: 'uuid_generate_v4()',


IIUC the column_default here is uuid_generate_v4() instead of uuid_generate_v1() because extension is missing in the property def, so I am thinking, maybe add a warning before line 318 to let people know it falls back to the default value due to missing extension?

Correct, it should use the default extension and function for this case. And I have uuid_generate_v4() here 😄

hacksparrow · 2019-12-18T13:59:13Z

We need tests to assert the behavior when useDefaultIdType is not set and when the value is set to true.

agnes512 · 2019-12-18T14:11:11Z

We need tests to assert the behavior when useDefaultIdType is not set and when the value is set to true.

I think useDefaultIdType is being tested in loopbackio/loopback-datasource-juggler#1783 ? Cause when uuid is set, the lb property will be set to string.

jannyHou

👏

nabdelgadir

I don't know this area too well but I think the changes look reasonable to me 👍

shreyanshgoel · 2020-06-02T10:40:28Z

Great work people. But still I think we have a problem.

"my_id": {
	"id": true,
	"type": "string",
	"required": false,
	"generated": true,
	"useDefaultIdType": false,
	"postgresql": {
		"dataType": "uuid",
		"extenstion": "uuid-ossp",
		"defaultFn": "uuid_generate_v4()"
	}
},

So everything works well but when I try to do find on any model, I get id as NaN. Guessing because loopback is still treating my id as a number. This happens only when generated is true so i guess useDefaultIdType is no where being used. I checked the whole code, could not find this parameter being user any where. I am using loopback 3.27.0.

Also when i am doing realtions, the foreign key are being treated as numbers for some reason.

Any update would be much apprecitated, thank you.
Cheers

bajtos suggested changes Dec 12, 2019

View reviewed changes

lib/migration.js Outdated Show resolved Hide resolved

agnes512 mentioned this pull request Dec 13, 2019

Preserve custom type of auto-generated id property loopbackio/loopback-next#4270

Merged

3 tasks

agnes512 changed the title ~~fix: allow string type id to be auto-generated~~ fix: allow string type attribute to be auto-generated Dec 13, 2019

agnes512 changed the title ~~fix: allow string type attribute to be auto-generated~~ fix: allow string type attribute to be auto-generated in Postgres Dec 13, 2019

agnes512 force-pushed the default-id branch from 684a4ef to 5695198 Compare December 14, 2019 03:33

agnes512 mentioned this pull request Dec 14, 2019

uuid #405

Closed

4 tasks

agnes512 force-pushed the default-id branch from 0be7b7e to 4c5d178 Compare December 16, 2019 15:10

agnes512 force-pushed the default-id branch 3 times, most recently from 1ef89c2 to e0e7f0d Compare December 16, 2019 15:36

nabdelgadir reviewed Dec 16, 2019

View reviewed changes

lib/migration.js Outdated Show resolved Hide resolved

lib/migration.js Outdated Show resolved Hide resolved

lib/migration.js Outdated Show resolved Hide resolved

lib/migration.js Show resolved Hide resolved

test/postgresql.migration.test.js Show resolved Hide resolved

emonddr approved these changes Dec 17, 2019

View reviewed changes

jannyHou reviewed Dec 17, 2019

View reviewed changes

agnes512 marked this pull request as ready for review December 18, 2019 13:18

agnes512 requested review from b-admike, dhmlau, elv1s and zbarbuto as code owners December 18, 2019 13:18

agnes512 force-pushed the default-id branch from e0e7f0d to 6d9868c Compare December 18, 2019 19:22

jannyHou approved these changes Dec 18, 2019

View reviewed changes

fix: allow string type id to be auto-generated

656ce28

agnes512 force-pushed the default-id branch from 6d9868c to 656ce28 Compare December 18, 2019 20:40

nabdelgadir approved these changes Dec 18, 2019

View reviewed changes

dhmlau assigned agnes512 Dec 19, 2019

agnes512 removed request for elv1s, zbarbuto, b-admike and dhmlau December 19, 2019 13:55

agnes512 merged commit 9a9114a into master Dec 19, 2019

delete-merged-branch bot deleted the default-id branch December 19, 2019 15:56

This was referenced Dec 19, 2019

docs: add link to connectors page as reference for auto-migrate loopbackio/loopback-next#4321

Merged

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

Open

agnes512 mentioned this pull request Jan 2, 2020

Column is always integer when running npm run migrate loopbackio/loopback-next#2398

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: allow string type attribute to be auto-generated in Postgres #404

fix: allow string type attribute to be auto-generated in Postgres #404

agnes512 commented Dec 10, 2019 •

edited

Loading

bajtos commented Dec 12, 2019

agnes512 commented Dec 13, 2019 •

edited

Loading

bajtos commented Dec 13, 2019

agnes512 commented Dec 14, 2019

agnes512 commented Dec 14, 2019

agnes512 commented Dec 14, 2019

agnes512 commented Dec 14, 2019

agnes512 commented Dec 14, 2019

agnes512 commented Dec 15, 2019

emonddr commented Dec 16, 2019

agnes512 commented Dec 16, 2019

nabdelgadir left a comment

jannyHou Dec 17, 2019 •

edited

Loading

jannyHou Dec 17, 2019

jannyHou Dec 17, 2019

agnes512 Dec 17, 2019

jannyHou left a comment

jannyHou Dec 17, 2019

agnes512 Dec 17, 2019

jannyHou Dec 18, 2019

jannyHou Dec 17, 2019

agnes512 Dec 17, 2019 •

edited

Loading

hacksparrow commented Dec 18, 2019

agnes512 commented Dec 18, 2019

jannyHou left a comment

nabdelgadir left a comment

shreyanshgoel commented Jun 2, 2020 •

edited

Loading

fix: allow string type attribute to be auto-generated in Postgres #404

fix: allow string type attribute to be auto-generated in Postgres #404

Conversation

agnes512 commented Dec 10, 2019 • edited Loading

please review it with whitespace hided

Issue

Proposal

Checklist

bajtos commented Dec 12, 2019

agnes512 commented Dec 13, 2019 • edited Loading

bajtos commented Dec 13, 2019

agnes512 commented Dec 14, 2019

agnes512 commented Dec 14, 2019

agnes512 commented Dec 14, 2019

agnes512 commented Dec 14, 2019

agnes512 commented Dec 14, 2019

agnes512 commented Dec 15, 2019

emonddr commented Dec 16, 2019

agnes512 commented Dec 16, 2019

nabdelgadir left a comment

Choose a reason for hiding this comment

jannyHou Dec 17, 2019 • edited Loading

Choose a reason for hiding this comment

jannyHou Dec 17, 2019

Choose a reason for hiding this comment

jannyHou Dec 17, 2019

Choose a reason for hiding this comment

agnes512 Dec 17, 2019

Choose a reason for hiding this comment

jannyHou left a comment

Choose a reason for hiding this comment

jannyHou Dec 17, 2019

Choose a reason for hiding this comment

agnes512 Dec 17, 2019

Choose a reason for hiding this comment

jannyHou Dec 18, 2019

Choose a reason for hiding this comment

jannyHou Dec 17, 2019

Choose a reason for hiding this comment

agnes512 Dec 17, 2019 • edited Loading

Choose a reason for hiding this comment

hacksparrow commented Dec 18, 2019

agnes512 commented Dec 18, 2019

jannyHou left a comment

Choose a reason for hiding this comment

nabdelgadir left a comment

Choose a reason for hiding this comment

shreyanshgoel commented Jun 2, 2020 • edited Loading

agnes512 commented Dec 10, 2019 •

edited

Loading

agnes512 commented Dec 13, 2019 •

edited

Loading

jannyHou Dec 17, 2019 •

edited

Loading

agnes512 Dec 17, 2019 •

edited

Loading

shreyanshgoel commented Jun 2, 2020 •

edited

Loading