Fix #1056: Remove extra method generated by "x-ms-pageable" custom nextlink operation #1281
Conversation
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
…om nextlink operation
AlexanderSher
requested review from
allenjzhang,
chamons,
m-nash,
pakrym and
ShivangiReja
as code owners
pakrym
reviewed
Jun 1, 2021
| @@ -85,6 +85,11 @@ public static IEnumerable<PagingMethod> BuildPagingMethods(OperationGroup operat | |||
| RestClientMethod method = restClient.GetOperationMethod(serviceRequest); | |||
| RestClientMethod? nextPageMethod = restClient.GetNextOperationMethod(serviceRequest); | |||
|
|
|||
| if (method == nextPageMethod) | |||
There was a problem hiding this comment.
This check doesn't seem to be correct at first look. Why do we need to skip methods where get next is the same as get first?
pakrym
reviewed
Jun 1, 2021
|
So after digging some more, with a test case, Pavel and I think this is a deeper issue. We should be having paging.NextLinkOperation defined as non-null in my test case but it isn't, possibly because the code model has a recursive definition of itself. |
6 tasks
|
Closing in favor of #1303 |
chamons
added a commit
that referenced
this pull request
Jun 9, 2021
…e methods (#1303) # Description This is a second attempt at #1281, with a bit of a deeper dig into how we ran into the situtation. Pavel pointed out during a discussion that we really shouldn't be seeing any of these "extra" methods at the level #1281 was filtering them out. After applying industrial strength bisect I was able to come up with this test case: ```yaml !CodeModel operationGroups: - !OperationGroup operations: - !Operation language: !Languages default: paging: nextLinkName: nextLink - !Operation language: !Languages default: paging: nextLinkName: nextLink nextLinkOperation: !Operation &ref_17 language: !Languages default: paging: nextLinkName: nextLink nextLinkOperation: *ref_17 - *ref_17 ``` In it this code: ```csharp foreach (var op in v.OperationGroups.First().Operations) { var paging = op.Language.Default.Paging; bool hasNextOperation = paging?.NextLinkOperation != null; ``` would return true for the second and third operation in my simplified model but the third (*ref_17) would differ in the "full" autorest.csharp model. The culprit was surprisingly: ``` - internal partial class Language : IDictionary<string, object> + internal partial class Language ``` For some, honestly unknown, reason YamlDotNet would not handle this case correctly as long as we implemented the dictionary interface. I decided against deep diving into that 3rd party dependency to understand this further. This already cost too many days. I did try bumping to latest, which failed for unrelated reasons but did not change this behavior. Removing the IDictionary interface required adding a few fields (header, summary) that were being indirectly handled, and may require more when I test this with azure-sdk-for-net this afternoon. Test case included for #1056 in particular. The removals in the paging test suites are expected.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
No description provided.