Add totalseconds canonical function binding - #1578
Open
RicardoGS98 wants to merge 2 commits into
Open
Conversation
Implements the OData V4 canonical function totalseconds(Edm.Duration) which was recognized by the URI parser but not bound by the LINQ expression binder, causing "Unknown function 'totalseconds'" errors at query time. Maps to TimeSpan.TotalSeconds cast to decimal (per spec). Also works naturally when the argument is a DateTimeOffset subtraction, since the parser types that as Edm.Duration: $filter=totalseconds(EndDate sub StartDate) gt 3600 Adds: - ClrCanonicalFunctions.TotalSecondsFunctionName and PropertyInfo for TimeSpan.TotalSeconds - QueryBinder.BindTotalSeconds (virtual) routed from BindSingleValueFunctionCallNode - AllowedFunctions.TotalSeconds flag, included in AllDateTimeFunctions - FilterQueryValidator and QueryValidatorHelpers map "totalseconds" to the new flag - Unit tests covering TimeSpan property and DateTimeOffset subtraction - PublicAPI baselines updated Note: totaloffsetminutes was intentionally not added in the same change because it would require the high bit (0x80000000), which breaks the existing signed-int range check in ODataValidationSettings. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Author
|
@microsoft-github-policy-service agree |
Member
|
@RicardoGS98 |
Addresses the review feedback on OData#1578 asking for E2E coverage. Adds test/Microsoft.AspNetCore.OData.E2E.Tests/DurationFunctions/ with 37 cases over a 5-row in-memory data source. Because the source is in-memory the LINQ provider is System.Linq, so HandleNullPropagation resolves to True: these are the first tests to exercise the null propagation branch of BindTotalSeconds, which the FilterBinderTests unit tests (running with it False) never reached. Coverage: - $filter with gt/lt/ge+le/eq/ne on a non-nullable Edm.Duration, including eq 0 and the constant-on-the-left form - nullable Edm.Duration: eq null, ne null and numeric comparisons - totalseconds(End sub Start) on DateTimeOffset subtraction, including a negative duration and a lifted subtraction (NullableEnd sub Start) - $orderby asc/desc on both the nullable and the non-nullable property - $compute combined with $filter, $orderby and $select - a fractional case (PT1.5S) asserted both via $filter and via an exact payload, showing the result is Edm.Decimal and not a truncated integer - AllowedFunctions validation: 400 for both $filter and $orderby when TotalSeconds is excluded, 200 under AllDateTimeFunctions, with symmetric controls The suite has no EF Core or LocalDB dependency: TimeSpan.TotalSeconds is not translatable by EF Core, consistent with the existing "EFCore could not be translated" notes in DateAndTimeOfDayWithEfTest. Also adds the unit tests that were missing: - DateFunctions_TotalSecondsFunction_Nullable, for parity with fractionalseconds (needs a TimeSpan? property on the Product test model) - two OrderByQueryValidatorTests cases: QueryValidatorHelpers.ToODataFunction is only reached through $orderby and had no coverage at all And completes the public API baseline: adding TotalSeconds to AllDateTimeFunctions also changes the value of AllFunctions, and the analyzer serializes enum flags in ascending order of value, so AllDateTimeFunctions moves to the end of the AllFunctions decomposition. Without that entry the build emits RS0016 and RS0017. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JJ2xdVpZtVBDKog32Jjuy7
Author
Thanks for the review — E2E coverage added in |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Implements the OData V4 canonical function
totalseconds(Edm.Duration), which is recognized by the URI parser but is not bound by the LINQ expression binder — currently producingUnknown function 'totalseconds'at query time (seeSRResources.ODataFunctionNotSupported).Per the spec,
totalsecondsreturns anEdm.Decimalrepresenting the duration as a fractional number of seconds. The CLR mapping isTimeSpan.TotalSecondscast todecimal.This also makes the common diff-between-dates pattern work out of the box:
because the OData parser types
DateTimeOffset sub DateTimeOffsetasEdm.Duration, and the binary subtract binder already produces aTimeSpanLINQ expression, whichBindTotalSecondsthen consumes.Changes
ClrCanonicalFunctions: addTotalSecondsFunctionNameandTimeSpanTotalSecondsPropertyInfo.QueryBinder.SingleValueFunctionCall: routetotalsecondsto a newvirtual BindTotalSecondsmethod.AllowedFunctions: addTotalSeconds = 0x40000000; include it inAllDateTimeFunctions(and transitively inAllFunctions).FilterQueryValidator+QueryValidatorHelpers: map"totalseconds"toAllowedFunctions.TotalSecondsin both (they were duplicated).FilterBinderTests: coverstotalseconds(TimeSpanProp)andtotalseconds(DateTimeOffset sub DateTimeOffset).FilterQueryValidatorTestsandAllowedFunctionsTests: data sets updated so the group/exclude tests cover the new flag.PublicAPI.Unshipped.txtandPublicApi.bslbaselines updated.Note on
totaloffsetminutesI considered including
totaloffsetminutesin the same change since it tends to come up together, but mapping it requires the sign bit (0x80000000) of theAllowedFunctionsenum, which makesAllFunctionsnegative and breaks thevalue > AllowedFunctions.AllFunctionsrange check inODataValidationSettings. That would need either widening the underlying type tolong(binary breaking) or a separate validation strategy, so I left it out of this PR.Test plan
dotnet build src/Microsoft.AspNetCore.OData/Microsoft.AspNetCore.OData.csproj— 0 errors.dotnet test test/Microsoft.AspNetCore.OData.Tests/Microsoft.AspNetCore.OData.Tests.csproj— 6336 passed, 5 skipped, 0 failed (locally).totalseconds(Edm.Duration)without further model registration (it does on the cases I tested; flagging for confirmation).🤖 Generated with Claude Code