piccolo-orm / piccolo

A fast, user friendly ORM and query builder which supports asyncio.
https://piccolo-orm.com/
MIT License
1.45k stars 91 forks source link

Add support for composite unique constraint in auto migration #984

Open atkei opened 7 months ago

atkei commented 7 months ago

Related to #172, #175 and based on #957.

Add UniqueConstraint to support composite unique constraint in auto migration.

from piccolo.columns import Text
from piccolo.constraint import UniqueConstraint
from piccolo.table import Table

class Band(Table):
    name = Text(required=True)
    label = Text(required=True)
    unique_name_label = UniqueConstraint(["name", "label"])

Some work remains such as adding doc and testing, but I would like to have feedback on my proposal first.

codecov-commenter commented 7 months ago

Codecov Report

Attention: Patch coverage is 95.85253% with 9 lines in your changes are missing coverage. Please review.

Project coverage is 89.72%. Comparing base (363d683) to head (07f8b94). Report is 1 commits behind head on master.

Files Patch % Lines
piccolo/apps/migrations/auto/schema_differ.py 92.85% 3 Missing :warning:
piccolo/apps/migrations/auto/migration_manager.py 97.36% 2 Missing :warning:
piccolo/apps/migrations/auto/diffable_table.py 95.00% 1 Missing :warning:
piccolo/apps/migrations/auto/schema_snapshot.py 80.00% 1 Missing :warning:
piccolo/constraint.py 96.96% 1 Missing :warning:
piccolo/table.py 90.90% 1 Missing :warning:

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #984 +/- ## ========================================== - Coverage 92.78% 89.72% -3.07% ========================================== Files 108 109 +1 Lines 8182 8399 +217 ========================================== - Hits 7592 7536 -56 - Misses 590 863 +273 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

sinisaos commented 7 months ago

@dantownsend I hope it's not a problem, but I have approved and run the workflow for this PR. I have already tried these changes in the @atkei local branch and in my case everything works great as I wrote in this comment. Can you try and review this PR. Thanks.

dantownsend commented 7 months ago

@sinisaos Awesome thanks - feel free to run the CI whenever you want.

@atkei Thanks a lot for this - looks great!

AlexanderMakarov commented 6 months ago

@sinisaos @dantownsend - could you make a hint - when it would be merged? Would it mean new version/release?

dantownsend commented 6 months ago

@AlexanderMakarov I'll try my best to review it properly this week.

MoSheikh commented 6 months ago

Currenty looking for this funtionality - would appreciate it someone on the team would be able to review!

AlexanderMakarov commented 3 months ago

@sinisaos @dantownsend - do you have some plans to deliver this functionality soon? Thank you in advance.

sinisaos commented 3 months ago

@AlexanderMakarov Sorry, but you have to wait for @dantownsend response.

dantownsend commented 3 months ago

Yes, it's definitely high on my radar. I need to try and get some improvements done to Piccolo Admin, but this is high on my list. Thanks for the patience.

devsarvesh92 commented 2 months ago

@dantownsend Is this released?

devsarvesh92 commented 2 months ago

@dantownsend is there any workaround to acheive this?

sinisaos commented 2 months ago

@devsarvesh92 For now, the workaround is to use raw sql (in migrations or script) to add a composite unique constraint, as described here.

chilico commented 1 month ago

@dantownsend Any advances on this getting merged & released?

stronk7 commented 3 weeks ago

Hi,

great to see this coming! For sure support for composite unique indexes/constraints is a real use case!

I just wanted to introduce here a related question that is… why not make this to also support non-unique composite indexes? After all, the uniqueness is “only” an attribute of the index, isn’t it?

For sure, if in the future support for composite PKs/FKs is added, then the composite UK will need to be considered again (because FKs should be able to point to UKs too. But right now (after having read various issues here), that seems to be not trivial to happen.

So, back to the question, why not get the opportunity of this PR to, also, support non unique indexes?

Ciao :)

Disclaimer: I’m completely new to Piccolo, so maybe what I’m asking doesn’t make much sense or already can be done (apart from doing it in manual migrations), but from the research that I’m doing over various Python ORM libraries, where Piccolo is one of the “finalists”, this is pretty much, the only feature that I’m missing right now, so just sharing.