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

Bugfix/improve db sessions #204

Merged
merged 8 commits into from
Feb 18, 2021
Merged

Conversation

louise-davies
Copy link
Member

Description

These are the changes to improve the session handling that have been on preprod for months. Essentially, the session_manager object was flawed, and reading the session docs for SQLAlchemy (https://docs.sqlalchemy.org/en/13/orm/contextual.html), and it suggests using an "off-the-shelf" solution in Flask-SQLAlchemy if we're unfamiliar with session handling/threading. I tried it as a quick fix, and it seems a lot more stable, so it's easier to just import Flask-SQLAlchemy purely for its session handling than to try and fix the original solution.

Testing Instructions

  • Review code
  • Check GitHub Actions build
  • Review changes to test coverage
  • Verify that preprod (which this branch is deployed on) is not having regular 500 errors due to sessions.

@codecov
Copy link

codecov bot commented Feb 4, 2021

Codecov Report

Merging #204 (7faee5c) into master (460b680) will decrease coverage by 0.15%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #204      +/-   ##
==========================================
- Coverage   87.25%   87.10%   -0.16%     
==========================================
  Files          28       27       -1     
  Lines        2221     2225       +4     
  Branches      192      193       +1     
==========================================
  Hits         1938     1938              
- Misses        242      246       +4     
  Partials       41       41              
Impacted Files Coverage Δ
...way-api/datagateway_api/common/database/helpers.py 76.72% <0.00%> (-1.25%) ⬇️
...datagateway_api/common/database/session_manager.py
...gateway-api/datagateway_api/src/api_start_utils.py 86.36% <0.00%> (+1.94%) ⬆️

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 460b680...9e8bed2. Read the comment docs.

Copy link
Collaborator

@MRichards99 MRichards99 left a comment

Choose a reason for hiding this comment

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

I'm happy with these changes - I've tested them locally and on preprod and didn't experiences any issues on either.

@louise-davies louise-davies merged commit 856c31e into master Feb 18, 2021
@louise-davies louise-davies deleted the bugfix/improve-db-sessions branch February 18, 2021 09:14
MRichards99 added a commit that referenced this pull request Mar 15, 2021
- When running the API, it was difficult to distinguish between the AuthenticationError affected in this commit, and the one a couple of lines above it
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.

2 participants