piccolo-orm / piccolo

A fast, user friendly ORM and query builder which supports asyncio.
https://piccolo-orm.com/
MIT License
1.45k stars 91 forks source link

Auto-created index name have a big chance to clash with the table name or name of some structure in the database. #990

Open metakot opened 5 months ago

metakot commented 5 months ago

So I have this two tables in postgres:

from piccolo.columns import *
from piccolo.table import Table

class EmployeeRole(Table):
    name = Varchar()

class Employee(Table):
    name = Varchar()
    role = ForeignKey(EmployeeRole, index=True)

The migration file is created successfully, but the migration itself ends with the error: asyncpg.exceptions.DuplicateTableError: relation "employee_role" already exists

It turns out that piccolo generates the following SQL: CREATE INDEX employee_role ON "employee" USING btree ("role"); And this clashes with the name of the table.

The code for index name lives here:

    @classmethod
    def _get_index_name(cls, column_names: t.List[str]) -> str:
        """
        Generates an index name from the table name and column names.
        """
        return "_".join([cls._meta.tablename] + column_names)

To prevent that kind of name conflicts, can we add the suffix to the index name, like _idx_XXXX there X is the random hex symbol?

UPD: however, in order to be able to gracefully undo the migration backwards the migration file must store the index name somethere or look it up from the database.

dantownsend commented 5 months ago

I think we should probably add an index_name argument for situations like this.

The downside is migrations would have to be aware of this, and if the user changes index_name we need to change it in the database. It's not too complex though:

ALTER INDEX name RENAME TO new_name