Check if maxpagesize should be reinjected #9355
Conversation
microsoft-github-policy-service
bot
added
the
emitter:client:csharp
|
No changes needing a change description found. |
radhgupta
requested review from
JoshLove-msft,
christothes,
jorgerangel-msft,
jsquire and
m-nash
as code owners
...rs/TestData/RestClientProviderTests/TestNextLinkReinjectedParametersInCreateRequestMethod.cs
Outdated
Show resolved
Hide resolved
| if ((maxPageSize != null)) | ||
| { | ||
| uri.AppendQuery("maxPageSize", global::Sample.TypeFormatters.ConvertToString(maxPageSize), true); | ||
| if (!uri.Query.Contains("maxPageSize=")) |
There was a problem hiding this comment.
I think we need to handle updating the maxPageSize parameter value. The value in the nextLink might need to be replaced by the value passed to this method. So this is essentially updating the query param if it already exists, else appending.
There was a problem hiding this comment.
Side note, is Query an accessible property of the uribuilder type?
There was a problem hiding this comment.
Thanks for letting me know, I assumed the bug wrong! I have added the logic to rather update the parameter value. I have made sure to keep the logic specific to maxpagesize but also ensured that we can add more parameter in future.
| if ((maxPageSize != null)) | ||
| { | ||
| if (!uri.Query.Contains("maxPageSize=")) | ||
| if (uri.Query.Contains("maxPageSize=")) |
There was a problem hiding this comment.
I think there is still an issue here. Query isn't an accessible property in the ClientUriBuilder class, unless I'm missing something here, so this code won't compile. In general, when it comes to writing code for a generator, I highly recommend handwriting the changes first to get the implementation down and then adding the code to get it generated. In this case, I think we should use the ClientUriBuilderSnippets to construct the condition. It is okay if we have to add/transform a generated property in that class for the sake of supporting this.
| if (!uri.Query.Contains("maxPageSize=")) | ||
| if (uri.Query.Contains("maxPageSize=")) | ||
| { | ||
| string currentQuery = uri.Query; |
There was a problem hiding this comment.
I'm wondering if this should be a helper method in ClientUriBuilder? Something like "UpdateQuery"? It would also enable us to add a unit test for this query updating logic.
There was a problem hiding this comment.
I agree, I was actually thinking the same but I thought I might be overcomplicating it. I should I have discussed it. Thanks for confirming though.
3 participants
This pull request updates the logic for appending query parameters in the generated REST client code to ensure that duplicate query parameters are not added when handling "next link" requests (typically used for pagination). The changes introduce runtime checks to prevent reinjecting parameters that are already present in the query string, and update the relevant test cases to reflect this behavior.
Fixes: #9201