Skip to content
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

[ARM] az deployment: Fix the bug of 'bytes' object has no attribute 'get' for error handling in retry cases #21220

Merged
merged 1 commit into from
Feb 24, 2022

Conversation

zhoxing-ms
Copy link
Contributor

@zhoxing-ms zhoxing-ms commented Feb 8, 2022

Issue: #19743

Description

This error bytes' object has no attribute 'get' is due to the bug of JsonCTemplatePolicy in the retry operation.
In the case of retry, because the first request has been processed and converted the type of request.http_request.data from string to bytes code link , so there is no need to process request.http_request.data again during retry,

Testing Guide

History Notes

[Component Name 1] BREAKING CHANGE: az command a: Make some customer-facing breaking change.
[Component Name 2] az command b: Add some customer-facing feature.


This checklist is used to make sure that common guidelines for a pull request are followed.

@yonzhan
Copy link
Collaborator

yonzhan commented Feb 8, 2022

ARM

@@ -410,7 +410,15 @@ class JsonCTemplatePolicy(SansIOHTTPPolicy):

def on_request(self, request):
http_request = request.http_request
if (getattr(http_request, 'data', {}) or {}).get('properties', {}).get('template'):
request_data = getattr(http_request, 'data', {}) or {}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zhoxing-ms, have we run all tests impacted by this logic?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It mainly runs the tests that affects the scope of resource group. The logic of other scopes (such as: tenant sub mg) calling this policy is the same, so the scope of resource group can basically cover all use cases~

@zhoxing-ms zhoxing-ms merged commit 899f18c into Azure:dev Feb 24, 2022
@jiasli jiasli mentioned this pull request Apr 4, 2022
2 tasks
@jiasli
Copy link
Member

jiasli commented Apr 4, 2022

The purpose and logic of JsonCTemplatePolicy is hard to understand.

It seems JSONSerializer wraps the jsonc in a JsonCTemplate and then JsonCTemplatePolicy extracts the jsonc. Why isn't the jsonc serialization logic

http_request.data = partial_request[:-2] + ", template:" + template.template_as_bytes + r"}}"

done in JSONSerializer?

Also, The name template_as_bytes is misleading (#10389 (comment)).

We need better comments!

@zhoxing-ms
Copy link
Contributor Author

It seems JSONSerializer wraps the jsonc in a JsonCTemplate and then JsonCTemplatePolicy extracts the jsonc. Why isn't the jsonc serialization logic done in JSONSerializer?

In fact, it's technically feasible. I've tried it and it works fine. I guess this may be a decision in code design. Maybe this splicing is not appropriate to put it in the serialization logic in code design.
Because this is legacy code, I can't answer this question perfectly. 😂

Also, The name template_as_bytes is misleading (#10389 (comment)).

I agree! It will be converted to bytes in the subsequent step of JsonCTemplatePolicy, but not in the step of JSONSerializer. So I submitted a PR #21991 to correct the variable name and add some comments to explain the function of JsonCTemplatePolicy

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants