Skip to content

Conversation

DanSheps
Copy link
Member

@DanSheps DanSheps commented Sep 1, 2025

Fixes: #19825 - Prevent cache for config revisions from being overwritten when in debug mode when not intended

  • Obtain the current config revision information
  • Reinitialize that information when app is restarted in debug mode

…tten when in debug mode when not intended
@DanSheps DanSheps requested a review from a team September 1, 2025 15:35
@DanSheps DanSheps self-assigned this Sep 1, 2025
@DanSheps DanSheps requested review from jeremystretch and removed request for a team September 1, 2025 15:35
@DanSheps
Copy link
Member Author

DanSheps commented Sep 1, 2025

Looks like this was changed in #18021 to automatically clear cache on startup.

We may want to revisit this completely as this could hit way more then just swagger.

Copy link
Member

@jeremystretch jeremystretch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is the optimal way to approach this. cache.clear() might be called elsewhere (including by plugins), so we need to ensure that the active configuration is always reset correctly in case the entire cache is cleared. This will probably require tweaking the initialization logic for the Config class.

@DanSheps
Copy link
Member Author

DanSheps commented Sep 4, 2025

I don't think this is the optimal way to approach this. cache.clear() might be called elsewhere (including by plugins), so we need to ensure that the active configuration is always reset correctly in case the entire cache is cleared. This will probably require tweaking the initialization logic for the Config class.

I was kind of wrestling with this as well. As the Config class is supposed to be immutable I didn't want to directly set an status parameter on the class.

@DanSheps
Copy link
Member Author

DanSheps commented Sep 9, 2025

Thoughts on adding a active = models.BooleanField(default=False) on the ConfigRevision model to properly track which ConfigRevision is active using the .update() or .bulk_update() method to bypass the signals and update the fields directly with an appropriate database constraint to ensure only one revision active at a time?

This would remove some of the non-concrete logic we have going on (based on the last config saved). Alternatively, have a date which we utilize instead.

@jnovinger
Copy link
Member

Hey @DanSheps ,

I've been thinking about this PR and Jeremy's feedback about needing to address the broader architectural issue rather than just the debug mode cache clearing. And really, I think the issue as reported is a bit off--I think the caching issue has just be papering over the flawed "assume the most recent config revision is active" approach.

I think you have identified the core of a clean solution that addresses Jeremy's concerns while being doable for v4.4.1. Thinking about it seems like we would need to do at least the following:

  1. Add the active field as you described, with something like UniqueConstraint(fields=['active'], condition=Q(active=True)), along with the required schema migration
  2. A data migration that does its best to intuit the current active config revision before fallback to the "most recently" created method of determing which is active
  3. Update _populate_from_db() to use .get(active=True) and update edge-case handling accordingly
  4. Update activate() method to manage the boolean field
  5. Update is_active property to use database field

Given the Tuesday deadline for including this in v4.4.1, I wanted to check: do you still want to implement this approach, or would you prefer I take ownership?

@DanSheps
Copy link
Member Author

I think I can get that done by Tuesday

@DanSheps
Copy link
Member Author

Turns out, I had most of this done... lol

@jnovinger jnovinger self-requested a review September 15, 2025 16:30
@jnovinger jnovinger removed the request for review from jeremystretch September 15, 2025 18:40
…igration logic and add comments

* Revise config initialization to be more explicit
* Remove old gating of cache.clear()
* Add comments to migration
* Clean up migration
@DanSheps DanSheps requested a review from jnovinger September 16, 2025 13:11
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

Successfully merging this pull request may close these issues.

In debug mode (only); executing any ./manage.py command resets the current configuration revision to #1.
3 participants