-
Notifications
You must be signed in to change notification settings - Fork 371
Add Spector cases for serializing lossy duration encodings as integers #10835
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 6 commits
f1ccfbe
fa48c47
9aabc3f
fa87ef4
b9a6161
7a11acd
bd267b1
a96701d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| --- | ||
| changeKind: internal | ||
| packages: | ||
| - "@typespec/http-specs" | ||
| --- | ||
|
|
||
| Add encode/duration int32-seconds-fractional Spector scenarios verifying a fractional duration is serialized as an integer |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -45,6 +45,21 @@ namespace Query { | |
| input: duration, | ||
| ): NoContentResponse; | ||
|
|
||
| @route("/int32-seconds-fractional") | ||
| @scenario | ||
| @scenarioDoc(""" | ||
| Test int32 seconds encode for a duration parameter whose value has a fractional (sub-second) component. | ||
| The duration is 36.25 seconds, e.g. TimeSpan.FromSeconds(36.25) in C#. | ||
| Even though the underlying value is fractional, the client must serialize it as an integer (not a floating point number such as `36.25`). | ||
| This scenario specifically exercises the lossy encode case where the source type carries more precision than the target encoding; it is not about arbitrary type mismatches, which are already covered by other round-trip scenarios. | ||
| The value is chosen so that rounding and truncating both yield the same integer, so the expected query parameter is `input=36`. | ||
| """) | ||
| op int32SecondsFractional( | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we move this (and potentially the milli test) to another category, this doesn't feel like this belongs under
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The problem with doing this is that we still want to have header/query/property variants for the lossy encoding tests to exercise all code paths.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think we need all 3 I would just keep a single in a body. |
||
| @query | ||
| @encode(DurationKnownEncoding.seconds, int32) | ||
| input: duration, | ||
| ): NoContentResponse; | ||
|
|
||
| @route("/int32-seconds-larger-unit") | ||
| @scenario | ||
| @scenarioDoc(""" | ||
|
|
@@ -202,6 +217,11 @@ namespace Property { | |
| value: duration; | ||
| } | ||
|
|
||
| model Int32SecondsFractionalDurationProperty { | ||
| @encode(DurationKnownEncoding.seconds, int32) | ||
| value: duration; | ||
| } | ||
|
|
||
| model FloatSecondsDurationProperty { | ||
| @encode(DurationKnownEncoding.seconds, float) | ||
| value: duration; | ||
|
|
@@ -320,6 +340,31 @@ namespace Property { | |
| """) | ||
| op int32Seconds(@body body: Int32SecondsDurationProperty): Int32SecondsDurationProperty; | ||
|
|
||
| @route("/int32-seconds-fractional") | ||
| @scenario | ||
| @scenarioDoc(""" | ||
| Test operation with request and response model contains a duration property with int32 seconds encode whose value has a fractional (sub-second) component. | ||
| The duration is 36.25 seconds, e.g. TimeSpan.FromSeconds(36.25) in C#. | ||
| Even though the underlying value is fractional, the client must serialize it as an integer (not a floating point number such as `36.25`). | ||
| This scenario specifically exercises the lossy encode case where the source type carries more precision than the target encoding; it is not about arbitrary type mismatches, which are already covered by other round-trip scenarios. | ||
| The value is chosen so that rounding and truncating both yield the same integer. | ||
| Expected request body: | ||
| ```json | ||
| { | ||
| "value": 36 | ||
| } | ||
| ``` | ||
| Expected response body: | ||
| ```json | ||
| { | ||
| "value": 36 | ||
| } | ||
| ``` | ||
| """) | ||
| op int32SecondsFractional( | ||
| @body body: Int32SecondsFractionalDurationProperty, | ||
| ): Int32SecondsFractionalDurationProperty; | ||
|
|
||
| @route("/float-seconds") | ||
| @scenario | ||
| @scenarioDoc(""" | ||
|
|
@@ -603,6 +648,21 @@ namespace Header { | |
| duration: duration, | ||
| ): NoContentResponse; | ||
|
|
||
| @route("/int32-seconds-fractional") | ||
| @scenario | ||
| @scenarioDoc(""" | ||
| Test int32 seconds encode for a duration header whose value has a fractional (sub-second) component. | ||
| The duration is 36.25 seconds, e.g. TimeSpan.FromSeconds(36.25) in C#. | ||
| Even though the underlying value is fractional, the client must serialize it as an integer (not a floating point number such as `36.25`). | ||
| This scenario specifically exercises the lossy encode case where the source type carries more precision than the target encoding; it is not about arbitrary type mismatches, which are already covered by other round-trip scenarios. | ||
| The value is chosen so that rounding and truncating both yield the same integer, so the expected header is `duration: 36`. | ||
| """) | ||
| op int32SecondsFractional( | ||
| @header | ||
| @encode(DurationKnownEncoding.seconds, int32) | ||
| duration: duration, | ||
| ): NoContentResponse; | ||
|
|
||
| @route("/int32-seconds-larger-unit") | ||
| @scenario | ||
| @scenarioDoc(""" | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -23,6 +23,44 @@ function createBodyServerTests(uri: string, data: any, value: any) { | |
| kind: "MockApiDefinition", | ||
| }); | ||
| } | ||
|
|
||
| // Validates that a duration with a fractional (sub-second) component is serialized as an integer. | ||
| // The duration (36.25s) is chosen so rounding and truncating both yield 36. | ||
| function createBodyIntServerTests(uri: string) { | ||
| return passOnSuccess({ | ||
| uri, | ||
| method: "post", | ||
| request: { | ||
| body: json({ value: 36 }), | ||
| }, | ||
| response: { | ||
| status: 200, | ||
| body: json({ value: 36 }), | ||
| }, | ||
| handler: (req: MockRequest) => { | ||
| const value = req.body?.value; | ||
| if (typeof value !== "number" || !Number.isInteger(value)) { | ||
| throw new ValidationError( | ||
| `Expected body property "value" to be serialized as an integer but got ${value}`, | ||
| "an integer", | ||
| value, | ||
| ); | ||
| } | ||
| if (value !== 36) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. probably unlikely but an implementation could also choose ceil so we might want to allow 37 to be explicit that all of this is allowed. |
||
| throw new ValidationError( | ||
| `Expected body property "value" to be 36 but got ${value}`, | ||
| "36", | ||
| value, | ||
| ); | ||
| } | ||
| return { | ||
| status: 200, | ||
| body: json({ value }), | ||
| }; | ||
| }, | ||
| kind: "MockApiDefinition", | ||
| }); | ||
| } | ||
| Scenarios.Encode_Duration_Property_default = createBodyServerTests( | ||
| "/encode/duration/property/default", | ||
| { | ||
|
|
@@ -51,6 +89,9 @@ Scenarios.Encode_Duration_Property_int32Seconds = createBodyServerTests( | |
| }, | ||
| 36, | ||
| ); | ||
| Scenarios.Encode_Duration_Property_int32SecondsFractional = createBodyIntServerTests( | ||
| "/encode/duration/property/int32-seconds-fractional", | ||
| ); | ||
| Scenarios.Encode_Duration_Property_iso8601 = createBodyServerTests( | ||
| "/encode/duration/property/iso8601", | ||
| { | ||
|
|
@@ -175,6 +216,38 @@ function createQueryFloatServerTests(uri: string, paramData: any, value: number) | |
| kind: "MockApiDefinition", | ||
| }); | ||
| } | ||
|
|
||
| // Validates that a duration with a fractional (sub-second) component is serialized as an integer. | ||
| // The duration (36.25s) is chosen so rounding and truncating both yield 36. | ||
| function createQueryIntServerTests(uri: string, paramData: any) { | ||
| return passOnSuccess({ | ||
| uri, | ||
| method: "get", | ||
| request: { | ||
| query: paramData, | ||
| }, | ||
| response: { | ||
| status: 204, | ||
| }, | ||
| handler: (req: MockRequest) => { | ||
| const actual = req.query["input"] as string; | ||
| if (!/^[-+]?\d+$/.test(actual)) { | ||
| throw new ValidationError( | ||
| `Expected query param input to be serialized as an integer but got ${actual}`, | ||
| "an integer", | ||
| actual, | ||
| ); | ||
| } | ||
| if (actual !== "36") { | ||
| throw new ValidationError(`Expected query param input=36 but got ${actual}`, "36", actual); | ||
| } | ||
| return { | ||
| status: 204, | ||
| }; | ||
| }, | ||
| kind: "MockApiDefinition", | ||
| }); | ||
| } | ||
| Scenarios.Encode_Duration_Query_default = createQueryServerTests( | ||
| "/encode/duration/query/default", | ||
| { | ||
|
|
@@ -196,6 +269,12 @@ Scenarios.Encode_Duration_Query_int32Seconds = createQueryServerTests( | |
| }, | ||
| "36", | ||
| ); | ||
| Scenarios.Encode_Duration_Query_int32SecondsFractional = createQueryIntServerTests( | ||
| "/encode/duration/query/int32-seconds-fractional", | ||
| { | ||
| input: 36, | ||
| }, | ||
| ); | ||
| Scenarios.Encode_Duration_Query_int32SecondsArray = createQueryServerTests( | ||
| "/encode/duration/query/int32-seconds-array", | ||
| { | ||
|
|
@@ -321,6 +400,40 @@ function createHeaderFloatServerTests(uri: string, value: number) { | |
| }); | ||
| } | ||
|
|
||
| // Validates that a duration with a fractional (sub-second) component is serialized as an integer. | ||
| // The duration (36.25s) is chosen so rounding and truncating both yield 36. | ||
| function createHeaderIntServerTests(uri: string) { | ||
| return passOnSuccess({ | ||
| uri, | ||
| method: "get", | ||
| request: { | ||
| headers: { | ||
| duration: "36", | ||
| }, | ||
| }, | ||
| response: { | ||
| status: 204, | ||
| }, | ||
| handler: (req: MockRequest) => { | ||
| const actual = req.headers["duration"]; | ||
| if (!/^[-+]?\d+$/.test(actual)) { | ||
| throw new ValidationError( | ||
| `Expected header duration to be serialized as an integer but got ${actual}`, | ||
| "an integer", | ||
| actual, | ||
| ); | ||
| } | ||
| if (actual !== "36") { | ||
| throw new ValidationError(`Expected header duration=36 but got ${actual}`, "36", actual); | ||
| } | ||
| return { | ||
| status: 204, | ||
| }; | ||
| }, | ||
| kind: "MockApiDefinition", | ||
| }); | ||
| } | ||
|
|
||
| Scenarios.Encode_Duration_Header_default = createHeaderServerTests( | ||
| "/encode/duration/header/default", | ||
| { | ||
|
|
@@ -342,6 +455,9 @@ Scenarios.Encode_Duration_Header_int32Seconds = createHeaderServerTests( | |
| }, | ||
| "36", | ||
| ); | ||
| Scenarios.Encode_Duration_Header_int32SecondsFractional = createHeaderIntServerTests( | ||
| "/encode/duration/header/int32-seconds-fractional", | ||
| ); | ||
| Scenarios.Encode_Duration_Header_floatSeconds = createHeaderServerTests( | ||
| "/encode/duration/header/float-seconds", | ||
| { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
from discussion on the issue I don't think we can add a test for this as floor vs round is not something TypeSpec should have an opinion here and it more of an emitter implementation detail
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test can still validate that the proper number type is serialized (e.g. integer rather than double/decimal).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated the scenarios to validate only the number type. The mock handlers reject a floating point value and assert an integer is serialized, without checking an exact value. I also reworded the scenarioDocs to state that floor vs. round is an emitter implementation detail and that the test only validates that an integer is sent (e.g.
35or36). Done in 4b13fac.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot actually we can just use input data such that the rounding vs truncating decision wouldn't matter - e.g. 36.25.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
by that logic should we have a test to make sure you don't somehow send a string instead of a int as well? It just feels like this is one case you caught here but there is an infinite number of potential failures here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
an emitter could also very well here decide that is this an error (crash) so can never get this test to work
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These type violations are already covered by every round trip test. This case is special because the source type carries more precision than the target encoding. This is a lossy encode scenario - not arbitrary type mismatch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The contract is "encode this duration as int32 seconds," so crashing is non-conformant behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hhm alright fair enough, though do we need the same for milliseconds encoding?