Opened 6 years ago

Closed 6 years ago

#29505 closed Bug (fixed)

SchemaEditor prohibits setting a field's default value to a callable that'll be used as the default

Reported by: Eugene Pakhomov Owned by: nobody
Component: Migrations Version: 2.0
Severity: Normal Keywords: default
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

I'm trying to use https://github.com/cognitect/transit-python and I created a custom KeywordField for this.
The problem is that transit.transit_types.Keyword is a callable. Now, even if I override _get_default in KeywordField, it still won't work because django.db.backends.base.schema.BaseDatabaseSchemaEditor#effective_default checks the value again.

I think a simple fix would be to remove this check from effective_default and immediately make a call where it's necessary (datetime.date etc).

Attachments (1)

29505.diff (1015 bytes ) - added by Eugene Pakhomov 6 years ago.

Download all attachments as: .zip

Change History (9)

comment:1 by Tim Graham, 6 years ago

Component: Database layer (models, ORM)Migrations
Easy pickings: unset

I don't understand the issue. Please explain in more detail and perhaps offer a patch to explain.

comment:2 by Eugene Pakhomov, 6 years ago

Suppose I have some custom type like this one

class MyStr(str):
    def __call__(self, other):
        return self + other

and a corresponding custom field type that inherits from Django's TextField and implements all necessary methods so that MyStr is used everywhere instead of the regular str.
Currently, it's not possible without manually creating a DatabaseWrapper with a custom DatabaseSchemaEditor because even if you handle the fact that instances of MyStr are callable in the custom field type, they will still be called in django.db.backends.base.schema.BaseDatabaseSchemaEditor#effective_default.
Attached a patch that fixes this issue. Tests appear to be fine.

by Eugene Pakhomov, 6 years ago

Attachment: 29505.diff added

comment:3 by Tim Graham, 6 years ago

Resolution: wontfix
Status: newclosed
Summary: Impossible to set a Field's default value to a callableImpossible to set a Field's default value to a callable that takes an argument

I believe the proposed patch would break callable defaults. In effective_defautl(), field.get_default() may return a callable and Field.get_prep_value() won't work properly if it receives a callable instead of a value. I guess test coverage is lacking.

I think you should change your __call__() method to make other an optional argument.

Version 0, edited 6 years ago by Tim Graham (next)

comment:4 by Eugene Pakhomov, 6 years ago

As I said in the issue description - the types under question are not under my control, so I cannot change __call__(). And my concern doesn't have to do anything with my particular situation - I have already solved it by setting a custom DatabaseSchemaEditor. My concern is that this issue is still there, and other people will have to spend some time debugging it.

field.get_default() may return a callable

You're incorrect. Django itself doesn't do that. And Model#__init__() implementation calls get_default() without checking whether it's result is callable or not because the default implementation of get_default() always does this check. If get_default() could return a callable, Model#__init__() would not work correctly - it would set the corresponding field in all instances to the callable, without calling it to get the default value.

comment:5 by Eugene Pakhomov, 6 years ago

Also, the change in title is incorrect.
Technically, it's possible to set the default value to something that is a callable without arguments, yes. But it's impossible to also have this value as the default, instead of just have it be called for a default.
Consider my example above but without the other argument. You still won't be able to set an instance of MyStr as the default value.

comment:6 by Tim Graham, 6 years ago

Has patch: set
Needs tests: set
Resolution: wontfix
Status: closednew
Summary: Impossible to set a Field's default value to a callable that takes an argumentSchemaEditor prohibits setting a field's default value to a callable that'll be used as the default
Triage Stage: UnreviewedAccepted
Type: UncategorizedBug

After a second look, I agree with your analysis. The patch looks okay, and the test will probably go in tests/schema.

comment:7 by Tim Graham, 6 years ago

Needs tests: unset

comment:8 by Tim Graham <timograham@…>, 6 years ago

Resolution: fixed
Status: newclosed

In e62f6e09:

Fixed #29505 -- Removed SchemaEditor's calling of callable defaults.

Thanks Eugene Pakhomov for the suggested fix.

Note: See TracTickets for help on using tickets.
Back to Top