Pagination Fix (#4613)

2024-09-16 16:12:23 -04:00 · 2024-09-16 16:12:23 -04:00 · d6793b97cd
--- a/8-2024/PaginationBug.http
+++ b/8-2024/PaginationBug.http
@ -0,0 +1,110 @@
+@hostname = localhost:44348
+
+### Get the bearer token, if authentication is enabled
+# @name bearer
+POST https://{{hostname}}/connect/token
+content-type: application/x-www-form-urlencoded
+
+grant_type=client_credentials
+&client_id=globalAdminServicePrincipal
+&client_secret=globalAdminServicePrincipal
+&scope=fhir-api
+
+###
+@token = {{bearer.response.body.access_token}}
+
+### Pagination bug 1 - Counting included resources and missing matched resources when using include, decending sort, and the right _count value 
+# Setup test data
+# @name batch
+POST https://{{hostname}}
+Content-Type: application/json
+Authorization: Bearer {{token}}
+
+< ./PaginationBugData.json
+
+### Reindex
+# Trigger a reindexing operation.
+# @name reindex
+POST https://{{hostname}}/$reindex HTTP/1.1
+Authorization: Bearer {{token}}
+content-type: application/json
+
+{  "resourceType": "Parameters", "parameter": [] }
+
+### Records the reindex job location
+@reindexLocation = {{reindex.response.headers.Content-Location}}
+
+###
+# Check the status of the reindexing operation
+GET {{reindexLocation}} HTTP/1.1
+Authorization: Bearer {{token}}
+
+###
+# Count <=2 gives one match and no next link, total = 1
+# Count =3 gives one match and a next link, total = 2
+# Count >=4 gives two matches and no next link, total = 2
+#
+# With just Condition:extension-care-goals a count of 2 produces the issue
+POST https://{{hostname}}/Condition/_search
+Content-Type: application/x-www-form-urlencoded
+Accept: application/json, text/plain, */*
+Authorization: Bearer {{token}}
+
+patient=859f091b-75a0-4690-8f45-ce192c5e045a
+&category=main
+&_count=2
+&_total=accurate
+&_sort=-onset-date
+&_include=Condition:extension-care-goals
+
+###
+# when the sort order is decreasing 
+GET https://{{hostname}}/MedicationDispense?_include=MedicationDispense:prescription&_sort=-whenprepared&_count=3&_total=accurate&_tag=a696ad4e-9e07-4266-8727-d1e3b9f193cd
+Authorization: Bearer {{token}}
+
+
+### Pagination bug 2 - Reverse chain counting deleted resources when it shouldn't
+# Setup data
+POST https://{{hostname}}
+Content-Type: application/json
+Authorization: Bearer {{token}}
+
+< ./PaginationBugData2.json
+
+### Delete Patient
+DELETE https://{{hostname}}/Patient/pagination-patient2
+Authorization: Bearer {{token}}
+
+### Gets the first patient but has the wrong total value
+# @name getservicerequest
+POST https://{{hostname}}/Patient/_search
+Content-Type: application/x-www-form-urlencoded
+Authorization: Bearer {{token}}
+
+_total=accurate
+&_count=1
+&_has:CareTeam:patient:participant=pagination-practitioner1
+
+### Gets the summary count (also wrong)
+GET https://{{hostname}}/Patient?_summary=count&_has:CareTeam:patient:participant=pagination-practitioner1
+Authorization: Bearer {{token}}
+
+### Tests if a non-existant referenced resource is counted (it isn't)
+PUT https://{{hostname}}/CareTeam/pagination-careteam4
+Content-Type: application/json
+Authorization: Bearer {{token}}
+
+{
+    "resourceType": "CareTeam",
+    "id": "pagination-careteam4",
+    "participant": [
+        {
+            "member": {
+                "reference": "Practitioner/pagination-practitioner1"
+            }
+        }
+    ],
+    "subject": {
+        "reference": "Patient/pagination-patient-invalid"
+    }
+}
--- a/8-2024/PaginationBugData.json
+++ b/8-2024/PaginationBugData.json
@ -0,0 +1,153 @@
+{
+	"resourceType": "Bundle",
+	"type": "batch",
+	"entry": [
+	{
+		"resource": {
+			"resourceType": "Condition",
+			"id": "c96b32a1-ac09-4c1a-8007-0baa001f16ac",
+			"extension": [
+			{
+				"extension": [
+				{
+					"url": "care_goal",
+					"valueReference": {
+						"reference": "Goal/fc7e482e-c69c-4027-9d06-a3ca1131c03f",
+						"type": "Goal",
+						"display": "John Smith"
+					}
+				},
+				{
+					"url": "care_goal",
+					"valueReference": {
+						"reference": "Goal/18f1d605-351e-4db9-ae2a-c2749b42ccbf",
+						"type": "Goal",
+						"display": "John Smith"
+					}
+				}],
+				"url": "https://domo.health/fhir/extension/care_goals"
+			}],
+			"category": [
+			{
+				"coding": [
+				{
+					"system": "http://www.domo.health/coding",
+					"code": "main",
+					"display": "John Smith"
+				}],
+				"text": "Diagnosis that can have Subsidiary diagnosis"
+			}],
+			"subject": {
+				"reference": "Patient/859f091b-75a0-4690-8f45-ce192c5e045a",
+				"type": "Patient"
+			},
+			"onsetDateTime": "2024",
+			"recordedDate": "2024-08-07T10:50:35.441Z"
+		},
+		"request": { 
+			"method": "PUT",
+			"url": "Condition/c96b32a1-ac09-4c1a-8007-0baa001f16ac"
+		}
+	},
+	{
+		"resource": {
+			"resourceType": "Goal",
+			"id": "fc7e482e-c69c-4027-9d06-a3ca1131c03f",
+			"lifecycleStatus": "active",
+			"subject": {
+				"reference": "Patient/859f091b-75a0-4690-8f45-ce192c5e045a",
+				"type": "Patient"
+			},
+			"description": [
+			{
+				"text": "something"	
+			}],
+			"startDate": "2024-06-24",
+			"statusDate": "2024-06-24"
+		},
+		"request": { 
+			"method": "PUT",
+			"url": "Goal/fc7e482e-c69c-4027-9d06-a3ca1131c03f"
+		}
+	},
+	{
+		"resource": {
+			"resourceType": "Goal",
+			"id": "18f1d605-351e-4db9-ae2a-c2749b42ccbf",
+			"lifecycleStatus": "active",
+			"subject": {
+				"reference": "Patient/859f091b-75a0-4690-8f45-ce192c5e045a",
+				"type": "Patient"
+			},
+			"description": [
+			{
+				"text": "something"	
+			}],
+			"startDate": "2024-07-10",
+			"statusDate": "2024-07-10"
+		},
+		"request": { 
+			"method": "PUT",
+			"url": "Goal/18f1d605-351e-4db9-ae2a-c2749b42ccbf"
+		}
+	},
+	{
+		"resource": {
+			"resourceType": "Condition",
+			"id": "7b06a53c-1eb4-4352-a810-d9b4651d1b1f",
+			"category": [
+			{
+				"coding": [
+				{
+					"system": "http://www.domo.health/coding",
+					"code": "main",
+					"display": "John Smith"
+				}],
+				"text": "Diagnosis that can have Subsidiary diagnosis"
+			}],
+			"subject": {
+				"reference": "Patient/859f091b-75a0-4690-8f45-ce192c5e045a",
+				"type": "Patient"
+			},
+			"recordedDate": "2024-06-12T11:40:41.846Z"
+		},
+		"request": { 
+			"method": "PUT",
+			"url": "Condition/7b06a53c-1eb4-4352-a810-d9b4651d1b1f"
+		}
+	},
+	{
+		"resource": {
+			"resourceType": "SearchParameter",
+			"url": "https://domo.health/fhir/search-params/extension-care-goals",
+			"name": "extensionCareGoals",
+			"publisher": "DomoSafety SA",
+			"status": "active",
+			"contact": [
+			{
+				"telecom": [
+				{
+					"system": "url",
+					"value": "https://www.domo-safety.com/"
+			}]
+			}],
+			"description": "Returns care goals with a goal id matching the specified string.",
+			"code": "extension-care-goals",
+			"base": [
+				"Composition",
+				"Condition",
+				"MedicationRequest",
+				"ServiceRequest"
+			],
+			"type": "reference",
+			"expression": "extension.where(url = 'https://domo.health/fhir/extension/care_goals').extension.where(url = 'care_goal').value",
+			"multipleOr": true,
+			"multipleAnd": true
+		},
+		"request": { 
+			"method": "POST",
+			"url": "SearchParameter"
+		}
+	}
+	]
+}
--- a/8-2024/PaginationBugData2.json
+++ b/8-2024/PaginationBugData2.json
@ -0,0 +1,106 @@
+{
+	"resourceType": "Bundle",
+	"type": "batch",
+	"entry": [
+    {
+        "resource": {
+            "resourceType": "CareTeam",
+            "id": "pagination-careteam1",
+            "participant": [
+                {
+                    "member": {
+                        "reference": "Practitioner/pagination-practitioner1"
+                    }
+                }
+            ],
+            "subject": {
+                "reference": "Patient/pagination-patient1"
+            }
+        },
+        "request": {
+            "method": "PUT",
+            "url": "CareTeam/pagination-careteam1"
+        }
+    },
+    {
+        "resource": {
+            "resourceType": "CareTeam",
+            "id": "pagination-careteam2",
+            "participant": [
+                {
+                    "member": {
+                        "reference": "Practitioner/pagination-practitioner1"
+                    }
+                }
+            ],
+            "subject": {
+                "reference": "Patient/pagination-patient2"
+            }
+        },
+        "request": {
+            "method": "PUT",
+            "url": "CareTeam/pagination-careteam2"
+        }
+    },
+    {
+        "resource": {
+            "resourceType": "CareTeam",
+            "id": "pagination-careteam3",
+            "participant": [
+                {
+                    "member": {
+                        "reference": "Practitioner/pagination-practitioner1"
+                    }
+                }
+            ],
+            "subject": {
+                "reference": "Patient/pagination-patient3"
+            }
+        },
+        "request": {
+            "method": "PUT",
+            "url": "CareTeam/pagination-careteam3"
+        }
+    },
+    {
+        "resource": {
+            "resourceType": "Patient",
+            "id": "pagination-patient1"
+        },
+        "request": {
+            "method": "PUT",
+            "url": "Patient/pagination-patient1"
+        }
+    },
+    {
+        "resource": {
+            "resourceType": "Patient",
+            "id": "pagination-patient2"
+        },
+        "request": {
+            "method": "PUT",
+            "url": "Patient/pagination-patient2"
+        }
+    },
+    {
+        "resource": {
+            "resourceType": "Patient",
+            "id": "pagination-patient3"
+        },
+        "request": {
+            "method": "PUT",
+            "url": "Patient/pagination-patient3"
+        }
+    },
+    {
+        "resource": {
+            "resourceType": "Practitioner",
+            "id": "pagination-practitioner1"
+        },
+        "request": {
+            "method": "PUT",
+            "url": "Practitioner/pagination-practitioner1"
+        }
+    }
+    ]
+}
--- a/src/Microsoft.Health.Fhir.SqlServer/Features/Search/Expressions/Visitors/QueryGenerators/SqlQueryGenerator.cs
+++ b/src/Microsoft.Health.Fhir.SqlServer/Features/Search/Expressions/Visitors/QueryGenerators/SqlQueryGenerator.cs
@ -735,6 +735,7 @@ namespace Microsoft.Health.Fhir.SqlServer.Features.Search.Expressions.Visitors.Q

                // We should remove IsHistory from ReferenceSearchParam (Source) only but keep on Resource (Target)
                AppendHistoryClause(delimited, context.ResourceVersionTypes, null, referenceTargetResourceTableAlias);
+                AppendDeletedClause(delimited, context.ResourceVersionTypes, referenceTargetResourceTableAlias);

                delimited.BeginDelimitedElement().Append(VLatest.ReferenceSearchParam.ResourceTypeId, referenceSourceTableAlias)
                    .Append(" IN (")
--- a/src/Microsoft.Health.Fhir.SqlServer/Features/Search/SqlServerSearchService.cs
+++ b/src/Microsoft.Health.Fhir.SqlServer/Features/Search/SqlServerSearchService.cs
@ -146,7 +146,7 @@ namespace Microsoft.Health.Fhir.SqlServer.Features.Search
            SqlSearchOptions sqlSearchOptions = new SqlSearchOptions(searchOptions);

            SearchResult searchResult = await SearchImpl(sqlSearchOptions, cancellationToken);
-            int resultCount = searchResult.Results.Count();
+            int resultCount = searchResult.Results.Count(r => r.SearchEntryMode == SearchEntryMode.Match);

            if (!sqlSearchOptions.IsSortWithFilter &&
                searchResult.ContinuationToken == null &&
@ -157,6 +157,10 @@ namespace Microsoft.Health.Fhir.SqlServer.Features.Search
            {
                // We seem to have run a sort which has returned less results than what max we can return.
                // Let's determine whether we need to execute another query or not.
+                //
+                // When order is descending, what would make DidWeSearchForSortValue false?
+                // Right now if no results are found that have the sort value and the order is descending this won't run.
+                // What is the point of DidweSearchForSortValue at all???
                if ((sqlSearchOptions.Sort[0].sortOrder == SortOrder.Ascending && sqlSearchOptions.DidWeSearchForSortValue.HasValue && !sqlSearchOptions.DidWeSearchForSortValue.Value) ||
                    (sqlSearchOptions.Sort[0].sortOrder == SortOrder.Descending && sqlSearchOptions.DidWeSearchForSortValue.HasValue && sqlSearchOptions.DidWeSearchForSortValue.Value && !sqlSearchOptions.SortHasMissingModifier))
                {
--- a/test/Microsoft.Health.Fhir.Shared.Tests.E2E/Rest/Search/ChainingSearchTests.cs
+++ b/test/Microsoft.Health.Fhir.Shared.Tests.E2E/Rest/Search/ChainingSearchTests.cs
@ -5,6 +5,7 @@

 using System;
 using System.Collections.Generic;
+using System.Diagnostics.Metrics;
 using System.Linq;
 using System.Threading.Tasks;
 using Hl7.Fhir.Model;
@ -348,6 +349,15 @@ namespace Microsoft.Health.Fhir.Tests.E2E.Rest.Search
        }
 #endif

+        [Fact]
+        public async Task GivenCountOnlyReverseChainSearchWithDeletedResource_WhenSearched_ThenCorrectCountIsReturned()
+        {
+            string query = $"_has:CareTeam:patient:_tag={Fixture.Tag}&_summary=count";
+
+            Bundle bundle = await Client.SearchAsync(ResourceType.Patient, query);
+            Assert.Equal(1, bundle.Total);
+        }
+
        public class ClassFixture : HttpIntegrationTestFixture
        {
            public ClassFixture(DataStore dataStore, Format format, TestFhirServerFactory testFhirServerFactory)
@ -431,6 +441,12 @@ namespace Microsoft.Health.Fhir.Tests.E2E.Rest.Search
                SmithLoincDiagnosticReport = await CreateDiagnosticReport(SmithPatient, smithLoincObservation, loincCode);
                TrumanLoincDiagnosticReport = await CreateDiagnosticReport(TrumanPatient, trumanLoincObservation, loincCode);

+                var deletedPatient = (await TestFhirClient.CreateAsync(new Patient { Meta = meta, Gender = AdministrativeGender.Male, Name = new List<HumanName> { new HumanName { Given = new[] { "Delete" }, Family = "Delete" } } })).Resource;
+                await TestFhirClient.CreateAsync(new CareTeam() { Meta = meta, Subject = new ResourceReference($"Patient/{AdamsPatient.Id}") });
+                await TestFhirClient.CreateAsync(new CareTeam() { Meta = meta, Subject = new ResourceReference($"Patient/{deletedPatient.Id}") });
+
+                await TestFhirClient.DeleteAsync(deletedPatient);
+
                var group = new Group
                {
                    Meta = new Meta { Tag = new List<Coding> { new Coding("testTag", Tag) } },
--- a/test/Microsoft.Health.Fhir.Shared.Tests.E2E/Rest/Search/IncludeSearchTestFixture.cs
+++ b/test/Microsoft.Health.Fhir.Shared.Tests.E2E/Rest/Search/IncludeSearchTestFixture.cs
@ -172,9 +172,9 @@ namespace Microsoft.Health.Fhir.Shared.Tests.E2E.Rest.Search
            AdamsMedicationRequest = await CreateMedicationRequest(AdamsPatient, AndersonPractitioner, PercocetMedication);
            SmithMedicationRequest = await CreateMedicationRequest(SmithPatient, SanchezPractitioner, PercocetMedication);

-            AdamsMedicationDispense = await CreateMedicationDispense(AdamsMedicationRequest, AdamsPatient, TramadolMedication);
-            SmithMedicationDispense = await CreateMedicationDispense(SmithMedicationRequest, SmithPatient, TramadolMedication);
-            TrumanMedicationDispenseWithoutRequest = await CreateMedicationDispense(null, TrumanPatient, TramadolMedication);
+            AdamsMedicationDispense = await CreateMedicationDispense(AdamsMedicationRequest, AdamsPatient, TramadolMedication, "2000-01-01");
+            SmithMedicationDispense = await CreateMedicationDispense(SmithMedicationRequest, SmithPatient, TramadolMedication, "1990-01-01");
+            TrumanMedicationDispenseWithoutRequest = await CreateMedicationDispense(null, TrumanPatient, TramadolMedication, null);

            CareTeam = await CreateCareTeam();

@ -250,7 +250,7 @@ namespace Microsoft.Health.Fhir.Shared.Tests.E2E.Rest.Search
                    })).Resource;
            }

-            async Task<MedicationDispense> CreateMedicationDispense(MedicationRequest medicationRequest, Patient patient, Medication medication)
+            async Task<MedicationDispense> CreateMedicationDispense(MedicationRequest medicationRequest, Patient patient, Medication medication, string prepared)
            {
                return (await TestFhirClient.CreateAsync(
                    new MedicationDispense
@ -270,6 +270,7 @@ namespace Microsoft.Health.Fhir.Shared.Tests.E2E.Rest.Search
                                Actor = new ResourceReference($"Practitioner/{Practitioner.Id}"),
                            },
                        },
+                        WhenPrepared = prepared,
 #if R5
                        Medication = new CodeableReference
                        {
--- a/test/Microsoft.Health.Fhir.Shared.Tests.E2E/Rest/Search/IncludeSearchTests.cs
+++ b/test/Microsoft.Health.Fhir.Shared.Tests.E2E/Rest/Search/IncludeSearchTests.cs
@ -1261,6 +1261,25 @@ namespace Microsoft.Health.Fhir.Tests.E2E.Rest.Search
            ValidateOperationOutcome(expectedDiagnostics, expectedIssueSeverities, expectedCodeTypes, fhirException.OperationOutcome);
        }

+        [Fact]
+        [HttpIntegrationFixtureArgumentSets(DataStore.SqlServer)] // Cosmos doesn't support the sort parameter
+        public async Task GivenAnIncludeSearchWithSortAndResourcesWithAndWithoutTheIncludeParameter_WhenSearched_ThenCorrectResultsAreReturned()
+        {
+            string query = $"_include=MedicationDispense:prescription&_sort=-whenprepared&_count=3&_tag={Fixture.Tag}";
+
+            Bundle bundle = await Client.SearchAsync(ResourceType.MedicationDispense, query);
+
+            Assert.Equal("self", bundle.Link[0].Relation);
+
+            ValidateBundle(
+                bundle,
+                Fixture.AdamsMedicationDispense,
+                Fixture.SmithMedicationDispense,
+                Fixture.TrumanMedicationDispenseWithoutRequest,
+                Fixture.AdamsMedicationRequest,
+                Fixture.SmithMedicationRequest);
+        }
+
        // This will not work for circular reference
        private static void ValidateSearchEntryMode(Bundle bundle, ResourceType matchResourceType)
        {
--- a/test/Microsoft.Health.Fhir.Shared.Tests.E2E/Rest/Search/SortTests.cs
+++ b/test/Microsoft.Health.Fhir.Shared.Tests.E2E/Rest/Search/SortTests.cs
@ -10,6 +10,7 @@ using System.Linq;
 using System.Net;
 using System.Threading.Tasks;
 using System.Web;
+using DotLiquid;
 using Hl7.Fhir.Model;
 using Microsoft.Health.Fhir.Client;
 using Microsoft.Health.Fhir.Core.Extensions;
@ -1116,6 +1117,23 @@ namespace Microsoft.Health.Fhir.Tests.E2E.Rest.Search
            SortTestsAssert.AssertOrganizationNamesAreEqualInRange(expectedOrganizations.Length, expectedOrganizations, returnedResults);
        }

+        /*
+         * Needs investigation, this breaks GivenPatients_WhenSearchedWithSortParamAndMissingIdentifier_SearchResultsReturnedShouldHonorMissingIdentifier
+        [Theory]
+        [InlineData("address-postalcode")]
+        [InlineData("-address-postalcode")]
+        [HttpIntegrationFixtureArgumentSets(DataStore.SqlServer)]
+        public async Task GivenNoResourcesWithSortValue_WhenSearchedWithSortParameter_ThenResourcesAreReturned(string sort)
+        {
+            var tag = Guid.NewGuid().ToString();
+            var patients = await CreatePatients(tag);
+
+            var returnedResults = await GetResultsFromAllPagesAsync($"Patient?_tag={tag}&_sort={sort}");
+
+            SortTestsAssert.AssertNumberOfResources(patients, returnedResults);
+        }
+        */
+
        private async Task<Patient[]> CreatePatients(string tag)
        {
            // Create various resources.