Skip to content

Allow extracting recorded requests #29

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.

Sign up for GitHub

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

Open
pwalters wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Allow extracting recorded requests #29

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a4233e2
Add better debug descriptions for request/response objects
pwalters Sep 18, 2019
d4551da
Fix hash method to consider the httpBodyStreamData
pwalters Sep 18, 2019
ccbf277
Add support for extracting recorded requests
pwalters Sep 18, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Fix hash method to consider the httpBodyStreamData
  • Loading branch information
@pwalters
pwalters committed Sep 18, 2019
commit d4551dae54bd62119fcd6fd40a5c11b6efb74939
4 changes: 4 additions & 0 deletions MockDuck/Sources/MockRequestResponse.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,10 @@ final class MockRequestResponse: Codable, CustomDebugStringConvertible {
hashData.append(body)
}

if let bodyData = normalizedRequest.httpBodyStreamData {
Copy link
Contributor

@scelis scelis Sep 25, 2019

Choose a reason for hiding this comment

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

Will this break any existing recordings?

Copy link
Contributor

@abbasmousavi abbasmousavi Oct 2, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

This will change the hash for the requests that have something in the .httpBodyStream and will break the recordings. We can tag and release a version for users that have some recordings already and then merge this and release a 2.0 version.

Copy link
Contributor

@abbasmousavi abbasmousavi Oct 2, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

The better approach in my opinion is adding a delegate method that lets user compose the hash for each request based on their needs. We can keep the current hashing method as default to prevent broken recordings and we can expose a delegate so that people can compose the hash for each request based on their project needs. What do you think @scelis? If you agree with this approach we can merge this PR without the hashing change and I can open another PR to expose that delegate method.

Copy link
Contributor

@scelis scelis Oct 4, 2019

Choose a reason for hiding this comment

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

Instead of bumping the version, what do you think about calculating multiple hashes for backwards compatibility? When loading, if the new/updated hash doesn't match anything on disk calculate the backwards-compatible hash and check for that file, too.

Copy link
Contributor

@abbasmousavi abbasmousavi Oct 4, 2019

Choose a reason for hiding this comment

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

My suggestion, as I explained in another comment, is keeping the hashing code as is, so don’t breaking the previous versions, and expose an optional delegate method for people to calculate hash based on their needs because I think each project has different requirements here. For one project the headers are not important for the other one the body of the request. Some project need to remove the headers or the body from the hash to get same response on a set of request. It completely depends on the project needs.

Copy link
Contributor

@scelis scelis Oct 5, 2019

Choose a reason for hiding this comment

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

I think that is a decent feature request, but that MockDuck should still provide basic hash-computation functionality that should work for the 95% use case.

hashData.append(bodyData)
}

if !hashData.isEmpty {
return String(CryptoUtils.md5(hashData).prefix(8))
} else {
Expand Down