-
Notifications
You must be signed in to change notification settings - Fork 40
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
Check submission table when computing submission history #16200
Check submission table when computing submission history #16200
Conversation
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.OpenSSF Scorecard
Scanned Manifest Files |
Integration Test Results 53 files 53 suites 27m 25s ⏱️ Results for commit 80a4e91. ♻️ This comment has been updated with latest results. |
7e96b98
to
3a4225d
Compare
3a4225d
to
d1f6f5c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there is a test failure when I run this on my workstation that does not happen when I do the same in master branch.
at gov.cdc.prime.router.history.azure.DeliveryFunctionTests.test delivery details(DeliveryFunctionTests.kt:465)
|
||
return if (submission == null) { | ||
logger.error(ex) | ||
HttpUtilities.notFoundResponse(request, ex.message) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why return the exception message to the caller?
The project is open source so it's not really an OWASP issue in the way it would be were our project proprietary. As such feel free to leave this alone if you feel strongly.
my view is - in general - to not return anything to the caller directly tied to a facet of system state and/or implementation detail unless the caller needs to have that information for some specific reason.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with you however this was pre-existing code so I didn't feel comfortable changing it.
For the most part this is catching exceptions that we throw, so the error messages are things we set. Though IllegalStateException
is pretty broad and could probably occur unexpectedly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tests now pass for me locally - nice work!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes look good. In the background will work to put up a follow-up PR to use dependency injection with SubmissionTableService
Quality Gate passedIssues Measures |
This PR updates the submission history code to check the new submission table in azure storage when a submissionId is not found in the primary postgres database.
Test Steps:
Changes
Submission
classChecklist
Testing
./prime test
or./gradlew testSmoke
against local Docker ReportStream container?Linked Issues