Platform/13506/int tests for fhir router #14241

david-navapbc · 2024-04-30T16:09:18Z

This PR ...

If you are suggesting a fix for a currently exploitable issue, please disclose the issue to the prime-reportstream team directly outside of GitHub instead of filing a PR, so we may immediately patch the affected systems before a disclosure. See SECURITY.md/Reporting a Vulnerability for more information.

Test Steps:

Include steps to test these changes

Changes

Include a comprehensive list of changes in this PR
(For web UI changes) Include screenshots/video of changes

Checklist

Testing

Tested locally?
Ran ./prime test or ./gradlew testSmoke against local Docker ReportStream container?
(For Changes to /frontend-react/...) Ran npm run lint:write?
Added tests?

Process

Are there licensing issues with any new dependencies introduced?
Includes a summary of what a code reviewer should test/verify?
Updated the release notes?
Database changes are submitted as a separate PR?
DevOps team has been notified if PR requires ops support?

Linked Issues

Fixes #issue

To Be Done

Create GitHub issues to track the work remaining, if any

#issue

Specific Security-related subjects a reviewer should pay specific attention to

Does this PR introduce new endpoints?
- new endpoint A
- new endpoint B
Does this PR include changes in authentication and/or authorization of existing endpoints?
Does this change introduce new dependencies that need vetting?
Does this change require changes to our infrastructure?
Does logging contain sensitive data?
Does this PR include or remove any sensitive information itself?

If you answered 'yes' to any of the questions above, conduct a detailed Review that addresses at least:

What are the potential security threats and mitigations? Please list the STRIDE threats and how they are mitigated
- Spoofing (faking authenticity)
  - Threat T, which could be achieved by A, is mitigated by M
- Tampering (influence or sabotage the integrity of information, data, or system)
- Repudiation (the ability to dispute the origin or originator of an action)
- Information disclosure (data made available to entities who should not have it)
- Denial of service (make a resource unavailable)
- Elevation of Privilege (reduce restrictions that apply or gain privileges one should not have)
Have you ensured logging does not contain sensitive data?
Have you received any additional approvals needed for this change?

github-actions · 2024-04-30T16:09:48Z

Dependency Review

✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.

OpenSSF Scorecard

Package	Version	Score	Details

Scanned Manifest Files

github-actions · 2024-04-30T16:12:35Z

Test Results

1 208 tests +10 1 204 ✅ +10 7m 15s ⏱️ +35s
153 suites + 1 4 💤 ± 0
153 files + 1 0 ❌ ± 0

Results for commit 36fda72. ± Comparison against base commit 58d528b.

♻️ This comment has been updated with latest results.

github-actions · 2024-04-30T16:12:47Z

Integration Test Results

62 files ±0 62 suites ±0 34m 9s ⏱️ +40s
395 tests ±0 385 ✅ ±0 10 💤 ±0 0 ❌ ±0
398 runs ±0 388 ✅ ±0 10 💤 ±0 0 ❌ ±0

Results for commit 36fda72. ± Comparison against base commit 58d528b.

♻️ This comment has been updated with latest results.

mkalish

Nice work! This is really good stuff. Couple of high level things I think need to get added before merging:

We should should be checking the action log details to make sure the filtering gets logged properly
Adding a test around the observation pruning
Adding a test where a report gets routed to multiple receivers
The processing mode filter

prime-router/src/main/kotlin/azure/DatabaseAccess.kt

prime-router/src/main/kotlin/fhirengine/utils/CompareFhirData.kt

mkalish · 2024-05-10T13:54:37Z

prime-router/src/test/kotlin/fhirengine/azure/FHIRRouterIntegrationTests.kt

+    }
+
+    private fun createReport(
+        fileFormat: Report.Format,


Intellij is saying that this will always be the FHIR I would maybe drop it or you could modify one of the tests so that the HL7 is what is in the receive step

does the route step ever receive an HL7 message? I thought everything got converted to FHIR by the preceding step?

Why was this resolved? Comment seems unaddressed?

that's on me - I thought the route step never received anything but FHIR hence I marked it as resolved. What would you like me to do here?

You are correct, the route step doesn't receive anything but FHIR. I think what Michael is saying is we should resolve the IntelliJ warning somehow. One option is to not pass fileFormat in at all. I think I'm actually okay with the way it is now because I see createReport getting abstracted into a common location where other integration tests, like translation, can use it?

mkalish · 2024-05-10T13:55:28Z

prime-router/src/test/kotlin/fhirengine/azure/FHIRRouterIntegrationTests.kt

+        nextAction: TaskAction,
+        nextEventAction: Event.EventAction,
+        topic: Topic,
+        taskIndex: Long = 0,


Same here on this default, could amend one of the receive steps to have more than one item. Also, this name seems strange?

happy to make whatever changes - but what name seems strange?

you are passing taskIndex to ReportLineage call as reportLineageId, I think that is confusing? Also yes, same issue is in FhirFunctionIntegrationTests. I think this was left over from my original approach.

ack. will change reportLineageId to something else

I think you can just remove taskIndex param from createReport and hard code a 0 for the ReportLineage ID?

mkalish · 2024-05-10T13:57:13Z

prime-router/src/test/kotlin/fhirengine/azure/FHIRRouterIntegrationTests.kt

+                reportFile, txn, action
+            )
+            if (childReport != null) {
+                ReportStreamTestDatabaseContainer.testDatabaseAccess


I'm not sure that it would impact your test, but this is not quite right since the item lineages are not getting created? It might not impact your test though in this case

what do you mean the item lineages are not being created?

If you look at our database tables, you'll see report_lineage and item_lineage. We are updating the report_lineage table but ignoring the item_lineage table. The FHIRRouter does not depend on the item_lineage table to perform its function, but it does update it so that means we should mock it properly so we can verify the item_lineage is correct after the step has completed.

I could see this being a different ticket so we can move forward with Routing refactor. @mkalish thoughts?

mkalish · 2024-05-10T13:58:33Z

prime-router/src/test/kotlin/fhirengine/azure/FHIRRouterIntegrationTests.kt

+        every { BlobAccess getProperty "defaultBlobMetadata" } returns getBlobContainerMetadata()
+        mockkObject(BlobAccess.BlobContainerMetadata)
+        every { BlobAccess.BlobContainerMetadata.build(any(), any()) } returns getBlobContainerMetadata()
+        mockkConstructor(DatabaseLookupTableAccess::class)


Maybe as a follow up, but I think in the spirit of these being integration gets I think it would great to not mock the lookup tables

This needs to be addressed or ticket created and added to comment. Only then resolve the conversation.

on me - I didn't take this as an action item so much as a passing thought. Do you want me to refactor the test or do you want me to write a ticket? Happy to do either. I'm ambivalent.

How about let's just add a comment for now? I don't think we need a ticket, this seems low prio tech debt to me.

prime-router/src/test/kotlin/fhirengine/azure/FHIRRouterIntegrationTests.kt

prime-router/src/main/kotlin/azure/DatabaseAccess.kt

prime-router/src/main/kotlin/fhirengine/utils/CompareFhirData.kt

…out its nonsense...

sonarcloud · 2024-05-29T19:29:14Z

Quality Gate failed

Failed conditions
42.1% Coverage on New Code (required ≥ 80%)

See analysis details on SonarCloud

arnejduranovic

Went through and unresolved some conversations that I wasn't sure were properly addressed.

arnejduranovic · 2024-05-30T15:09:38Z

prime-router/src/test/kotlin/fhirengine/azure/FHIRRouterIntegrationTests.kt

+    }
+
+    private fun createReport(
+        fileFormat: Report.Format,


Why was this resolved? Comment seems unaddressed?

arnejduranovic · 2024-05-30T15:12:16Z

prime-router/src/test/kotlin/fhirengine/azure/FHIRRouterIntegrationTests.kt

+        nextAction: TaskAction,
+        nextEventAction: Event.EventAction,
+        topic: Topic,
+        taskIndex: Long = 0,


you are passing taskIndex to ReportLineage call as reportLineageId, I think that is confusing? Also yes, same issue is in FhirFunctionIntegrationTests. I think this was left over from my original approach.

arnejduranovic · 2024-05-30T15:22:22Z

prime-router/src/test/kotlin/fhirengine/azure/FHIRRouterIntegrationTests.kt

+                reportFile, txn, action
+            )
+            if (childReport != null) {
+                ReportStreamTestDatabaseContainer.testDatabaseAccess


If you look at our database tables, you'll see report_lineage and item_lineage. We are updating the report_lineage table but ignoring the item_lineage table. The FHIRRouter does not depend on the item_lineage table to perform its function, but it does update it so that means we should mock it properly so we can verify the item_lineage is correct after the step has completed.

arnejduranovic · 2024-05-30T15:24:55Z

prime-router/src/test/kotlin/fhirengine/azure/FHIRRouterIntegrationTests.kt

+        every { BlobAccess getProperty "defaultBlobMetadata" } returns getBlobContainerMetadata()
+        mockkObject(BlobAccess.BlobContainerMetadata)
+        every { BlobAccess.BlobContainerMetadata.build(any(), any()) } returns getBlobContainerMetadata()
+        mockkConstructor(DatabaseLookupTableAccess::class)


This needs to be addressed or ticket created and added to comment. Only then resolve the conversation.

arnejduranovic · 2024-05-30T15:46:00Z

prime-router/src/test/kotlin/fhirengine/azure/FHIRRouterIntegrationTests.kt

+                reportFile, txn, action
+            )
+            if (childReport != null) {
+                ReportStreamTestDatabaseContainer.testDatabaseAccess


I could see this being a different ticket so we can move forward with Routing refactor. @mkalish thoughts?

arnejduranovic · 2024-05-30T15:47:15Z

prime-router/src/test/kotlin/fhirengine/azure/FHIRRouterIntegrationTests.kt

+            // for receiver Y all five observations should be intact
+            assertEquals(5, fhirBundleReceiverY.getObservations().size)
+            val expectedCodes = listOf("94558-5", "95418-0", "95417-2", "95421-4", "95419-8")
+            for (i in 0..fhirBundleReceiverX.getObservations().size - 1) {


Intellij doesn't like this syntax

what do you mean it doesn't like this syntax? can you be more specific? everything works AFAIK

prime-router/src/test/kotlin/fhirengine/azure/FHIRRouterIntegrationTests.kt

david-navapbc added the DRAFT label Apr 30, 2024

david-navapbc requested a review from a team as a code owner April 30, 2024 16:09

david-navapbc temporarily deployed to staging April 30, 2024 16:09 — with GitHub Actions Inactive

david-navapbc temporarily deployed to staging April 30, 2024 23:10 — with GitHub Actions Inactive

david-navapbc force-pushed the platform/13506/int_tests_for_fhir_router branch from 49a237d to 0a7a643 Compare May 9, 2024 02:46

david-navapbc temporarily deployed to staging May 9, 2024 02:46 — with GitHub Actions Inactive

david-navapbc temporarily deployed to staging May 9, 2024 17:23 — with GitHub Actions Inactive

david-navapbc added platform Platform Team and removed DRAFT labels May 9, 2024

david-navapbc changed the title ~~DRAFT -- Platform/13506/int tests for fhir router~~ Platform/13506/int tests for fhir router May 9, 2024

david-navapbc requested a review from arnejduranovic May 9, 2024 17:25

david-navapbc enabled auto-merge (squash) May 9, 2024 17:28

mkalish requested changes May 10, 2024

View reviewed changes

Andrey-Glazkv assigned mkalish May 10, 2024

david-navapbc temporarily deployed to staging May 11, 2024 03:18 — with GitHub Actions Inactive

david-navapbc temporarily deployed to staging May 14, 2024 03:56 — with GitHub Actions Inactive

david-navapbc force-pushed the platform/13506/int_tests_for_fhir_router branch from 04bc3b6 to 1d43fad Compare May 14, 2024 20:12

david-navapbc temporarily deployed to staging May 14, 2024 20:13 — with GitHub Actions Inactive

adegolier reviewed May 15, 2024

View reviewed changes

prime-router/src/main/kotlin/azure/DatabaseAccess.kt Show resolved Hide resolved

adegolier reviewed May 15, 2024

View reviewed changes

prime-router/src/main/kotlin/fhirengine/utils/CompareFhirData.kt Show resolved Hide resolved

david-navapbc temporarily deployed to staging May 16, 2024 01:19 — with GitHub Actions Inactive

david-navapbc added the DRAFT label May 16, 2024

david-navapbc temporarily deployed to staging May 16, 2024 22:45 — with GitHub Actions Inactive

david-navapbc temporarily deployed to staging May 16, 2024 22:52 — with GitHub Actions Inactive

david-navapbc temporarily deployed to staging May 17, 2024 22:22 — with GitHub Actions Inactive

david-navapbc force-pushed the platform/13506/int_tests_for_fhir_router branch from bc818c1 to 487a6a2 Compare May 20, 2024 18:57

david-navapbc temporarily deployed to staging May 20, 2024 18:57 — with GitHub Actions Inactive

David Holiday added 25 commits May 29, 2024 13:12

ty arnej

d279bf4

now failing for a different reason

0792e61

asDFASDFASDFDSFADSSADFSADFADFSADFSFADSADFSADFSAEFWADFS

3c3b5eb

weeee are the champions my friends..

45331d2

cleaned up a bit

1a60786

all cleaned up

acc10d2

stubs for filter tests

8c7ed59

adds structure to easily handle filter tests

92c71ac

filtering is wonky

ca44a54

should be gtg

d8d1f5a

about to rebase on master

b491f0d

updates files mangled by rebase

2558295

and stuff is broken again

c8566f9

updates as per review

2e4fa4d

ran things to ground but more work to do

e1bf8bc

refactors tests to expect that odd observation filter entry

7900bb3

reverses change to check for itemlineages

b91745d

last thing is to deal with the itemlineages stuff

9784687

hopefully finally for real this time done with this

58b74ba

non zero indexing

6443604

two down one to go

918b28a

observation filters acting weird

6c76ccd

asdf

1606d26

our linter and I are going to have to have a long and serious talk ab…

2335c04

…out its nonsense...

not sure why there is still a changed file

36fda72

david-navapbc force-pushed the platform/13506/int_tests_for_fhir_router branch from b8c6058 to 36fda72 Compare May 29, 2024 19:15

david-navapbc temporarily deployed to staging May 29, 2024 19:15 — with GitHub Actions Inactive

arnejduranovic reviewed May 30, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Platform/13506/int tests for fhir router #14241

Platform/13506/int tests for fhir router #14241

david-navapbc commented Apr 30, 2024

github-actions bot commented Apr 30, 2024 •

edited

github-actions bot commented Apr 30, 2024 •

edited

github-actions bot commented Apr 30, 2024 •

edited

mkalish left a comment

mkalish May 10, 2024

david-navapbc May 10, 2024

arnejduranovic May 30, 2024

david-navapbc May 30, 2024

arnejduranovic May 31, 2024

mkalish May 10, 2024

david-navapbc May 10, 2024

arnejduranovic May 30, 2024 •

edited

david-navapbc May 30, 2024

arnejduranovic May 31, 2024

mkalish May 10, 2024

david-navapbc May 10, 2024

arnejduranovic May 30, 2024

arnejduranovic May 30, 2024

mkalish May 10, 2024

arnejduranovic May 30, 2024

david-navapbc May 30, 2024

arnejduranovic May 31, 2024

sonarcloud bot commented May 29, 2024

arnejduranovic left a comment

arnejduranovic May 30, 2024

arnejduranovic May 30, 2024 •

edited

arnejduranovic May 30, 2024

arnejduranovic May 30, 2024

arnejduranovic May 30, 2024

arnejduranovic May 30, 2024

david-navapbc May 30, 2024

arnejduranovic May 31, 2024

Platform/13506/int tests for fhir router #14241

Are you sure you want to change the base?

Platform/13506/int tests for fhir router #14241

Conversation

david-navapbc commented Apr 30, 2024

Changes

Checklist

Testing

Process

Linked Issues

To Be Done

Specific Security-related subjects a reviewer should pay specific attention to

github-actions bot commented Apr 30, 2024 • edited

Dependency Review

OpenSSF Scorecard

Scanned Manifest Files

github-actions bot commented Apr 30, 2024 • edited

Test Results

github-actions bot commented Apr 30, 2024 • edited

Integration Test Results

mkalish left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arnejduranovic May 30, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarcloud bot commented May 29, 2024

Quality Gate failed

arnejduranovic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arnejduranovic May 30, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Apr 30, 2024 •

edited

github-actions bot commented Apr 30, 2024 •

edited

github-actions bot commented Apr 30, 2024 •

edited

arnejduranovic May 30, 2024 •

edited

arnejduranovic May 30, 2024 •

edited