Add headers and params in operation and request builder kwargs #1183

changlong-liu · 2022-03-11T05:11:11Z

for issue #1162

iscai-msft

looks good!

autorest/codegen/models/parameter.py

iscai-msft · 2022-03-16T15:19:37Z

autorest/codegen/serializers/builder_serializer.py

@@ -860,6 +878,8 @@ def _call_request_builder_helper(
        if not self.code_model.options["version_tolerant"]:
            template_url = template_url or f"self.{builder.name}.metadata['url']"
            retval.append(f"    template_url={template_url},")
+        retval.append('    headers=_headers,')
+        retval.append('    params=_params,')


can you also update line 1015 to
retval.append("error_map.update(kwargs.pop('error_map', {})) or {}")

updated as this:

retval.append("error_map.update(kwargs.pop('error_map', {}) or {})")

since we want to avoid kwargs['error_map'] is None?

yeah, if the user passes in operation(error_map=None), we would have error_map = None, then map_error gets mad

autorest/codegen/serializers/utils.py

iscai-msft · 2022-03-16T15:33:01Z

.../legacy/Expected/AcceptanceTests/BodyComplex/bodycomplex/aio/operations/_basic_operations.py

@@ -151,10 +164,15 @@ async def get_invalid(self, **kwargs: Any) -> "_models.Basic":
        error_map = {401: ClientAuthenticationError, 404: ResourceNotFoundError, 409: ResourceExistsError}
        error_map.update(kwargs.pop("error_map", {}))

+        _headers = case_insensitive_dict(kwargs.pop("headers", {}) or {})


I know yesterday you didn't love that we had to convert to case_insensitive_dict in operations and in request builders. I think a good idea could be to only convert to case_insensitive_dict in the main operation if we need to, for example on line 121, we need case insensitive dict bc we're checking api-version. However, here we don't need a case insensitive dict bc we pass it directly to the request builder, which will convert it for us

If it makes autorest code too gross feel free to disregard, but I think it can be done neatly

Yes @iscai-msft , this make lots sense on the generated code. I modified two parts of logic to achieve this:

model.Operation._imports_base (for checking whether need to import case_insensitive_dict)

serializer._OperationBaseSerializer.pop_kwargs_from_signature (for checking whether need to convert pop outcomes to ``case_insensitive`)

fix fix regenerate

import for headers and queries

lint

revert-patch don't import for body regenerate regenerate

iscai-msft

so close, it looks like we're not inspecting query / maybe header dicts for some of the params though

iscai-msft · 2022-03-17T18:44:02Z

autorest/codegen/serializers/builder_serializer.py

@@ -860,6 +878,8 @@ def _call_request_builder_helper(
        if not self.code_model.options["version_tolerant"]:
            template_url = template_url or f"self.{builder.name}.metadata['url']"
            retval.append(f"    template_url={template_url},")
+        retval.append('    headers=_headers,')
+        retval.append('    params=_params,')


yeah, if the user passes in operation(error_map=None), we would have error_map = None, then map_error gets mad

iscai-msft · 2022-03-18T15:53:22Z

autorest/codegen/serializers/utils.py

@@ -102,13 +103,36 @@ def method_signature_and_response_type_annotation_template(
        return f"{method_signature} -> {response_type_annotation}:"
    return f"{method_signature}:\n    # type: (...) -> {response_type_annotation}"

-def pop_kwargs_from_signature(kwargs_to_pop: List[Parameter]) -> List[str]:
+class PopKwargType(Enum):
+    NO = auto()


like the enum for the three kinds of popping!

iscai-msft · 2022-03-18T15:56:30Z

...on/multiapi/generated/azure/multiapi/sample/v2/operations/_operation_group_two_operations.py

-    _query_parameters = kwargs.pop("params", {})  # type: Dict[str, Any]
-    _query_parameters['parameterOne'] = _SERIALIZER.query("parameter_one", parameter_one, 'bool')
-    _query_parameters['api-version'] = _SERIALIZER.query("api_version", api_version, 'str')
+    _params['parameterOne'] = _SERIALIZER.query("parameter_one", parameter_one, 'bool')


it seems like we're missing a case here at least for query params: parameterOne is a query param but we're not checking the param dict for it here

Hi @iscai-msft , I think this can be a problem for some build_xx_request functions, so I added changes at line 42 of this file (as generated result) in commit 8db1a08

Add I added some test case for this scenario: 20bfab2

hey @changlong-liu sorry I should've thought more before I added this comment. Even though these required parameters are kwargs in the builders (and in some version tolerant operations), they are still required and there will be an error if users don't input them, so I think the reversal I did still stands: we don't need to check headers or params because users will be required to input and we want to keep raising the error if users don't put them in. What do you think?

Thanks for the talk in meeting about this. Keeping current logic is the best way to ensure same behavior across different style of build_xx_request signatures.
So I reverted changes in 8db1a08 and 20bfab2

This reverts commit 20bfab2.

This reverts commit 8db1a08.

iscai-msft

yay this looks great!

…into fix_dpg_customize_tests * 'autorestv3' of https://github.com/Azure/autorest.python: Add headers and params in operation and request builder kwargs (#1183) use dict to handle generations (#1200)

…into update_models_filename * 'autorestv3' of https://github.com/Azure/autorest.python: Add headers and params in operation and request builder kwargs (#1183) use dict to handle generations (#1200) pylint and mypy support for version tolerant with msrest models (#1202) rename-dpg-swagger (#1201)

…into post_process_mypy * 'autorestv3' of https://github.com/Azure/autorest.python: switch to dict models (#1203) change models filename for version tolerant (#1204) get tests green (#1205) Add headers and params in operation and request builder kwargs (#1183) use dict to handle generations (#1200)

changlong-liu force-pushed the 20220311-header-parameters branch 6 times, most recently from 7c7df7d to fa7303d Compare March 15, 2022 23:38

iscai-msft reviewed Mar 16, 2022

View reviewed changes

iscai-msft and others added 20 commits March 18, 2022 09:51

check user inputted dict first

4771230

add tests and regenerate

ac3d6af

regnerate

4184eac

fix fix regenerate

use _headers in initial_call

3b2ce5b

import for headers and queries

use-1.23.0

363327e

lint

remove-error-{}

88308b3

fix

0dfb0c8

revert-patch don't import for body regenerate regenerate

remove-vendor.py

67fd984

use case_insensitive_dict and fix mypy

ab75aec

fixlint

6fd3043

fix

c42a479

diff

d7dbe79

patch.py

f8fff99

clean

3390bd1

review-fix

80d1eb8

error_map and changelog

61dddb1

import

7967753

add-model

3af6115

fix

3455010

regenerate

9003dc5

changlong-liu force-pushed the 20220311-header-parameters branch from 42ed37c to 9003dc5 Compare March 18, 2022 02:45

changlong-liu marked this pull request as ready for review March 18, 2022 05:15

changlong-liu requested a review from msyyc as a code owner March 18, 2022 05:15

changlong-liu changed the title ~~WIP: headers and params~~ Add headers and params in operation and request builder kwargs Mar 18, 2022

iscai-msft reviewed Mar 18, 2022

View reviewed changes

changlong-liu added 6 commits March 20, 2022 13:39

use-kwargs-for-non-defaults

8db1a08

testcase

20bfab2

Revert "testcase"

080947a

This reverts commit 20bfab2.

Revert "use-kwargs-for-non-defaults"

2cf79bd

This reverts commit 8db1a08.

Merge branch 'autorestv3' into 20220311-header-parameters

f8cbdc4

use [email protected] and regenerate

2e3d407

iscai-msft approved these changes Mar 23, 2022

View reviewed changes

iscai-msft merged commit d7291e1 into Azure:autorestv3 Mar 23, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add headers and params in operation and request builder kwargs #1183

Add headers and params in operation and request builder kwargs #1183

changlong-liu commented Mar 11, 2022 •

edited

Loading

iscai-msft left a comment

iscai-msft Mar 16, 2022

changlong-liu Mar 17, 2022

iscai-msft Mar 17, 2022

iscai-msft Mar 16, 2022

changlong-liu Mar 18, 2022

iscai-msft left a comment

iscai-msft Mar 17, 2022

iscai-msft Mar 18, 2022

iscai-msft Mar 18, 2022

changlong-liu Mar 22, 2022

iscai-msft Mar 22, 2022

changlong-liu Mar 23, 2022

iscai-msft left a comment

Add headers and params in operation and request builder kwargs #1183

Add headers and params in operation and request builder kwargs #1183

Conversation

changlong-liu commented Mar 11, 2022 • edited Loading

iscai-msft left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iscai-msft left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iscai-msft left a comment

Choose a reason for hiding this comment

changlong-liu commented Mar 11, 2022 •

edited

Loading