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

Properties refactor #318

Merged
merged 4 commits into from
Aug 29, 2013
Merged

Properties refactor #318

merged 4 commits into from
Aug 29, 2013

Conversation

dxg
Copy link
Collaborator

@dxg dxg commented Aug 28, 2013

To fix #310 I needed a way to reliably pass a model property definition to a DML drivers valueToProperty method from within an Instance.
Since some properties were set on Model.properties whilst others like ids & association fields existed only in an array of strings (and their types were inferred by the drivers DDL sync code) this made life difficult.
It also introduced duplicate code, and issues with redefining key columns, and no good way to specify exact types for ALL columns.

I've come up with Model.allProperties which stores definitions of all constant properties (normal properties, id props, association fields). Extra properties remain separate as they are transient.

This allowed for some nice code cleanups in driver sync methods, and a simplification of opts.data initialization code inside Instance.

It also fixes some inconsistent behaviour I found along the way, like Model.create casting properties when they are set and instance.save not doing so.

Lastly, it introduces a new syntax (only internally for now) for defining keys, as discussed in #313 :

var Person = db.define("person", {
  id:   { type: 'serial', key: true },
  name: { type: 'text' }
});

Composite keys:

var Person = db.define("person", {
    firstname: { type: 'text', key: true},
    lastname: { type: 'text', key: true}
});

Keys are currently still set via the id property, however now that the underlying work is done, we can change this in future to the key syntax without too much pain.

Note: We now have both Model.allProperties and Model.properties. It should be possible to remove the latter in the future. It was taking me forever to finish this refactor, so consider this code a transition step.

@dresende the failing tests depend on sql-query 0.11.12 being published - the change in sql-query is small.

@dresende
Copy link
Owner

I just published [email protected]. What happens if no key is defined in properties? Does the default primary key is used (defined or not as a property)?

@dxg
Copy link
Collaborator Author

dxg commented Aug 28, 2013

Hmm looking at:

// ORM.prototype.define
id             : opts.id || this.settings.get("properties.primary_key"),

// Instance.js
    for (var i = 0; i < opts.id.length; i++) {
        k = opts.id[i];
        allProperties[k] = opts.properties[k] || {
            type: 'serial', rational: 'false', key: true, klass: 'key'
        };
    }

if someone defines id property without saying it's a key, it won't be a key.
Does that answer you question, or do you mean that we should set the defined properties key value to true at the end of the for loop?

@dresende
Copy link
Owner

What I meant was:

  1. If no key defined, use default (usually id);
  2. If no key defined but a property exists with the same name as the default id, use it (property type).

@dxg
Copy link
Collaborator Author

dxg commented Aug 29, 2013

This PR doesn't change the behaviour. It's an intermediate step that allowed some code cleanups.

That said, I can work on part two (with key api changes) once this one is merged to keep things manageable.
Both 1. and 2. sound good, I will implement them in the next PR if we're happy with the proposed API.

@dresende
Copy link
Owner

Yes, go for it. Looks good to me :)

dxg added a commit that referenced this pull request Aug 29, 2013
@dxg dxg merged commit 3d5820f into master Aug 29, 2013
@dxg dxg deleted the properties branch August 29, 2013 23:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

column "nan" does not exist
2 participants