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

Composite primary key error

Open mauricioklein opened this issue 8 years ago • 8 comments

I'm facing this error when calling a custom command:

*** ROM::SQL::DatabaseError Exception: Mysql2::Error: Operand should contain 2 column(s)

The entity, repository, command, relation and sql command to create table can be found on this gits.

My database table has two columns, both primary key: user_id and company_id.

I've the container registered as following:

container.register('create_user_company') do
    container['persistence.rom'].command(:user_company)[:create]
end

So, when calling:

container['persistence.create_user_company'].({user_id: 1, company_id: 1})

I got the SQL error described above.

Any hint on how solve this or at least see which query is actually being called to create the records?

Thanks in advance!

mauricioklein avatar Jul 16 '16 00:07 mauricioklein

It almost works in master, I added a pending spec for now. The issue on the rom-sql side is fixed, now the real problem we have is that Sequel's dataset returns 0 when we insert a tuple with a composite PK. We gotta figure out how to make it return the actual PK value in order to be able to return inserted tuples. In postgres we have RETURNING support so it works there.

solnic avatar Jul 16 '16 14:07 solnic

@solnic it has nothing to do with sequel. MySQL does not support returning. The only way to get info about inserted record AFAIK is to call LAST_INSERT_ID function wich returns last fetched sequence (called AUTO_INCREMENT in MySQL) value. http://dev.mysql.com/doc/refman/5.7/en/information-functions.html#function_last-insert-id When you create a composite PK for a table that implements many-to-many relation you don't use sequences at all, both columns of the PK are actually FKs. This is why LAST_INSERT_ID returns 0, no sequences was touched, so let's return 0, why not? Sequel can determine if a database supports RETURNING clause or not, combining this with info if the PK is composite or not, we can decide (though cannot guarantee!) if we need to use values returned from Sequel or use values from the input tuple.

flash-gordon avatar Jul 16 '16 15:07 flash-gordon

@flash-gordon right, and because of that we have a problem with mysql since we can't easily get back inserted tuple when there's no auto-incremental PK...maybe we should simply return the input tuples when we have a composite pk.

solnic avatar Jul 16 '16 15:07 solnic

@solnic this is not the best we can do, a better option is to fetch inserted record based on provided tuples, ie reload them. Underlying dataset (table or DB view) could have triggers which could transform tuples as well (a common example is timestamp fields) and we do not want to return stale/incomplete data to the user.

Oh, also I want to admit that composite PKs are not a great idea after all because of this ^ kind of problems fwiw

flash-gordon avatar Jul 16 '16 15:07 flash-gordon

@flash-gordon yeah immediately after posting my comment I realized that what you just said is what needs to be done :D

And yeah, I wouldn't recommend using composite pks for non-join tables.

Anyhow, commands are extendible and we already have pg-specific extensions. I guess we should add a mysql specific ext and do the tricks there.

I'm gonna mark this as "help-wanted" because I don't use mysql and my time is super limited and I really need to wrap up a lot of things for the release. This can be fixed later on if we don't manage to include it in 0.8.0.

solnic avatar Jul 16 '16 15:07 solnic

Thanks for the attention on this matter.

In fact the records are persisted correctly, the problem is to map back the inserted records to Rom objects, as you already saw.

Since I'm just persisting these records as setup to my specs, I can deal with this error until a patch is released.

If I can help in anything, just let me know.

Thanks in advance!

mauricioklein avatar Jul 16 '16 17:07 mauricioklein

If you're just seeding data for specs, then I'd recommend using Relation#insert, this way your setup will be much simpler and faster. We're still discussing how to approach test data btw. Probably we'll introduce rom-fixtures at some point.

solnic avatar Jul 16 '16 19:07 solnic

Thanks @solnic ! I'm getting familiar with rom-rb yet, so all my knowledge came from official documentation. I'll use insert so. Thanks again!

mauricioklein avatar Jul 16 '16 19:07 mauricioklein