docs: finalize the spike

loopbackio · Apr 15, 2019 · 2a7c662 · 2a7c662
1 parent 39a8999
commit 2a7c662
Show file tree

Hide file tree

Showing 3 changed files with 297 additions and 27 deletions.
diff --git a/_SPIKE_.md b/_SPIKE_.md
@@ -1,7 +1,20 @@
 # Declarative definition of FOREIGN KEY and UNIQUE constraints
 
+- [Current status](#current-status)
+- [Proposal](#proposal)
+  - [Indexes at property level](#indexes-at-property-level)
+  - [Indexes at model level](#indexes-at-model-level)
+  - [Foreign keys at property level](#foreign-keys-at-property-level)
+  - [Foreign keys at model level](#foreign-keys-at-model-level)
+  - [Decorators](#decorators)
+  - [Additional changes](#additional-changes)
+- [Next steps](#next-steps)
+- [Detailed description of the current syntax](#detailed-description-of-the-current-syntax)
+
 ## Current status
 
+UNIQUE index is supported as a specialized version of a generic index feature.
+
 Support for indexes and foreign keys depends on the connector. Some do support
 all features, some support only some of them. Depending on the database used,
 certain features are not possible to implement at all because of database design
@@ -18,6 +31,10 @@ all SQL connectors that provide migration of foreign keys. Unfortunately, this
 syntax does not support composite foreign keys and there is also no support for
 expressing `ON UPDATE` and `ON DELETE` behavior.
 
+You can find more details in
+[Detailed description of the current syntax](#detailed-description-of-the-current-syntax)
+at the bottom.
+
 ## Proposal
 
 ### Indexes at property level
@@ -47,15 +64,17 @@ form to define indexes that are more complex.
   }
   ```
 
-We should also allow models to configure indexes for certain databases only,
-using any of the forms shown above.
+We should also allow models to configure indexes for certain databases only by
+using database-specific options we already use to control column name, storage
+data type, etc.
 
 ```js
 {
   email: {
     type: 'string',
     mysql: {
       index: true,
+      // unique: true
     }
   }
 }
@@ -73,11 +92,98 @@ Problem 2: do we want to use database column names or LB property names? These
 two names can be different, especially when using database-specific config to
 map property names to custom column names.
 
-The proposal:
+**The proposal:**
+
+At high-level, keep the current syntax where indexes are defined via a key-value
+map stored in `settings.indexes` property, the key is the index name and the
+value is an index definition object.
+
+```js
+{
+  strict: false,
+  forceID: true,
+  indexes: {
+    uniqueEmail: {
+      // index definition
+    },
+    nameQueries: {
+      // index definition
+    }
+  }
+}
+```
+
+Individual indexes can be defined as follows:
+
+- Add a new field `properties` as a key-value map from property names to
+  indexing order:
+
+  ```js
+  // definition of an individual index
+  {
+    properties: {
+      email: 1, // ASC
+      createdAt: 'DESC', // alias for -1
+      bio: 'text', // database-specific value (MongoDB's "text")
+    }
+  }
+  ```
+
+  Important: property names are mapped to database column names when building
+  the index definition.
+
+- Keep supporting `keys` field as a key-value map from database column name to
+  indexing order, see the description of the actual status below. Entries from
+  `keys` should me merged with entries from `properties`, `keys` taking
+  precedence (replacing `properties` entries).
+
+- Keep supporting `unique` field (set it to `true` to let the index enforce
+  uniqueness).
+
+- Database-specific options will be stored under a key with the connector name:
+
+  ```js
+  {
+    properties: {
+      email: 'ASC',
+    },
+    mongodb: {
+      sparse: true,
+    },
+    mysql: {
+      kind: 'fulltext',
+      type: 'hash',
+    },
+    postgresql: {
+      type: 'hash',
+    }
+  }
+  ```
+
+- To support more complex indexes offered by e.g. PostgreSQL, we should support
+  `columns` field too. This field should contain an array of column-index
+  specification, for example:
 
+  ```js
+  ['email ASC', 'lower(name) DESC'];
+  ```
 
+  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:
 
-**TBD**
+  ```js
+  {
+    properties: {
+      // default setup used by most connectors
+      email: 'ASC',
+    },
+    postgresql: {
+      // in PostgreSQL, use a different index definition
+      columns: ['lower(email) ASC']
+    },
+  }
+  ```
 
 ### Foreign keys at property level
 
@@ -122,16 +228,41 @@ Support connector-specific configuration too.
 
 ### Foreign keys at model level
 
-**TBD**
+Modify the current connector-dependant syntax to make it easier to read and
+support composite foreign keys too.
 
-### Decorators
+```js
+{
+  foreignKeys: {
+    [keyName]: {
+      // optional, overrides keyName
+      name: 'constraint_name_for_db',
+
+      // Property name(s) (will be mapped to column name)
+      // formerly: foreignKey
+      sourceProperties: ['source property name'],
+
+
+      // formerly: entity
+      targetModel: 'TargetModel',
+
+      // Property name(s) (will be mapped to column name)
+      // formerly: entityKey
+      targetProperties: ['target property name'],
+
+      // referential actions
+      onUpdate: 'CASCADE',
+      onDelete: 'CASCADE',
+    }
+  }
+}
+```
 
-**TBD**
+### Decorators
 
-- index at property level
-- index at model level
-- foreign key at property level
-- foreign key at model level
+I am proposing to reuse existing `@property` and `@model` decorators. They are
+accepting full Model/Property definition interfaces, no additional changes are
+needed to make them support the new index/FK syntax.
 
 ### Additional changes
 
@@ -141,22 +272,65 @@ the problem. To preserve backwards compatibility, I am proposing to print a
 console warning only.
 
 We can also introduce a new `options` flag for `autoupdate`/`automigrate` to
-control what happens when the model is specifying metadata that the connector
-cannot process. The flag can allow three values:
+control what happens when a model definition specifies metadata that the
+connector cannot process. The flag can allow three values:
 
 - true: abort migration with error
 - 'warn': print a warning and ignore unsupported metadata
 - false: silently ignore unsupported metadata
 
 ## Next steps
 
-- Describe the syntax in LB4 documentation
-- Implement new decorators in `@loopback/repository`
-- Spike: create a template implementation of index & constraint migration in
-  `Connector` and `SqlConnector`, this will be a pre-requisite for later work in
-  connectors.
-- Update `examples/todo-list` to define FK and UNIQUE constraints
-- Update CLI templates for relations to define the constraints too
+1. Describe the new syntax in definition interfaces in
+   [`model.ts`](https://github.com/strongloop/loopback-next/blob/master/packages/repository/src/model.ts),
+   include comprehensive API documentation.
+
+   Modify `DefaultCrudRepository` constructor to process model-level indexes and
+   foreign keys; it needs to fill the corresponding fields in juggler model
+   settings.
+
+   Update `examples/todo-list` to define FK and UNIQUE constraints
+
+   Update CLI templates for relations to define the constraints too. If the pull
+   request [#2426](https://github.com/strongloop/loopback-next/pull/2426) is not
+   landed yet then create a follow-up story instead.
+
+2. Add a new documentation page `How-tos >> Configure indexes and foreign keys`,
+   provide a high-level overview of indexes, foreign keys and how to define them
+   for LB4 models.
+
+   Explain that we don't have full support for this feature yet, link to GitHub
+   issues tracking the work.
+
+   Show how to configure indexes and foreign keys at model level via legacy
+   juggler settings, link to LB3 docs:
+   https://loopback.io/doc/en/lb3/Model-definition-JSON-file.html#indexes. Offer
+   this approach as a workaround until we implement full support. (See also
+   https://github.com/strongloop/loopback-next/issues/2723)
+
+3. Implement a helper functions in base `Connector` class to process index and
+   foreign-key definitions in an unified way and convert them into data suitable
+   for consumption by connector implementation. The helpers should apply
+   database-specific options, merges keys/properties fields in index
+   definitions, etc. and produce definition that can be directly mapped to
+   database commands. This will become a prerequisite for implementation of
+   indexes & foreign keys in other connectors, most notably `memory` connector
+   ([#2333](https://github.com/strongloop/loopback-next/issues/2333)) and
+   `SqlConnector` (see below).
+
+4. Spike: a template implementation of index & constraint migration in
+   `SqlConnector`. The intention is to share as much of index/FK migration logic
+   among all SQL connectors. This spike will allow us to better understand the
+   effort needed to implement migration in our SQL connectors
+   ([#2332](https://github.com/strongloop/loopback-next/issues/2332))
+
+   Use the existing implementation in MySQL, PostgreSQL and MSSQL connectors for
+   inspiration.
+
+   Detect index/FK metadata not supported by SQL and report warnings to console.
+
+5. Modify the description of the story to implement index/FK in `memory`
+   connector, require the connector to warn about index/PK fields not supported.
 
 ## Detailed description of the current syntax
 
@@ -346,11 +520,14 @@ Foreign keys are always defined at model-settings level.
 - 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`
+- Not supported: composite foreign keys
 
 ```js
 {
   foreignKeys: {
-    keyName: {
+    [keyName]: {
+      // optional, overrides keyName
       name: 'constraint_name_for_db',
       entity: 'TargetModel',
 
@@ -362,5 +539,3 @@ Foreign keys are always defined at model-settings level.
   }
 }
 ```
-
-TODO: support `ON UPDATE` and `ON DELETE` options, e.g. `ON DELETE CASCADE`
diff --git a/examples/todo-list/src/models/todo.model.ts b/examples/todo-list/src/models/todo.model.ts
@@ -3,10 +3,29 @@
 // This file is licensed under the MIT License.
 // License text available at https://opensource.org/licenses/MIT
 
-import {Entity, property, model, belongsTo} from '@loopback/repository';
+import {belongsTo, Entity, model, property} from '@loopback/repository';
 import {TodoList} from './todo-list.model';
 
-@model()
+@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 {
   @property({
     type: 'number',
@@ -17,6 +36,8 @@ export class Todo extends Entity {
   @property({
     type: 'string',
     required: true,
+    /*==== The title must be unique ====*/
+    unique: true,
   })
   title: string;
 
@@ -27,10 +48,23 @@ export class Todo extends Entity {
 
   @property({
     type: 'boolean',
+    /*==== Index this flag for faster lookup of completed todos ====*/
+    index: true,
   })
   isComplete: boolean;
 
-  @belongsTo(() => TodoList)
+  @belongsTo(
+    () => TodoList,
+    {},
+    {
+      /*==== Define a foreign key to enforce referential integrity ===*/
+      references: {
+        model: () => TodoList,
+        property: 'id',
+        onDelete: 'CASCADE',
+      },
+    },
+  )
   todoListId: number;
 
   getId() {