From 1b548e61c9c58f8bdc3f8e770c3918f66c8e8c58 Mon Sep 17 00:00:00 2001 From: rajithaalurims <110048715+rajithaalurims@users.noreply.github.com> Date: Fri, 5 Jan 2024 10:02:46 -0600 Subject: [PATCH] Using missing modifier for Sort (#3655) * Added logic to use missing modifier for Sort * using missing modifier for sort * modifier fixed * new methods added to create test data with missing birthdates * PR review addressed * PR review fixes * combined tests to run on same data --- .../Expressions/Visitors/SortRewriter.cs | 11 ++++- .../Features/Search/SqlSearchOptions.cs | 5 +++ .../Features/Search/SqlServerSearchService.cs | 7 ++- .../Rest/Search/SortTests.cs | 45 ++++++++++++++++++- 4 files changed, 65 insertions(+), 3 deletions(-) diff --git a/src/Microsoft.Health.Fhir.SqlServer/Features/Search/Expressions/Visitors/SortRewriter.cs b/src/Microsoft.Health.Fhir.SqlServer/Features/Search/Expressions/Visitors/SortRewriter.cs index 1857d41210..658f49c84e 100644 --- a/src/Microsoft.Health.Fhir.SqlServer/Features/Search/Expressions/Visitors/SortRewriter.cs +++ b/src/Microsoft.Health.Fhir.SqlServer/Features/Search/Expressions/Visitors/SortRewriter.cs @@ -47,9 +47,17 @@ public override Expression VisitSqlRoot(SqlRootExpression expression, SqlSearchO // If the parameter being sorted on is part of a filter then we don't need to run the seperate search for resources that are missing a value for the field being sorted on. // If the parameter being sorted on is not part of a filter we need to run a seperate search to get resources that don't have a value for the field being sorted on. bool matchFound = false; + bool sortHasMissingModifier = false; for (int i = 0; i < expression.SearchParamTableExpressions.Count; i++) { Expression updatedExpression = expression.SearchParamTableExpressions[i].Predicate.AcceptVisitor(this, context); + + if (expression.SearchParamTableExpressions[i].Predicate is MissingSearchParameterExpression && !sortHasMissingModifier) + { + MissingSearchParameterExpression misingSPExpression = (MissingSearchParameterExpression)expression.SearchParamTableExpressions[i].Predicate; + sortHasMissingModifier = !misingSPExpression.IsMissing && context.Sort[0].searchParameterInfo.Name.Equals(misingSPExpression.Parameter.Name, System.StringComparison.OrdinalIgnoreCase); + } + if (updatedExpression == null) { matchFound = true; @@ -57,11 +65,12 @@ public override Expression VisitSqlRoot(SqlRootExpression expression, SqlSearchO } } + context.SortHasMissingModifier |= sortHasMissingModifier; var newTableExpressions = new List(); newTableExpressions.AddRange(expression.SearchParamTableExpressions); var continuationToken = ContinuationToken.FromString(context.ContinuationToken); - if (!matchFound) + if (!matchFound && !sortHasMissingModifier) { // We are running a sort query where the parameter by which we are sorting // is not present as part of other search parameters in the query. diff --git a/src/Microsoft.Health.Fhir.SqlServer/Features/Search/SqlSearchOptions.cs b/src/Microsoft.Health.Fhir.SqlServer/Features/Search/SqlSearchOptions.cs index aa4c43bdb0..dd58032b3f 100644 --- a/src/Microsoft.Health.Fhir.SqlServer/Features/Search/SqlSearchOptions.cs +++ b/src/Microsoft.Health.Fhir.SqlServer/Features/Search/SqlSearchOptions.cs @@ -30,6 +30,11 @@ public SqlSearchOptions(SearchOptions searchOptions) /// public bool? DidWeSearchForSortValue { get; internal set; } + /// + /// Keeps track of whether missing modifier is specified for search parameter used in sort. + /// + public bool SortHasMissingModifier { get; internal set; } + /// /// Performs a shallow clone of this instance /// diff --git a/src/Microsoft.Health.Fhir.SqlServer/Features/Search/SqlServerSearchService.cs b/src/Microsoft.Health.Fhir.SqlServer/Features/Search/SqlServerSearchService.cs index c06c706a95..1d164fc31a 100644 --- a/src/Microsoft.Health.Fhir.SqlServer/Features/Search/SqlServerSearchService.cs +++ b/src/Microsoft.Health.Fhir.SqlServer/Features/Search/SqlServerSearchService.cs @@ -141,7 +141,7 @@ public override async Task SearchAsync(SearchOptions searchOptions // 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. 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.Sort[0].sortOrder == SortOrder.Descending && sqlSearchOptions.DidWeSearchForSortValue.HasValue && sqlSearchOptions.DidWeSearchForSortValue.Value && !sqlSearchOptions.SortHasMissingModifier)) { if (sqlSearchOptions.MaxItemCount - resultCount == 0) { @@ -539,6 +539,11 @@ await _sqlRetryService.ExecuteSql( sqlSearchOptions.IsSortWithFilter = true; } + if (clonedSearchOptions.SortHasMissingModifier) + { + sqlSearchOptions.SortHasMissingModifier = true; + } + searchResult = new SearchResult(resources, continuationToken?.ToJson(), originalSort, clonedSearchOptions.UnsupportedSearchParams); } } diff --git a/test/Microsoft.Health.Fhir.Shared.Tests.E2E/Rest/Search/SortTests.cs b/test/Microsoft.Health.Fhir.Shared.Tests.E2E/Rest/Search/SortTests.cs index 537105ebd7..a9075fe24c 100644 --- a/test/Microsoft.Health.Fhir.Shared.Tests.E2E/Rest/Search/SortTests.cs +++ b/test/Microsoft.Health.Fhir.Shared.Tests.E2E/Rest/Search/SortTests.cs @@ -87,6 +87,24 @@ public async Task GivenMoreThanTenPatients_WhenSearchedWithSortParam_ThenPatient } } + [Theory] + [InlineData("birthdate")] + [InlineData("-birthdate")] + [HttpIntegrationFixtureArgumentSets(dataStores: DataStore.SqlServer)] + public async Task GivenPatients_WhenSearchedWithSortParamAndMissingIdentifier_SearchResultsReturnedShouldHonorMissingIdentifier(string sortParameterName) + { + var tag = Guid.NewGuid().ToString(); + var patients = await CreatePaginatedPatientsWithMissingBirthDates(tag); + + // Search without missing modifier should return all Patients + var returnedResults = await GetResultsFromAllPagesAsync($"Patient?_tag={tag}&_sort={sortParameterName}"); + Assert.Equal(12, returnedResults.Count()); + + // Search with missing modifier false should return only Patients with Birth dates + returnedResults = await GetResultsFromAllPagesAsync($"Patient?_tag={tag}&birthdate:missing=false&_sort={sortParameterName}"); + Assert.Equal(10, returnedResults.Count()); + } + [Theory] [InlineData("birthdate")] [InlineData("_lastUpdated")] @@ -1117,7 +1135,27 @@ private async Task CreatePaginatedPatients(string tag) p => SetPatientInfo(p, "Seattle", "Robinson", tag, new DateTime(1940, 01, 15)), p => SetPatientInfo(p, "Portland", "Williamas", tag, new DateTime(1942, 01, 15)), p => SetPatientInfo(p, "Portland", "James", tag, new DateTime(1943, 10, 23)), - p => SetPatientInfo(p, "Seatt;e", "Alex", tag, new DateTime(1943, 11, 23)), + p => SetPatientInfo(p, "Seattle", "Alex", tag, new DateTime(1943, 11, 23)), + p => SetPatientInfo(p, "Portland", "Rock", tag, new DateTime(1944, 06, 24)), + p => SetPatientInfo(p, "Seattle", "Mike", tag, new DateTime(1946, 02, 24)), + p => SetPatientInfo(p, "Portland", "Christie", tag, new DateTime(1947, 02, 24)), + p => SetPatientInfo(p, "Portland", "Lone", tag, new DateTime(1950, 05, 12)), + p => SetPatientInfo(p, "Seattle", "Sophie", tag, new DateTime(1953, 05, 12)), + p => SetPatientInfo(p, "Portland", "Peter", tag, new DateTime(1956, 06, 12)), + p => SetPatientInfo(p, "Portland", "Cathy", tag, new DateTime(1960, 09, 22)), + p => SetPatientInfo(p, "Seattle", "Jones", tag, new DateTime(1970, 05, 13))); + + return patients; + } + + private async Task CreatePaginatedPatientsWithMissingBirthDates(string tag) + { + // Create various resources. + Patient[] patients = await Client.CreateResourcesAsync( + p => SetPatientInfoWithMissingBirthDate(p, "Seattle", "Robinson", tag), + p => SetPatientInfoWithMissingBirthDate(p, "Portland", "Williamas", tag), + p => SetPatientInfo(p, "Portland", "James", tag, new DateTime(1943, 10, 23)), + p => SetPatientInfo(p, "Seattle", "Alex", tag, new DateTime(1943, 11, 23)), p => SetPatientInfo(p, "Portland", "Rock", tag, new DateTime(1944, 06, 24)), p => SetPatientInfo(p, "Seattle", "Mike", tag, new DateTime(1946, 02, 24)), p => SetPatientInfo(p, "Portland", "Christie", tag, new DateTime(1947, 02, 24)), @@ -1177,6 +1215,11 @@ private void SetPatientInfo(Patient patient, string city, string family, string SetPatientInfoInternal(patient, city, family, tag, "1970-01-01"); } + private void SetPatientInfoWithMissingBirthDate(Patient patient, string city, string family, string tag) + { + SetPatientInfoInternal(patient, city, family, tag, null); + } + private void SetPatientInfo(Patient patient, string city, List familyNames, string tag) { SetPatientInfoInternal(patient, city, familyNames, tag, "1970-01-01");