Support composite unique constraints in auto migration

[atkei]

Related to #172 , #175 and based on #582, #583.

• Add UniqueConstraint class with the same behavior as a Column to support auto migration with composite unique constraint
• Update existing tests for internal changes and add tests for auto maigration with adding/dropping unique constraint

[dantownsend]

Thanks a lot for this!

I did a quick review, and it looks good. I appreciate the work you've put into it.

As it's quite a big feature, I'll do a more in depth review, and play around with it a bit.

[codecov-commenter]

Codecov Report

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

Project coverage is 92.07%. Comparing base (1b0f9b3) to head (3de9337).

Files	Patch %	Lines
piccolo/apps/migrations/auto/migration_manager.py	88.23%	2 Missing ⚠️
piccolo/columns/constraints.py	95.00%	1 Missing ⚠️

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

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #957      +/-   ##
==========================================
+ Coverage   92.01%   92.07%   +0.05%     
==========================================
  Files         108      109       +1     
  Lines        8240     8300      +60     
==========================================
+ Hits         7582     7642      +60     
  Misses        658      658              

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

[atkei]

@dantownsend
Thank you for your great work.
I love piccolo and I'm using it in my new project.
Let me help.

[dantownsend]

@sinisaos Can you help me test this if you get some spare time?

[sinisaos]

@dantownsend Sure. I will look into this during the day.

[sinisaos]

@dantownsend Migrations are fine and constraints are correctly added to the database.

The problems come after that when we try to query table. First error is

File "/home/rkl/dev/unique_env/lib/python3.11/site-packages/piccolo/columns/base.py", line 831, in get_select_string
    if self._alias:
       ^^^^^^^^^^^
AttributeError: 'UniqueConstraint' object has no attribute '_alias'

If I prevent AttributeError with try-except block another error is occured

 File "asyncpg/protocol/protocol.pyx", line 166, in prepare
asyncpg.exceptions.UndefinedColumnError: column task.task_constraints does not exist

I think the main problem is that UniqueConstraint is treated as a column, but that column does not exist, which is confirmed if print querystring like this print(Task.select().order_by(Task.id)), result is

SELECT ALL "task"."id", "task"."name", "task"."description", "task"."completed", "task"."task_constraints" FROM "task" ORDER BY "task"."id" ASC

[atkei]

@sinisaos @dantownsend

Thank you for your support.
I underestimated the impact of extending column class.
Considering extensibility and ease of testing, is the following better?

• Add a Constraint class like SQLAlchemy that does not inherit from the Column class, and a UniqueConstraint inherit from it, so easy to add CheckConstraint if users want
• In the Migration Manager, run _run_add_constraints() and _run_drop_constraints() to add and drop constraints

If you agree, I will close this PR and try again with this approach.

[sinisaos]

@atkei First of all, thank you for your efforts. Extending the column class is tempting because it's easier to implement migrations (also the case in the original PR), but then we run into problems because Piccolo later treats unique constraints as a column that doesn't actually exist (and it shouldn't be like that), as I wrote in a previous comment.

• Add a Constraint class like SQLAlchemy that does not inherit from the Column class, and a UniqueConstraint inherit from it, so easy to add CheckConstraint if users want
• In the Migration Manager, run _run_add_constraints() and _run_drop_constraints() to add and drop constraints

It would be great if you could make a new PR and that approach seems much better to me (although I don't know how hard it is to implement). I'll be happy to try that new approach.

Meanwhile we have an easy way to add and drop constraints using empty migration and raw sql like this

# add constraints
# create empty migration with command `piccolo migrations new your_app_name`
async def run():
    q = "ALTER TABLE task ADD CONSTRAINT task_constraints UNIQUE (name, description);"
    await Task.raw(q)

# drop constraints
# create another empty migration with command `piccolo migrations new your_app_name`
async def run():
    q = "ALTER TABLE task DROP CONSTRAINT task_constraints;"
    await Task.raw(q)

. Thanks again.

[atkei]

@sinisaos Yeah, I use raw SQL in my project, so this feature is not mandatory:)
Thank you very much.

[sinisaos]

@dantownsend @atkei I have a question for both of you. Did it make sense to add and drop unique constraints through the Piccolo CLI? This would avoid defining unique constraints on the table definition. User input should be validated, but that's basically it. It would look like in the video.

constraints.webm

It's just an idea, so please give your opinion.

[dantownsend]

@sinisaos It's a nice idea.

I do like having constraints defined in migrations, because when collaborating with other team members they get the constraints without having to do anything besides run the migrations.

I think this PR has a lot of promise, but constraints were treated like a special form of column, which while elegant in some ways, might also be confusing. I think I'd prefer to treat them as totally different things if possible.

[sinisaos]

I do like having constraints defined in migrations, because when collaborating with other team members they get the constraints without having to do anything besides run the migrations.

@dantownsend You're right. I didn't think about that at all. I'm not a professional developer and have never worked in a team, but what you say absolutely makes sense if more than one person is working on the same project. Thanks for the clarification and forget about the CLI solution. That is not good and as you say, it is much better to have defined constraints on migrations. Sorry and thanks again.