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

Fix index creation in Tortoise.generate_schemas() for MySQL and Postgres #1847

Merged
merged 4 commits into from
Jan 20, 2025

Conversation

henadzit
Copy link
Contributor

@henadzit henadzit commented Jan 10, 2025

Description

  • Replace CREATE INDEX IF NOT EXISTS queries with indexes defined inside the table definition for MySQL
  • Fix partial index creation for Postgres - CREATE INDEX "idx_index_partial_c5be6a" ON "index" USING ("partial") WHERE id = 1; isn't correct syntax because USING should be followed by the index type.
  • Replace the logic for index creation from Index and its subclasses with schema_generator, hence making schema_generator a single place to have schema generation logic. This also also makes the Index and schema_generator relation one directional.
  • Fix the issue with using Model.describe when Index is in Model.Meta.indexes

Motivation and Context

This should fix #1836.

How Has This Been Tested?

make ci

I also tested the generated SQLs against corresponding databases.

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added the changelog accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

Copy link

codspeed-hq bot commented Jan 10, 2025

CodSpeed Performance Report

Merging #1847 will not alter performance

Comparing henadzit:fix/mysql-and-postgres-idx (d4f7c11) with develop (e34c2c0)

Summary

✅ 8 untouched benchmarks

@coveralls
Copy link

coveralls commented Jan 10, 2025

Pull Request Test Coverage Report for Build 12751398752

Details

  • 42 of 48 (87.5%) changed or added relevant lines in 8 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.006%) to 90.258%

Changes Missing Coverage Covered Lines Changed/Added Lines %
tortoise/backends/oracle/schema_generator.py 2 3 66.67%
tortoise/indexes.py 5 6 83.33%
tortoise/models.py 5 6 83.33%
tortoise/backends/base/schema_generator.py 14 17 82.35%
Totals Coverage Status
Change from base Build 12714096970: 0.006%
Covered Lines: 6441
Relevant Lines: 7019

💛 - Coveralls

@abondar
Copy link
Member

abondar commented Jan 10, 2025

@waketzheng what do you think about this approach?

Even though it moves more code - I think end result seems a bit clearer, as we don't introduce new entities and improve existing code by moving out dialect-specific schema generation code to it's intended place

@waketzheng
Copy link
Contributor

Looks good.

@henadzit Cloud you add more test cases? For example, add class Meta: Index(fields=['name']) to the Node class in tests/testmodels.py

@henadzit
Copy link
Contributor Author

Looks good.

@henadzit Cloud you add more test cases? For example, add class Meta: Index(fields=['name']) to the Node class in tests/testmodels.py

Will do!

@henadzit henadzit force-pushed the fix/mysql-and-postgres-idx branch from a833893 to 6ce5711 Compare January 13, 2025 15:44
@henadzit
Copy link
Contributor Author

I added tests and fixed the issue with using Model.describe when an Index is in Model.Meta.indexes.

@henadzit henadzit requested a review from abondar January 15, 2025 18:46
@waketzheng waketzheng merged commit 71624e7 into tortoise:develop Jan 20, 2025
10 checks passed
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.

MySQL failed to create schema with indexes=[Index(fields=...)] in Meta
4 participants