{"action":"created","issue":{"url":"https://api.github.com/repos/sequelize/sequelize/issues/6270","repository_url":"https://api.github.com/repos/sequelize/sequelize","labels_url":"https://api.github.com/repos/sequelize/sequelize/issues/6270/labels{/name}","comments_url":"https://api.github.com/repos/sequelize/sequelize/issues/6270/comments","events_url":"https://api.github.com/repos/sequelize/sequelize/issues/6270/events","html_url":"https://github.com/sequelize/sequelize/issues/6270","id":165408153,"number":6270,"title":"Diff against a clone of attributes instead of using setters","user":{"login":"felixfbecker","id":10532611,"avatar_url":"https://avatars.githubusercontent.com/u/10532611?v=3","gravatar_id":"","url":"https://api.github.com/users/felixfbecker","html_url":"https://github.com/felixfbecker","followers_url":"https://api.github.com/users/felixfbecker/followers","following_url":"https://api.github.com/users/felixfbecker/following{/other_user}","gists_url":"https://api.github.com/users/felixfbecker/gists{/gist_id}","starred_url":"https://api.github.com/users/felixfbecker/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/felixfbecker/subscriptions","organizations_url":"https://api.github.com/users/felixfbecker/orgs","repos_url":"https://api.github.com/users/felixfbecker/repos","events_url":"https://api.github.com/users/felixfbecker/events{/privacy}","received_events_url":"https://api.github.com/users/felixfbecker/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":9,"created_at":"2016-07-13T20:00:14Z","updated_at":"2016-07-17T19:05:03Z","closed_at":null,"body":"Currently, Sequelize holds all attributes inside `dataValues`. You can use them on the instance, but it is all one big lie as on instance creation, sequelize simply defines getters and setters that proxy to `dataValues` and mark the attribute as dirty. The problem is though that setters don't catch the following changes to attributes:\r\n\r\n - When using a JSON column, you can alter the object\r\n - When using an ARRAY column, you can push new items, remove item etc #6197 \r\n - When using a DATE column, you can call `setDate()` etc on the object\r\n\r\nYou have to manually call `instance.changed('myAttribute', true)` (no autocomplete for the attribute name) or reassign with `instance.myAttribute = instance.myAttribute` (wtf, should be a noop).\r\n\r\nNow think about how a Model is defined in Sequelize 4.0 and ES6 / TS:\r\n```ts\r\nclass User extends Sequelize.Model {\r\n  public username: string;\r\n  public roles: string[];\r\n  public createdAt: Date;\r\n}\r\n```\r\n\r\nYou define your class with these attributes, but in reality, they are not there - it's all a big lie. You look at the debugger and they are not there:\r\n\r\n![image](https://cloud.githubusercontent.com/assets/10532611/16817248/3d23a2d4-4942-11e6-82de-54a0acb68ebd.png)\r\n\r\nI think it would be a lot better if instead of `dataValues`, there was only `_previousDataValues`. When an instance is retrieved from the DB, the _actual_ instance gets all attributes set as real attributes. `_previousDataValues` gets set to a deep copy of the attributes - that means every `Date` object, every `JSON` object, every `Array` gets deeply cloned. This does not include aggregate columns, it's only about the values of the database row.\r\n\r\nA user can then modify and use the instance as much as he likes, nothing goes through any setters or getters (except if he defined them of course).\r\n\r\nOnly upon a call to `save()` we deeply compare the instance _once_ to the `_previousDataValues`. That includes `JSON` objects and arrays. We can then generate a minimal query that only contains the exact changes that were made to the instance.\r\n\r\nIt doesn't even have to replace a whole array column, since we have an exact diff we can even use advanced array functions to only remove / insert what changed.\r\n\r\n### Performance\r\n`saving` would be a bit more complex because we have to compare the objects, but every userland function containing property access (get/set) will be faster because getters and setters are an optimization killer."},"comment":{"url":"https://api.github.com/repos/sequelize/sequelize/issues/comments/233198075","html_url":"https://github.com/sequelize/sequelize/issues/6270#issuecomment-233198075","issue_url":"https://api.github.com/repos/sequelize/sequelize/issues/6270","id":233198075,"user":{"login":"mickhansen","id":699049,"avatar_url":"https://avatars.githubusercontent.com/u/699049?v=3","gravatar_id":"","url":"https://api.github.com/users/mickhansen","html_url":"https://github.com/mickhansen","followers_url":"https://api.github.com/users/mickhansen/followers","following_url":"https://api.github.com/users/mickhansen/following{/other_user}","gists_url":"https://api.github.com/users/mickhansen/gists{/gist_id}","starred_url":"https://api.github.com/users/mickhansen/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/mickhansen/subscriptions","organizations_url":"https://api.github.com/users/mickhansen/orgs","repos_url":"https://api.github.com/users/mickhansen/repos","events_url":"https://api.github.com/users/mickhansen/events{/privacy}","received_events_url":"https://api.github.com/users/mickhansen/received_events","type":"User","site_admin":false},"created_at":"2016-07-17T19:05:03Z","updated_at":"2016-07-17T19:05:03Z","body":"> I was under the impression that get() returned only columns in the database\r\n\r\nIt returns dataValues, but with `plain` it also iterates over them so you get values back and not instances when you have includes.\r\n\r\n```js\r\nget() {\r\n  return _.omit(this, ['isNewRecord', 'hasPrimaryKeys', '_previousDataValues']);\r\n}\r\n```\r\n\r\nCould work, requires a loop on each `get`, but so does most cases of `get` currently.\r\n\r\n> Well, when we do diff on save, we just don't need the setters on the instance anymore\r\n\r\n`set` also handles the logic for building nested instances, which are used in a few scenarios. Could be solved in other ways though.\r\n\r\nI'm not at all against this, i just see a lot of edge cases that might end up broken with this - But it's worth experimenting with.\r\nI'm not at all against this, i just see a ton of edge cases that might end up broken with this - But it's worth experimenting with."}}
{"actor":{"display_login":"mickhansen"}}