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

Using a custom id creates incorrect mysql queries. #313

Closed
ptnplanet opened this issue Aug 24, 2013 · 10 comments
Closed

Using a custom id creates incorrect mysql queries. #313

ptnplanet opened this issue Aug 24, 2013 · 10 comments

Comments

@ptnplanet
Copy link

I have not yet tried this with any other driver, but I can at least say, mysql is affected.

Here is a model definition using a custom id:

var Session = db.define('session', {
    id:        { type: 'text', size: 36 },
    data:      { type: 'text', big: true },
    expiresAt: { type: 'date' }
});

will correctly create the following table:

CREATE TABLE `session` (
  `id` varchar(36) NOT NULL DEFAULT '',
  `data` longtext,
  `expiresAt` datetime DEFAULT NULL,
  PRIMARY KEY (`id`)
)

But will select the id column twice on SELECT queries:

Session.one({ id: 'abc' }, function (err, sess) { /* ... */ });

results in

SELECT `id`, `id`, `data`, `expiresAt`
FROM `session`
WHERE `id` = 'abc' LIMIT 1

Notice the duplicate id column in the select query. This does not do any harm - it is just a cosmetic thing.

@dxg
Copy link
Collaborator

dxg commented Aug 25, 2013

What happens when you use this define syntax?

That said, I'm curious about having a different define syntax:

Instead of:

var Person = db.define("person", {
    firstname: String,
    lastname: String
}, {
    id: ['firstname', 'lastname']
});

have this:

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

Combine this with an explicitly defined id field for the simple use case:

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

and it might avoid the confusion surrounding id fields.
Thoughts?

cc @spartan563

@notheotherben
Copy link
Collaborator

Looks like a nice syntax, but I'm not sure it's worth the effort or maintenance requirements at this stage in the project. Would mean that code which explicitly declared the id: ['firstname', 'lastname'] would either have to be supported as well (ala the Error_Codes changes) or we'd have to make it blatantly clear that it is a breaking change.

While I don't have an issue with the later if we bump a major/minor version number, it seems like a big change for not really any major gain.

I must admit that I'm also intrigued as to why defining just the id field (without declaring it as the id in the options) causes this issue, I wouldn't have expected it - and it probably means something else funky is going on behind the scenes.

I'll try and take a look if I get a break this week, but I've got a whole string of tests coming up for my degree - so that's probably unlikely. If it's still open after that though, I'll get on it and fix.

@dxg
Copy link
Collaborator

dxg commented Aug 25, 2013

Main gain I see is less confusion for people using ORM - and this is a common problem surrounding composite keys.
After posting that comment I was fixing something unrelated, and I ended up refactoring/simplifying quite a bit of code related to properties as it was getting out of hand.

The front-end API hasn't changed, but the internals work more like my suggestion above.
Once I fix the last few tests, I'll put up a pull request so you can see what I mean.

Good luck with the tests! What degree is it?

@notheotherben
Copy link
Collaborator

Well, anything which makes our lives easier is a plus in my book. I think that an API change is definitely something worth considering (or at least a revision), maybe with the version bump to 3.x. I'd really like to go over much of the design and consider more modular ways to handle things, since everything seems rather tightly coupled at the moment, but that'll be a rather massive job.

Looking forward to it, I'll see what I can do to help with it when I've got time again - maybe create a branch for us to twiddle around with it.

Thanks, BEng Electronic - got lured into a false sense of security with the first 2 years, which seemed relatively manageable. This year doesn't seem like a lot more work, but I never seem to have any free time anymore; weirdest thing :)

@dxg
Copy link
Collaborator

dxg commented Aug 26, 2013

Heh, I gave up after the second year of my [similar.. comp systems] eng degree.. too much maths and didn't care much for electron volts. Ended up in the much easier comp sci. department.
And I can definitely relate to the lack of free time problem..

@dresende
Copy link
Owner

Go for the key property. But perhaps that should then recreate the id option after looking at all the properties.

@ptnplanet
Copy link
Author

Just a follow up. This issue has gone far beyond my initial report of duplicate id fields in select queries. I really appreciate you considering to change the syntax for id-definition. I've changed to orm2 from Sequelize partly because Sequelize has great problems with custom ids when working with associations.

With Sequelize custom ids are created using the following syntax:

sequelize.define('tablename', {
    identifier: { type: Sequelize.INTEGER, primaryKey: true, autoIncrement: true }
});

This would match your intended api changes.


But back to topic. ;-) Using the alternative syntax

var Session = db.define('session', {
    id: { type: 'text', size: 36 } /*, ... */
}, {
    id: 'id'
});

or

var Session = db.define('session', {
    sessId: { type: 'text', size: 36 } /*, ... */
}, {
    id: 'sessId'
});

still results in two id or sessId fields being selected. As I pointed out, this does not do any harm. The queries still work, but there seems to be some black magic going on in the background.

@dresende
Copy link
Owner

I think I know what is happening, I'll see if I can fix the SELECT query. Other than that, everything works just fine.

dresende added a commit that referenced this issue Aug 26, 2013
This was because a property that was defined as an ID was already added
to model_fields before.
@dresende
Copy link
Owner

Try the latest commit :)

@ptnplanet
Copy link
Author

Perfect. Issue resolved. Thanks and keep up the good work!

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

No branches or pull requests

4 participants