i wonder if this PR will change the location of the values in a model's instance. will it move from instance.foo to instance.dataValues.foo ?

@sdepold that is exactly what happens with explicitly defined 'data property' values of the model, for each and every property created on the 'internal' dataValues object a suitable property getter and setter is defined (if the model does not already define a getter or/and setter for the property in question.

'association' properties (as generated/set by the relevant mixins) are still just stuck on the actual data object instance (i.e. instance.myForeignId instead of instance.dataValues.myForeignId) ... this was mainly so I could avoid screwing with any of the association related code.

as such the fact that actual data-values exist in instance.dataValues is completely transparent to any/all consumers of the instance (including DAO and DAOFactory) ... all instance data values are accessed in the same way as before (i.e. instance.foo)

I understand this is quite a tricky change to incorporate and that it may require work to get it into a shape you are willing to merge ... please instruct me if you are seriously interested in this patch (I would be willing to write some docs after a merge).

as a sidenote I am running this PR in my code (MySQL project) and it is working nicely for me.

So you store the values in dataValues instead of the DAO Object and then just map from the Object to dataValues with getters/setters?
Interesting idea, had the same thought - It would make the code cleaner. Although i think values would probably be a fine name aswell for the property :)

setDataValues & getDataValues could probably also be names set/get, and then just have the getters/setters on the object map to those, and then handle custom getter/setter logic there

@mickhansen I did not name the property values because each DAO instance already has a getter property named values and I did not want to BC on that.

I would advise against naming the 'real' data-value getting and setting functions simply get and set ... they seems ripe for misinterpretation. note that the only time a user should be using setDataValue and getDataValue are inside a custom property getter or setter (and even then it is only needed when the property getter/setter has the same name as the underlying property ... to avoid an endless loop!)

currently the default getters/setter are defined as such:

    if (has !== true) {
      this.__defineGetter__(attribute, has.get || function()  { return this.dataValues[attribute]; });
      this.__defineSetter__(attribute, has.set || function(v) { this.dataValues[attribute] = v;    });
    } 

I suppose these could be changed like so (untested) - but I chose not to put another function call on the stack:

    if (has !== true) {
      this.__defineGetter__(attribute, has.get || this.getDataValue);
      this.__defineSetter__(attribute, has.set || this.setDataValue);
    } 

I don't think it really matters either way, I implemented setDataValue and getDataValue with only one purpose, namely as helpers when defining custom property getters/setters (the idea being that direct access to this.dataValues should be offlimits so that the underlying implementation could be changed/tweaked without breaking people's model implementations).

look forward to hearing if there are any changes that I need to make in order to get this PR through!

Well, personally i'd like to see my code move from using model.attribute to model.get('attribute') in the feature.
And i don't think get/set are easy to misinterpret, Backbone seems to get away with it fine :)

Currently .values is a custom getter that returns values + eagerly loaded associations, but that should arguebly be a job for toJSON - seems weird to me to have both values and dataValues.

My argument for using set/get methods on objct would be that it would set/get both on dataValues, the object, and handle custom getters/setters - so we could keep all that logic in one place.

@mickhansen - trying to stick every use-case into the form instance.get('attribute') does not work, you will still need an internal mechanism for setting the actual value of an instance attribute - if I have a 'name' attribute and I want to implement a custom setter for said attribute that forces the value to always be uppercase then I would have to be able to set the actual value from within the custom setter function, if all I had was instance.set('name') then I would be stuck with an endless loop.

assume instance is an instance of some model that has a 'name' attribute:

   instance.set = function(a, v) { this[a] = v; }

   // endless loop here:
  instance.__defineSetter__('name', function(v) { this.set('name', v.toString().toUpperCase()); });
  // name attribute set okay here: 
  instance.__defineSetter__('name', function(v) { this.setDataValues('name', v.toString().toUpperCase()); });

put another way, setDataValue() & getDataValue() are meant for use only inside custom getters and setters (in order to shield a specific model implementation from the specifics of where DAO instances actually stuff their instance data (i.e. in this.dataValues) ... they are intended to be 'protected' interfaces.

... I see adding public get() & set() wrappers for instance properties as a seperate possibility. If you decided to completely remove the ability to 'directly' access instance data via object property access and force all access via your suggested get() & set() accessors then my PR is not really relevant anymore because you could then implement the custom getters and setters without the use of defineGetter and defineSetter (and thereby sidestep any endless loop issues). Personally I have my doubts that making such a breaking change is tenable.

I also did think that instance.values was a bit bogus, but I didn't want to mess with it - it exists and someone probably relies on it.

I would be easy to implement 'public' get() and set() methods on top of this PR as a stepping stone to deprecating 'direct' attribute access (and subsequently refactoring the underlying attribute get/set mechanism).

Ah, yeah i think i get your point now.
But if using a custom setter via defineSetter, it won't actually set the value on the object will it? Unless the user implementing the setter implicitely says so right? So he'll have to do both this[a] = v + this.setDataValue?

Unless he of course implements an opposite getter.

I see that your code takes care of implementing an opposite getter/setter if not defined actually.

hey @mickhansen. yes each (non-associative) attribute that exists will always have a a getter and setter defined (if the model doesn't define a specific getter or setter for an attribute a 'std' one will be setup).

...to try and clarify the use of this.setDataValue():

given a model that defines a "name" attribute and a custom setter function for "name", the definition of the custom setter must be like so:

function(v) { this.setDataValue('name', v.toString().toUpperCase()); }

note that the implementor of the custom "name" setter only uses the method this.setDataValue() to set the underlying value (trying to do this.name = v.toString().toUpperCase() inside the custom setter would result in an endless loop).

Application code (i.e. outside the model definition) would set the name attribute as follows (i.e. nothing changes for code that 'consumes' model instances):

instance.name = "bart";

setDataValue() and getDataValue() are never meant to be called from 'outside the model', the only reason these method even exist is too afford the developers of Sequelize a bit of abstraction with regard to where the data is actually kept, that is to say the "name" custom setter from above could be technically implemented as follows:

function(v) { this.dataValues.name = v.toString().toUpperCase(); }

that would work but now someone's model definition tightly coupled to the dataValues and you would no longer be able to change the underlying implementation without breaking existing models.

Now imagine a similar model with a "name" attribute, one custom "name_low" getter and one custom "name_up" setter. the property setter/getter implementations can be done as follows (note that these could be considered as pseudo-attributes):

// a bit more of the surrounding boiler plate code to give context:

Sequelize.define({
  id          : { type: Sequelize.INTEGER, autoIncrement: true, primaryKey: true },
  name    : { type: Sequelize.STRING, allowNull: false }
}, {
  getterMethods : {
     name_low: function() { this.name.toString().toLowerCase(); }
  },
  setterMethods : {
    name_up : function(v) {this.name = v.toString().toUpperCase(); }
  }
});

usage of this model would like so:

instance.name       = 'foo'; // name attribute now equal 'foo'
instance.name_up = 'foo'; // name attribute now equal 'FOO'
console.log(instance.name_low) // outputs 'foo' even though name attribute is equal to 'foo' 

... possibly my explainations are about as clear as mud, hopefully not :)

I see, now i understand your design - I think my confusion stems from me having a different design in my head initially (i also thought of implementing this).

I was considering doing something like:
Have all attributes have a getter/setter that map to two set/get methods.
Then have set/get manipulate a value object internally, effectively removing the storing values on the main object bit but maintaining the public API. Not sure of the performance implications of having all attributes and custom setters/Getters be defineGetter/defineSetters on the main object though.

I believe there is a significant performance penalty for using property getter/setters when compared to the speed of using straight forward properties on a model instance - this conclusion is based on a bit of searching for relevant benchmarks.

if you wish to keep the public interface for data attributes in it's current form (i.e. being able to do inst.mycounter = inst.mycounter + 1; rather than moving to explicit get() and set() methods for all data attribute access and you want to abstract the data attributes somewhere to allow for a layer of indirection (i.e. ability to transparently inject custom property getter/setter functions) then you are going to have to take a performance hit one way or another. forcing the use of explicit get() and set() methods would likely result in less of a performance hit but would require a breaking change in Sequelize.

@iamjochem any chance you can update your branch to the latest sequelize changes? Also cc @janmeier @sdepold @mickhansen :)

@durango how should I do that, I imagine rebasing the branch on which this PR is based will not work ... or does it? or is it better to create a new PR (that seems like a crufty way to go - you'd lose PR commentary). or should I merge the upstream branch into my feature branch and push that?

basically: help me out with the correct workflow please!

or should I merge the upstream branch into my feature branch and push that? <--

@durango took me a moment to figure out you were not asking a question but pointing out what I should do!

I merged sequelize/master into my feature branch but now I get errors when I run npm run test (I have run npm update && npm install), firstly this:

warning: possible EventEmitter memory leak detected. 11 listeners added. Use emitter.setMaxListeners() to increase limit.
Trace
    at process.EventEmitter.addListener (events.js:160:15)
    at process.on.process.addListener (node.js:768:26)
    at new module.exports.ConnectorManager (/work/forks/sequelize/lib/dialects/mysql/connector-manager.js:122:13)
    at new module.exports.Sequelize (/work/forks/sequelize/lib/sequelize.js:106:30)
    at Object.module.exports.createSequelizeInstance (/work/forks/sequelize/spec/buster-helpers.js:44:12)
    at Object.module.exports.initTests (/work/forks/sequelize/spec/buster-helpers.js:11:26)
    at Object.<anonymous> (/work/forks/sequelize/spec/dao.spec.js:14:13)
    at asyncFunction (/work/forks/sequelize/node_modules/buster/node_modules/buster-test/lib/buster-test/test-runner.js:189:16)
    at callAndWait (/work/forks/sequelize/node_modules/buster/node_modules/buster-test/lib/buster-test/test-runner.js:221:23)
    at next (/work/forks/sequelize/node_modules/buster/node_modules/buster-test/lib/buster-test/test-runner.js:241:31)

I get a number of these - I don't think they are anything to do with my changes - is there known to be stuff 'broken' in sequelize/master ? am I missing something?

secondly I get the following error:

Error: [POSTGRES] HasMany (N:M) adds three items to the query chainer when calling sync
    Error: Cannot find module '/work/forks/sequelize/node_modules/pg/lib/native/../../build/default/binding'
    at Function.Module._resolveFilename (module.js:338:15)
    at Function.Module._load (module.js:280:25)
    at Module.require (module.js:364:17)
    at require (module.js:380:17)
    at Object.<anonymous> (./node_modules/pg/lib/native/index.js:12:12)
    at Module._compile (module.js:456:26)
    at Object.Module._extensions..js (module.js:474:10)
    at Module.load (module.js:356:32)
    at Function.Module._load (module.js:312:12)
    at Module.require (module.js:364:17)

how do I get rid of that??

other than this all tests are passing, a bit of help getting the above cleaned up so I can push the changes would be appreciated.

_loose thought:_
maybe this burden of keeping a PR up2date should not be on the creator of the PR ? mostly because it a lost cause to except everyone who creates a PR for a project to keep it up2date - more so when you factor in that there are different ways of doing it and not everyone has the same level of skill/experience with git.

projects such as https://github.com/defunkt/github-gem are an attempt to make that a workable proposition.

The first error is to be expected, no problem in that

The second one seems to be with the native pg binding, there seems to have
been a compilation error when it was installed. Try npm uninstall pg and
npm install pg

@janmeier - hi, thanks that fixes the problem ... apparently I had to uninstall/install sqlite3 ... not sure exactly why, I guess I'm still learning to crawl when it comes to node/npm/et-al ... was this problem related to updates in the actual sequelize code base or is it more likely to be a result of a change in the version of node I am running (I vaguely remember using n to update to v0.11.1 not so long ago.

I have just pushed the results of merging in sequelize@1.7.0-alpha1 I hope that the result is usable!

@durango i like this PR, however i was discussing the future data access API with sdepold.
And we will move towards storing the values in an object internally. Using set/get methods + mapping getters from the main object to the getter function.

@mickhansen - hi, I am interested if you guys have any idea whether this PR will be accepted as it is or if you intend to strip out the interesting bits and reuse them in your own reimaging of the data access API ... and whether I can do anything to help?

@iamjochem Lots of stuff going on for me currently so will be a while before i can implement the proposed/decided API.

So i say we merge this and provide the functionality untill we can refactor the internals.

@durango @janmeier @sdepold agreed?

@iamjochem You can help me in designing the new API when i get to it :) Would love your input when i get there.

@mickhansen & co - sure I'd love to help if and where I can ... just give me a poke when the time is ripe.

if you do go ahead and merge this then I think it would be good to tweak the PR slightly ... currently custom property getters and setters are defined via the options are in the model ... which I think is handy because it makes model composition more 'natural' (in my project I have 'meta objects' that specify what my sequelize models should look like, including shared/reusable stuff, that gets read by a 'builder' that composes, defines & associates the actual models) ... that is all well and god but I notice that someone suggested something like the following:

sequelize.define('User', 
{ 
  firstname: { 
    type: Sequelize.STRING,
    get: function() { /* magic here */ },
    set: function() { /* magic here */ }
  }
}, {/*options*/})

i.e. place the getters/setters in the property definition itself ... If you agree to merge I would like to implement that on top of current functionality I have (in such a way that getters/setters given in the actual property definition take preference to any getters/setters that may be passed in, for said property, in the model options.

what this would give is this:

• cleaner model definition in simple cases (getters and setters can be defined right with the property the wrap)
• 'shared' getter/setters (i.e. ones added to many/all models) could be passed in via the options object and they could be overruled by model-specific ones where applicable.

what do you think?

@iamjochem's loose thought:
Yes, it does seem like a bit of work to ask the creator of the PR to keep his branch up to date until it can be merged. Mostly it's just because we are lazy :-) What we want to see is the big green button that we can just press here on github to merge it

But the tool you linked seems to do a lot of the tiresome work of adding a new remote and checking out the right branch just to merge a single PR. The github gem seems to be abandoned though, and does not work properly on windows. Instead i used https://github.com/defunkt/hub , and it worked fine in merging #589. It was as simple as

hub merge https://github.com/sequelize/sequelize/pull/589
git push sequelize master

Thanks a lot showing me a new cool tool! :)

@iamjochem I love your last example the most, it definitely feels more natural in my opinion (although the other way is useful for creating "virtual" fields as well).

@durango - that last idea is a fairly small addition me thinks - I'll get right on it.

@durango, @mickhansen, @janmeier, @sdepold - I have updated the PR to include wat I suggested above ... I believe there is a big green button up top that would love a click ;-)

ping - any change of actually merging this?

awesome!!! very happy about this, means I can go back to using stock sequelize in my project rather than a fork :-)

Hi, updated from 1.6 to 1.7 to play with this, works like a charm.
But by default properties defined in getterMethods are not transient right?
When I send json data to client they are not included so I need to redefine toJSON and add them by hand.
Any other ways to do it may be?

hi @wonderwhy-er ... so-called 'pseudo-properties' defined by way of a getter are indeed not included in the toJSON() output because to toJSON() relies on the values getter property which is a convience wrapper for retrieving the data 'fields' of a model instance (i.e. the properties which map to a field in your DB).

I think you're best off redefining toJSON() ... do something like (in you models instance methods):

   toJSON : function() {
       return _.extend(this.values, {fakeprop_one: this.fakepropone, another_fakeprop: this.another_fakeprop });
   }

maybe there should be a standard mechanism for retrieving all data including the 'fake properties', regardless I don't think that the values of the fake properties should be included by default. (that said I have reservations regarding simply exposing the complete contents of a model instance via an API, I'm more inclined to explicitly define which data properties should be made available via API than just exposing all properties by using toJSON())

@iamjochem thanks for quick reply. I actually seen this hack in that transient properties thread mentioned before.
My problem with overriding toJSON is that it may be unsafe in case standard DAO.toJSON changes.

Also, my problem actually is two fold as I also need to "remove" one of the values from toJSON result.
Actually object for which I am doing it is User and I don't want password to be included in data that is sent to client.

Ended up writing something like this:

            instanceMethods: {
                hidden_values: ['password'],
                toJSON: function()
                {
                    var res = this.values;
                    for(var getter_name in User.options.getterMethods)
                    {
                        res[getter_name] = this[getter_name];
                    }

                    for (var i = 0; i < this.hidden_values.length; i++)
                    {
                        var value_name = this.hidden_values[i];
                        delete res[value_name];
                    }

                    return res;
                }
            }

As you see I also prefer for user to be able to choose. I was just asking if there may be is a better way.

when I was writing my reply I was contemplating on adding an example (specifically with a User model and it's password field)!

you might consider simply defining and using some app specific method (e.g. getApiJSON()) that said I believe toJSON() is merely a convienience method ... I wouldn't worry about the default implementation changing (besides you should be using a 'fixed' version of sequelize so that it doesn't automatically upgrade and bite you with breaking changes :-))

one thing worth checking is whether you need to clone the object that this.values returns (in order to avoid clobbering the underlying object) - I can't remember offhand whether it always returns an new object or not.

@iamjochem yeah user is most obvious case for this :)

Ok I actually changed a little how I do this

    sequelize = new Sequelize(config.name, config.username, config.pass, {
        define: {
            instanceMethods: {
                toJSON: function ()
                {
                    var res = this.values;
                    console.log(this);
                    for (var getter_name in this.__options.getterMethods)
                    {
                        res[getter_name] = this[getter_name];
                    }

                    if(this.hidden_values)
                    {
                        for (var i = 0; i < this.hidden_values.length; i++)
                        {
                            var value_name = this.hidden_values[i];
                            delete res[value_name];
                        }
                    }
                    return res;
                }
            }
        }
    });

Now its less of a problem I guess as its in one place.
On other hand I end up using __options which probably again fragile thing to do.
As for this.values, I checked the code, apparently it runs trough all attributes + all eagerly loaded stuff each time constructing value object when you call it.

So at the moment in my tests it works fine.

I am trying out the 1.7 alpha and had some questions about this specific issue. As per sdepolds initial comment, this removes, by default, the ability to access properties directly on a model (eg no more 'instance.foo'). So basically, doesn't this break backwards compatibility with any code that used sequelize before this update? If so, it seems like this shouldn't be in a minor update, right? Seems like a default getter should be defined to keep that backwards compatibility.

Or am I missing something here/having a bug on my end?

@greghart - instance.foo is accessable exactly as before - under the hood 'getting' the foo property of instance calls a function to retrieve the value.

see here for instance (pun intended :) http://ejohn.org/blog/javascript-getters-and-setters/

@iamjochem Great, that's what I was hoping. I wasn't seeing it but with verification that it should work I managed to track down the bug causing me to believe it was not working. I doubt it's a valid use case, but this does mean you can no longer build instances with other instances fed straight as the first argument. May sound like an odd use case, but I had to juggle two different Model's, one for a read only database, so when switching the connection to use for persistence I would do something like:

instance = Instance.find(1).complete(...)
// This no longer works, but I'm not sure if this is proper anyways
instance2 = InstanceReadOnly.build(instance, {isNewRecord:false})
// Easy fix
instance2 = InstanceReadOnly.build(instance.values, {isNewRecord:false})

In retrospect, I believe using instance.values was probably the more proper way to do this anyways.