New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor of setters/getters/values in regards to hydration, toJSON and more #1182

Merged
merged 26 commits into from Jan 4, 2014

Conversation

5 participants
@mickhansen
Contributor

mickhansen commented Dec 31, 2013

Working on set and get methods as interfaces to custom getters and general value changing.
set will have a raw option that bypasses all setters (will be used by find/findAll/reload etc).
set will ignore timestamp attributes (these should be set directly by save/destroy)

build -> initAttributes will now use set aswell

Custom setters and getters on attributes will receive the attribute key as it's second parameter.

toJSON / .values will implement get with no key which will loop through all the values and apply custom getters

_previousDataValues will be populated on set if the current data value being overwritten is not undefined

save will set _previousDataValues = dataValues, any changed() call will then return false

API

Set

instance.set(key, value)
instance.set(key, value, options)
instance.set(keyValuePair, options)

set will call the appropriate setters if any are defined, else it will use internal logic

Options:

raw: (true|false), default: true - makes the setter skip dynamic setters, used internally for hydration from the database

Get

instance.get()
instance.get(key)

get will call the approprirate getters if any are defined (for no-key calls aswell), else it will use internal logic

Changed

instance.changed() // Returns an array of changed attributes
instance.changed(key) // Returns true/false
isDirty will map to instance.changed()

Previous

instance.previous(key) // Returns previous value for key

@tremby

This comment has been minimized.

Contributor

tremby commented Dec 31, 2013

As mentioned in #1181, it'd be great if the column name was given as an extra argument to the setter and getter methods, so that this is possible:

setNullIfEmptyString = function(value, columnName) {
    this.setDataValue(columnName, value === '' ? null : value)
}
sequelize.define('address', {
    subpremise: {
        type: Sequelize.STRING,
        allowNull: true,
        validate: {notEmpty: true},
        set: setNullIfEmptyString
    }
})

This would allow easy reuse of getters/setters.

The same sort of thing would be very useful for getters too (it'd be the only argument in that case).

@durango

This comment has been minimized.

Member

durango commented Jan 4, 2014

+1 from me

@@ -23,6 +23,7 @@ describe(Support.getTestDialectTeaser("Hooks"), function () {
}, {
hooks: {
beforeValidate: function(user, fn) {
console.log("beforeValidate")

This comment has been minimized.

@durango

durango Jan 4, 2014

Member

do we need this?

This comment has been minimized.

@mickhansen

mickhansen Jan 4, 2014

Contributor

probably not ;)

@janmeier

This comment has been minimized.

Member

janmeier commented Jan 4, 2014

+1

@mickhansen

This comment has been minimized.

Contributor

mickhansen commented Jan 4, 2014

Thank you both

mickhansen added a commit that referenced this pull request Jan 4, 2014

Merge pull request #1182 from mickhansen/hydration-setter-getter-refa…
…ctor

Refactor of setters/getters/values in regards to hydration, toJSON and more

@mickhansen mickhansen merged commit 58d5172 into sequelize:master Jan 4, 2014

1 check passed

default The Travis CI build passed
Details

@mickhansen mickhansen deleted the mickhansen:hydration-setter-getter-refactor branch Jan 4, 2014

@tremby

This comment has been minimized.

Contributor

tremby commented Jan 6, 2014

Was my request above #1182 (comment) considered/implemented? If not I'll open another ticket for it.

@mickhansen

This comment has been minimized.

Contributor

mickhansen commented Jan 6, 2014

@tremby it was :) - Implemented that is

@tremby

This comment has been minimized.

Contributor

tremby commented Jan 6, 2014

Just tested both setters and getters -- they work perfectly. Thanks very much for implementing that tweak. I just noticed your updated first post. Not sure if it has made it into the documentation yet, but when it does, perhaps avoid saying the attribute key is given as the "second parameter", since for the getter it is the only (first) parameter.

@mickhansen

This comment has been minimized.

Contributor

mickhansen commented Jan 6, 2014

@tremby right you are, will remember that when i update the API reference :)

@SteveEisner

This comment has been minimized.

SteveEisner commented on lib/dao.js in 6367758 Jan 11, 2014

@mickhansen I haven't looked into it enough yet but I think this might have introduced a bug where custom getters are being called twice (ie. the result of the first call is fed in as the raw dataValue for the 2nd time it gets called.) I've filed bug #1234

This comment has been minimized.

Contributor

mickhansen replied Jan 11, 2014

This line is just BC support. No internals should depend on this, hopefully.

@SteveEisner

This comment has been minimized.

SteveEisner commented on lib/dao.js in 6367758 Jan 11, 2014

Related to bug #1234 this might be why line 45 above was changed. Perhaps there's confusion about when the raw/unhydrated dataValues are being used vs hydrated, in internal code?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment