Skip to content
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

Read-only timestamps #2

Open
dresende opened this issue Aug 1, 2013 · 8 comments
Open

Read-only timestamps #2

dresende opened this issue Aug 1, 2013 · 8 comments

Comments

@dresende
Copy link
Collaborator

dresende commented Aug 1, 2013

This would be cool but not mandatory. Perhaps it needs some more integration between plugins and models/instances.

The idea would be to have the possibility to read this methods but not change them.

@notheotherben
Copy link
Owner

Yeah, I'm not sure how we could do this given the current way everything is handled. If we had a way for a plugin to be triggered after an instance was instantiated (i.e. having its own beforeReady hook or something) then it may be possible for us to adjust some of the instance's properties or something.

That being said, it doesn't pose a problem for modified_at, since it will be updated the moment it is saved anyway. So I guess we could store a copy of the created_at value after loading and just re-assign it when we save.

@dresende
Copy link
Collaborator Author

dresende commented Aug 7, 2013

Yes, there's need to be more integration between plugins and models/instances. About read-only I was thinking more of a pedantic approach, like throwing an exception if trying to change. Maybe this could be a feature of orm - defining properties that after being set or after fetching from database you don't want people to change it.

@dresende
Copy link
Collaborator Author

dresende commented Aug 7, 2013

This may look lame, but when you work with many developers, you sometimes need to impose some rules. Also, when working solo, sometimes you may be working late or with too much coffee and this could be some limitations you might want.

When I was re-reading this post, I found this can be easily done by a beforeSave hook but could be a module feature (a readonly property option).

@notheotherben
Copy link
Owner

What I'd prefer would be the ability to hook into property accessors and setters to provide logic.

For example, maybe we could have something like this:

function Plugin(db, opts) {
    return {
        propertyRead: function(model, property, instance_data) {
            if(property != 'changed_at' && property != 'modified_at') return;
            return "a different value";
        },
        propertyWrite: function(model, property, instance_data, value) {
            return instance_data[property]; //Prevent changes
        }
    }
}

Obviously we wouldn't be able to give it access to the instance itself, since that would result in an infinite loop if it attempted to read/write anything to an instance. So maybe giving it access to the opts.data object would suffice?
Then for the handlers, maybe check for an undefined return value (i.e. we just called return;) to ignore the hook, otherwise replace the property's value/getter result with the value that the hook returns.

Some of the nice things you could do would include transparent conversion of one type to another (for example, JSON <=> PostgreSQL HStore) or updating another instance property when changes have been made to another property (e.g. updating a last_password_change field whenever password_hash has been changed).

This also leads to the possibility of extending it beyond just the plugin domain, and rather moving it up to a model level hook, since that would expose the ability to plugins and standard models.

db.define('user', {
    username: { type: 'text', required: true }
    password_hash: String,
    last_password_change: Date
}, {
    hooks: {
        propertyChanging: function(property, newvalue) {
            if(property == 'password_hash') {
                this.last_password_change = new Date();
            }
            //Implied return;
        }
    }
}

If we're doing it this way, we would need to ensure that the hooks don't generate infinite loops. This could be done by having a set of 'locks' in place for each property changing hook, so that one property change hook cannot cause itself to be triggered while in the hook body.

Also, not sure that it would be possible to support async hooks for this, since I don't believe JavaScript's getter/setters support the concept...

Take note, this is a VERY rough idea of how it could be implemented, and I'm not entirely satisfied with the design of it yet. Hopefully it gives us a starting point from which to work from though...

@dresende
Copy link
Collaborator Author

dresende commented Aug 8, 2013

I like the idea but the problem can be when you have several plugins. How do we choose which plugin handles a property? Perhaps we could instead define a getter/setter in a property.

// somewhere in this plugin..
options.dbtype.get = function () { ... };
options.dbtype.set = function (val) { ... };
properties[options.createdProperty] = options.dbtype;

@notheotherben
Copy link
Owner

I was actually thinking we handle it the same way we handle hooks at the moment, then if someone wants to support cascading handlers they can do something similar to what I did RE the wrapper function in this plugin.

To be honest, I can't actually think of a good way to handle cascading handlers in a good fashion, so maybe forcing people to avoid it is a good idea - since you would have no idea which handler could be executed first etc.

Another thing I was considering was the possibility of adding it as an option for field types, for example:

db.define('model', {
    field: {
        type: 'text',
        get: function() { return JSON.parse(this.value); },
        set: function(value) { this.value = JSON.stringify(value); }
}

We would then be able to easily add these handlers to fields either using plugins, or using ORM itself.

@dresende
Copy link
Collaborator Author

Your example is the same as mine, I took the line from your code and just added the getand set. I think this method is simpler, you can focus on specific properties and it should be quite easy to support 😄 (just a change on addInstanceProperty function in Instance.js and maybe one or two more things)

@notheotherben
Copy link
Owner

Ah, my apologies, I must have misunderstood you. I'll see what I can do RE implementing it this weekend, but I've got another two projects on hand - so this is gonna take a back seat to those for the time being.

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

No branches or pull requests

2 participants