PetaPoco icon indicating copy to clipboard operation
PetaPoco copied to clipboard

Should support multi-part primary keys.

Open pynej opened this issue 9 years ago • 7 comments

Used in many-to-many style table structures: IE:

Users
 userId

Groups
 groupId

UserGroups
 userId
 groupId

This can be loaded as expected using the following:

sql.From("Users")
                .LeftJoin("UserGroups")
                .On("User.MarketId = userGroups")
                .LeftJoin("Groups")
                .On("UserGroups.ServiceId = Groups.ServiceId")

The problem is though this data can be loaded and correlated into a hierarchy with a Relator no other normal operations can be done with the UserGroups models due to the multi-part key.

Ie one can't check .Exists, .IsNew, .Update, .Delete, or even .Fetch records directly from the joiner element. We can work around this with manually created update statements but it would be more consistent if they worked internally.

pynej avatar Aug 12 '16 17:08 pynej

I have started a PR for code changes and logical method changes to support multi-part keys. https://github.com/pynej/PetaPoco/tree/multi-part-pks

Basically I made two changes. First off to make the PrimaryKey attribute support multi-part keys, and secondly for all the exposed methods where a user can supply a primary key value to be params instead so they can supply more then one as needed.

This still needs updates to internal code so that other cases where the user isn't providing values but models instead will also use the primary key array instead.

pynej avatar Aug 12 '16 18:08 pynej

@pynej Looking good so far mate. I made a few comments on your commit; I hope this is ok with you?

pleb avatar Aug 13 '16 03:08 pleb

@pynej Don't forget a couple of integration tests to ensure composite keys works.

If you don't docker - that's ok - just test against whatever Db you use. I can test against all the others.

If you do docker, all you need to do is have the latest docker installed (the new fancy windows version) and then run docker up against the compose.yml. However you may want to remove oracledb from the file first; because A) it's a huge docker image and B) I haven't had time to add integration tests for it yet anyway so it's pointless.

pleb avatar Aug 13 '16 03:08 pleb

Cool. I'll look. Yah in not that farmilure with the internal code And that will need a bunch of update as well to get multi-part key working whe specified on the model level. But I'll keep working on it and get as far as I can.

pynej avatar Aug 13 '16 03:08 pynej

I'v done a good bit more work on this.

Basically I have:

  • Changed TableInfo.PrimaryKey to an array and updated references to it.
  • Added alternate methods to all the public API's to support specifying primaryKeyValue's as parameters. This will also support supplying them as a single object[] so the users can do it either way. Eg: DB.Update("SpecificPeople", new string[] {"Id", "cId"}, personOther, _person.Id, _person.cId) or DB.Update("SpecificPeople", new string[] {"Id", "cId"}, personOther, _person)
  • In some places there was also a parameter to specify the primaryKeyName so I also added changed that to string[] int he new methods.
  • Most of the duplicate methods just convert the single primaryKey to an array and then call the other method to minimize code.
  • Updated some of the internal code to properly handle multiple primary keys.

It may turn out that passing primaryKeyValues as params is creating to many issues and typing problems. If that is the case then all the params object[] primaryKeyValue definitions I added can be removed and the definition left as object primaryKeyValue. Then each function could check to see if primaryKeyValue is a object or array and handle it appropriately. This will mean only the DB.Update("SpecificPeople", new string[] {"Id", "cId"}, personOther, new[] {1, 1}) style call will work.

I can't really do much else to this though as I don't understand some of what is going on, and I don't have the setup to run the actual tests. So feel free to review the proposal continue to work on it.

pynej avatar Aug 17 '16 21:08 pynej

@pynej Can you make a PR to the CompositeKeySuppport branch. I'll add some integration tests

pleb avatar Aug 17 '16 23:08 pleb

@pynej I'll look at this over the weekend ;)

pleb avatar Aug 26 '16 05:08 pleb