belongsToMany incorrectly assumes idAttribute is a column name.

[foster]

My column names are snake_case, but my attribute names are camelCase. My database's primary key is entity_id, for instance, the accounts table has a primary key of account_id.

The belongsToMany association is incorrectly using the camelized idAttribute to join the tables.

// the code below will run this query. 
// Notice the incorrect column name in the ON clause:
// 'select "foos".*, "bars_foos"."bar_id" as "_pivot_bar_id", "bars_foos"."foo_id" as "_pivot_foo_id" 
// from "foos" inner join "bars_foos" on "bars_foos"."foo_id" = "foos"."fooId" 
// where "bars_foos"."bar_id" = $1'

var Foo = bookshelf.Model.extend({
    tableName: 'foos',
    idAttribute: 'fooId', // column name is actually foo_id.

    format: function(attrs) {
        return {
            'foo_id': attrs.fooId
        };
    },

    parse: function(record) {
        return {
            fooId: record['foo_id']
        };
    }
});

var Bar = bookshelf.Model.extend({
    tableName: 'bars',
    foos: function() {
        return this.belongsToMany(Foo, 'bars_foos', 'bar_id', 'foo_id');
    }
});

var foos = Bar.forge({id: 1}).related('foos').fetch();

[tgriesser]

Yeah, this is incorrect. I'm going to actually provide support for camel casing the keys directly in the library in the upcoming minor version, but in the meantime I'll see if I can fix this one. Thanks for the test case.

[foster]

Nice, thanks.

It seems that my problem is really specific to id, since other fields are .formatted before insert or update. I encountered a similar problem that .save() relies on idAttribute as a column name. Would it break abstraction too much for models to have an idColumn property?

[coolaj86]

has this been fixed? I think it just bit me too.

is there a workaround?

[peteut]

I had a related issue with collection#create (.related('related').create({ })), saveConstraints returns model.set(data) but should return model.set(model.parse(data)).

[rhys-vdw]

@peteut This is still broken in the current version.

[peteut]

master...peteut:4b15b012707edb8c0c9250380a8ec73f3cef6c91 worked for me. Maybe this is not related?

[rhys-vdw]

It may do, I can check if you like. Is there a reason why it hasn't been merged yet?

[peteut]

I was too lazy and didn't create a test case and pull request. Sorry for that.
Could you please give it a try? Thanks a lot!

On Monday, May 4, 2015 2:28 PM, Rhys van der Waerden notifications@github.com wrote:

It may do, I can check if you like. Is there a reason why it hasn't been merged yet?
—
Reply to this email directly or view it on GitHub.

[rhys-vdw]

@peteut Finally got the test environment set up. PR: #771.

[LordSputnik]

Just wondering what the status of this is? #771 doesn't seem to have fixed it - not sure if it was meant to. Thanks!

[rhys-vdw]

I had a look at it last night. It's fixable but will require a bit of a
change. #771 fixed a different problem that was discussed in this issue.

On Thu, 3 Sep 2015 8:14 am Ben Ockmore notifications@github.com wrote:

Just wondering what the status of this is? #771
#771 doesn't seem to have
fixed it - not sure if it was meant to. Thanks!

—
Reply to this email directly or view it on GitHub
#397 (comment)
.

[crunchtime-ali]

The project leadership of Bookshelf recently changed. In an effort to advance the project we close all issues older than one year.

If you think this issue needs to be re-evaluated please post a comment on why this is still important and we will re-open it.

We also started an open discussion about the future of Bookshelf.js here #1600. Feel free to drop by and give us your opinion.
Let's make Bookshelf great again