slick icon indicating copy to clipboard operation
slick copied to clipboard

ignore autoinc column on update

Open cvogt opened this issue 12 years ago • 16 comments

We are ignoring autoinc column for inserts, we could to the same for updates. Either like this:

FoobarTable.filter(_.id === someFoobar.id).update(someFoobar)

where newRow's autoinc column is automatically excluded or even like this

FoobarTable.update(someFoobar)

where it is automatically used as a discriminator (as requested in #570).

cvogt avatar Jan 07 '14 15:01 cvogt

I second this, we're currently generating our own UpdateProjections to remove an auto updated timestamp column.

I vote for option 2:

FoobarTable.update(someFoobar)

brendanator avatar Jan 17 '14 17:01 brendanator

What does a timestamp column have to do with autoinc? On Jan 17, 2014 12:12 PM, "brendanator" [email protected] wrote:

I second this, we're currently generating our own UpdateProjections to remove an auto updated timestamp column.

I vote for option 2:

FoobarTable.update(someFoobar)

— Reply to this email directly or view it on GitHubhttps://github.com/slick/slick/issues/601#issuecomment-32624472 .

nafg avatar Jan 22 '14 22:01 nafg

We have a database standard where all of our tables have an auto-generated timestamp column on them. When you insert or update to a table this field will be set to the current timestamp. Therefore this could easily be described as auto-incrementing.

If you attempt to set this field in an insert or update statement it will fail with an error, therefore we've created our own projections to not set this field

N.B. We're working with an existing db schema

brendanator avatar Jan 23 '14 09:01 brendanator

Maybe I've created a bit of confusion here by suggesting

FoobarTable.update(someFoobar)

I was assuming a different field to the timestamp was marked as being the primary key.

It may also be of interesting to hear that we use this timestamp column to implement optimistic locking on top of slick

brendanator avatar Jan 23 '14 10:01 brendanator

Here is an instance of that problem: http://stackoverflow.com/questions/24631166/slick-update-of-field-on-sqlserver-jtds

cvogt avatar Jul 08 '14 17:07 cvogt

A lot of preliminary work was done for soft inserts and insertAndUpdate support in 2.0 and 2.1. It should be possible for updates to use the same mechanism as for inserts, i.e. optionally compile to two diferent statements for forced and soft update, and use different result converters for them that match the generated column list.

szeiger avatar Nov 24 '14 08:11 szeiger

Hello,

Just checked the status of this one because I saw earlier that it was scheduled for 3.2.0 but it's missing from 3.2.0-RC1. Apparently is was recently changed to be included in 3.2.1 instead.

@szeiger can you please confirm the milestone or give some insights on the current status of this improvement? At the present time, code that is designed to be ported across databases may not execute generic update() operations over a given SQLServer table that contains at least one auto-inc column.

I could try to modify the update statement for such use cases in order to remove the auto-inc column(s) from the statement itself, but that would be kind of clumsy. I really prefer to have this done by Slick itself.

gonmarques avatar Feb 20 '17 22:02 gonmarques

@gonmarques so yes, this has been moved to 3.2.1 to get 3.2.0 out. I don't think specific work has been going on into this direction further than what was described, but @szeiger may know better.

cvogt avatar Feb 21 '17 01:02 cvogt

It's still not clear - at least to me - if this issue is really an improvement or if it should have been labeled with some higher priority instead.

True fact is that Slick is generating erroneous SQL for a very specific RDBMS - SQLServer in this case - and such problematic generation could well be avoided using the metadata we have in our hands.

It may well be the very legitimate case to put the responsibility on the user. After all he/she is defining an action that instructs Slick to update all the record's columns. But on the other hand, we know before hand that auto-inc columns are probably to be left untouched - especially in this case where erroneous SQL is generated for this particular RDBMS.

We should also no forget that - as @cvogt mentions in the first comment - we are already "ignoring autoinc column for inserts". In order to keep things consistent, shouldn't this update() action also exclude auto-inc columns?

Should this be enough to force a new Release Candidate for 3.2.0? I understand that the combination of Slick + SQLServer + need for the update() action probably represents a little percentage of the overall use cases, but nevertheless it feels like something is being released with a well known issue left unresolved.

gonmarques avatar Feb 21 '17 10:02 gonmarques

We only force a new RC if an issue is a regression. I agree that this would be great to have. Things like this generally get fixed when either

  • one of the maintainers looks into it and community feedback is a guiding force here, but available time, interest and personal priority is as well. Most of us are volunteers.
  • an open source contributor fixes it
  • a Lightbend customer is pushing for it

cvogt avatar Feb 21 '17 13:02 cvogt

Apparently, it's still missing even in 3.2.1. Is there is any hope that it'll show up in the next release?

qamar2 avatar Nov 05 '17 09:11 qamar2

Hi, this looks like the problem I'm currently having, I can't get the following behaviour to work correctly:

Column A with AutoIncrement and NOT NULL

Insert with Column A set, should insert with the set value Insert without Column A set, should insert with next value

If I make the column nullable (in the database), I can get it to work this way, but then I can't use it in an index

Daxten avatar Jun 18 '18 12:06 Daxten

@Daxten your case is unrelated. You can ask for help on the gitter.im slick/slick channel.

hvesalai avatar Jun 18 '18 19:06 hvesalai

I'm also looking forward to this We have to use insertOrUpdate instead of update because our PostgreSQL does not allow updating of autogenerated primary keys. Is this difficult to implement? Could I help?

roti avatar Feb 28 '19 06:02 roti

I encountered a similar problem. We have an auto-increment column that gets updated on update or upsert operations. Any plans to address this? I see that upsert and updateInsert compilers are InsertCompiler.AllColumns rather than InsertCompiler.NonAutoInc:

  lazy val upsertCompiler = QueryCompiler(Phase.assignUniqueSymbols, Phase.inferTypes, new InsertCompiler(InsertCompiler.AllColumns), new JdbcInsertCodeGen(createUpsertBuilder))
  lazy val updateInsertCompiler = QueryCompiler(Phase.assignUniqueSymbols, Phase.inferTypes, new InsertCompiler(InsertCompiler.AllColumns), new JdbcInsertCodeGen(createUpdateInsertBuilder))

Is this the issue?

talha-sen-carbon avatar May 23 '23 10:05 talha-sen-carbon

you can checkout the latest version and use my test from this PR to check if it fixes it: https://github.com/slick/slick/pull/1930

Daxten avatar May 23 '23 11:05 Daxten