Skip to content

Use Tortoise model instead of raw SQL for managing internal table#17

Open
merlinz01 wants to merge 1 commit intohenadzit:mainfrom
merlinz01:use-tortoise-model
Open

Use Tortoise model instead of raw SQL for managing internal table#17
merlinz01 wants to merge 1 commit intohenadzit:mainfrom
merlinz01:use-tortoise-model

Conversation

@merlinz01
Copy link
Copy Markdown
Contributor

Main benefit:
The managing of the tortoise_pathway table is cross-database compatible.

Main disadvantage:
Users have to add the tortoise_pathway model to their TORTOISE_ORM configuration.
If I understand correctly this is necessary for aerich, so it's not out of the ordinary.

Side benefit:
The code is more maintainable.

Obscure side benefit:
It would be possible store the migrations data in a separate database from the managed database. That said, I don't know of a situation where that would be useful.

Copy link
Copy Markdown
Owner

@henadzit henadzit left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution.

I don't think this is really needed though. All operations with tortoise_migrations are pretty straightforward that can be described with universal SQL. And as you mentioned, this change complicates the configuration.

@merlinz01
Copy link
Copy Markdown
Contributor Author

The biggest reason this is needed is that although the SQL itself is universal, the substitution identifiers are different for Postgres vs SQLite vs MySQL. I was getting SQL syntax errors when trying to rollback because the SQL was using ? instead of $1 with Postgres.

Also, while I 100% agree that there isn't really risk of SQL injection with format strings, yet it is still industry best practices to do native substitution, and eliminates one however small area of risk. I could see problems with vulnerability scanners flagging it.

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.

2 participants