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

Insert causes error when no `id` field defined. #18

Closed
vjpr opened this Issue Jun 16, 2013 · 4 comments

Comments

Projects
None yet
3 participants
@vjpr
Contributor

vjpr commented Jun 16, 2013

{ sql: 'insert into "users" ("first_name") values ($1) returning "id"',
  bindings: [ 'xxx' ],
  __cid: '__cid2' }

timers.js:103
            if (!process.listeners('uncaughtException').length) throw e;
                                                                      ^
error: column "id" does not exist
@tgriesser

This comment has been minimized.

Owner

tgriesser commented Jun 17, 2013

For the time being, the fix for this is to add .idAttribute(null) on the builder chain... though I'm looking to change this to be .insert(values, [returning])... where if you specified the returning column you'd get back that from the insert (only required for postgres). Does that seem like it'd be a logical api?

@vjpr

This comment has been minimized.

Contributor

vjpr commented Jun 17, 2013

Yeh looks good.

I'd just mention that the sequelize library uses the 2nd parameter of its create method to pass in an array of settable attributes. The benefit is that it saves the user having to write checks for what attributes the user (of say a web app) is allowed to change. However, this is probably more Bookshelf.js territory.

@tgriesser

This comment has been minimized.

Owner

tgriesser commented Jun 18, 2013

Yeah, that'd fall more under Bookshelf... I had added whitelist/blacklist attributes in there for mass assignment, but then I decided to remove them and leave that up to the user to implement.

It's really easy to add the kitchen sink when it comes to validations/assignment and they can get pretty complex, so I'm going to try to keep it bare bones for now and focus mostly on doing a good job with relations (just added polymorphic associations btw).

I'll try to get to this returning issue a little later today.

@neilk

This comment has been minimized.

neilk commented Jul 2, 2013

Just got bitten with this one too. In my case my table doesn't even have a single 'id' field (there's a compound key). The assumption of an 'id' field seems to be pervasive in Knex/Bookshelf, but maybe that's okay.

I like your idea of a "returning" option - my expectation would be that it would be off by default.

@tgriesser tgriesser closed this in f79d423 Aug 7, 2013

elliotf pushed a commit to elliotf/knex that referenced this issue Nov 24, 2014

elliotf pushed a commit to elliotf/knex that referenced this issue Nov 24, 2014

Merge branch 'master' into gh-pages
* master:
  0.1.6
  using parentIdAttr for belongsToMany eager-fetch, fixes tgriesser#18
  test case for tgriesser#18
  adding test titles to make debugging easier
  create the model with appropriate foreignKey/val if not eager loading
  one more _reset
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment