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

Add support for composite unique constraint in auto migration #984

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

Conversation

atkei
Copy link

@atkei atkei commented Apr 12, 2024

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

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 ⚠️
piccolo/apps/migrations/auto/migration_manager.py 97.36% 2 Missing ⚠️
piccolo/apps/migrations/auto/diffable_table.py 95.00% 1 Missing ⚠️
piccolo/apps/migrations/auto/schema_snapshot.py 80.00% 1 Missing ⚠️
piccolo/constraint.py 96.96% 1 Missing ⚠️
piccolo/table.py 90.90% 1 Missing ⚠️

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

Additional details and impacted files
@@            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     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@sinisaos
Copy link
Member

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

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

@atkei Thanks a lot for this - looks great!

@AlexanderMakarov
Copy link

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

@dantownsend
Copy link
Member

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

@MoSheikh
Copy link

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

@AlexanderMakarov
Copy link

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

@sinisaos
Copy link
Member

sinisaos commented Aug 8, 2024

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

@dantownsend
Copy link
Member

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

@dantownsend Is this released?

@devsarvesh92
Copy link

@dantownsend is there any workaround to acheive this?

@sinisaos
Copy link
Member

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

@chilico
Copy link

chilico commented Oct 4, 2024

@dantownsend Any advances on this getting merged & released?

@stronk7
Copy link

stronk7 commented Oct 25, 2024

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.

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