Model JSON default scope declaration breaks update

[spirospolitis]

Assuming a model definition like so:

(test-model.json)

{
  "name": "TestModel",
  "plural": "TestModels",
  "base": "PersistedModel",
  "idInjection": false,
  "mysql": {
    "schema": "test_schema",
    "table": "test_model"
  },
  "properties": {
     "id": {
      "type": "Number",
      "id": true,
      "required": true,
      "index": true,
      "length": null,
      "precision": 11,
      "scale": 0,
      "mysql": {
        "columnName": "id",
        "dataType": "int",
        "dataLength": null,
        "dataPrecision": 11,
        "dataScale": 0,
        "nullable": "N"
      },
      "_selectable": true
    },
    "name": {
      "type": "String",
      "required": true,
      "length": 100,
      "precision": null,
      "scale": null,
      "mysql": {
        "columnName": "name",
        "dataType": "varchar",
        "dataLength": 100,
        "dataPrecision": null,
        "dataScale": null,
        "nullable": "N"
      },
      "_selectable": true
    },
    "deleted": {
        "type": "Boolean",
        "required": true,
        "length": null,
        "precision": null,
        "scale": null,
        "mysql": {
            "columnName": "deleted",
            "dataType": "tinyint",
            "dataLength": null,
            "dataPrecision": 1,
            "dataScale": 0,
            "nullable": "N"
        },
        "_selectable": false
    }
  },
  "scope": {
      "where": {
          "deleted": false
      }
  },
  "validations": [],
  "relations": {},
  "acls": [],
  "methods": []
}

and having overridden the TestModel.deleteById method:

(test-model.js)

module.exports = function(TestModel) {
    TestModel.on('attached', function() {
        TestModel.deleteById = function(id, cb) {
            console.log('Overridden TestModel.deleteById');

            TestModel.update({'id': id}, {'deleted': true}, cb);
        }
    });
};

the query produced is the following (datasource "debug": true):

--> ComQueryPacket
{ command: 3,
  sql: 'UPDATE `test_model` SET `deleted` = 0 WHERE (`id` = 1) AND (`deleted` = 0)' }

Seems that the default scope column "deleted" value is assigned as the SET value of the "deleted" param.

Note that if one is to remove the default scope declaration from the .json file, the update works as expected.

Regards.

[raymondfeng]

Default scope is applied to operations that have a where filter.

[spirospolitis]

Hi Raymond,

Surely I understand that, my point is that the default scope actually changes the value of the attribute that is to be set in the update clause (i.e. converts the SQL string from 'UPDATE test_model SET deleted = 1 WHERE (id = 1) AND (deleted = 0)' to 'UPDATE test_model SET deleted = 0 WHERE (id = 1) AND (deleted = 0)').

[raymondfeng]

I see what you mean now. The default scope idea is from Ruby On Rails (http://api.rubyonrails.org/classes/ActiveRecord/Scoping/Default/ClassMethods.html#method-i-default_scope).

The default_scope is also applied while creating/building a record. It is not applied while updating a record.

It would be tricky for upsert though.

@fabien What's your take?

[fabien]

@raymondfeng I think the current behavior is correct, and you can fine-tune the behavior by overriding the defaultScope and possibly applyProperties methods of your model. It is actually encouraged to alter these methods for cases like this.

Note that defaultScope came about to fully support the new context handling within Loopback. That is the most useful use-case. Otherwise you'd usually be better off using normal scopes instead.

[raymondfeng]

Closing the issue per @fabien

[smehrbrodt]

@fabien Are you sure this is not a bug? Why would you not allow to update a property that you used to define a scope?

[spirospolitis]

@smehrbrodt +1

[markleusink]

Took me a while to figure out why this wasn't working. And I agree with @spirospolitis and @smehrbrodt : setting a default scope shouldn't affect updating properties used for that scope. If this won't be fixed, then please add some clarification to the docs.

[samzhao]

@markleusink +1

Please let us know how to get around this problem. Thanks!

[smehrbrodt]

This is the workaround I used:

Report.trash = function(reportId, cb) {

  // Need to reset the default scope because of https://github.com/strongloop/loopback/issues/1018
  var defaultScope = Report.defaultScope;
  Report.defaultScope = function(){};

  Report.upsert({id: reportId, 'deleted': true}, function(err, obj){
    cb(null, '');
  });

  // Restore the default scope
  Report.defaultScope = defaultScope;
};

[samzhao]

@smehrbrodt thanks for posting your workaround! I couldn't get this to work for the built-in remote methods though... what I was trying to do is I have 2 models: Project and Status. Project model has "status_id", and I wish to include the entire status object in the response after I update the "status_id" on a project. Does that mean I have to override the default update or upsert remote method as well?

[gausie]

This bug should be reopened.

The default_scope is also applied while creating/building a record. It is not applied while updating a record.

The above, as @raymondfeng quoted, is currently not the case. The defaultScope is currently applied when updating a record.

[slgoldberg]

The docs still make absolutely no mention of this that I can find (either on create or update). From reading the docs, it just sounds like the default scope works on queries (not on creates or updates). It took me 3 hours of pulling my hair out to figure out what was going on.

To make things further confusing, the model definition JSON file format doesn't refer to this as defaultScope, but rather just as scope (vs. scopes for the named scopes). So, I could never have figured out on my own (without pouring over the code) that I just needed to override/stub out the defaultScope method to get the behavior I was expecting.

And it never dawned on me for a second that a default scope filter with a where clause would force the values of upserts to match the where filter. That really shocked me when I finally discovered that on my own (after hours of wasted time).

I read the docs thoroughly, and was just looking at them going, "but why?" Then, I found this thread. I wish I'd found this thread first. :-(

Anyway -- the docs should really simply say something like this:

---

Default Scopes with where Filters

Adding a scope to a model definition (e.g., in model.json) causes a method to be generated
for the model, called defaultScope(). This method is called automatically whenever a model
is created, updated, or queried.

But it is important to note that default scopes with a where filter may not work as you expect!

Each time a model instance is created or updated, the generated defaultScope() method will
modify the model's properties matching the where filter to enforce the values specified.

If you do not wish to have the default scope applied in this way, we recommend using named
scopes where possible.

If you must use a default scope, but don't want it to affect upsert(), for example, you simply need to override the model's defaultScope() method prior to calling upsert(), as in this example:

  var defaultScope = Report.defaultScope;
  Report.defaultScope = function(){};
  Report.upsert({id: reportId, 'deleted': true}, ...);
  Report.defaultScope = defaultScope;

---

Since the model definition calls it scope, despite the docs referring to it as defaultScope, this might also help clarify the difference between these two ways of referring to the same thing.

Note that for create, I agree it makes sense (and seems like a powerful feature) to be able to enforce constraints on inserted objects (but again, it should be documented if it's doing that).

But for update, the case is not nearly as obvious why this is the behavior. ( @raymondfeng @fabien ) Especially if that is different than the way it works in Rails from which the feature draws its inspiration. You can't expect people to know about the Rails model and then deviate from it, and in neither case documenting it! :-(

Either way, it'd be fine if only the docs explained this. Expecting people to infer how it works based solely on the inspiration the feature drew from Rails seems a bit of a stretch.

Please update the docs and consider reopening the bug to perhaps change update to not trash our models :-) without at least making us explicitly ask it to (e.g. via an option).

Thanks.

Steve