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: mysql generated schema syntax error #1845

Closed
wants to merge 8 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions tests/schema/test_generate_schema.py
Original file line number Diff line number Diff line change
Expand Up @@ -726,8 +726,8 @@ async def test_index_safe(self):
`full_text` LONGTEXT NOT NULL,
`geometry` GEOMETRY NOT NULL
) CHARACTER SET utf8mb4;
CREATE FULLTEXT INDEX IF NOT EXISTS `idx_index_full_te_3caba4` ON `index` (`full_text`) WITH PARSER ngram;
CREATE SPATIAL INDEX IF NOT EXISTS `idx_index_geometr_0b4dfb` ON `index` (`geometry`);""",
CREATE FULLTEXT INDEX `idx_index_full_te_3caba4` ON `index` (`full_text`) WITH PARSER ngram;
CREATE SPATIAL INDEX `idx_index_geometr_0b4dfb` ON `index` (`geometry`);""",
)

async def test_index_unsafe(self):
Expand Down Expand Up @@ -1102,7 +1102,7 @@ async def test_index_unsafe(self):
CREATE INDEX "idx_index_gist_c807bf" ON "index" USING GIST ("gist");
CREATE INDEX "idx_index_sp_gist_2c0bad" ON "index" USING SPGIST ("sp_gist");
CREATE INDEX "idx_index_hash_cfe6b5" ON "index" USING HASH ("hash");
CREATE INDEX "idx_index_partial_c5be6a" ON "index" USING ("partial") WHERE id = 1;""",
CREATE INDEX "idx_index_partial_c5be6a" ON "index" USING ("partial") WHERE id = 1;""",
)

async def test_index_safe(self):
Expand All @@ -1126,7 +1126,7 @@ async def test_index_safe(self):
CREATE INDEX IF NOT EXISTS "idx_index_gist_c807bf" ON "index" USING GIST ("gist");
CREATE INDEX IF NOT EXISTS "idx_index_sp_gist_2c0bad" ON "index" USING SPGIST ("sp_gist");
CREATE INDEX IF NOT EXISTS "idx_index_hash_cfe6b5" ON "index" USING HASH ("hash");
CREATE INDEX IF NOT EXISTS "idx_index_partial_c5be6a" ON "index" USING ("partial") WHERE id = 1;""",
CREATE INDEX IF NOT EXISTS "idx_index_partial_c5be6a" ON "index" USING ("partial") WHERE id = 1;""",
)

async def test_m2m_no_auto_create(self):
Expand Down
4 changes: 4 additions & 0 deletions tests/testmodels.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
from tortoise import fields
from tortoise.exceptions import ValidationError
from tortoise.fields import NO_ACTION
from tortoise.indexes import Index
from tortoise.manager import Manager
from tortoise.models import Model
from tortoise.queryset import QuerySet
Expand Down Expand Up @@ -150,6 +151,9 @@ class ModelTestPydanticMetaBackwardRelations3(Model):
class Node(Model):
name = fields.CharField(max_length=10)

class Meta:
indexes = [Index(fields=("name",))]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like you added this index because it exposes the issue with serializing Index in Model.describe. I was able to identify it only because I worked on the same issue in my PR, it wouldn't be clear to anyone reviewing this PR.

Even though this index exposes an issue, it isn't documented and completely not obvious why it is here. Someone can remove it later without knowing its purpose and the same regression can happen again. I added a dedicated class for testing indexes in my PR.



class Tree(Model):
parent: fields.ForeignKeyRelation[Node] = fields.ForeignKeyField(
Expand Down
9 changes: 8 additions & 1 deletion tests/utils/test_describe_model.py
Original file line number Diff line number Diff line change
Expand Up @@ -25,12 +25,19 @@
ManyToManyFieldInstance,
OneToOneFieldInstance,
)
from tortoise.indexes import Index


def dump_index(obj):
if isinstance(obj, Index):
return repr(obj)
return obj


class TestDescribeModels(test.TestCase):
def test_describe_models_all_serializable(self):
val = Tortoise.describe_models()
json.dumps(val)
json.dumps(val, default=dump_index)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This hides the issue that Index cannot be described in a serializable way, however, it doesn't fix it. I fixed it in my PR by implementing Index.describe.

self.assertIn("models.SourceFields", val.keys())
self.assertIn("models.Event", val.keys())

Expand Down
2 changes: 1 addition & 1 deletion tortoise/backends/mysql/executor.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import enum

from pypika_tortoise import functions, SqlContext
from pypika_tortoise import SqlContext, functions
from pypika_tortoise.enums import SqlTypes
from pypika_tortoise.functions import Cast, Coalesce
from pypika_tortoise.terms import BasicCriterion, Criterion
Expand Down
6 changes: 3 additions & 3 deletions tortoise/indexes.py
Original file line number Diff line number Diff line change
Expand Up @@ -59,11 +59,11 @@ def get_sql(
ctx = schema_generator.client.query_class.SQL_CONTEXT
expressions = [f"({expression.get_sql(ctx)})" for expression in self.expressions]
fields = ", ".join(expressions)

exists = "IF NOT EXISTS " if safe and schema_generator.DIALECT != "mysql" else ""
return self.INDEX_CREATE_TEMPLATE.format(
exists="IF NOT EXISTS " if safe else "",
exists=exists,
index_name=schema_generator.quote(self.index_name(schema_generator, model)),
index_type=f" {self.INDEX_TYPE} ",
index_type=f" {self.INDEX_TYPE} " if self.INDEX_TYPE else " ",
table_name=schema_generator.quote(model._meta.db_table),
fields=fields,
extra=self.extra,
Expand Down
5 changes: 2 additions & 3 deletions tortoise/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -37,9 +37,8 @@ async def generate_schema_for_client(client: "BaseDBAsyncClient", safe: bool) ->
:param safe: When set to true, creates the table only when it does not already exist.
"""
generator = client.schema_generator(client)
schema = get_schema_sql(client, safe)
logger.debug("Creating schema: %s", schema)
if schema: # pragma: nobranch
if schema := generator.get_create_schema_sql(safe): # pragma: nobranch
logger.debug("Creating schema: %s", schema)
await generator.generate_from_string(schema)


Expand Down
Loading