-
Notifications
You must be signed in to change notification settings - Fork 33
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
Optional Query Parameter Test #722
Closed
Closed
Changes from 2 commits
Commits
Show all changes
22 commits
Select commit
Hold shift + click to select a range
749b6b6
update
mcgallan 3712d7c
Update mockapi.ts
mcgallan 0612107
update
mcgallan 5787406
fix
mcgallan faa2af2
Merge branch 'main' into optional-query-0904
mcgallan ddc31c1
update
mcgallan 733eedb
Merge branch 'optional-query-0904' of https://github.com/mcgallan/cad…
mcgallan f1bc36f
Merge remote-tracking branch 'upstream/main' into optional-query-0904
mcgallan 8244e3c
update
mcgallan 37156f9
Update mockapi.ts
mcgallan d274e60
Merge branch 'main' into optional-query-0904
mcgallan 915b8b1
Update mockapi.ts
mcgallan b688ac6
Merge branch 'optional-query-0904' of https://github.com/mcgallan/cad…
mcgallan a0a675f
Update mockapi.ts
mcgallan 04d0bc9
Merge branch 'main' into optional-query-0904
mcgallan 926f7f8
Update mockapi.ts
mcgallan 0210b31
Merge branch 'optional-query-0904' of https://github.com/mcgallan/cad…
mcgallan 6ea814a
Merge branch 'main' into optional-query-0904
mcgallan b3a96cf
Merge branch 'main' into optional-query-0904
mcgallan 70e83de
Merge branch 'main' into optional-query-0904
mcgallan 3d81e86
Update mockapi.ts
mcgallan 59f7d42
Merge remote-tracking branch 'upstream/main' into optional-query-0904
mcgallan File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains 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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
--- | ||
"@azure-tools/cadl-ranch-specs": minor | ||
--- | ||
|
||
Add test in parameters/query-optional. |
This file contains 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
59 changes: 59 additions & 0 deletions
59
packages/cadl-ranch-specs/http/parameters/query-optionality/main.tsp
This file contains 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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,59 @@ | ||
import "@azure-tools/cadl-ranch-expect"; | ||
import "@azure-tools/typespec-azure-core"; | ||
import "@azure-tools/typespec-client-generator-core"; | ||
import "@typespec/http"; | ||
import "@typespec/rest"; | ||
import "@typespec/versioning"; | ||
|
||
using TypeSpec.Http; | ||
using Azure.Core; | ||
using global.Azure.Core.Traits; | ||
using Azure.ClientGenerator.Core; | ||
using TypeSpec.Versioning; | ||
|
||
#suppress "@azure-tools/typespec-azure-core/casing-style" "For spec" | ||
@doc("Test describing optionality of the query.") | ||
@scenarioService( | ||
"/parameters/query-optionality", | ||
{ | ||
versioned: Versions, | ||
} | ||
) | ||
namespace Parameters.QueryOptionality; | ||
|
||
@doc("The API version.") | ||
enum Versions { | ||
@doc("The 2022-12-01-preview version.") | ||
@useDependency(global.Azure.Core.Versions.v1_0_Preview_2) | ||
v2022_12_01_preview: "2022-12-01-preview", | ||
} | ||
|
||
@scenario | ||
@scenarioDoc(""" | ||
Test scenarios for using a combination of required and optional query parameters. | ||
|
||
Should generate an operation like below: | ||
``` | ||
fromRequired(start: string, end?: string) | ||
``` | ||
|
||
Expected query parameter: api-version=2022-12-01-preview | ||
Expected query parameter: start=required | ||
""") | ||
@route("/fromrequired") | ||
@head | ||
op FromRequired(@query start: string, @query end?: string, @query("api-version") apiVersion: string): void; | ||
|
||
@scenario | ||
@scenarioDoc(""" | ||
Test another existing scenario where the query optional parameter is placed before the required parameter, but in the generated code, the required parameter still appears before the optional parameter. | ||
|
||
Should generate an operation like below: | ||
``` | ||
fromoptional(end: string, start?: string) | ||
``` | ||
Expected query parameter: end=required | ||
""") | ||
@route("/fromoptional") | ||
@head | ||
op FromOptional(@query start?: string, @query end: string): void; |
19 changes: 19 additions & 0 deletions
19
packages/cadl-ranch-specs/http/parameters/query-optionality/mockapi.ts
This file contains 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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,19 @@ | ||
import { passOnSuccess, mockapi, json } from "@azure-tools/cadl-ranch-api"; | ||
import { ScenarioMockApi } from "@azure-tools/cadl-ranch-api"; | ||
|
||
export const Scenarios: Record<string, ScenarioMockApi> = {}; | ||
|
||
Scenarios.Parameters_QueryOptionality_FromRequired = passOnSuccess( | ||
mockapi.head("/parameters/query-optionality/fromrequired", (req) => { | ||
req.expect.containsQueryParam("api-version", "2022-12-01-preview"); | ||
req.expect.containsQueryParam("start", "required"); | ||
return { status: 204 }; | ||
}), | ||
); | ||
|
||
Scenarios.Parameters_QueryOptionality_FromOptional = passOnSuccess( | ||
mockapi.head("/parameters/query-optionality/fromoptional", (req) => { | ||
req.expect.containsQueryParam("end", "required"); | ||
return { status: 204 }; | ||
}), | ||
); |
Oops, something went wrong.
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.
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.
we already have tests like these in versioning, what is this adding?
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.
In fact, the versioning only tests a single optional query parameter and does not test its combination with other query parameters. Additionally, this operation is intended to contrast with the following operation to demonstrate that, regardless of whether the optional parameter is before or after the required parameter, in the generated SDK, the required parameter always precedes the optional parameter.
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.
I think if we want to test more ordering, we can have
http/parameters/optionality
with aquery
section. And the naming shouldn't befromRequired
, because that signifies a growing-up from a required param to a required param + an optional param. Instead we can call thisorderingWithRequired
or something like that. I also don't see what the difference is between thefromOptional
andfromRequired
tests in this tsp. They both seem to be testing ordering to me, can you describe how they're different?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.
Regarding the placement and naming of the operations in this part, I believe your suggestion is a more suitable solution. As for the difference between the two, it might be more intuitive in the generated code from Autorest.Csharp: operation2 represents the expected form from fromoptional, while operation represents the expected form from fromrequired. The difference between the two is that in fromrequired, the required query parameter is placed first, followed by the optional parameter. In fromoptional, it is the opposite, with the required parameter placed after the optional parameter. However, in the generated code, the optional parameter is always placed after the required parameter. This test is meant to illustrate this issue, and the Parameter-TypeSpec project is also aimed at testing this issue.
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.
@iscai-msft Do you have any suggestions on how to handle this part?
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.
sorry, missed this during vacation. Do you see this test as testing ordering more or optionality? Since the file name is optionality, but the operation names are more ordering
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.
@iscai-msft I think this test focuses more on testing ordering, specifically the impact of optional query parameters on the ordering of query parameters. Since the optional query test case has already appeared in Cadl Ranch, it is not necessary to add this test if it is meant to test optional query parameters. Therefore, renaming this file to query-ordering or something else might be better?
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.
i think this could also be included with the optional query test case, since this tests ordering with optionality. What are your thoughts on adding these 2 tests next to existing tests about optional parameters. Additionally, I'm not sure how important it is that they are "query" parameters, this seems to be testing ordering with optionality
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.
@iscai-msft I made some modifications based on your suggestions. How does this version of the test look now?
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.
@iscai-msft I have optimized the query parameter to a general parameter to ensure the test focuses on the optionality ordering of the parameters. I have also moved it to the parameter/body-optionality section. Additionally, I have made changes to the code based on the subsequent mockapi upgrades for the PR. Please help review.