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

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

Open
kwando opened this issue Nov 24, 2016 · 8 comments
Open

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

kwando opened this issue Nov 24, 2016 · 8 comments

Comments

@kwando
Copy link
Contributor

kwando commented Nov 24, 2016

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
Copy link
Member

solnic commented Nov 26, 2016

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
Copy link
Contributor Author

kwando commented Nov 29, 2016

The workaround you provided works as intended.

@solnic
Copy link
Member

solnic commented Nov 30, 2016

@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
Copy link
Member

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
Copy link
Member

solnic commented Dec 4, 2016

@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
Copy link
Member

@solnic sounds like a plan, I like it

@solnic solnic added this to the v1.0.1 milestone Jan 27, 2017
@solnic solnic removed the discussion label Jan 27, 2017
@solnic solnic modified the milestones: v2.0.0, v1.0.1 Mar 2, 2017
@GustavoCaso
Copy link
Member

Love to help with this one

@solnic solnic modified the milestones: v2.0.0, v1.0.1, v2.1.0 Oct 16, 2017
@solnic
Copy link
Member

solnic commented Oct 17, 2017

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

@solnic solnic removed this from the v2.1.0 milestone Oct 20, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants