Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -100,10 +100,44 @@ void ISearchValueVisitor.Visit(DateTimeSearchValue dateTime)
var startTicks = dateTime.Start.UtcTicks;
var endTicks = dateTime.End.UtcTicks;

var differenceTicks = (long)((Clock.UtcNow.Ticks - Math.Max(startTicks, endTicks)) * ApproximateMultiplier);

var approximateStart = dateTime.Start.AddTicks(-differenceTicks);
var approximateEnd = dateTime.End.AddTicks(differenceTicks);
// Calculate the difference safely to avoid overflow
long nowTicks = Clock.UtcNow.Ticks;
long maxTicks = Math.Max(startTicks, endTicks);

// Use checked arithmetic to detect overflow, but handle it gracefully
long differenceTicks;
try
{
checked
{
long rawDifference = nowTicks - maxTicks;
differenceTicks = (long)(rawDifference * ApproximateMultiplier);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm confused how this could ever overflow. nowTicks is approximately known to be 6.37e17 and maxTicks can't be greater than roughly 1/3 long.maxValue or less than 0, so their difference can't be outside the range of long (~9.22e18). ApproximateMultiplier is less than one so differenceTicks is less than rawDifference.
Is there something about this calculation I'm missing?

}
}
catch (OverflowException)
{
// If overflow occurs in calculation, use a safe maximum difference
// This represents approximately 10% of the maximum safe tick range
differenceTicks = long.MaxValue / 20;
}

// Ensure we don't overflow when adding/subtracting ticks
long ticksToSubtract = differenceTicks;
long ticksToAdd = differenceTicks;

// Clamp to prevent overflow before calling AddTicks
if (startTicks - ticksToSubtract < DateTimeOffset.MinValue.Ticks)
{
ticksToSubtract = startTicks - DateTimeOffset.MinValue.Ticks;
}

if (endTicks + ticksToAdd > DateTimeOffset.MaxValue.Ticks)
{
ticksToAdd = DateTimeOffset.MaxValue.Ticks - endTicks;
}

DateTimeOffset approximateStart = dateTime.Start.AddTicks(-ticksToSubtract);
DateTimeOffset approximateEnd = dateTime.End.AddTicks(ticksToAdd);

_outputExpression = Expression.And(
Expression.GreaterThanOrEqual(FieldName.DateTimeStart, _componentIndex, approximateStart),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -236,5 +236,35 @@
Assert.Fail($"A non-expected '{e.GetType()}' was raised. Url: {Client.HttpClient.BaseAddress}. No Activity Id present. Error: {e.Message}");
}
}

[Theory]
[InlineData("ap9500-01-01")] // Approximate search with future date that causes overflow
[InlineData("ap0001-01-01")] // Approximate search with very old date
[InlineData("ap9999-12-31")] // Approximate search near max date
public async Task GivenAnApproximateDateSearchParamWithExtremeDate_WhenSearched_ThenServerShouldNotCrash(string queryValue)
{
try
{
// The server should handle extreme dates gracefully and return a successful response
// even if no results are found (empty bundle is OK)
Bundle bundle = await Client.SearchAsync(ResourceType.Patient, $"birthdate={queryValue}");

// If we get here, the server handled it correctly - either with results or an empty bundle
Assert.NotNull(bundle);
Assert.Equal(Bundle.BundleType.Searchset, bundle.Type);
}
catch (FhirClientException fce)
{
// The server should NOT return 500 Internal Server Error for valid approximate date searches
// It's acceptable to return BadRequest if the server decides the date is out of reasonable bounds
Assert.True(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this test should have two valid scenarios, it should either expect success or a bad request.

fce.StatusCode != HttpStatusCode.InternalServerError,
$"Server returned '{HttpStatusCode.InternalServerError}' for a valid approximate date search. This indicates an unhandled exception (overflow) in the date calculation logic. Url: {Client.HttpClient.BaseAddress}. Activity Id: {fce.Response.GetRequestId()}. Error: {fce.Message}");
}
catch (Exception e)
{
Assert.Fail($"An unexpected '{e.GetType()}' was raised. The server should handle extreme approximate dates gracefully. Url: {Client.HttpClient.BaseAddress}. Error: {e.Message}");
}
Comment on lines +264 to +267

Check notice

Code scanning / CodeQL

Generic catch clause Note test

Generic catch clause.

Copilot Autofix

AI about 1 month ago

General approach: Replace the generic catch (Exception e) with either more specific exception types or remove it altogether so that truly unexpected exceptions propagate naturally. Since we already handle FhirClientException explicitly and everything else is “unexpected,” the cleanest fix is to let the test framework report unexpected exceptions without catching them, thereby eliminating the generic catch clause.

Best concrete fix here: Remove the catch (Exception e) block from GivenAnApproximateDateSearchParamWithExtremeDate_WhenSearched_ThenServerShouldNotCrash. This preserves the desired behavior around FhirClientException (checking that the status code is not 500) while allowing any other unexpected exceptions to surface directly with full stack traces. No new imports or helpers are needed, and existing functionality is not materially changed: the test still fails when an unexpected exception occurs, but now via the framework instead of a manual Assert.Fail.

Specifically, in test/Microsoft.Health.Fhir.Shared.Tests.E2E/Rest/Search/DateSearchTests.cs, within the second test method shown (GivenAnApproximateDateSearchParamWithExtremeDate_WhenSearched_ThenServerShouldNotCrash), delete lines 264–267 containing the generic catch (Exception e) and the associated Assert.Fail call. Leave the rest of the method, including the try block and catch (FhirClientException fce), unchanged.


Suggested changeset 1
test/Microsoft.Health.Fhir.Shared.Tests.E2E/Rest/Search/DateSearchTests.cs

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/test/Microsoft.Health.Fhir.Shared.Tests.E2E/Rest/Search/DateSearchTests.cs b/test/Microsoft.Health.Fhir.Shared.Tests.E2E/Rest/Search/DateSearchTests.cs
--- a/test/Microsoft.Health.Fhir.Shared.Tests.E2E/Rest/Search/DateSearchTests.cs
+++ b/test/Microsoft.Health.Fhir.Shared.Tests.E2E/Rest/Search/DateSearchTests.cs
@@ -261,10 +261,6 @@
                     fce.StatusCode != HttpStatusCode.InternalServerError,
                     $"Server returned '{HttpStatusCode.InternalServerError}' for a valid approximate date search. This indicates an unhandled exception (overflow) in the date calculation logic. Url: {Client.HttpClient.BaseAddress}. Activity Id: {fce.Response.GetRequestId()}. Error: {fce.Message}");
             }
-            catch (Exception e)
-            {
-                Assert.Fail($"An unexpected '{e.GetType()}' was raised. The server should handle extreme approximate dates gracefully. Url: {Client.HttpClient.BaseAddress}. Error: {e.Message}");
-            }
         }
     }
 }
EOF
@@ -261,10 +261,6 @@
fce.StatusCode != HttpStatusCode.InternalServerError,
$"Server returned '{HttpStatusCode.InternalServerError}' for a valid approximate date search. This indicates an unhandled exception (overflow) in the date calculation logic. Url: {Client.HttpClient.BaseAddress}. Activity Id: {fce.Response.GetRequestId()}. Error: {fce.Message}");
}
catch (Exception e)
{
Assert.Fail($"An unexpected '{e.GetType()}' was raised. The server should handle extreme approximate dates gracefully. Url: {Client.HttpClient.BaseAddress}. Error: {e.Message}");
}
}
}
}
Copilot is powered by AI and may make mistakes. Always verify output.
}
}
}
Loading