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

Foreign key input type is not in any way checked to actually represent the same model as in ForeignKey(model=...) parameter #1791

Open
HeroBrine1st opened this issue Nov 27, 2024 · 1 comment
Labels
enhancement New feature or request good first issue Good for newcomers

Comments

@HeroBrine1st
Copy link

HeroBrine1st commented Nov 27, 2024

Describe the bug

If fields.ForeignKeyField model parameter is not the same as in fields.ForeignKeyRelation, there's no help pointing at this inconsistency. It will work sometimes, sometimes it will fail

It is deterministic but took me like 2 hours using various search engines and LLMs to actually spot that by sanity check, when I managed to minimise one passing test's preamble (creating initial objects in database) close enough to second, failing test.

It also should be said somewhere in the start of this issue that I tested this bug only on SQLite, using aiosqlite.

To Reproduce
Create empty folder. Create testapp folder in it.

Create file models.py:

from tortoise import fields
from tortoise import Model

class TableA(Model):
    id = fields.IntField(primary_key=True)
    name = fields.CharField(max_length=255)

class TableB(Model):
    id = fields.IntField(primary_key=True)
    name = fields.CharField(max_length=255)

class TableC(Model):
    id = fields.IntField(primary_key=True)
    fkrelation: fields.ForeignKeyRelation[TableA] = fields.ForeignKeyField(
        # Look at it as it is a typo and it should be "models.TableA" like in type annotation
        # In my case it was an actual typo - I copied code and changed only related_name (and strangely changed to absolutely unrelated name (no pun intended), so I changed related_name again when I found that "model" parameter is incorrect).
        model_name="models.TableB",
        related_name="c_list"
    )
    name = fields.CharField(max_length=255)

Create file main.py:

Long file so I put it under spoiler
import asyncio
from tortoise import Tortoise
from testapp.models import TableA, TableB, TableC

async def will_fail():
    a = await TableA.create(name="tableA")
    b = await TableB.create(id=5, name="tableB") # unrelated id
    await TableC.create(name="tableC", fkrelation=a)

async def will_probably_pass():
    a = await TableA.create(name="tableA")
    b = await TableB.create(name="tableB")
    assert a.id == b.id # if this is false, next line will fail
    await TableC.create(name="tableC", fkrelation=a) # Here TableA as I intended provided in place of TableB as expected by library

async def will_definitely_pass():
    a = await TableA.create(id=10, name="tableA")
    b = await TableB.create(id=10, name="tableB")
    await TableC.create(name="tableC", fkrelation=a) # Here TableA as I intended provided in place of TableB as expected by library

async def main():
    config = {
        "connections": {"default": "sqlite://:memory:"},
        "apps": {
            "models": {
                "models": ["testapp.models"],
                "default_connection": "default",
            }
        }
    }
    await Tortoise.init(config=config)
    await Tortoise.generate_schemas()

    try:
        await will_fail()
    except Exception as e:
        print("1. Failed as excepted")
        print("1.", e)

    # run in isolation - close_connections will remove database from memory
    await Tortoise.close_connections()
    await Tortoise.init(config=config)
    await Tortoise.generate_schemas()

    try:
        await will_probably_pass()
        print("2. Passed as expected")
    except Exception as e:
        print("2. Failed as not expected but possible")
        print("2.", e)

    await Tortoise.close_connections()
    await Tortoise.init(config=config)
    await Tortoise.generate_schemas()

    try:
        await will_definitely_pass()
        print("3. Passed as excepted")
    except Exception as e:
        print("3. Failed as not excepted")
        print("3.", e)

    # exit does not work here, and it is not obvious that I need to close connections as it is an in-memory test
    await Tortoise.close_connections()

if __name__ == '__main__':
    asyncio.run(main())

Also I naively tried to exit(0) at the end when I see that script hangs, but it does not work (idk why). It is another (documentation?) issue but is not as frustrating as the one I wrote this reproducer for.

Run main.py. It will output this:

1. Failed as excepted
1. FOREIGN KEY constraint failed
2. Passed as expected
3. Passed as excepted

Second test is probabilistic but more close to production code (as it does not have hardcoded id values). Third test is deterministic. Otherwise they are the same.

It may be confusing that this script prints "as expected" in all three cases, but let me clarify that it is expected to reproduce bug.

Bonus points to the one who will make this reproducer one-file, it complains that default connection cannot be None otherwise.

Expected behavior

I think that all three tests should fail with invalid type passed. Or probably this behavior is documented somewhere (didn't found, documentation is very confusing), okay, then Tortoise ORM should verify that type annotation is consistent with actual runtime type, it will at least catch errors in the class of "changed in one place and forgot in other".

Additional context
Add any other context about the problem here.

I ran pipenv update and I have no version pinning, so I am on latest version as verified by pipenv requirements:

...
tortoise-orm==0.22.1; python_version >= '3.8' and python_version < '4.0'
...

This bug is pretty mischief. As you see in third case, I create rows in both tables in the same id:

async def will_definitely_pass():
    a = await TableA.create(id=10, name="tableA")
    b = await TableB.create(id=10, name="tableB")
    ...

It is the requirement for this bug to hide: there's row with the same id in another table. Idk how about production deployments, but it did not interfere with my first test as I first created rows in tables without foreign keys, and when I started writing second test where I needed to modify only some of all tables first test touched, I started thinking that sqlite://:memory: is cleared between Tortoise.init and Tortoise.close_connections, but it is not true. And the reproducer above works with in-file databases too, just remember to delete database file between all case.

If you look to generated schema, you'll see foreignkey from TableC to TableB, so foreign key violation is obvious (once you take a look at generated schema of course).

Also, type annotations are not a requirement - the only inconsistency required is between model parameter in fields.ForeignKeyField and actual type passed to this field at runtime.

@henadzit henadzit added enhancement New feature or request good first issue Good for newcomers labels Nov 28, 2024
@henadzit
Copy link
Contributor

Thank you for providing the complete reproducible example!

I think we definitely need to verify the type of relations during model instantiation as well as when assigning the relation to an object. I marked this ticket as "good first issue". I haven't dug deep but I think we need to do the following changes:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

2 participants