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

fix: plumb context into client db methods #450

Merged
merged 1 commit into from Mar 4, 2024
Merged

Conversation

mistahj67
Copy link
Contributor

@mistahj67 mistahj67 commented Feb 29, 2024

Description

Plumb context into gorm client db methods
https://gorm.io/docs/context.html#Context-Timeout

Motivation and Context

Currently as it stands any endpoints that rely on purely gorm do not respect request.Context and as such timeouts are not respected either. While we want to eventually get to the removal of the gorm dependency altogether, that is a long way and a lot of effort away and this I think this approach would give us that much room to maneuver while we get there.

How Has This Been Tested?

Using a rest client and the prefer header
Add a sleep inside of the handler
Look for the 500 request timed out after exceeding the timeout. (Note: the request will take as long as the sleep is set, it's not tied to the prefer header)

Types of changes

  • Chore (a change that does not modify the application functionality)
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • Documentation updates are needed, and have been made accordingly.
  • I have added and/or updated tests to cover my changes.
  • All new and existing tests passed.
  • My changes include a database migration.

Copy link
Collaborator

@zinic zinic left a comment

Choose a reason for hiding this comment

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

Have a comment around InitContextFromToken that I'd like addressed before approval.

@mistahj67 mistahj67 requested a review from zinic March 4, 2024 16:12
Base automatically changed from add-ctx-to-gorm-poc to main March 4, 2024 16:30
Copy link
Collaborator

@zinic zinic left a comment

Choose a reason for hiding this comment

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

LGTM. Pull!

@mistahj67 mistahj67 merged commit 34961c9 into main Mar 4, 2024
3 checks passed
@mistahj67 mistahj67 deleted the add-ctx-to-clients-gorm branch March 4, 2024 18:04
@github-actions github-actions bot locked and limited conversation to collaborators Mar 4, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants