RFC: Declarative definition of FOREIGN KEY and UNIQUE constraints #2712

bajtos · 2019-04-09T11:56:59Z

To support strong relations with referential integrity enforced by the database, we need a mechanism allowing models to define FOREIGN KEY and UNIQUE constraints in a declarative way.

Please review my proposal outlined in _SPIKE_.md, you can also review the changes proposed in model/property definition interfaces (TypeScript types).

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
API Documentation in code was updated
Documentation in /docs/site was updated
Affected artifact templates in packages/cli were updated
Affected example projects in examples/* were updated

👉 Check out how to submit a PR 👈

bajtos · 2019-04-11T08:36:45Z

The spike is ready for review.

jannyHou · 2019-04-12T13:35:23Z

_SPIKE_.md

+
+Individual indexes can be defined as follows:
+
+- Add a new field `properties` as a key-value map from property names to


I don't quite follow the meaning of "Individual indexes", could you explain more about it?

I understand the example above uses LB3 syntax:

{ strict: false, forceID: true, indexes: { uniqueEmail: { // index definition }, nameQueries: { // index definition } } }

While confused about the individual index...is it a syntax supported in LB3 or a proposal that will be supported?

I have same comment as @jannyHou .

Do you mean

uniqueEmail: { // index definition },

can be filled in like this?:

uniqueEmail: { properties: { email: 1, // ASC createdAt: 'DESC', // alias for -1 bio: 'text', // database-specific value (MongoDB's "text") } },

Sorry for not being more clear. In model definition, indexes property contains a key-value map from "index name" to "index definition". By "individual index", I mean an entry in this map for an index.

The example posted by @emonddr is correct. I am cross-posting the snippet from examples/todo-list code that can be found at the bottom of this pull request.

@model({ indexes: { /*==== Dummy demonstration of a model-level index definition ===*/ demo: { properties: {desc: 'ASC'}, mysql: { kind: 'FULLTEXT', }, }, }, foreignKeys: { /*==== Dummy demonstration of a model-level foreign-key definition ===*/ demo: { sourceProperties: ['todoListId'], targetModel: () => TodoList, targetProperties: ['id'], onDelete: 'CASCADE', }, }, }) export class Todo extends Entity { // ... }

_SPIKE_.md

jannyHou · 2019-04-12T13:48:17Z

_SPIKE_.md

+    properties: {
+      email: 'ASC',
+    },
+    mongodb: {


I might misunderstand your proposal here...wouldn't the database configurations stay inside each model property's definition like:

properties: { email: { type: 'string', index: true, mongodb: {sparse: true}, mysql: {kind: 'fulltext', type: 'hash'} } }

I am talking about database-specific configuration of an index. Please note the code snippet we are commenting on is showing definition of an index, not a model.

@model({ indexes: { demo: { properties: {email: 'ASC'}, mongodb: { sparse: true }, }, }, }) export class MyModel extends Entity { @property() email: string; // ... }

jannyHou · 2019-04-12T14:28:29Z

_SPIKE_.md

+connector cannot process. The flag can allow three values:
+
+- true: abort migration with error
+- 'warn': print a warning and ignore unsupported metadata


sounds good, I would suggest warn as default.

+1 on warning to be the default.

_SPIKE_.md

emonddr

Please clarify our questions on individual indexes. thx.

dhmlau

LGTM. I mostly come from the users' perspective, so might miss out on some of the technical details.

dhmlau · 2019-04-12T15:27:45Z

_SPIKE_.md

+
+- UNIQUE index with no special configuration
+
+  ```js


In the case of Cloudant and CouchDB, they don't have UNIQUE index as you mentioned... If I'm using Cloudant as the datasource and happen to have unique set. What's going to happen? We'll ignore it because it's not supported by the database?

I think the only unique field in cloudant/couchdb is the document id _id, other loopback flavor constrains will be ignored.

See https://forums.couchbase.com/t/can-i-do-unique-constraint-on-some-attribute-not-the-id/5621
and https://stackoverflow.com/questions/1541239/unique-constraints-in-couchdb

As mentioned in "Additional changes" below, I am proposing to ignore such index (as we are doing now), but let the user know that the constraints expected by the model will not be enforced by the database.

When a model specifies an index or constraint that's not supported by the
connector running automigration, the connector should let the user know about
the problem. To preserve backwards compatibility, I am proposing to print a
console warning only.

dhmlau · 2019-04-12T15:29:07Z

_SPIKE_.md

+
+### Decorators
+
+I am proposing to reuse existing `@property` and `@model` decorators. They are


+1 on reusing existing decorators from the users' point of view.

What about relationship decorators such as @belongsTo, @hasMany, @hasOne. I know we recently added support to add property data to those decorators, but I don't think it is something we want to encourage.

(EDITED) ~~See my comment above.~~ See my other comment: #2712 (comment)

The new property-definition field references will be usable with the relational decorators supporting custom property definition metadata.

@model() class Todo extends Entity { // ... @belongsTo( () => TodoList, {}, { /*==== Define a foreign key to enforce referential integrity ===*/ references: { model: () => TodoList, property: 'id', onDelete: 'CASCADE', }, }, ) todoListId: number; }

dhmlau · 2019-04-12T15:30:34Z

_SPIKE_.md

+connector cannot process. The flag can allow three values:
+
+- true: abort migration with error
+- 'warn': print a warning and ignore unsupported metadata


+1 on warning to be the default.

elv1s

A couple questions, but I like the changes.

elv1s · 2019-04-12T17:07:25Z

_SPIKE_.md

+
+### Decorators
+
+I am proposing to reuse existing `@property` and `@model` decorators. They are


What about relationship decorators such as @belongsTo, @hasMany, @hasOne. I know we recently added support to add property data to those decorators, but I don't think it is something we want to encourage.

elv1s · 2019-04-12T17:17:54Z

_SPIKE_.md

+
+  In the contrary with the current implementation, I am proposing that `columns`
+  should replace any column list created from `properties` and `keys`, and that
+  `columns` should be nested inside connector-specific options:


I agree with the syntax for postgresql.columns = ['lower (email) ASC']. Just want to make sure what will happen if a user defines both, properties and postgreqsl.columns.

It seems like the connector will just use whatever is defined in the connector property. I don't know how you could know programmatically that [lower(email) ASC was intended to replace email

I don't know how you could know programmatically that lower(email) ASC was intended to replace email.

Exactly! That's why I am proposing that columns should take precedence over keys/properties and replace anything defined via those key/value maps.

If postgresql.columns is defined, then the connector uses columns and ignored keys & properties.

If postgresql.columns is not defined, the connector will merge entries defined by keys and properties.

If postgresql.columns is not defined, the connector will merge entries defined by keys and properties.

So the order is postgresql.columns -> keys -> properties, correct?

Yes.
If columns are set, then keys and properties are ignored completely.

Otherwise the content from keys and properties is merged, e.g. the following definition will create a composite index over two columns.

{ keys: { name: 'ASC', }, properties: { email: 'DESC', } }

elv1s · 2019-04-12T17:26:23Z

_SPIKE_.md

+
+### Foreign keys at property level
+
+Introduce a new property metadata "references" (inspired by ANSI SQL):


I like the idea of references, but isn't this duplicating data? How can this example be applied to the TodoList example? In the todo model, we define todoListId as belongsTo (aka references) TodoList

https://github.com/strongloop/loopback-next/blob/master/examples/todo-list/src/models/todo.model.ts#L33

Maybe there is a way to leverage or combine this syntax with relational decorators?

I like the idea of references, but isn't this duplicating data? How can this example be applied to the TodoList example?

I have modified the TodoList example to show how the new syntax could be used, see the bottom half of this pull request.

I feel we are not duplicating data here, because belongsTo decorator can be used without referential integrity, just to navigate to related models in GraphQL style.

Also as we have discussed in the past, we need to rethink how relation decorators are applied. At the moment, @hasMany is applied on the navigational property (todoList.todos) while @belongsTo is applied on the foreign key (todo.todoListId) - this is inconsistent & possibly confusing. The navigational property does not belong to the model base class because relational data is not part of model data, create/update operations are not able to update related data in a single call. So far, we are leaning to rework decorators to be applied on the model constructor, and let the foreign-key property be defined independently of the relation.

Having said that, I can imagine that for now, we can modify @belongsTo decorator to add references metadata to the property definition it creates. The difficult part is how to infer keyTo (the primary key of the target model) at the time the decorator is invoked.

https://github.com/strongloop/loopback-next/blob/cb308add60bbe938c8e85812676eb817e9e0efc9/packages/repository/src/relations/belongs-to/belongs-to.decorator.ts#L26-L41

if (!('references' in propertyDefinition)) { propertyDefinition.references = { model: targetResolver, property: keyTo, }; } const propMeta: PropertyDefinition = Object.assign( // etc. (no change) );

bajtos

Thank you all for valuable feedback!

bajtos · 2019-04-15T09:55:44Z

_SPIKE_.md

+
+- UNIQUE index with no special configuration
+
+  ```js


As mentioned in "Additional changes" below, I am proposing to ignore such index (as we are doing now), but let the user know that the constraints expected by the model will not be enforced by the database.

When a model specifies an index or constraint that's not supported by the
connector running automigration, the connector should let the user know about
the problem. To preserve backwards compatibility, I am proposing to print a
console warning only.

bajtos · 2019-04-15T09:58:42Z

_SPIKE_.md

+
+Individual indexes can be defined as follows:
+
+- Add a new field `properties` as a key-value map from property names to


Sorry for not being more clear. In model definition, indexes property contains a key-value map from "index name" to "index definition". By "individual index", I mean an entry in this map for an index.

The example posted by @emonddr is correct. I am cross-posting the snippet from examples/todo-list code that can be found at the bottom of this pull request.

@model({ indexes: { /*==== Dummy demonstration of a model-level index definition ===*/ demo: { properties: {desc: 'ASC'}, mysql: { kind: 'FULLTEXT', }, }, }, foreignKeys: { /*==== Dummy demonstration of a model-level foreign-key definition ===*/ demo: { sourceProperties: ['todoListId'], targetModel: () => TodoList, targetProperties: ['id'], onDelete: 'CASCADE', }, }, }) export class Todo extends Entity { // ... }

bajtos · 2019-04-15T10:01:16Z

_SPIKE_.md

+    properties: {
+      email: 'ASC',
+    },
+    mongodb: {


I am talking about database-specific configuration of an index. Please note the code snippet we are commenting on is showing definition of an index, not a model.

@model({ indexes: { demo: { properties: {email: 'ASC'}, mongodb: { sparse: true }, }, }, }) export class MyModel extends Entity { @property() email: string; // ... }

bajtos · 2019-04-15T10:03:19Z

_SPIKE_.md

+
+  In the contrary with the current implementation, I am proposing that `columns`
+  should replace any column list created from `properties` and `keys`, and that
+  `columns` should be nested inside connector-specific options:


I don't know how you could know programmatically that lower(email) ASC was intended to replace email.

Exactly! That's why I am proposing that columns should take precedence over keys/properties and replace anything defined via those key/value maps.

If postgresql.columns is defined, then the connector uses columns and ignored keys & properties.

If postgresql.columns is not defined, the connector will merge entries defined by keys and properties.

bajtos · 2019-04-15T10:15:31Z

_SPIKE_.md

+
+### Foreign keys at property level
+
+Introduce a new property metadata "references" (inspired by ANSI SQL):


I like the idea of references, but isn't this duplicating data? How can this example be applied to the TodoList example?

I have modified the TodoList example to show how the new syntax could be used, see the bottom half of this pull request.

I feel we are not duplicating data here, because belongsTo decorator can be used without referential integrity, just to navigate to related models in GraphQL style.

Also as we have discussed in the past, we need to rethink how relation decorators are applied. At the moment, @hasMany is applied on the navigational property (todoList.todos) while @belongsTo is applied on the foreign key (todo.todoListId) - this is inconsistent & possibly confusing. The navigational property does not belong to the model base class because relational data is not part of model data, create/update operations are not able to update related data in a single call. So far, we are leaning to rework decorators to be applied on the model constructor, and let the foreign-key property be defined independently of the relation.

Having said that, I can imagine that for now, we can modify @belongsTo decorator to add references metadata to the property definition it creates. The difficult part is how to infer keyTo (the primary key of the target model) at the time the decorator is invoked.

https://github.com/strongloop/loopback-next/blob/cb308add60bbe938c8e85812676eb817e9e0efc9/packages/repository/src/relations/belongs-to/belongs-to.decorator.ts#L26-L41

if (!('references' in propertyDefinition)) { propertyDefinition.references = { model: targetResolver, property: keyTo, }; } const propMeta: PropertyDefinition = Object.assign( // etc. (no change) );

bajtos · 2019-04-15T10:17:27Z

_SPIKE_.md

+
+### Decorators
+
+I am proposing to reuse existing `@property` and `@model` decorators. They are


(EDITED) ~~See my comment above.~~ See my other comment: #2712 (comment)

The new property-definition field references will be usable with the relational decorators supporting custom property definition metadata.

@model() class Todo extends Entity { // ... @belongsTo( () => TodoList, {}, { /*==== Define a foreign key to enforce referential integrity ===*/ references: { model: () => TodoList, property: 'id', onDelete: 'CASCADE', }, }, ) todoListId: number; }

_SPIKE_.md

raymondfeng · 2019-04-15T21:45:20Z

_SPIKE_.md

+  ```ts
+  @property({
+    type: 'string',
+    unique: true,


I'm wondering if we can just support index: {unique: true} to avoid the unique keyword. Do we need to support unique constraint beyond indexing?

I find the syntax unique: true shorter and easier to write than index: {unique: true}.

Also the longer form index: {unique: true} is opening doors for adding additional index metadata, e.g. index: {unique: true, sparse: true}. I am little bit concerned that it makes it easier for users to define something that's not supported - see the other comment below.

@raymondfeng What are your concerns, why would you like to avoid adding the new keyword unique?

IMO the shorthand is more convenient - at least for first iteration.

raymondfeng · 2019-04-15T21:46:02Z

_SPIKE_.md

+  ```ts
+  @property({
+    type: 'string',
+    index: true,


Do we allow index: boolean | IndexOptions?

See my previous answer. My intention is to keep property-level indexes simple and ask users to define a model-level index if they need anything more complex.

Allowed all IndexOptions properties is opening doors to complex combinations, for example how do you propose to treat property-level index defined as follows?

class MyModel extends Entity { @property({ index: { properties: { // "name" is a different property name: 'ASC', } }, postgresql: { // custom "columns" typically replace // configuration provided by properties/keys columns: ['lower(email) ASC'], } }) email: string; @property() name: string; }

b-admike

Thank you for the detailed overview of the current status of FK and unique constraints. I've read through the proposal and the spike code demonstrating the interfaces and decorators to be created. Overall, I agree with the direction of the spike and the next steps.

b-admike · 2019-04-16T12:03:44Z

_SPIKE_.md

+  required: true,
+  references: {
+    // a TypeResolver
+    model: () => Category,


I'm wondering why we'd like to support three ways of declaring the target model here. Can we keep this simple and support the typeResolver flavour only (at least for first iteration)?

Good point 👍

b-admike · 2019-04-16T12:05:25Z

_SPIKE_.md

+
+  In the contrary with the current implementation, I am proposing that `columns`
+  should replace any column list created from `properties` and `keys`, and that
+  `columns` should be nested inside connector-specific options:


If postgresql.columns is not defined, the connector will merge entries defined by keys and properties.

So the order is postgresql.columns -> keys -> properties, correct?

b-admike · 2019-04-16T12:08:41Z

_SPIKE_.md

+- Supported by: MySQL, PostgreSQL
+- Missing support in: MSSQL, Oracle
+- Not possible to implement in: MongoDB, CouchDB, Cloudant
+- Not supported: `ON UPDATE` and `ON DELETE` options, e.g. `ON DELETE CASCADE`


Should we add tasks for supporting these features in the connectors as part of next actions?

My expectation is that we will create those tasks as part of "Spike: a template implementation of index & constraint migration in SqlConnector". I'll update capture this information in the spike acceptance criteria.

bajtos · 2019-04-16T14:12:35Z

Pushed two new commits:

address Biniam's feedback 09bc09a
clarify handling of unsupported options 4029988

bajtos · 2019-04-16T16:31:21Z

FWIW, we already have a community-contributed pull request adding support for onUpdate and onDelete fields for Foreign Keys, see loopbackio/loopback-connector-mysql#370

dhmlau

I have limited knowledge on the implementation details, but your proposal looks good to me as an overview. Thanks.

b-admike

LGTM 👏

bajtos · 2019-04-18T07:18:07Z

Created the following follow-up stories:

New syntax for INDEX and FOREIGN KEY definition New syntax for INDEX and FOREIGN KEY definition #2766
Document how to configure indexes and foreign keys Document how to configure indexes and foreign keys #2767
Connector helpers to obtain INDEX and FOREIGN KEY configurations from model definitions Connector helpers to obtain INDEX and FOREIGN KEY configurations from model definitions loopback-connector#143
Spike: a template implementation of index & constraint migration in SqlConnector Spike: a template implementation of index & constraint migration in SqlConnector loopback-connector#144

Closing this pull request as done.

bajtos added spike Relations Model relations (has many, etc.) labels Apr 9, 2019

bajtos added this to the April 2019 milestone milestone Apr 9, 2019

bajtos self-assigned this Apr 9, 2019

bajtos mentioned this pull request Apr 9, 2019

Spike: Declarative definition of FOREIGN KEY and UNIQUE constraints #2606

Closed

5 tasks

bajtos requested review from b-admike, raymondfeng, emonddr, jannyHou, nabdelgadir, a team and elv1s April 11, 2019 08:34

jannyHou reviewed Apr 12, 2019

View reviewed changes

emonddr approved these changes Apr 12, 2019

View reviewed changes

dhmlau reviewed Apr 12, 2019

View reviewed changes

elv1s reviewed Apr 12, 2019

View reviewed changes

bajtos commented Apr 15, 2019

View reviewed changes

bajtos force-pushed the spike/fk-unique-constraints branch from 80d4c0d to f4a4539 Compare April 15, 2019 11:05

raymondfeng reviewed Apr 15, 2019

View reviewed changes

b-admike reviewed Apr 16, 2019

View reviewed changes

bajtos added 6 commits April 16, 2019 16:08

docs: describe current syntax for indexes and foreign keys

99c6af3

docs: add high-level overview and first proposals

c9c150b

docs: more updates

f8e3bc2

docs: finalize the spike

42d42bf

fixup! add task to update shopping example

12181ae

fixup! clarify the proposal

63f6683

bajtos added 4 commits April 16, 2019 16:08

fixup! typo

92b5e4c

fixup! add new task to deprecate ModelDefinition API

88ca775

fixup! address Biniam's feedback

9f2ee16

fixup! clarify handling of unsupported options

7f92406

bajtos force-pushed the spike/fk-unique-constraints branch from 4029988 to 7f92406 Compare April 16, 2019 14:08

bajtos requested review from elv1s, jannyHou, raymondfeng, dhmlau and b-admike and removed request for raymondfeng April 16, 2019 16:32

dhmlau approved these changes Apr 16, 2019

View reviewed changes

b-admike approved these changes Apr 16, 2019

View reviewed changes

This was referenced Apr 18, 2019

Defining index for a model in @model decorator #2753

Closed

New syntax for INDEX and FOREIGN KEY definition #2766

Open

Document how to configure indexes and foreign keys #2767

Closed

bajtos closed this Apr 18, 2019

This was referenced Apr 18, 2019

Connector helpers to obtain INDEX and FOREIGN KEY configurations from model definitions loopbackio/loopback-connector#143

Open

Spike: a template implementation of index & constraint migration in SqlConnector loopbackio/loopback-connector#144

Open

This was referenced Jun 4, 2019

indexes wrong format in integration test #2809

Closed

fix(loopback4-example-shopping): add unique email constraint to user … loopbackio/loopback4-example-shopping#136

Merged

dhmlau mentioned this pull request Aug 23, 2019

Loopback model definition is not adding foreign key relation in database table #3594

Closed

bajtos mentioned this pull request Oct 15, 2019

Fix - fail to honor non-unique property constraint loopbackio/loopback-connector-postgresql#389

Closed

2 tasks


		Individual indexes can be defined as follows:

		- Add a new field `properties` as a key-value map from property names to


		### Decorators

		I am proposing to reuse existing `@property` and `@model` decorators. They are


		### Foreign keys at property level

		Introduce a new property metadata "references" (inspired by ANSI SQL):

RFC: Declarative definition of FOREIGN KEY and UNIQUE constraints #2712

RFC: Declarative definition of FOREIGN KEY and UNIQUE constraints #2712

Conversation

bajtos commented Apr 9, 2019 • edited Loading

Checklist

bajtos commented Apr 11, 2019

Choose a reason for hiding this comment

emonddr Apr 12, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emonddr left a comment

Choose a reason for hiding this comment

dhmlau left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bajtos Apr 15, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elv1s left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elv1s Apr 12, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bajtos Apr 16, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bajtos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bajtos Apr 15, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bajtos Apr 16, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

b-admike left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bajtos commented Apr 16, 2019

bajtos commented Apr 16, 2019

dhmlau left a comment

Choose a reason for hiding this comment

b-admike left a comment

Choose a reason for hiding this comment

bajtos commented Apr 18, 2019 • edited Loading

bajtos commented Apr 9, 2019 •

edited

Loading

emonddr Apr 12, 2019 •

edited

Loading

bajtos Apr 15, 2019 •

edited

Loading

elv1s Apr 12, 2019 •

edited

Loading

bajtos Apr 16, 2019 •

edited

Loading

bajtos Apr 15, 2019 •

edited

Loading

bajtos Apr 16, 2019 •

edited

Loading

bajtos commented Apr 18, 2019 •

edited

Loading