Skip to content

{AD} Remove unnecessary logic from MSGraphClientPasswordReplacer#22783

Merged
jiasli merged 1 commit intoAzure:devfrom
jiasli:graph-replacer
Jun 9, 2022
Merged

{AD} Remove unnecessary logic from MSGraphClientPasswordReplacer#22783
jiasli merged 1 commit intoAzure:devfrom
jiasli:graph-replacer

Conversation

@jiasli
Copy link
Copy Markdown
Member

@jiasli jiasli commented Jun 8, 2022

Description

Synapse test src/azure-cli/azure/cli/command_modules/synapse/tests/latest/test_synapse_scenario.py::SynapseScenarioTests::test_workspace_package uploads a jar file as the request body:

azure-cli/src/azure-cli/azure/cli/command_modules/synapse/tests/latest/test_synapse_scenario.py

Lines 2652 to 2657 in 85ef3c7

# upload workspace package
self.cmd(
'az synapse workspace-package upload --workspace-name {workspace} --package "{file}"',
checks=[
self.check('name', self.kwargs['name'])
])

As the jar is a binary file (bytes), _byte_to_str will fail to decode bytes as utf-8:

azure-cli/src/azure-cli-testsdk/azure/cli/testsdk/utilities.py

Lines 51 to 52 in cac3f06

def _byte_to_str(byte_or_str):
return str(byte_or_str, 'utf-8') if isinstance(byte_or_str, bytes) else byte_or_str

causing failure:

src\azure-cli-testsdk\azure\cli\testsdk\scenario_tests\base.py:165: in _process_request_recording
    request = processor.process_request(request)
src\azure-cli-testsdk\azure\cli\testsdk\utilities.py:166: in process_request
    if request.body and self.PWD_REPLACEMENT in _byte_to_str(request.body):
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

byte_or_str = b'PK\x03\x04\n\x00\x00\x00\x00\x00:\xbf\x82O\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\t\x00\x00\x00META-INF/PK\...INF/maven/Java/WordCount/pom.propertiesPK\x05\x06\x00\x00\x00\x00\x0b\x00\x0b\x00\xeb\x02\x00\x00O\x0e\x00\x00\x00\x00'

    def _byte_to_str(byte_or_str):
>       return str(byte_or_str, 'utf-8') if isinstance(byte_or_str, bytes) else byte_or_str
E       UnicodeDecodeError: 'utf-8' codec can't decode byte 0xbf in position 11: invalid start byte

src\azure-cli-testsdk\azure\cli\testsdk\utilities.py:52: UnicodeDecodeError

Those removed lines came from AD Graph GraphClientPasswordReplacer:

azure-cli/src/azure-cli-testsdk/azure/cli/testsdk/utilities.py

Lines 123 to 125 in 8fc0912

# issue with how vcr.Request.body adds b' to text types if self.body is used.
if request.body and self.PWD_REPLACEMENT in str(request.body):
return request

We don't really know what its functionality and the original "issue" were. VCRPY invokes GraphClientPasswordReplacer 3 times for each request and the first 2 invocations take place as a pipeline:

  1. request -> replace(request) -> replace(replace(request)) # For some internal comparison
  2. request -> replace(request) # The result is saved to YAML

Perhaps those lines are used to eliminate duplicated replacement operation?

Since the request body of Microsoft Graph API addPassword will never contains a password, that doesn't matter now.

Testing Guide

pytest src/azure-cli/azure/cli/command_modules/synapse/tests/latest/test_synapse_scenario.py::SynapseScenarioTests::test_workspace_package -o log_cli=1

@ghost ghost requested a review from yonzhan June 8, 2022 10:57
@ghost ghost added the Auto-Assign Auto assign by bot label Jun 8, 2022
@ghost ghost assigned jiasli Jun 8, 2022
@ghost ghost added this to the Jun 2022 (2022-07-05) milestone Jun 8, 2022
@ghost ghost added the Graph (doesn't work with label-triggered comments; use Graph.Microsoft instead) az ad label Jun 8, 2022
@ghost ghost requested review from calvinhzy and evelyn-ys June 8, 2022 10:58
@ghost ghost assigned jsntcy Jun 8, 2022
@ghost ghost added the Synapse label Jun 8, 2022
@yonzhan
Copy link
Copy Markdown
Collaborator

yonzhan commented Jun 8, 2022

Graph

@jiasli jiasli merged commit 14f3905 into Azure:dev Jun 9, 2022
@jiasli jiasli deleted the graph-replacer branch June 9, 2022 09:41
@jiasli jiasli changed the title {Graph} Remove unnecessary logic from MSGraphClientPasswordReplacer {AD} Remove unnecessary logic from MSGraphClientPasswordReplacer Jun 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jsntcy jsntcy jsntcy approved these changes

@wangzelin007 wangzelin007 wangzelin007 approved these changes

@kairu-ms kairu-ms Awaiting requested review from kairu-ms

@yonzhan yonzhan Awaiting requested review from yonzhan yonzhan is a code owner

@evelyn-ys evelyn-ys Awaiting requested review from evelyn-ys

@calvinhzy calvinhzy Awaiting requested review from calvinhzy

Assignees

@jiasli jiasli

@jsntcy jsntcy

Labels

Auto-Assign Auto assign by bot Graph (doesn't work with label-triggered comments; use Graph.Microsoft instead) az ad Synapse

Projects

None yet

Milestone

Jun 2022 (2022-07-05)

Development

Successfully merging this pull request may close these issues.

4 participants

@jiasli @yonzhan @jsntcy @wangzelin007