Skip to content

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

Merged
xerial merged 11 commits intowvlet:masterfrom
NicolasRichard:fix_recorder
May 25, 2023
Merged

fix: Various fixes to make http-recorder usable again. #2983
xerial merged 11 commits intowvlet:masterfrom
NicolasRichard:fix_recorder

Conversation

@NicolasRichard
Copy link
Copy Markdown
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
Copy Markdown

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
Copy Markdown
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
Copy Markdown
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

def nonEmpty: Boolean = !isEmpty
def toContentString: String
def toContentBytes: Array[Byte]
def contentHash: Int = toContentBytes.hashCode()
Copy link
Copy Markdown
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