1602 / jugglingdb

Multi-database ORM for nodejs: redis, mongodb, mysql, sqlite3, postgresql, arango, in-memory...
http://1602.github.io/jugglingdb/
2.05k stars 241 forks source link

Defaults corruption #393

Open cha0s opened 10 years ago

cha0s commented 10 years ago

Any defaults that contain a reference will be corrupted when a value is set into the corresponding attribute. Here's a short example:


{Schema} = require 'jugglingdb'

schema = new Schema require('jugglingdb/lib/adapters/memory'), {}

Test = schema.define 'Test',

    attribute:
        type: Schema.JSON
        default: {}

test = new Test()

test.attribute['foo'] = 'bar'

test2 = new Test()

throw new Error "Corrupted default" if test2.attribute['foo'] is 'bar'

...and in JS in case you hate Coffee:


(function() {
  var Schema, Test, schema, test, test2;

  Schema = require('jugglingdb').Schema;

  schema = new Schema(require('jugglingdb/lib/adapters/memory'), {});

  Test = schema.define('Test', {
    attribute: {
      type: Schema.JSON,
      "default": {}
    }
  });

  test = new Test();

  test.attribute['foo'] = 'bar';

  test2 = new Test();

  if (test2.attribute['foo'] === 'bar') {
    throw new Error("Corrupted default");
  }

}).call(this);

You can see in model.js that the getDefault function simply returns the default, which in the case of Schema.JSON (and possibly others) is a reference to the default object. This means that when a key is set in a property of a specific model instance, the change will propagate up to (and corrupt) the default object.

I'm not sure what the correct fix is. I am working around this in my own code by using the function version of default, and returning an empty object from a simple closure.

anatoliychakkaev commented 10 years ago

Make it function that returns object.

On 5 April 2014 08:30, Ruben Rodriguez notifications@github.com wrote:

Any defaults that contain a reference will be corrupted when a value is set into the corresponding attribute. Here's a short example:

{Schema} = require 'jugglingdb' schema = new Schema require('jugglingdb/lib/adapters/memory'), {} Test = schema.define 'Test',

attribute:
    type: Schema.JSON
    default: {}

test = new Test() test.attribute['foo'] = 'bar' test2 = new Test() throw new Error "Corrupted default" if test2.attribute['foo'] is 'bar'

...and in JS in case you hate Coffee:

(function() { var Schema, Test, schema, test, test2;

Schema = require('jugglingdb').Schema;

schema = new Schema(require('jugglingdb/lib/adapters/memory'), {});

Test = schema.define('Test', { attribute: { type: Schema.JSON, "default": {} } });

test = new Test();

test.attribute['foo'] = 'bar';

test2 = new Test();

if (test2.attribute['foo'] === 'bar') { throw new Error("Corrupted default"); } }).call(this);

You can see in model.jshttps://github.com/cha0s/promised-jugglingdb/blob/master/lib/model.js#L109that the getDefault function simply returns the default, which in the case of Schema.JSON (and possibly others) is a reference to the default object. This means that when a key is set in a property of a specific model instance, the change will propagate up to (and corrupt) the default object.

I'm not sure what the correct fix is. I am working around this in my own code by using the function version of default, and returning an empty object from a simple closure.

Reply to this email directly or view it on GitHubhttps://github.com/1602/jugglingdb/issues/393 .

randunel commented 10 years ago

This is an excerpt from docs/schema.md:

var User = schema.define('User', {
    email: { type: String, limit: 150, index: true },
    password: { type: String, limit: 50 },
    birthDate: Date,
    registrationDate: {
        type: Date,
        default: function () { return new Date }
    },
    activated: { type: Boolean, default: false }
}, {
    table: 'users'
});

Take a look at default: function() {return new Date}

cha0s commented 10 years ago

@1602 Yeah, I did use the callback version.

@randunel Yeah, that one is a bit different because semantically it matters when you create a Date object.

How do you guys feel about making a special note about this in the docs? I could see other people running into this.

randunel commented 10 years ago

Yup, makes sense.