don't reformat query params for dpg next link calls #1168
Conversation
| async for item in pages: | ||
| items.append(item) | ||
| assert len(items) == 1 | ||
|
|
There was a problem hiding this comment.
ignore all the diff except the addition of the last test. Wanted to get rid of the unnecessary test class for the async paging test
| @@ -990,7 +989,6 @@ def prepare_request(next_link=None): | |||
| request = build_storage_accounts_list_by_resource_group_request( | |||
| resource_group_name=resource_group_name, | |||
| subscription_id=self._config.subscription_id, | |||
| api_version=api_version, | |||
There was a problem hiding this comment.
@johanste are we good with removing api_version formatting right now? We can add it back (either in core or in generation) once archboard has decided
| if ( | ||
| is_next_request and | ||
| not bool(builder.next_request_builder) and | ||
| self.code_model.options["version_tolerant"] and |
There was a problem hiding this comment.
#1165 is found in azure-mgmt-recoveryservicesbackup, so I think we shall remove the limitation version_tolerant
There was a problem hiding this comment.
Yeah, we ended up patching legacy code. The issue is it's a breaking change to change how we deal with next link calls, so we can't enable this for legacy, even if it might help one team (because other teams might be broken by it). But yeah, definitely good point because there is a legacy team affected by it
There was a problem hiding this comment.
If we are afraid to break current legacy code, how about adding flag like --disable-filter-query-params to permit some service like recoveryservicesbackup to avoid duplicated filter params in legacy code.
There was a problem hiding this comment.
added flag --reformat-next-link, which defaults to True for legacy code generations, is forced to False for DPG generations. Thanks!
| def test_duplicate_params(client): | ||
| pages = list(client.paging.duplicate_params(filter="foo")) | ||
| assert len(pages) == 1 | ||
| assert pages[0]["properties"]["id"] == 1 | ||
| assert pages[0]["properties"]["name"] == "Product" |
There was a problem hiding this comment.
This test seems not work well: https://github.com/Azure/autorest.python/pull/1168/files#r818465625
There was a problem hiding this comment.
what issues are you running into? it passes locally for me and in CI as well
| request = build_paging_duplicate_params_request( | ||
| filter=filter, | ||
| ) |
There was a problem hiding this comment.
Even if we add the code back, the test: https://github.com/Azure/autorest.python/pull/1168/files#r818465191 still pass.
There was a problem hiding this comment.
oh yeah good point, thanks for noticing! Turns out we were always doing the right thing in version tolerant, bc for single operation in swagger paging calls, we were setting next link requests url to the next_link.
Still going to keep this code change because it's clearer if we never pass the query parameters to begin with (and thanks to you it also helps legacy generations with the flag). Thanks!
…into next_link_opaque * 'autorestv3' of https://github.com/Azure/autorest.python: Update package.json Update ChangeLog.md default optional const parameter to none (#1171) add storage to ci checks (#1176) fail on regeneration diff (#1173) Drop Python 2.7 (#1175) Add secrets test (#1174) bump testserver version (#1172)
…python into next_link_opaque * 'next_link_opaque' of https://github.com/Azure/autorest.python:
…into inherit_customization * 'autorestv3' of https://github.com/Azure/autorest.python: add coverage var (#1178) improve logging (#1177) don't reformat query params for dpg next link calls (#1168)
…into mypy_code * 'autorestv3' of https://github.com/Azure/autorest.python: (26 commits) dpg service-driven-test (#1180) prepare for release (#1181) [Package mode] add new flag `--package-mode` (#1154) hide operation group docs (#1179) add coverage var (#1178) improve logging (#1177) don't reformat query params for dpg next link calls (#1168) Update package.json Update ChangeLog.md default optional const parameter to none (#1171) add storage to ci checks (#1176) fail on regeneration diff (#1173) Drop Python 2.7 (#1175) Add secrets test (#1174) bump testserver version (#1172) make content type keyword only if multiple (#1167) add default value info for params to docs (#1164) update release tests to hopefully get coverage report green (#1152) fix windows: clean the outputFolderUri (#1135) Update CODEOWNERS ...
fixes #1165