-
Notifications
You must be signed in to change notification settings - Fork 166
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
Resolve recursive language definations causing incorrect x-ms-pageable methods #1303
Conversation
@@ -268,6 +268,10 @@ | |||
"commandName": "Project", | |||
"commandLineArgs": "--standalone $(SolutionDir)\\test\\TestProjects\\OperationGroupMappings\\Generated" | |||
}, | |||
"PageNextLink": { |
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.
Does this project add something new on top of the existing test-server paging swagger?
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.
Those were in autorest.testserver and this felt possibly specific to C#'s handle of x-ms-pagable. I have no strong feeling here, I can push them up if you think that's preferable.
Was able to generate azure-sdk-for-net with local nuget with no issues (fwict) through gen code and building. Will redo test tomorrow. |
There we go, removed the specific test and added it to the general paging test suite. |
Wait for next week. |
Decide to land it now and not land bump PR until next week. |
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:
In it this code:
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:
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.
Checklist
To ensure a quick review and merge, please ensure:
Ready to Land?