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

Add support for binary serialization/deserialization for HttpRecorder #4378

Closed
wants to merge 2 commits into from

Conversation

cthrash
Copy link
Contributor

@cthrash cthrash commented May 29, 2018

The HttpRecorder did not correctly play back binary content. The propsed fix is to introduce
new fields in RecordEntry named IsRequestBodyBinary/IsResponseBodyBinary, and use base64
encoding/decoding when the payload is deemed to be binary. Currently the test for binary-ness
is a simple MIME type regex match.

Note that this PR includes tests to confirm the new behavior. It will failed to build without
hacking around the fact that the test code depends on the NuGet-installed Microsoft.Azure.Test.HttpRecorder.dll.
If it is preferable to split the PR so you can update NuGet first without the tests failing, please
let me know.

This fix is required for some upcoming additions to CognitiveService/ComputerVision tests.

Description


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

General Guidelines

  • Title of the pull request is clear and informative.
  • There are a small number of commits, each of which have an informative message. This means that previously merged commits do not appear in the history of the PR. For more information on cleaning up the commits in your PR, see this page.

Testing Guidelines

  • Pull request includes test coverage for the included changes.

SDK Generation Guidelines

  • If an SDK is being regenerated based on a new swagger spec, a link to the pull request containing these swagger spec changes has been included above.
  • The generate.cmd file for the SDK has been updated with the version of AutoRest, as well as the commitid of your swagger spec or link to the swagger spec, used to generate the code.
  • The *.csproj and AssemblyInfo.cs files have been updated with the new version of the SDK.

The HttpRecorder did not correctly play back binary content.  The propsed fix is to introduce
new fields in `RecordEntry` named `IsRequestBodyBinary`/`IsResponseBodyBinary`, and use base64
encoding/decoding when the payload is deemed to be binary.  Currently the test for binary-ness
is a simple MIME type regex match.

Note that this PR includes tests to confirm the new behavior.  It will failed to build without
hacking around the fact that the test code depends on the NuGet-installed Microsoft.Azure.Test.HttpRecorder.dll.
If it is preferable to split the PR so you can update NuGet first without the tests failing, please
let me know.
Copy link
Member

@shahabhijeet shahabhijeet left a comment

Choose a reason for hiding this comment

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

@cthrash thank you for your contribution.
How soon do you need this change.
Apart from just getting this PR merged with CI passed, we would be taking this change and testing it against the entire repo.
We are still working on updating our CI/CD pipeline so that changes like these can be tested easily via a PR, but we are not their yet.
Here are the steps you will need to roll out this change:

  1. Get a private signed binary for HttpRecorder
  2. Update the entire repo to start using the new recorder (this will also include your newly written tests)
  3. Have the private signed copy part of a private feed that can be used during CI
  4. Once the PR is successfully, we can merge that PR.
  5. Release the new version of HttpRecorder to nuget.
    OR
    We can do this in our next sprint (as we have started planning for it)
    What is your timeline when you need this change available for your scenarios.

@cthrash
Copy link
Contributor Author

cthrash commented May 30, 2018

If you can take over the process, that'd be easiest for me. For now I will disable to the test that breaks with the existing HttpRecorder.
You mentioned a sprint - roughly how soon can I expect a fix?

@shahabhijeet
Copy link
Member

@cthrash if this is something that blocks you, we can have this in our next sprint that starts next week.
So let me know how critical is this for your scenario.

@cthrash
Copy link
Contributor Author

cthrash commented May 31, 2018

It doesn't block me. Is there an existing issue so I can track?

@shahabhijeet
Copy link
Member

@cthrash added an issue to track it

@shahabhijeet
Copy link
Member

@cthrash closing this PR as this task is being tracked with an issue linked above.

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.

2 participants