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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 10 commits into from

Conversation

@bajtos
Copy link
Member

commented Apr 9, 2019

See #2606

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 bajtos added this to the April 2019 milestone milestone Apr 9, 2019

@bajtos bajtos self-assigned this Apr 9, 2019

@bajtos bajtos requested review from b-admike, raymondfeng, emonddr, jannyHou, nabdelgadir, strongloop/loopback-next and elv1s Apr 11, 2019

@bajtos

This comment has been minimized.

Copy link
Member Author

commented Apr 11, 2019

The spike is ready for review.


Individual indexes can be defined as follows:

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

This comment has been minimized.

Copy link
@jannyHou

jannyHou Apr 12, 2019

Contributor

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?

This comment has been minimized.

Copy link
@emonddr

emonddr Apr 12, 2019

Contributor

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")
       }
},

This comment has been minimized.

Copy link
@bajtos

bajtos Apr 15, 2019

Author Member

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 {
  // ...
}
Show resolved Hide resolved _SPIKE_.md Outdated
properties: {
email: 'ASC',
},
mongodb: {

This comment has been minimized.

Copy link
@jannyHou

jannyHou Apr 12, 2019

Contributor

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'}
  }
}

This comment has been minimized.

Copy link
@bajtos

bajtos Apr 15, 2019

Author Member

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;
  // ...
}
connector cannot process. The flag can allow three values:

- true: abort migration with error
- 'warn': print a warning and ignore unsupported metadata

This comment has been minimized.

Copy link
@jannyHou

jannyHou Apr 12, 2019

Contributor

sounds good, I would suggest warn as default.

This comment has been minimized.

Copy link
@dhmlau

dhmlau Apr 12, 2019

Contributor

+1 on warning to be the default.

Show resolved Hide resolved _SPIKE_.md Outdated
@emonddr
Copy link
Contributor

left a comment

Please clarify our questions on individual indexes. thx.

@dhmlau
Copy link
Contributor

left a comment

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

_SPIKE_.md Outdated

- UNIQUE index with no special configuration

```js

This comment has been minimized.

Copy link
@dhmlau

dhmlau Apr 12, 2019

Contributor

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?

This comment has been minimized.

Copy link
@jannyHou

jannyHou Apr 12, 2019

Contributor

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

This comment has been minimized.

Copy link
@bajtos

bajtos Apr 15, 2019

Author Member

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.

_SPIKE_.md Outdated

### Decorators

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

This comment has been minimized.

Copy link
@dhmlau

dhmlau Apr 12, 2019

Contributor

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

This comment has been minimized.

Copy link
@elv1s

elv1s Apr 12, 2019

Member

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.

This comment has been minimized.

Copy link
@bajtos

bajtos Apr 15, 2019

Author Member

(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;
}
connector cannot process. The flag can allow three values:

- true: abort migration with error
- 'warn': print a warning and ignore unsupported metadata

This comment has been minimized.

Copy link
@dhmlau

dhmlau Apr 12, 2019

Contributor

+1 on warning to be the default.

@elv1s
Copy link
Member

left a comment

A couple questions, but I like the changes.

_SPIKE_.md Outdated

### Decorators

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

This comment has been minimized.

Copy link
@elv1s

elv1s Apr 12, 2019

Member

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.


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:

This comment has been minimized.

Copy link
@elv1s

elv1s Apr 12, 2019

Member

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

This comment has been minimized.

Copy link
@bajtos

bajtos Apr 15, 2019

Author Member

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.

This comment has been minimized.

Copy link
@b-admike

b-admike Apr 16, 2019

Member

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?

This comment has been minimized.

Copy link
@bajtos

bajtos Apr 16, 2019

Author Member

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',
  }
}

### Foreign keys at property level

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

This comment has been minimized.

Copy link
@elv1s

elv1s Apr 12, 2019

Member

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?

This comment has been minimized.

Copy link
@bajtos

bajtos Apr 15, 2019

Author Member

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.

const propMeta: PropertyDefinition = Object.assign(
{},
// properties provided by the caller
propertyDefinition,
// properties enforced by the decorator
{
type: MetadataInspector.getDesignTypeForProperty(
decoratedTarget,
decoratedKey,
),
// TODO(bajtos) Make the foreign key required once our REST API layer
// allows controller methods to exclude required properties
// required: true,
},
);
property(propMeta)(decoratedTarget, decoratedKey);

    if (!('references' in propertyDefinition)) {
      propertyDefinition.references = {
         model: targetResolver,
         property: keyTo,
      };
    }

    const propMeta: PropertyDefinition = Object.assign(
      // etc. (no change)
    );
@bajtos
Copy link
Member Author

left a comment

Thank you all for valuable feedback!

_SPIKE_.md Outdated

- UNIQUE index with no special configuration

```js

This comment has been minimized.

Copy link
@bajtos

bajtos Apr 15, 2019

Author Member

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.


Individual indexes can be defined as follows:

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

This comment has been minimized.

Copy link
@bajtos

bajtos Apr 15, 2019

Author Member

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 {
  // ...
}
properties: {
email: 'ASC',
},
mongodb: {

This comment has been minimized.

Copy link
@bajtos

bajtos Apr 15, 2019

Author Member

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;
  // ...
}

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:

This comment has been minimized.

Copy link
@bajtos

bajtos Apr 15, 2019

Author Member

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.

### Foreign keys at property level

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

This comment has been minimized.

Copy link
@bajtos

bajtos Apr 15, 2019

Author Member

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.

const propMeta: PropertyDefinition = Object.assign(
{},
// properties provided by the caller
propertyDefinition,
// properties enforced by the decorator
{
type: MetadataInspector.getDesignTypeForProperty(
decoratedTarget,
decoratedKey,
),
// TODO(bajtos) Make the foreign key required once our REST API layer
// allows controller methods to exclude required properties
// required: true,
},
);
property(propMeta)(decoratedTarget, decoratedKey);

    if (!('references' in propertyDefinition)) {
      propertyDefinition.references = {
         model: targetResolver,
         property: keyTo,
      };
    }

    const propMeta: PropertyDefinition = Object.assign(
      // etc. (no change)
    );
_SPIKE_.md Outdated

### Decorators

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

This comment has been minimized.

Copy link
@bajtos

bajtos Apr 15, 2019

Author Member

(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;
}
Show resolved Hide resolved _SPIKE_.md Outdated

@bajtos bajtos force-pushed the spike/fk-unique-constraints branch from 80d4c0d to f4a4539 Apr 15, 2019

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

This comment has been minimized.

Copy link
@raymondfeng

raymondfeng Apr 15, 2019

Member

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?

This comment has been minimized.

Copy link
@bajtos

bajtos Apr 16, 2019

Author Member

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?

This comment has been minimized.

Copy link
@b-admike

b-admike Apr 16, 2019

Member

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

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

This comment has been minimized.

Copy link
@raymondfeng

raymondfeng Apr 15, 2019

Member

Do we allow index: boolean | IndexOptions?

This comment has been minimized.

Copy link
@bajtos

bajtos Apr 16, 2019

Author Member

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
Copy link
Member

left a comment

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.

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

This comment has been minimized.

Copy link
@b-admike

b-admike Apr 16, 2019

Member

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)?

This comment has been minimized.

Copy link
@bajtos

bajtos Apr 16, 2019

Author Member

Good point 馃憤


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:

This comment has been minimized.

Copy link
@b-admike

b-admike Apr 16, 2019

Member

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?

- 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`

This comment has been minimized.

Copy link
@b-admike

b-admike Apr 16, 2019

Member

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

This comment has been minimized.

Copy link
@bajtos

bajtos Apr 16, 2019

Author Member

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 added some commits Apr 9, 2019

bajtos added some commits Apr 15, 2019

@bajtos bajtos force-pushed the spike/fk-unique-constraints branch from 4029988 to 7f92406 Apr 16, 2019

@bajtos

This comment has been minimized.

Copy link
Member Author

commented Apr 16, 2019

Pushed two new commits:

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

This comment has been minimized.

Copy link
Member Author

commented Apr 16, 2019

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

@bajtos bajtos requested review from elv1s, jannyHou, raymondfeng, dhmlau and b-admike and removed request for raymondfeng Apr 16, 2019

@dhmlau

dhmlau approved these changes Apr 16, 2019

Copy link
Contributor

left a comment

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

@b-admike
Copy link
Member

left a comment

LGTM 馃憦

@bajtos

This comment has been minimized.

Copy link
Member Author

commented Apr 18, 2019

Created the following follow-up stories:

Closing this pull request as done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can鈥檛 perform that action at this time.