Skip to content

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

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.

Sign up for GitHub

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

Merged
agnes512 merged 1 commit into from
Dec 19, 2019
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
107 changes: 103 additions & 4 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -436,7 +436,58 @@ Foreign key constraints can be defined in the model `options`. Removing or updat

If there is a reference to an object being deleted then the `DELETE` will fail. Likewise if there is a create with an invalid FK id then the `POST` will fail.

**Note**: The order of table creation is important. A referenced table must exist before creating a foreign key constraint.
**Note**: The order of table creation is important. A referenced table must exist before creating a foreign key constraint.

For **LoopBack 4** users, define your models under the `models/` folder as follows:

`customer.model.ts`:

```ts
@model()
export class Customer extends Entity {
@property({
id: true,
type: 'Number',
required: false,
length: 20
})
id: number;

@property({
type: 'string',
length: 20
})
name: string;
}
```
`order.model.ts`:

```ts
@model()
export class Order extends Entity {
@property({
id: true,
type: 'Number',
required: false,
length: 20
})
id: number;

@property({
type: 'string',
length: 20
})
name: string;

@property({
type: 'Number',
length: 20
})
customerId: number;
}
```

For **LoopBack 3** users, you can define your model JSON schema as follows:

```json
{
Expand All @@ -446,7 +497,7 @@ If there is a reference to an object being deleted then the `DELETE` will fail.
},
"properties": {
"id": {
"type": "String",
"type": "Number",
"length": 20,
"id": 1
},
Expand All @@ -473,12 +524,12 @@ If there is a reference to an object being deleted then the `DELETE` will fail.
},
"properties": {
"id": {
"type": "String",
"type": "Number",
"length": 20,
"id": 1
},
"customerId": {
"type": "String",
"type": "Number",
"length": 20
},
"description": {
Expand All @@ -490,6 +541,54 @@ If there is a reference to an object being deleted then the `DELETE` will fail.
}
```

Auto-migrate supports the automatic generation of property values. For PostgreSQL, the default id type is _integer_. If you have `generated: true` in the id property, it generates integers by default:

```ts
{
id: true,
type: 'Number',
required: false,
generated: true // enables auto-generation
}
```

It is common to use UUIDs as the primary key in PostgreSQL instead of integers. You can enable it with the following settings:

```ts
{
id: true,
type: 'String',
required: false,
// settings below are needed
generated: true,
useDefaultIdType: false,
postgresql: {
dataType: 'uuid',
},
}
```
The setting uses `uuid-ossp` extension and `uuid_generate_v4()` function as default.

If you'd like to use other extensions and functions, you can do:

```ts
{
id: true,
type: 'String',
required: false,
// settings below are needed
generated: true,
useDefaultIdType: false,
postgresql: {
dataType: 'uuid',
extension: 'myExtension',
defaultFn: 'myuuid'
},
}
```

WARNING: It is the users' responsibility to make sure the provided extension and function are valid.

## Running tests

### Own instance
Expand Down
88 changes: 66 additions & 22 deletions lib/migration.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
const SG = require('strong-globalize');
const g = SG();
const async = require('async');
const chalk = require('chalk');
const debug = require('debug')('loopback:connector:postgresql:migration');

module.exports = mixinMigration;
Expand Down Expand Up @@ -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);
Copy link
Contributor

@jannyHou jannyHou Dec 17, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@jannyHou jannyHou Dec 17, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


// checks if dataType is set to uuid
let postgDefaultFn;
let postgType;
const postgSettings = prop.postgresql;
if (postgSettings && postgSettings.dataType) {
postgType = postgSettings.dataType.toUpperCase();
}

if (prop.generated) {
return 'SERIAL';
if (result === 'INTEGER') {
return 'SERIAL';
} else if (postgType === 'UUID') {
if (postgSettings && postgSettings.defaultFn && postgSettings.extension) {
Copy link
Contributor

@jannyHou jannyHou Dec 17, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@agnes512 agnes512 Dec 17, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// if user provides their own extension and function
postgDefaultFn = postgSettings.defaultFn;
return result + ' NOT NULL' + ' DEFAULT ' + postgDefaultFn;
}
return result + ' NOT NULL' + ' DEFAULT uuid_generate_v4()';
} else {
console.log(chalk.red('>>> WARNING: ') +
`auto-generation is not supported for type "${chalk.yellow(prop.type)}". \
Please add your own function to the table "${chalk.yellow(model)}".`);
}
}
let result = self.columnDataType(model, propName);
if (!self.isNullable(prop)) result = result + ' NOT NULL';

result += self.columnDbDefault(model, propName);
return result;
};
Expand All @@ -313,32 +335,53 @@ function mixinMigration(PostgreSQL) {
PostgreSQL.prototype.createTable = function(model, cb) {
const self = this;
const name = self.tableEscaped(model);
const modelDef = this.getModelDefinition(model);

// collects all extensions needed to be created
let createExtensions;
Object.keys(this.getModelDefinition(model).properties).forEach(function(propName) {
const prop = modelDef.properties[propName];

// checks if dataType is set to uuid
const postgSettings = prop.postgresql;
if (postgSettings && postgSettings.dataType && postgSettings.dataType === 'UUID'
&& postgSettings.defaultFn && postgSettings.extension) {
createExtensions += 'CREATE EXTENSION IF NOT EXISTS "' + postgSettings.extension + '";';
}
});
// default extension
if (!createExtensions) {
createExtensions = 'CREATE EXTENSION IF NOT EXISTS "uuid-ossp";';
}

// Please note IF NOT EXISTS is introduced in postgresql v9.3
self.execute('CREATE SCHEMA ' +
self.execute(
createExtensions +
'CREATE SCHEMA ' +
self.escapeName(self.schema(model)),
function(err) {
if (err && err.code !== '42P06') {
return cb && cb(err);
}
self.execute('CREATE TABLE ' + name + ' (\n ' +
self.propertiesSQL(model) + '\n)',
function(err, info) {
if (err) {
return cb(err, info);
function(err) {
if (err && err.code !== '42P06') {
return cb && cb(err);
}
self.addIndexes(model, undefined, function(err) {
self.execute('CREATE TABLE ' + name + ' (\n ' +
self.propertiesSQL(model) + '\n)',
function(err, info) {
if (err) {
return cb(err);
return cb(err, info);
}
const fkSQL = self.getForeignKeySQL(model,
self.getModelDefinition(model).settings.foreignKeys);
self.addForeignKeys(model, fkSQL, function(err, result) {
cb(err);
self.addIndexes(model, undefined, function(err) {
if (err) {
return cb(err);
}
const fkSQL = self.getForeignKeySQL(model,
self.getModelDefinition(model).settings.foreignKeys);
self.addForeignKeys(model, fkSQL, function(err, result) {
cb(err);
});
});
});
});
});
},
);
};

PostgreSQL.prototype.buildIndex = function(model, property) {
Expand Down Expand Up @@ -481,7 +524,7 @@ function mixinMigration(PostgreSQL) {
default:
case 'String':
case 'JSON':
return 'TEXT';
case 'Uuid':
Copy link
Contributor

@jannyHou jannyHou Dec 17, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@agnes512 agnes512 Dec 17, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@jannyHou jannyHou Dec 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

case 'Text':
return 'TEXT';
case 'Number':
Expand Down Expand Up @@ -645,6 +688,7 @@ function mixinMigration(PostgreSQL) {
case 'CHARACTER':
case 'CHAR':
case 'TEXT':
case 'UUID':
return 'String';

case 'BYTEA':
Expand Down
71 changes: 70 additions & 1 deletion test/postgresql.migration.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ describe('migrations', function() {
before(setup);

it('should run migration', function(done) {
db.automigrate('UserDataWithIndexes', done);
db.automigrate(['UserDataWithIndexes', 'OrderData', 'DefaultUuid'], done);
});

it('UserDataWithIndexes should have correct indexes', function(done) {
Expand Down Expand Up @@ -73,6 +73,42 @@ describe('migrations', function() {
done();
});
});

it('OrderData should have correct prop type uuid with custom generation function', function(done) {
checkColumns('OrderData', function(err, cols) {
assert.deepEqual(cols, {
ordercode:
{column_name: 'ordercode',
column_default: 'uuid_generate_v1()',
data_type: 'uuid'},
ordername:
{column_name: 'ordername',
column_default: null,
data_type: 'text'},
id:
{column_name: 'id',
column_default: 'nextval(\'orderdata_id_seq\'::regclass)',
data_type: 'integer'},
});
done();
});
});

it('DefaultUuid should have correct id type uuid and default function v4', function(done) {
checkColumns('DefaultUuid', function(err, cols) {
assert.deepEqual(cols, {
defaultcode:
{column_name: 'defaultcode',
column_default: 'uuid_generate_v4()',
Copy link
Contributor

@jannyHou jannyHou Dec 17, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@agnes512 agnes512 Dec 17, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

data_type: 'uuid'},
id:
{column_name: 'id',
column_default: 'nextval(\'defaultuuid_id_seq\'::regclass)',
data_type: 'integer'},
});
done();
});
});
});

function setup(done) {
Expand Down Expand Up @@ -118,6 +154,23 @@ function setup(done) {
},
},
});
const OrderData = db.define('OrderData', {
ordercode: {type: 'String', required: true, generated: true, useDefaultIdType: false,
postgresql: {
dataType: 'uuid',
defaultFn: 'uuid_generate_v1()',
extension: 'uuid-ossp',
}},
ordername: {type: 'String'},
});

const DefaultUuid = db.define('DefaultUuid', {
defaultCode: {type: 'String', required: true, generated: true, useDefaultIdType: false,
postgresql: {
dataType: 'uuid',
defaultFn: 'uuid_generate_v1()', // lack extension
}},
});

done();
}
Expand Down Expand Up @@ -161,3 +214,19 @@ function table(model) {
function query(sql, cb) {
db.adapter.query(sql, cb);
}

function checkColumns(table, cb) {
const tableName = table.toLowerCase();
query('SELECT column_name, column_default, data_type FROM information_schema.columns \
WHERE(table_schema, table_name) = (\'public\', \'' + tableName + '\');',
function(err, data) {
const cols = {};
if (!err) {
data.forEach(function(index) {
cols[index.column_name] = index;
delete index.name;
});
}
cb(err, cols);
});
}