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

finagle (fix): Properly set http method and path parameters in convertToFinagleRequest #3179

Merged
merged 2 commits into from
Aug 31, 2023

Conversation

yuokada
Copy link
Contributor

@yuokada yuokada commented Aug 31, 2023

Overview

  • Currently, convertToFinagleRequest doesn't respect Method, request path and etc

@github-actions github-actions bot added the bug label Aug 31, 2023
@codecov
Copy link

codecov bot commented Aug 31, 2023

Codecov Report

Merging #3179 (e6c96c7) into main (a6ec410) will increase coverage by 0.01%.
Report is 1 commits behind head on main.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3179      +/-   ##
==========================================
+ Coverage   82.88%   82.89%   +0.01%     
==========================================
  Files         350      350              
  Lines       14743    14745       +2     
  Branches     2447     2401      -46     
==========================================
+ Hits        12219    12223       +4     
+ Misses       2524     2522       -2     
Files Changed Coverage Δ
...in/scala/wvlet/airframe/http/finagle/package.scala 85.29% <100.00%> (+2.21%) ⬆️

... and 2 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 a6ec410...e6c96c7. Read the comment docs.

@yuokada yuokada force-pushed the fix-converter branch 2 times, most recently from bac9203 to 23f300e Compare August 31, 2023 10:32
@yuokada yuokada marked this pull request as ready for review August 31, 2023 10:33
@yuokada yuokada requested a review from xerial August 31, 2023 10:40
val response = convertToFinagleRequest(req)

response.method.name shouldBe "GET"
response.path shouldBe "/foo"
Copy link
Member

@xerial xerial Aug 31, 2023

Choose a reason for hiding this comment

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

I thought the path can contain the query string ?bar=true&baz=1234. Does it need to be set as params?

Copy link
Contributor Author

@yuokada yuokada Sep 1, 2023

Choose a reason for hiding this comment

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

Does it need to be set as params?

IIRC, there is a way to set a path with query parameters. If the way to get a path with a query string exists, I would use a simpler way.
Considering the duplication of parameters, value encoding issues and etc, I adopted the above way.


In my understanding, path doesn't contain a query string.
And, according to the wikipedia, behavior of the uri parameter in the case class, Request , is different with the definition of URI in the wikipedia.
I was actually confused with it before.

case class Request(
method: String = HttpMethod.GET,
// Path and query string beginning from "/"
uri: String = "/",
header: HttpMultiMap = HttpMultiMap.empty,
message: Message = EmptyMessage,
remoteAddress: Option[ServerAddress] = None
) extends HttpMessage[Request] {

@xerial xerial changed the title Fix convertToFinagleRequest finagle (fix): convertToFinagleRequest should pass Method, Request Path parameters Aug 31, 2023
@xerial xerial changed the title finagle (fix): convertToFinagleRequest should pass Method, Request Path parameters finagle (fix): convertToFinagleRequest failed to set Method, Request Path parameters Aug 31, 2023
@xerial xerial changed the title finagle (fix): convertToFinagleRequest failed to set Method, Request Path parameters finagle (fix): Properly set http method and path parameters in convertToFinagleRequest Aug 31, 2023
@xerial xerial merged commit 7033637 into wvlet:main Aug 31, 2023
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