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

Ensure only one active InstanceConfig can exist #5974

Merged
merged 1 commit into from
Jul 21, 2021

Conversation

rmol
Copy link
Contributor

@rmol rmol commented Jun 4, 2021

Status

Ready for review

Description of Changes

Fixes #5973.

Adds a unique partial index on InstanceConfig.valid_until where it is null, and in InstanceConfig.get_current, handles the now-possible IntegrityError and queries again to pick up the row added between the previous query and the insertion attempt.

Testing

  • git checkout develop
  • git checkout origin/there_can_be_only_one -- securedrop/tests/test_db.py
  • make test TESTFILES="tests/test_db.py::test_only_one_active_instance_config_can_exist" will fail because it doesn't encounter IntegrityError.
  • git checkout there_can_be_only_one
  • Run the test again. It should pass.

Deployment

The migration that adds the index could conceivably fail if a site had multiple rows in instance_config with valid_until null, but every production database should have been created with migrations, which would have created a default row in instance_config. Any site that did somehow have multiple active configurations would not be functioning.

Checklist

If you made changes to the server application code:

  • Linting (make lint) and tests (make test) pass in the development container

If you made non-trivial code changes:

  • I have written a test plan and validated it for this PR

Choose one of the following:

  • I have opened a PR in the docs repo for these changes, or will do so later
  • I would appreciate help with the documentation
  • These changes do not require documentation

@rmol rmol requested a review from a team as a code owner June 4, 2021 20:07
@lgtm-com
Copy link

lgtm-com bot commented Jun 4, 2021

This pull request introduces 1 alert when merging 4bcc5de into 59da5b7 - view on LGTM.com

new alerts:

  • 1 for Testing equality to None

@rmol rmol force-pushed the there_can_be_only_one branch from 4bcc5de to ffc8bec Compare June 7, 2021 15:48
@lgtm-com
Copy link

lgtm-com bot commented Jun 7, 2021

This pull request introduces 1 alert when merging ffc8bec into 59da5b7 - view on LGTM.com

new alerts:

  • 1 for Testing equality to None

@codecov-commenter
Copy link

Codecov Report

Merging #5974 (ffc8bec) into develop (2971b91) will decrease coverage by 0.08%.
The diff coverage is 72.72%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #5974      +/-   ##
===========================================
- Coverage    85.32%   85.23%   -0.09%     
===========================================
  Files           53       54       +1     
  Lines         3883     3900      +17     
  Branches       481      481              
===========================================
+ Hits          3313     3324      +11     
- Misses         457      463       +6     
  Partials       113      113              
Impacted Files Coverage Δ
...81fb88c2_unique_index_for_instanceconfig_valid_.py 66.66% <66.66%> (ø)
securedrop/models.py 91.32% <80.00%> (-0.33%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2971b91...ffc8bec. Read the comment docs.

Copy link
Contributor

@zenmonkeykstop zenmonkeykstop left a comment

Choose a reason for hiding this comment

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

Tsst plan passes, and new instance configs can be created via the JI without any issues. Agreed on deployment comments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

In dev environment, multiple active InstanceConfig records can be created, breaking things
4 participants