Support multi-tenancy in change replication - allow apps to extend Change model with extra properties #2959

kobaska · 2016-11-21T08:14:55Z

Description

Enable replication to scope changes by tenant. This makes querying for changes faster, as we can filter by tenant where it is provided.

Related issues

None

Checklist

New tests added or existing tests modified to cover all changes
Code conforms with the style
guide

slnode · 2016-11-21T08:14:57Z

Can one of the admins verify this patch?

slnode · 2016-11-21T08:14:57Z

Can one of the admins verify this patch?

slnode · 2016-11-21T08:14:57Z

Can one of the admins verify this patch?

slnode · 2016-11-21T08:14:57Z

Can one of the admins verify this patch?

kobaska · 2016-11-21T23:27:33Z

@bajtos Can this test be rerun. It is failing on existing code.

bajtos · 2016-11-22T08:32:47Z

Hi @kobaska, thank you for the pull request. I am rather reluctant to add this feature directly to LoopBack, as it's too specific in my opinion. I would prefer to see it in an independent mixin instead. If the current code in LoopBack does not provide enough extension points allowing to implement such mixin, then I am happy to add those extension points only.

bajtos

See the comments below for more details how to implement your tenant-based change replication in a mixin.

bajtos · 2016-11-22T08:36:57Z

common/models/change.js

-        change.debug('rev and prev are equal (not updating anything)');
-        return cb(null, change);
-      }
+      change.tenant = getTenant(model, inst);


If I understand this part correctly, then it's adding a computed tenant property to each Change instance. I think it should be possible to accomplish this by using "before save" or "persist" hook.

module.exports = myMixinFunction(TargetModel, config) { TargetModel.getChangeModel().observe('before save', function(ctx, next) { var tenant = getTenant(TargetModel, ctx.instance || ctx.data); if (tenant) { (ctx.instance || ctx.data).tenant = tenant; } }); });

@bajtos
An issue that has risen with this solution, is that we have to pass the Model instance instead of change instance to the getTenant function. We could do a query for the model instance as we have the model id from the ctx, but this adds another query to each save.

Adding tenant to the change object and calculating the rev, both require the model instance. Our solution gets the model once in rectify function and uses that to set the tenant and revision.

First of all, I would like to acknowledge that I consider your use case, where you would like to copy certain model properties to each change record, as a valid one that we should support.

The question now is how to implement this feature in a way that's generic enough to be useful for other users of the framework too.

Can we perhaps build on top of my proposal in #2959 (comment), where I am describing a new method PersistedModel.createChangeFilter used when querying changes, and use a similar approach for the other part where we are creating/updating the change records? Something like PersistedModel.createChangeData that should supersede current Change.revisionForInstance method. The downside is that this proposal will require complex changes in several places, as we will have to stop passing a single property revision and start passing a set of properties instead.

Alternatively, can we perhaps keep your current design, but make it more flexible by calling a method on PersistedModel instead of calling a hard-coded getTenant method?

Here is a quick mock-up:

// here in this method model.findById(id, function(err, inst) { if (err) return cb(err); if (inst) { inst.fillChangeProperties(change); } change.currentRevision(inst, function(err, rev) { // etc. }); }); // in lib/persisted-model.js PersistedModel.prototype.fillChangeProperties = function(change) { // no-op by default };

Can we come up with a better/more descriptive method name than fillChangeProperties?

Should this new method by async (accepting a callback arg) to allow the method to possibly fetch additional data?

bajtos · 2016-11-22T08:38:18Z

common/models/change.js

-    });
+
+    if (instance) {
+      cb(null, Change.revisionForInst(instance));


This does not make sense to me. Just replace calls of change.currentRevision(inst) with change.constructor.revisionForInst(inst) and then you don't need to change this method.

bajtos · 2016-11-22T08:42:41Z

lib/persisted-model.js

   * @callback {Function} callback Callback function called with `(err, changes)` arguments.  Required.
   * @param {Error} err Error object; see [Error object](http://docs.strongloop.com/display/LB/Error+object).
   * @param {Array} changes An array of [Change](#change) objects.
   */

-  PersistedModel.changes = function(since, filter, callback) {
+  PersistedModel.changes = function(since, filter, tenant, callback) {


Ok, I think this part would be difficult to implement in a mixin with the current loopback code.

Since the "tenant" property is copied from the target PersistedModel to the Change model, one should be able to express the desire to limit synchronization to a certain tenant using existing filter argument only.

MyModel.changes(since, { where: { tenant: tenantId } }, cb);

Then, inside changes function, we should be able to detect whether filter contains tenant clause and copy it to changeFilter. The only change needed from LoopBack is to allow mixins to override/customize the way how changeFilter is built.

// in loopback PersistedModel.createChangeFilter = function(since, modelFilter) { return { where: { checkpoint: {gte: since}, modelName: this.modelName, }, }; }; // in your mixin module.exports = myMixinFunction(TargetModel, config) { // ... originalFilterFactory = TargetModel.createChangeFilter; TargetModel.createChangeFilter = function(since, modelFilter) { var tenantProperty = // find it out var filter = originalFilterFactory.call(this, since, modelFilter); if (modelFilter && modelFilter.where && modelFilter.where[tenantProperty]) { filter.where.tenant = modelFilter.where[tenantProperty]; } return filter; }; };

bajtos · 2016-12-01T14:41:32Z

@kobaska ping, what's the status of this patch? Are you still keen to get it finished?

kobaska · 2016-12-23T05:06:33Z

@bajtos, I'm going to be away for a month on holiday. Will do the modifications when I'm back.

kobaska · 2017-01-23T06:46:56Z

@bajtos I have done the changes using mixins instead as you suggested. Can you review this please?

bajtos

@kobaska Excellent, welcome back from your holiday :)

The code looks much better now, you are on the right track 👍 Please see my comments below for things to fix and improve.

bajtos · 2017-01-24T13:49:14Z

common/models/change.js

@@ -230,7 +248,7 @@ module.exports = function(Change) {
          if (currentRev) {
            change.prev = currentRev;
          } else if (!change.prev) {
-            change.debug('ERROR - could not determing prev');
+            change.debug('ERROR - could not determining prev');


could not determine prev

bajtos · 2017-01-24T13:51:54Z

common/models/change.js

    return cb.promise;

+    function prepareAndDoRectify(inst) {
+      change.currentRevision(inst, function(err, rev) {


Is there any particular reason why we cannot call directly change.constructor.revisionForInst here? That way we can keep Change.prototype.currentRevision unchanged.

bajtos · 2017-01-24T13:52:48Z

common/models/change.json

+    },
+    "tenant": {
+      "type": "string",
+      "default": null


Please remove. Any custom Change properties should be defined by the 3rd-party code (loopback unit-tests, a mixin implementing multi-tenancy).

bajtos · 2017-01-24T13:54:39Z

lib/persisted-model.js

+   * filter as part the change search filter
+   * @param {Number}   since    Return only changes since this checkpoint.
+   * @param {Object}   modelFilter   filter used for the model
+   * @returns {{where: {checkpoint: {gte: *}, modelName: *}}}


@returns {Object} The filter object to pass to `Change.find()`, typically `{{where: {checkpoint: {gte: *}, modelName: *}}}`

Could you please include a short example showing how to correctly customize this method (see one of my earlier comments for a code snippet to use).

bajtos · 2017-01-24T13:56:37Z

lib/persisted-model.js

+   * @callback {Function} callback
+   * @param change
+   */
+  PersistedModel.prototype.fillChangeTenantValue = function(change, cb) {


We need a generic method that will work for other use cases beyond multi-tenancy. I am proposing fillChangeProperties or addCustomChangeData

bajtos · 2017-01-24T14:12:42Z

test/replication.test.js

+    test.startingCheckpoint = -1;
+  });
+
+  describe('Model.changes(since, filter, callback) - tenant variant', function() {


No need to repeat tenant variant, please remove.

bajtos · 2017-01-24T14:13:37Z

test/replication.test.js

+  describe('Model.changes(since, filter, callback) - tenant variant', function() {
+    var tenant = '123';
+
+    it('Get changes since the given checkpoint for the given tenant', function(done) {


queries changes using improved filter for custom properties

bajtos · 2017-01-24T14:16:45Z

test/replication.test.js

+        setTimeout(function() {
+          test.SourceModelWithTenant.changes(test.startingCheckpoint, {where: {tenantId: tenant}}, function(err, changes) {
+            assert.equal(changes.length, 1);
+            assert.equal(changes[0].tenant, tenant);


I believe this test would pass even without your changes applied. While it's ok to verify that the expected changes are returned, we also need to verify that correct queries were made against the datasource, i.e. that tenantId filter was applied when making the first find query on the Change model.

bajtos · 2017-01-24T14:17:54Z

test/replication.test.js

+        SourceModelWithTenant.changes(FUTURE_CHECKPOINT, {where: {tenantId: tenant}}, function(err, changes) {
+          if (err) return done(err);
+
+          expect(changes).to.be.empty; //jshint ignore:line


expect(changes).to.be.empty();

Just make sure expect is defined as var expect = require('./helpers/expect') at the top of this file.

bajtos · 2017-01-24T14:19:13Z

test/replication.test.js

+      });
+    });
+
+    it('excludes changes from older checkpoints', function(done) {


I think this test is redundant - checkpoint filter is already tested, a custom createChangeFilter should not affect it as long as it preserves the original filter produced by built-in createChangeFilter method.

bajtos · 2017-02-09T14:22:09Z

@kobaska ping, what's the status of this patch?

kobaska · 2017-02-09T21:50:59Z

@bajtos I'll get on it next week. Been busy with other stuff.

kobaska · 2017-02-23T01:00:44Z

Hi @bajtos,

I have had another go, have a look and see. Regarding adding the custom property on Change model, I couldn't figure out a way to do that through mixins, as they all run after setup, which is too late. Hence I have added a new property additionalChangeModelProperties in settings to do this. Let me know if there is a better way to do this.

bajtos · 2017-03-02T12:42:53Z

I have added a new property additionalChangeModelProperties in settings to do this

I think that's a reasonable solution 👍

The code changes looks mostly good to me, I'll make few final improvements myself. First of all, I am going to rebase this patch against master, as we are no longer landing new features to 2.x.

bajtos · 2017-03-02T14:05:52Z

common/models/change.js

@@ -184,15 +184,15 @@ module.exports = function(Change) {

    cb = cb || utils.createPromiseCallback();

-    var model = this.getModelCtor();
-    var id = this.getModelId();
+    const model = this.getModelCtor();


http://loopback.io/doc/en/contrib/style-guide.html

bajtos · 2017-03-02T14:07:03Z

lib/persisted-model.js

-   *     var filter = originalFilterFactory.call(this, since, modelFilter);
-   *     if (modelFilter && modelFilter.where && modelFilter.where[customProperty]) {
-   *       filter.where[customProperty] = modelFilter.where[customProperty];
+   *   TargetModel.createChangeFilter = function(since, modelFilter) {


Simplified the example - use hard-coded property name "tenantId", get the base createChangeFilter from the parent model.

bajtos · 2017-03-02T14:10:55Z

test/change.test.js

@@ -610,71 +610,66 @@ describe('Change', function() {
 });

 describe('Change with with custom properties', function() {
-  var ChangeWithCustomProperty, TestModelWithModifiedChangeModel;
-  var customProperty = 'tenantId';


Similarly here, I find the tests easier to read/understand when the property name is hard-coded.

bajtos · 2017-03-02T14:11:52Z

test/change.test.js

-
-    TestModelWithModifiedChangeModel = loopback.PersistedModel
-    .extend('ChangeTestModel',
+    TestModel = loopback.PersistedModel.extend('ChangeTestModelWithTenant',


Since we are already within "Change with with custom properties" block, I feel there is no need to repeat that information in model property names.

It is important to use a unique model name in extend() arguments though.

bajtos · 2017-03-02T14:12:40Z

test/change.test.js

      {
        id: {id: true, type: 'string', defaultFn: 'guid'},
+        tenantId: 'string',


This was probably an oversight - the custom property should be defined on both Model and Change, right?

bajtos · 2017-03-02T14:13:02Z

test/change.test.js


-    beforeEach(function() {
-      var test = this;
+    beforeEach(givenChangeInstance);


http://loopback.io/doc/en/contrib/style-guide.html

bajtos · 2017-03-02T14:14:16Z

test/replication.test.js

+        additionalChangeModelProperties: {customProperty: {type: 'string'}},
+      });
+
+    SourceModel.createChangeFilter = function(since, modelFilter) {


Moved the setup of createChangeFilter and fillCustomChangeProperties to the before-each hook, as it's tied to additionalChangeModelProperties that's configured in this hook too.

bajtos · 2017-03-02T14:15:24Z

test/change.test.js

-    it('creates a new change with the custom property', function() {
-      return change.rectify()
-      .then(function(ch) {
-        assert.equal(ch[customProperty], customPropertyValue);


We prefer the BDD style assertions.

bajtos

Hi @kobaska, thank you for the updated code. I have rebased it on top of the latest master and cleaned up the code to follow our style guide. I left few comments above to explain some of the changes I made.

Could you please take a look at the result and let me know if it looks good to you? If it does, then I'll squash the commits into a single one and land it.

bajtos · 2017-03-06T17:11:28Z

@slnode test please

Allow custom properties to be added to Change Model, and make change filter customizable through mixins to allow to add the custom property to the filter used to look up relevant changes during change replication.

bajtos · 2017-03-09T10:26:32Z

@slnode test please

bajtos · 2017-03-09T11:08:37Z

Landed 🎉 Thank you for your contribution! 👏

crandmck added the #community contribution label Nov 21, 2016

bajtos self-assigned this Nov 22, 2016

bajtos added the replication label Nov 22, 2016

bajtos suggested changes Nov 22, 2016

View reviewed changes

bajtos added the blocked label Dec 1, 2016

bajtos added the stale label Dec 8, 2016

kobaska force-pushed the add-multi-tenancy branch from be2d8aa to 5acecc8 Compare December 16, 2016 03:26

bajtos mentioned this pull request Jan 6, 2017

Offline synchronization strongloop-community/loopback-sdk-android#71

Closed

cgole removed #community contribution labels Jan 7, 2017

kobaska force-pushed the add-multi-tenancy branch from 5acecc8 to 3cf9e2b Compare January 23, 2017 05:45

bajtos suggested changes Jan 24, 2017

View reviewed changes

bajtos added feature and removed stale blocked labels Jan 26, 2017

kobaska force-pushed the add-multi-tenancy branch 4 times, most recently from 78402e8 to e9adf16 Compare February 23, 2017 00:53

bajtos self-requested a review March 1, 2017 15:11

bajtos changed the title ~~Add multitenancy~~ Support multi-tenancy in change replication - allow apps to extend Change model with extra properties Mar 2, 2017

bajtos changed the base branch from 2.x to master March 2, 2017 12:44

bajtos force-pushed the add-multi-tenancy branch from e9adf16 to f0d3cf5 Compare March 2, 2017 12:57

bajtos reviewed Mar 2, 2017

View reviewed changes

bajtos approved these changes Mar 2, 2017

View reviewed changes

Allow custom properties of Change Model

3f4b5ec

Allow custom properties to be added to Change Model, and make change filter customizable through mixins to allow to add the custom property to the filter used to look up relevant changes during change replication.

bajtos force-pushed the add-multi-tenancy branch from 19b5a74 to 3f4b5ec Compare March 9, 2017 07:59

bajtos merged commit 0448184 into strongloop:master Mar 9, 2017

Support multi-tenancy in change replication - allow apps to extend Change model with extra properties #2959

Support multi-tenancy in change replication - allow apps to extend Change model with extra properties #2959

Uh oh!

Conversation

kobaska commented Nov 21, 2016

Description

Related issues

Checklist

Uh oh!

slnode commented Nov 21, 2016

Uh oh!

slnode commented Nov 21, 2016

Uh oh!

slnode commented Nov 21, 2016

Uh oh!

slnode commented Nov 21, 2016

Uh oh!

kobaska commented Nov 21, 2016

Uh oh!

bajtos commented Nov 22, 2016

Uh oh!

bajtos left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bajtos commented Dec 1, 2016

Uh oh!

kobaska commented Dec 23, 2016

Uh oh!

kobaska commented Jan 23, 2017

Uh oh!

bajtos left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bajtos commented Feb 9, 2017

Uh oh!

kobaska commented Feb 9, 2017

Uh oh!

kobaska commented Feb 23, 2017

Uh oh!

bajtos commented Mar 2, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment