Opened 6 years ago

Closed 4 years ago

#29324 closed Cleanup/optimization (fixed)

Change Settings to raise ImproperlyConfigured on SECRET_KEY; not initialization

Reported by: Jon Dufresne Owned by: Florian Apolloner
Component: Core (Other) Version: 2.1
Severity: Normal Keywords:
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Since ticket #17800, initializing settings without a SECRET_KEY raises a an ImproperlyConfigured during settings initialization.

Instead, I think the error should be raised when the setting is accessed as Settings.SECRET_KEY.

My use case, my project has a number of management commands that run in a non-production, minimally configured environment. These management commands do not require SECRET_KEY, however, the environment is forced to provide one.

As a workaround this environment has been generating a random secret key each run. If Django were to instead raise the error on SECRET_KEY access, this workaround would not be necessary.

Change History (17)

comment:1 by Jon Dufresne, 6 years ago

Has patch: set

comment:2 by Tim Graham, 6 years ago

Patch needs improvement: set
Triage Stage: UnreviewedAccepted

comment:3 by Jon Dufresne, 6 years ago

Patch needs improvement: unset

Updated PR to address feedback. Thanks.

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

Resolution: fixed
Status: newclosed

In b3cffde5:

Fixed #29324 -- Made Settings raise ImproperlyConfigured if SECRET_KEY is accessed and not set.

comment:5 by Marten Kenbeek, 6 years ago

Resolution: fixed
Severity: NormalRelease blocker
Status: closednew

This causes a regression when using settings.configure(). UserSettingsHolder does not handle the missing SECRET_KEY attribute, and raises an AttributeError instead of ImproperlyConfigured.

Version 0, edited 6 years ago by Marten Kenbeek (next)

comment:6 by Tim Graham, 6 years ago

Has patch: unset
Version: master2.1

comment:7 by Tim Graham, 6 years ago

Has patch: set

PR for the regression.

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

Resolution: fixed
Status: newclosed

In 5cc81cd9:

Reverted "Fixed #29324 -- Made Settings raise ImproperlyConfigured if SECRET_KEY is accessed and not set."

This reverts commit b3cffde5559c4fa97625512d7ec41a674be26076 due to
a regression and performance concerns.

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

In 483f5d6c:

[2.1.x] Reverted "Fixed #29324 -- Made Settings raise ImproperlyConfigured if SECRET_KEY is accessed and not set."

This reverts commit b3cffde5559c4fa97625512d7ec41a674be26076 due to
a regression and performance concerns.

Backport of 5cc81cd9eb69f5f7a711412c02039b435c393135 from master

comment:10 by Tim Graham, 6 years ago

Has patch: unset
Resolution: fixed
Severity: Release blockerNormal
Status: closednew

I reverted the original patch for now as Claude expressed some concerns, "I'm not sure to like the additional test for every setting access, for a corner-case use case. I think we should consider reverting the initial patch instead."

I'll leave the ticket open for Jon to provide an alternative patch or to close the ticket as wontfix.

comment:11 by Jon Dufresne, 5 years ago

Has patch: set

comment:12 by Tim Graham, 5 years ago

Patch needs improvement: set

comment:13 by Jon Dufresne, 5 years ago

Resolution: wontfix
Status: newclosed

After researching the code necessary to handle the edge cases, I don't think the complexity is worth it to include in Django. I'll stick with my local project workaround instead.

comment:14 by Florian Apolloner, 4 years ago

Resolution: wontfix
Status: closednew

comment:15 by Mariusz Felisiak, 4 years ago

Owner: changed from nobody to Florian Apolloner
Patch needs improvement: unset
Status: newassigned

comment:16 by Carlton Gibson, 4 years ago

Triage Stage: AcceptedReady for checkin

comment:17 by GitHub <noreply@…>, 4 years ago

Resolution: fixed
Status: assignedclosed

In 948a8744:

Fixed #29324 -- Made SECRET_KEY validation lazy (on first access).

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