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

Client Request/Response Modifications #3601

Conversation

ChristopherDavenport
Copy link
Member

@ChristopherDavenport ChristopherDavenport commented Jul 25, 2020

  • Connection Keep-Alive added if not present to request
  • Date add if not present to request
  • Default User-Agent added, and added to request if not present along with builder options
  • Added Tests For the above and for response cancelation. We have an intriguing position that streams are scoped as closing before the last element. I wonder what else this could cause elsewhere as well.

@ChristopherDavenport ChristopherDavenport changed the base branch from master to series/0.21 Jul 25, 2020
rossabaker
rossabaker previously approved these changes Aug 6, 2020
def withUserAgent(userAgent: `User-Agent`) =
copy(userAgent = userAgent.some)
def withoutUserAgent =
copy(userAgent = None)
Copy link
Member

@rossabaker rossabaker Aug 6, 2020

Choose a reason for hiding this comment

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

This feature feels like a potential new middleware, though I think it's also good here, since a reasonable default depends on the backend.

@@ -27,7 +27,8 @@ private[ember] object Parser {
case None =>
Pull.raiseError[F](
EmberException.ParseError(
s"Incomplete Header received (sz = ${buff.size}): ${buff.decodeUtf8}"))
s"""Incomplete Header received (sz = ${buff.size}): utf8: ${buff.decodeUtf8} - b64: "${buff.toBase64}"""")
Copy link
Member

@rossabaker rossabaker Aug 6, 2020

Choose a reason for hiding this comment

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

This never gets into a response, correct?

The decodeUtf8 could fail as well, and I don't see a reason to treat it as UTF-8.

Copy link
Member Author

@ChristopherDavenport ChristopherDavenport Aug 7, 2020

Choose a reason for hiding this comment

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

The buff is a bytevector so if this fails its just a left. But if it is. It makes it much easier to debug.

Copy link
Member

@rossabaker rossabaker Aug 7, 2020

Choose a reason for hiding this comment

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

Oh, right, that's an Either. Yeah, that's fine.

I just wanted to verify that this ParseError message doesn't get rendered into a 400, as it can reflect bytes from a request.

@rossabaker rossabaker added this to the 0.21.7 milestone Aug 7, 2020
Copy link
Member

@rossabaker rossabaker left a comment

I don't know why I thought all these ember failures were formatting issues.

Copy link
Member

@rossabaker rossabaker left a comment

Fixed the MiMa errors.

@rossabaker rossabaker merged commit 0744001 into http4s:series/0.21 Aug 8, 2020
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.

None yet

2 participants