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

Support for composite primary keys #160

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

igrali
Copy link

@igrali igrali commented Apr 4, 2015

Code changes to enable composite primary keys.

It was mentioned here. #72

I think it would be really good to have this supported. I was only able to run tests under Generic, Win32 and WindowsPhone8, others wouldn't build for some reason, I'll have to investigate.

Let me know what you think!

EDIT: I see AppVeyor build failed for some reason, but I don't think it's because of my code.

@Shiney
Copy link

Shiney commented Apr 6, 2015

Would it be better to keep the current methods for if there is only one column of primary key. Also why have you only changed the delete methods to accept multiple primary keys a and missed out the get methods?

@igrali
Copy link
Author

igrali commented Apr 6, 2015

I'll gladly fix if I missed anything, could you point to exact methods?

Delete was the only one with changed signature as there is an overload which allows deleting based on the primary key. But maybe it would be worthwhile to keep the old one too, so if someone is using only single attribute keys they can keep using the old Delete call, without creating a dictionary every time.

Creating the table should behave the same if there's a single attribute key, allowing for autoincrement to be set.

Maybe I also should have left the name "PK" instead of changing it to "PKs" as technically there's always only one primary key (even if it spans over multiple columns).

@Shiney
Copy link

Shiney commented Apr 6, 2015

There are two "Find" methods that you would need to change also a "Get" method all on SQLiteConnection

@igrali
Copy link
Author

igrali commented Apr 6, 2015

Good point. I changed those 2 methods, and also Delete method to take object again as parameter to not break any calls with only a single column primary key.

@Shiney
Copy link

Shiney commented Apr 6, 2015

Also TableMapping is at the moment part of the publicAPI, so it may not be a good idea to remove a property like you have, maybe it would be better to provide both properties and have PK throw an exception if there is a composite primary key so that current users of the library can still use the Property if they are not using composite primary keys.

@igrali
Copy link
Author

igrali commented Apr 6, 2015

You mean, have both PK and PKs? Hmm.

@Shiney
Copy link

Shiney commented Apr 6, 2015

Yeah that would probably be better than breaking the code of people who are using the library.

@igrali
Copy link
Author

igrali commented Apr 6, 2015

Yeah, both options are kind of bad, but breaking change is probably worse. :)
I'll do it that way then.

@igrali
Copy link
Author

igrali commented Apr 11, 2015

I pushed the changes we discussed.

@oysteinkrog
Copy link
Owner

This sounds nice, I'll review/merge when I've got the current prerelease/beta out the door.

@igrali igrali mentioned this pull request May 7, 2015
@ghost
Copy link

ghost commented May 21, 2015

Any news on this?

@oysteinkrog
Copy link
Owner

I'm sorry but I don't have any time to spend on this project at the moment.

@pkl728
Copy link

pkl728 commented Jul 22, 2015

Any word on this? Just ran into a situation where Composite Keys would be really nice to have :)

@softlion
Copy link

It's already done here with latests paracleum updates: https://github.com/softlion/SQLite.Net-PCL
All test passed. + new tests for multiple keys and partial keys (select and delete).
This is my own implementation started 20 months ago and used in multiple commercial apps.

It breaks the Delete(primary key), as it conflicts with the Delete(object) and with Delete(primary keys[]).

@MisterJimson
Copy link

Can we get an update? Would love to have this.

softlion, do you have your nuget for your fork? I couldn't find it.

@softlion
Copy link

Yes i have a private nuget server on internet. PM me your email and i grant you access.

@wierzba3
Copy link

wierzba3 commented Jan 6, 2017

Is this supported now?

@gsaldana
Copy link

Any news with this??
When it's going to be released?

@filyjunior
Copy link

Any news with this?? Is working in Xamarin Ios?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants