Using a custom id creates incorrect mysql queries.

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.

@spartan563

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

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.

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?

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 :)

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..

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

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.

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

Fixes duplicate column in SELECT (#313)

Try the latest commit :)

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

Navigation Menu

Repository navigation

Using a custom id creates incorrect mysql queries. #313

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Using a custom id creates incorrect mysql queries. #313

Description

Activity

dxg commented on Aug 25, 2013

notheotherben commented on Aug 25, 2013

dxg commented on Aug 25, 2013

notheotherben commented on Aug 25, 2013

dxg commented on Aug 26, 2013

dresende commented on Aug 26, 2013

ptnplanet commented on Aug 26, 2013

dresende commented on Aug 26, 2013

dresende commented on Aug 26, 2013

ptnplanet commented on Aug 26, 2013

Metadata

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Issue actions