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

fix: Various fixes to make http-recorder usable again. #2983

Merged
merged 11 commits into from
May 25, 2023

Conversation

NicolasRichard
Copy link
Collaborator

As it stands, http-recorder only works with GET requests without a body. This situation is caused by two issues:

  • The hash code of the body is not computed correctly. Arrays.hashcode should be used to compute an hash code over the content of the array as the hashcode method on the array itself only relies on the address of the object.
  • RedirectToRxEndpoint only exposes a GET endpoint. No other verbs can pass-through. I don't know if there's a better way to fix it, but the quick fix I'm proposing works.

@github-actions github-actions bot added the bug label May 25, 2023
@codecov
Copy link

codecov bot commented May 25, 2023

Codecov Report

Merging #2983 (52a6532) into master (5e2919b) will decrease coverage by 0.04%.
The diff coverage is 11.11%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2983      +/-   ##
==========================================
- Coverage   82.85%   82.81%   -0.04%     
==========================================
  Files         348      348              
  Lines       14591    14604      +13     
  Branches     2382     2381       -1     
==========================================
+ Hits        12089    12095       +6     
- Misses       2502     2509       +7     
Impacted Files Coverage Δ
...et/airframe/http/router/RedirectToRxEndpoint.scala 0.00% <0.00%> (ø)
...c/main/scala/wvlet/airframe/http/HttpMessage.scala 87.50% <100.00%> (ø)

... and 6 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5e2919b...52a6532. Read the comment docs.

@@ -22,8 +22,31 @@ import wvlet.log.LogSupport
* @param endpoint
*/
class RedirectToRxEndpoint(endpoint: RxHttpEndpoint) extends LogSupport {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@xerial Any potential side-effects? Any probability something would be relying on the method name or the fact that it only has a GET endpoint?

Copy link
Member

Choose a reason for hiding this comment

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

This one is added only for providing an adapter for http-recorder, so this fix is ok. Thanks

@@ -141,7 +142,7 @@ object HttpMessage {
def nonEmpty: Boolean = !isEmpty
def toContentString: String
def toContentBytes: Array[Byte]
def contentHash: Int = toContentBytes.hashCode()
Copy link
Member

Choose a reason for hiding this comment

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

😮 Nice catch

@xerial xerial merged commit aacb665 into wvlet:master May 25, 2023
@NicolasRichard NicolasRichard deleted the fix_recorder branch May 25, 2023 19:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants