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

{AD} Remove unnecessary logic from MSGraphClientPasswordReplacer #22783

Merged
merged 1 commit into from
Jun 9, 2022

Conversation

jiasli
Copy link
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:

# 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:

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:

# 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 az ad label Jun 8, 2022
@ghost ghost requested review from evelyn-ys and calvinhzy June 8, 2022 10:58
@ghost ghost assigned jsntcy Jun 8, 2022
@ghost ghost added the Synapse label Jun 8, 2022
@yonzhan
Copy link
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
Labels
Auto-Assign Auto assign by bot Graph az ad Synapse
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants