Add async processing for patient bulk uploads

[emmastephenson]

BACKEND PULL REQUEST

Related Issue

• Fixes Add batching and async for patient bulk uploader #4662
• Also fixes Remove role restrictions on patient bulk upload #4371

Changes Proposed

• Adds async processing for patient saving in the CSV bulk uploader
• Expected behavior is that validation feedback is sent immediately, and a response is sent to the frontend while patients are still being saved
• Should not trigger Akamai in deployed environments

Additional Information

There are two main changes required to get async working.

1. Change the security context to INHERITABLETHREADLOCAL.

This means any new threads spawned will inherit the security context of the parent thread. This is required because we need the security context in PatientBulkUploadServiceAsync to verify that the user has permissions to call the method.

• We were previously using MODE_THREADLOCAL, the default
• Spring documentation on INHERITABLETHREADLOCAL
• More useful random documentation on INHERITABLETHREADLOCAL

2. Add try/catch blocks around getCurrentOrganizationRoles and getCurrentApiUser for ScopeNotActiveException.

This is the source of the dreaded "trying to access request outside of requesting thread" error. In this case it was actually true - we were trying to access CurrentOrganizationRolesContextHolder and CurrentApiUserContextHolder after the initial request had closed. The stack trace on these goes something like the following:

1. PatientBulkUploadServiceAsync calls _personService.addPatientsAndPhoneNumbers
2. The @Authorization annotation on that function eventually resolves to userHasPermissions
3. That in turn calls the getCurrentOrganizationRoles and getCurrentApiUser methods
4. Without the try/catch, the backend blows up because there's no longer a request in scope

In my opinion, this is not a security risk because the fallback methods we use to verify permissions (getCurrentApiUserNoCache and fetchCurrentOrganizationRoles ) in turn rely on the LoggedInAuthorizationService to determine roles and permissions. That service relies on SecurityContextHolder to determine user permissions, which is populated thanks to our nifty MODE_INHERITABLETHREADLOCAL above. So, if the request is out of scope but SecurityContextHolder is empty, an exception will eventually be thrown - just later down the stack.

TL;DR the ContextHolder pattern is a bit of a cheat to store user permissions inside a request scope, but now that we're operating outside a request scope they don't work. We can get around that to still access user permissions though.

Testing

• Bulk uploading patients returns an immediate response for validation, and patients are eventually loaded into the database
• All other security / context pieces of the application still WAI

EDIT: Testing Setup

Backend tests for this were failing for a while on seemingly unrelated tests (SmsServiceTest and TestResultUploadTest). This only happened when the full test suite was run. After in-depth investigation, I can confidently say that this was unrelated to the async changes in this PR and simply brought on as a result of adding another SpringBootTest to our setup - the database ran out of connections with so many tests. (@rin-skylight and I did thoroughly debug the code and ensured that it was properly opening and closing database connections in the new threads).
The solution was to add an alteration to the reset-db script, to increase the number of available connections from 100 to 500. This fixed the issue when running the test suite both locally and in CI.

Checklist for Primary Reviewer

• Any large-scale changes have been deployed to test, dev, or pentest and smoke tested
• Changes comply with the SimpleReport Style Guide
• Changes with security implications have been approved by a security engineer (changes to authentication, encryption, handling of PII, etc.)

[georgehager]

lgtm! thanks for updating the tests for batching too. I had a comment about the final call to addPatientsAndPhoneNumbers not being in a try block but I'm not too worried about that because my email PR will wrap it anyway.

[georgehager]

Is it ok that the batched calls to addPatientsAndPhoneNumbers are within a try block, but this one is not?

[emmastephenson]

I think it is, because that method is not currently configured to throw any exceptions anyway - I think it would just silently fail.

[georgehager]

did you mean to leave this comment in?

[emmastephenson]

I certainly did not 😆 Thanks for catching that! Removed

stale review; made requested changes

Stale review

[georgehager]

@zdeveloper when you get a chance, can you re-review this now that the naive batching changes are in? I'm pushing this PR forward per Emma's instructions while she's out.

[rin-skylight]

🚢

[georgehager]

Sorry @rin-skylight! Promise this is the last review request, just addressed some feedback from Zedd.

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

91.0% Coverage
0.0% Duplication

[rin-skylight]

🛳️ Bringing in the fleet of ships... 😆

[zdeveloper]

LGTM! thank you for all your hard work on this! Its gonna save us so much time!
This PR however will break local testing without okta. As you know, testing a patients file is done locally for PHI reasons.

@emmastephenson @georgehager Please make a ticket to address the changes needed for DemoOktaRepository to get this working locally without okta since many people on the team run their local dev without okta.

[emyl3]

LGTM! Tested locally and on dev7. Thank you for all your work on this emma and george!!! 😹

@georgehager I also created the ticket for the request Zedd made above: https://app.zenhub.com/workspaces/simplereport-growth--engagement-605b43a6ceb92c000f86279a/issues/gh/cdcgov/prime-simplereport/4900

Feel free to use this or discard/edit as needed!!