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

Cannot use callable default property on model ForeignKeys #20

Closed
airguru opened this issue Oct 25, 2020 · 3 comments
Closed

Cannot use callable default property on model ForeignKeys #20

airguru opened this issue Oct 25, 2020 · 3 comments

Comments

@airguru
Copy link

airguru commented Oct 25, 2020

Whenever I use something like this in my model:

system_currency = models.ForeignKey(Currency, on_delete=models.PROTECT, to_field='code', default=Currency.objects.first().code)

I get AttributeError: 'DatabaseFeatures' object has no attribute 'has_native_json_field' when trying to query a model that contains JSONField. Surprisingly, it doesn't seem to affect admin interface.

Changing the default property to constant value fixes the issue:

system_currency = models.ForeignKey(Currency, on_delete=models.PROTECT, to_field='code', default='eur')

It seems that maybe using queries inside model definitions somehow initializes the db config before this library can inject itself into it. Moving the library into the first place in INSTALLED_APPS doesn't help.

Phew! Took me a while to figure this out!

@laymonage
Copy link
Owner

laymonage commented Oct 25, 2020

Hi @airguru,

Yeah, the problem isn't about using a callable default property. It's because class definitions are loaded very early by Python. Therefore, with your field definition, Django tries to do a database query before everything is set up properly.

Even though it's possible to do this, I don't think this is something that you should do 🤔

Funnily enough, have you tried wrapping the value in an actual callable? Something like:

def default_system_currency():
    return Currency.objects.first().code

class SomeModel(models.Model):
    ...
    system_currency = models.ForeignKey(Currency, on_delete=models.PROTECT, to_field='code', default=default_system_currency)
    ...

I think it should defer the query until it's actually needed.

@airguru
Copy link
Author

airguru commented Oct 25, 2020

Yes, wrapping it actually works!

I thought database queries are lazy enough to be deferred until actually needed! But either way, even if it's possibly not something that one should do, it's unfortunately possible (and suggested as an option for default foreign key values in some stackoverflow answers) and when it breaks it's quite hard to find the cause. Especially because you can't really make sense of anything in debug mode. The attribute that's supposed to be mising actually shows up when the debugger breaks on the exception (so maybe the connection.features get set up in the meantime?), and no reasonable amount of backtracking will lead you to the problem. I had to gradually unroll my past commits one by one to get closer to the cause.

Maybe the from_db_value function in models.py can check whether the library has been initialized already? (or possibly something more robust to apply for other functions as well).

@laymonage
Copy link
Owner

I thought database queries are lazy enough to be deferred until actually needed!

Model fields are created as Python class attributes. That means they get evaluated very early on by Python, not Django (correct me if I'm wrong here). When defining default in a model field, the value that you pass as the keyword argument will be evaluated and stored in the field's default attribute, e.g. SomeModel.system_currency.default. If you pass default=Currency.objects.first().code, Python has to evaluate Currency.objects.first() in order to get its code attribute to be saved as SomeModel.system_currency.default.

Calling Currency.objects.first() will tell Django to create a new database connection and fetch the first object eagerly (unlike .filter() which returns a lazy QuerySet). It's likely that this happens before the ready() method of this app is called.

Your initial guess,

It seems that maybe using queries inside model definitions somehow initializes the db config before this library can inject itself into it. Moving the library into the first place in INSTALLED_APPS doesn't help.

is correct. I would've suggested moving the app to the top of INSTALLED_APPS if you hadn't tried it, so I'm not really sure why it didn't work. My guess is, the class definitions are processed by Python before Django even initiates apps, hence this error.

Thankfully Django accepts a callable for default which will only be called when the value is needed, so SomeModel.system_currency.default only refers to the callable object and not the returned value.

Maybe the from_db_value function in models.py can check whether the library has been initialized already? (or possibly something more robust to apply for other functions as well).

I guess we could do that by changing the direct attribute access (connection.features.some_new_feature) into a function call, but that'd be way less elegant 😄. I love the current approach because that's exactly how it works in Django 3.1 (e.g. here).

If we only care about from_db_value, I could try to fix #16 which would remove feature checking inside it. However, I'm still skeptical to approach it because it breaks compatibility with JSONField from contrib.postgres (if someone uses it along with this package for some reason).

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

No branches or pull requests

2 participants