rom-rb / rom-sql

SQL support for rom-rb
https://rom-rb.org
MIT License
218 stars 93 forks source link

Insert command does not work when primary key is not AUTO_INCREMENT #109

Open kwando opened 7 years ago

kwando commented 7 years ago

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`
solnic commented 7 years ago

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.

kwando commented 7 years ago

The workaround you provided works as intended.

solnic commented 7 years ago

@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 :)

flash-gordon commented 7 years ago

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.

solnic commented 7 years ago

@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.

flash-gordon commented 7 years ago

@solnic sounds like a plan, I like it

GustavoCaso commented 7 years ago

Love to help with this one

solnic commented 7 years ago

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