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

Composite idAttributes #93

Closed
wants to merge 3 commits into from
Closed

Composite idAttributes #93

wants to merge 3 commits into from

Conversation

tgriesser
Copy link
Member

Beginning a branch to address #86 - composite idAttribute keys on models.

So far, I've only got a rough implementation of the base model. I still need to figure out how this will play into the relations, but I figured if anyone has ideas or opinions they could chime in here.

/cc @brianjmiller - I think you were the one mentioning looking for this feature, do you have any thoughts on what the api should look like when forming relations on models that have multiple column pk's?

@brianjmiller
Copy link

I don't really have any great ideas. The one other ORM I've used to any great extent supported it and it is one of the few that I know of that does, so might be able to have a look at how they implemented them. From the Perl world, but overall same goal:

https://metacpan.org/pod/Rose::DB::Object

@linaGirl
Copy link

hi, whats the progress on this issue? it would be really nice to have this feature.

@tgriesser
Copy link
Member Author

Hey @eventEmitter - so I got the basics down for creating models with compound id's... but I'm not certain about how relation loading/pairing should work here.

Eager loading depend on where in queries, which I don't believe will work with multiple keys without a bit of an overhaul on how the queries are composed.

How do you anticipate using this feature (what types of relations, etc) - that might help me visualize how to best build this out.

@huliseerow
Copy link

Hey @tgriesser I'm answering on behalf of @eventEmitter. We use it mostly for internationalization. Meaning we have a table for example Country for this table we hava a CountryLocale Table with the country_id, language_id and the name of the Country in the corresponding language. So the PrimaryKey of CountryLocale is composed of the two keys id_language and id_country.

@nathasm
Copy link

nathasm commented May 4, 2015

Is this a dead issue? Or are there still plans for composite key support?

@TheBeege
Copy link

Hey @tgriesser - I'm dead in the water without this. I'll throw a use case at you in the event it helps.

We have timelines, posts, events, and users. A user can place an event on a timeline by creating a post. Timelines and events belong to many of each other through posts. Posts also belong to a user. Our post table looks like this:

CREATE TABLE posts
(
  timeline_id integer NOT NULL,
  event_id integer NOT NULL,
  create_date timestamp without time zone,
  poster integer,
  CONSTRAINT posts_pkey PRIMARY KEY (timeline_id, event_id)
)

Weird, part is, I found mention of composite keys at http://minocys.azurewebsites.net/designing-postgresql-tables-with-composite-keys/. When using this in practice, I hit Unhandled rejection error: column "id" does not exist. The debug output looks like this:

{
    method: 'insert',
    options: {},
    bindings: ['2015-09-01 00:00:00', 31, 474, 146],
    sql: 'insert into "posts" ("create_date", "event_id", "poster", "timeline_id") values (?, ?, ?, ?) returning "id"',
    returning: 'id'
}

It's trying to spit out an ID, even though I've specified idAttributes on my Post model as ['timeline_id', 'event_id'].

Is this evidence of BookshelfJS still not supporting composite keys, or is this some sort of unrelated bug?

EDIT: If you need help with this at all, I'm heavily invested in BookshelfJS due to this project. If I can get some context, I'll do the best I can.

@rhys-vdw
Copy link
Contributor

Is this evidence of BookshelfJS still not supporting composite keys, or is this some sort of unrelated bug?

Composite keys are unsupported. Adding comprehensive support would be difficult (eg. certain relation types). It would be easier to add partial support (eg. allowing the operation you're going for there by not trying to return id). If you'd like to open a new issue per supported feature we can discuss on a case by case basis and try to get some better behaviour for you.

However what you're describing is (in Bookshelf terms) a many to many relationship with a pivot table (in this case you can describe this without creating a specific model for posts.

var Event, Timeline;

var postColumns = ['create_date', 'poster'];

Event = bookshelf.Model.extend({
  tableName: 'events',
  events() {
    return this.belongsToMany(Timeline, 'posts').withPivot(postColumns);
  }
});

Timeline = bookshelf.Model.extend({
  tableName: 'timelines',
  events() {
    return this.belongsToMany(Event, 'posts').withPivot(postColumns);
  }
});

Timeline.forge({ id: 5 }).related('events').create({ /* event fields */ });

@rluba
Copy link

rluba commented Mar 31, 2016

@rhys-vdw But how would you set create_date and poster in this example when using .create(…)?

Related: How would you insert a new Event-Timeline relation for an existing Event and existing Timeline and set create_date and poster as part of the INSERT? Using attach and then updatePivot requires two roundtrips to the database and is impossible if eg. poster is a required field.

@rluba
Copy link

rluba commented Mar 31, 2016

Answered my own question in #648.

@jcowgar
Copy link

jcowgar commented Apr 27, 2016

Another ORM I have explored recently does composite relation keys as an array. Translating to Bookshelf(ish) talk:

return this.belongsTo(Client, ['timeline_id', 'event_id']);

@dnascimento
Copy link

Any update on this?

@linaGirl
Copy link

For me it's https://www.npmjs.com/package/related

@castarco
Copy link

Maybe this PR should be closed.... its too old to be fixed to match the current code base.

@mrhwick
Copy link
Member

mrhwick commented Oct 12, 2017

Closing until re-implementation. Collected all related issues under #1664

@mrhwick mrhwick closed this Oct 12, 2017
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.

None yet