Code

Opened 6 months ago

Closed 8 weeks ago

Last modified 8 weeks ago

#21798 closed Cleanup/optimization (fixed)

Model validation should complain if both `add_now` and `auto_add_now` are specified.

Reported by: charettes Owned by: anonymous
Component: Database layer (models, ORM) Version: master
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

It was reported in #21785 that add_now and auto_add_now can be both specified without triggering any kind of warning or validation or value error.

Not sure of the path we want to take here since it should be pretty uncommon and easily fixed by the users. Would an accelerated deprecation be acceptable?

In order to ease the merge of the check framework before the 1.7 alpha we should wait before landing a fix.

Attachments (0)

Change History (18)

comment:1 Changed 6 months ago by timo

  • Triage Stage changed from Unreviewed to Accepted

Accelerated deprecation sounds ok to me.

comment:2 Changed 6 months ago by russellm

Check framework is now merged; although if this is deprecation path, it's not a check framework thing - it's a DeprecationWarning where the feature is used. The check framework only applies if we're going to make the change and expect users to clean up the mess (as we did with the default change on BooleanField in 1.6).

comment:3 Changed 4 months ago by PirosB3

  • Owner changed from charettes to anonymous
  • Status changed from new to assigned

comment:4 Changed 4 months ago by anonymous

Hi,

I am working now to provide a fix for this.

Regards,
Daniel Pyrathon

comment:5 Changed 4 months ago by PirosB3

Hi again,

I think it would be good to warn the user that both parameters cannot be used, but also automatically make a decision for him/her (and notify it through the warning).
Does this sound like a reasonable solution?

The warning could be: "add_now and auto_add_now cannot be both specified, add_now has been disabled on this field"

Please let me know,
Daniel Pyrathon

comment:6 Changed 4 months ago by erikr

You are talking about auto_now and auto_now_add, right? There is no auto_add_now, as far as I know.

Provided that #21785 has resolved the migration issue, what is the actual need in deprecating this? Looking at the code, I see no reason why it should be invalid, or how it could cause issues. Yes, it would certainly be silly to add both auto_now and auto_now_addto a field, but there are tons of silly field definitions that we still allow. For example, we also allow the combination of auto_now_add and default.

A different situation would be if defining both would actually break in some cases, like the issue we resolved in #20484. But with #21785 fixed, I see no similar situation here. When defining both, as far as I know, it behaves correctly and precisely as one would expect. Therefore, why should we disallow this combination?

Last edited 4 months ago by erikr (previous) (diff)

comment:7 Changed 4 months ago by PirosB3

Hi erikr

I just read this now, after having done my PR (https://github.com/django/django/pull/2428)
I understand your point, but shouldn't we correct the user in any case? I think the fields should do some validation on parameters that are mutually exclusive.

Let me know if we would need this, in any case.

Regards,
Dan

comment:8 Changed 4 months ago by charettes

@erikr, I agree that we allow many silly definition, the combination of auto_now_add and default is a good example.

However I think we should at least warn the user if two of the add_now, auto_add_now and default options are defined since the defined behavior here is ambiguous.

Last edited 4 months ago by charettes (previous) (diff)

comment:9 Changed 4 months ago by charettes

By warn the user I mean adding a field check.

comment:10 Changed 4 months ago by PirosB3

Hi charettes,

so you would not take any action, other than warning the user? (you would still keep both options enabled?)

comment:11 Changed 4 months ago by erikr

Well, the decision of which option is enabled has already been made implicitly by the code. If one sets both auto_now_add and auto_now, the behaviour is identical to only providing auto_now. The warning could indicate that. I'm sure a similar deterministic precedence is present for default.

The warning could be something along the lines of (not polished): The combining of auto_now, auto_now_add and/or default paramaters of a ..field is deprecated. X takes precedence over Y, which takes precedence over Z. Note also that there are three fields that have these parameters: DateTimeField, DateField and TimeField, to my knowledge.

comment:12 Changed 4 months ago by erikr

Eh, pressed enter too soon there. Two more notes:

  • If we also take default into account with this warning, I definitely agree with charettes that we should fix this. Particularly as the behaviour in combination with default is a lot less obvious.
  • There are basically two options here: we could issue a DeprecationWarning, and add a field check in the next version, or just add the field check now. We went for the latter in #20484, but that was a little more severe: that actually broke in some conditions. If I understand comment:9 correctly, charette is in favour of adding the check now - I don't feel strongly either way.
Last edited 4 months ago by erikr (previous) (diff)

comment:13 Changed 4 months ago by pirosb3

I think it would be more sensible to add DeprecationWarning first, and then field check later. In makes it more smoother. But the final call is up to you two :)
I am willing to make the changes for the choice you make. Let me know.

Dan

comment:14 Changed 4 months ago by pirosb3

Hi

I have opted for a simple DeprecationWarning.
I also did a small amount of refactoring, to avoid duplication. Let me know if we want me to change it.

https://github.com/django/django/pull/2428

Probably we will want to squash merge (once ready)? I have done a few useless commits that shouldn't mess up the existing history.

comment:15 Changed 4 months ago by pirosb3

  • Has patch set

comment:16 Changed 3 months ago by timo

  • Easy pickings unset
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement set

I think we should just add the check and forget the deprecation warning. There are ways to ignore the check, so it wouldn't be backwards incompatible anyway.

comment:17 Changed 8 weeks ago by Tim Graham <timograham@…>

  • Resolution set to fixed
  • Status changed from assigned to closed

In cb15231888df2545356ad307eaea07f36aa0e8e0:

Fixed #21798 -- Added check for DateTime mutually exclusive options

Added DateTimeCheckMixin to avoid the use of default, auto_now, and
auto_now_add options together. Added the fields.E151 Error that is raised
if one or more of these options are used together.

comment:18 Changed 8 weeks ago by Russell Keith-Magee <russell@…>

In 2c176eb95cc53f29367e56e0a8eec336834a299d:

Refs #21798 - Modified error number to provide room for future expansion.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.