piccolo-orm / piccolo

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

using db_column_name with ForeignKey breaks join traversal when using select().where(...) #1101

Closed jessemcl-flwls closed 1 month ago

jessemcl-flwls commented 1 month ago

If I have two tables with a foreign key relationship between them, and I specify the relationship using a custom db_column_name, eg:

class FooTable(Table, tablename="foo", schema="public"):
    id = Varchar(length=20, primary_key=True, null=False)
    name = Varchar(length=64, unique=True, null=False)

class BarTable(Table, tablename="bar", schema="public"):
    id = Varchar(length=20, primary_key=True, null=False)
    foo = ForeignKey(references=FooTable, null=False, db_column_name="foo_id")

Then when I try to query this relationship using normal traversal:

foo = await BarTable.select().where(BarTable.foo.id == "abc").first()

Then I get an error saying that the column foo does not exist.

This all works normally if I leave out the db_column_name from the table definition.

sinisaos commented 1 month ago

@jessemcl-flwls Yes, you are right. It's kind of an edge case. We could change this part of the code with something like this

@property
def name(self) -> str:
    if not self._name:
        raise ValueError(
            "`_name` isn't defined - the Table Metaclass should set it."
        )
    column_name = (
        self._name if self._db_column_name is None else self._db_column_name
    )
    return column_name

and with those changes everything works for queries. But that change also has problems when creating the object. We need to pass db_column_name, not a name like this

bar_data = BarTable(id="123", foo_id="abc") # foo_id instead of foo

It doesn't seem to be the right solution.

dantownsend commented 1 month ago

Yeah, it looks like we're using the column's name instead of db_column_column in the join query.

I've put together a PR to fix it - it's a super minor change. Our unit tests must have missed this. Will add some more tests, then will merge in.

Thanks both.

dantownsend commented 1 month ago

This should be fixed in piccolo==1.21.0.