rom-sql icon indicating copy to clipboard operation
rom-sql copied to clipboard

Insert command does not work when primary key is not AUTO_INCREMENT

Open kwando opened this issue 7 years ago • 8 comments

I have a table where the primary key is not generated by the database, I'm using MySQL.

When using a Create command to insert data it will return the wrong tuple.

describe 'rom-bug' do
  let(:config) { ROM::Configuration.new(:sql, '<mysql url>') }
  let(:rom) {
    config.default_gateway.connection.create_table!(:tenants) {
      String :id, size: 36, null: false, primary_key: true
      String :name, null: false
    }

    config.relation(:tenants)

    config.commands(:tenants) {
      define(:create) {
        result(:one)
      }
    }

    ROM.container(config)
  }


  it 'creates the correct tupple' do
    create_tenant = rom.command(:tenants).create

    input         = {id: 'AAAA', name: 'Company A'}
    tenant_a      = create_tenant.call(input)

    expect(tenant_a).to eq(input)

    input         = {id: 'BBBB', name: 'Company B'}
    tenant_b      = create_tenant.call(input)

    expect(tenant_b).to eq(input)
  end
end
I, [2016-11-24T17:42:18.990413 #36340]  INFO -- : (0.000443s) INSERT INTO `tenants` (`id`, `name`) VALUES ('AAAA', 'Company A')
I, [2016-11-24T17:42:18.990921 #36340]  INFO -- : (0.000241s) SELECT `id`, `name` FROM `tenants` WHERE (`id` IN (0)) ORDER BY `tenants`.`id`
I, [2016-11-24T17:42:18.992827 #36340]  INFO -- : (0.000483s) INSERT INTO `tenants` (`id`, `name`) VALUES ('BBBB', 'Company B')
I, [2016-11-24T17:42:18.993585 #36340]  INFO -- : (0.000360s) SELECT `id`, `name` FROM `tenants` WHERE (`id` IN (0)) ORDER BY `tenants`.`id`

kwando avatar Nov 24 '16 16:11 kwando

This needs a custom command. The built in behavior depends on sequel datasets that are supposed to return PK values and it looks like it doesn't do that when the PK is not auto-generated.

If you're blocked by this you can define your own command class and override insert ie:

class CreateWithCustomPk < ROM::SQL::Commands::Create
  def insert(tuples)
    tuples.each { |tuple| relation.insert(tuple) }
    pks = tuples.map { |tuple| tuple.fetch(relation.primary_key) }
    relation.where(relation.primary_key => pks).to_a
  end
end

Let me know if this works for you, we can try to figure out how to add that to rom-sql I think.

solnic avatar Nov 26 '16 10:11 solnic

The workaround you provided works as intended.

kwando avatar Nov 29 '16 16:11 kwando

@kwando thanks for testing it out

@flash-gordon wdyt about adding a special command type for relations w/o an auto-incremental PK? it's not gonna be a common case but I'm sure it'll help people when they have to deal with legacy db schemas :)

solnic avatar Nov 30 '16 09:11 solnic

I think it's better to make it more straight-forward, that is if a PK value passed explicitly we'll use it for returning inserted records, how about that? :) P.S. You can't rely on the presence of auto-incremental flag or something because there can be triggers which can do literally anything, including using custom sequences or generating PKs from other column values.

flash-gordon avatar Dec 04 '16 11:12 flash-gordon

@flash-gordon we can do that, although I'm not sure if I'm OK with extra code that is there just to handle something that is rarely the case :) What we could do is add ability to mark a PK as a non-auto increment and if that's the case, then we can extend commands with behavior that is needed to handle it properly. We already have a hook that extends command with additional behaviors, it receives command class and a dataset, so if we change it to receive a whole relation class then we can use its schema to see if we need to apply extensions.

solnic avatar Dec 04 '16 11:12 solnic

@solnic sounds like a plan, I like it

flash-gordon avatar Dec 04 '16 12:12 flash-gordon

Love to help with this one

GustavoCaso avatar Oct 11 '17 15:10 GustavoCaso

This is super low priority. I moved it to 2.1.0 milestone

solnic avatar Oct 17 '17 08:10 solnic