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

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

Merged
merged 1 commit into from
Dec 19, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
105 changes: 102 additions & 3 deletions README.md
Original file line number Original file line Diff line number Diff line change
Expand Up @@ -438,6 +438,57 @@ If there is a reference to an object being deleted then the `DELETE` 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 ```json
{ {
"name": "Customer", "name": "Customer",
Expand All @@ -446,7 +497,7 @@ If there is a reference to an object being deleted then the `DELETE` will fail.
}, },
"properties": { "properties": {
"id": { "id": {
"type": "String", "type": "Number",
"length": 20, "length": 20,
"id": 1 "id": 1
}, },
Expand All @@ -473,12 +524,12 @@ If there is a reference to an object being deleted then the `DELETE` will fail.
}, },
"properties": { "properties": {
"id": { "id": {
"type": "String", "type": "Number",
"length": 20, "length": 20,
"id": 1 "id": 1
}, },
"customerId": { "customerId": {
"type": "String", "type": "Number",
"length": 20 "length": 20
}, },
"description": { "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 ## Running tests


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


module.exports = mixinMigration; module.exports = mixinMigration;
Expand Down Expand Up @@ -295,12 +296,33 @@ function mixinMigration(PostgreSQL) {
const self = this; const self = this;
const modelDef = this.getModelDefinition(model); const modelDef = this.getModelDefinition(model);
const prop = modelDef.properties[propName]; const prop = modelDef.properties[propName];
let result = self.columnDataType(model, propName);
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.

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

Copy link
Contributor

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) { if (prop.generated) {
if (result === 'INTEGER') {
return 'SERIAL'; return 'SERIAL';
} else if (postgType === 'UUID') {
if (postgSettings && postgSettings.defaultFn && postgSettings.extension) {
Copy link
Contributor

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

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'; if (!self.isNullable(prop)) result = result + ' NOT NULL';

result += self.columnDbDefault(model, propName); result += self.columnDbDefault(model, propName);
return result; return result;
}; };
Expand All @@ -313,9 +335,29 @@ function mixinMigration(PostgreSQL) {
PostgreSQL.prototype.createTable = function(model, cb) { PostgreSQL.prototype.createTable = function(model, cb) {
const self = this; const self = this;
const name = self.tableEscaped(model); 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 // 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)), self.escapeName(self.schema(model)),
function(err) { function(err) {
if (err && err.code !== '42P06') { if (err && err.code !== '42P06') {
Expand All @@ -338,7 +380,8 @@ function mixinMigration(PostgreSQL) {
}); });
}); });
}); });
}); },
);
}; };


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

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

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

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


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


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


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

it('OrderData should have correct prop type uuid with custom generation function', function(done) {
nabdelgadir marked this conversation as resolved.
Show resolved Hide resolved
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

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

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) { 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(); done();
} }
Expand Down Expand Up @@ -161,3 +214,19 @@ function table(model) {
function query(sql, cb) { function query(sql, cb) {
db.adapter.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);
});
}