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

ForeignKeyField with source_field argument #55

Closed
krolikladoshka opened this issue Oct 9, 2018 · 5 comments · Fixed by #166
Closed

ForeignKeyField with source_field argument #55

krolikladoshka opened this issue Oct 9, 2018 · 5 comments · Fixed by #166
Labels
bug Something isn't working

Comments

@krolikladoshka
Copy link

Please, provide support for this feature. Currently ModelMeta metaclass ignores source_field for ForeignKey fields (just appends '_id' to field name). I've managed myself to fix this with one-liner directly inside tortoise-orm source code (can't create an abstract subclass of ModelMeta/Model, because Tortoise.init isn't setting _db field to all internal objects of orm), but this does not seem like a reliable fix.

Example where this can be helpful:

class Lambda(Model):
    id = fields.IntField(pk=True, null=False)

class Related(Model):
    lambda_ = fields.ForeignKeyField(
        'models.A',
        source_field='lambda'  # !
    )
@abondar
Copy link
Member

abondar commented Oct 9, 2018

Hi
Disregarding source_field is actually not desired behaviour and I think I will try to fix it soon, but fixing it is a little more complex than I would like it to be, cause some parts of code at the moment relies on the fact that {}_id field presence is guaranteed for every FK field.

@abondar
Copy link
Member

abondar commented Oct 19, 2018

@godstopme Could you please provide description of database schema for your case, your models for this schema (write models the way you think they should work) and show examples of desired behaviour?

@grigi grigi added the bug Something isn't working label Oct 27, 2018
@grigi grigi mentioned this issue Feb 26, 2019
72 tasks
@rockstar
Copy link

rockstar commented Apr 8, 2019

Here's an stripped down example:

class Task(models.Model):
    id = fields.IntField(pk=True)
    name = fields.CharField(max_length=10)

class Server(models.Model):
    id = fields.IntField(pk=True)
    label = fields.CharField(max_length=64)

class ExecutionEntry(models.Model):

    class Meta:
        unique_together = ('task', 'server')

    executed_at = fields.DatetimeField()
    task = fields.ForeignKey('models.Task', related_name='history', source_field='task')
    server = fields.ForeignKey('models.Server', related_name='history', source_field='server')

In this case, the execution_entry table is a join table with some extra metadata, and rather that task_id and server_id as the fields, they are task and server (because existing database).

I was going to submit a patch to fix this, but then I saw the comment that it is more complicated than I thought the fix to be. I don't see how it's complicated in the source code, so I'd like to know what I'm missing. This bug is a showstopper for me being able to use this library (and contribute to it as needed).

@grigi
Copy link
Member

grigi commented Apr 8, 2019

A lot of the complication is taken away by #122, which now handles all PK's via a generic way, which would make this much more reliable to implement.
If you want to go ahead and implement this, please do :-)
Please base it on the #122 branch, and then rebase with master once that is merged (It appears fully functional, just missing out on some tests).

@grigi
Copy link
Member

grigi commented Jun 17, 2019

Ok, I tried to have a go at this, and yes, I have a version that passes naïve tests, but it pollutes the single namespace per model.
so, e.g.

    one = fields.IntField(source_field='two')
    two = fields.CharField(max_length=10, source_field='one')

will break.

The solution would be to break Instance construction up into a from-python and a from-db, and have each of those constructors use different namespaces.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants