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

migrate Accept, Content-Type, Link, MediaType, MediaRange to cats-parse #4042

Merged
merged 3 commits into from Dec 26, 2020

Conversation

novakov-alexey-zz
Copy link
Contributor

@novakov-alexey-zz novakov-alexey-zz commented Dec 20, 2020

migrate to cats-parse:

  • Accept
  • Content-Type
  • Link
  • MediaType
  • MediaRange

Some tests fail due to few TODOs:

Part of #3984

@novakov-alexey-zz
Copy link
Contributor Author

novakov-alexey-zz commented Dec 22, 2020

There is one test case failing, I can't figure out why:

[error]   x Do a round trip through the Accept header (102 ms)
[error]    Accept: text/csv; quoteChar="'"; rowDelimiter=";"; escapeChar="\\\\\\\\"; charset="UTF-8"; columnDelimiter=" " != Accept: text/csv; quoteChar="'"; rowDelimiter=";"; escapeChar="\\\\"; charset="UTF-8"; columnDelimiter=" " (MediaTypeSpec.scala:55)
[error] org.http4s.MediaTypeSpec.$anonfun$new$16(MediaTypeSpec.scala:55)
[error] Actual:   ..."\\\\[\\\\]"; ch...
[error] Expected: ..."\\\\[]"; ch...

org.http4s.util.Writer#quote escapes every "\" with one more "\".
Failing test is parsing header from already parsed header, so that we get escapeChar="\\\\\\\\" instead of expected escapeChar="\\\\". @rossabaker do you have an idea why that may happen?

Copy link
Member

@rossabaker rossabaker left a comment

Choose a reason for hiding this comment

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

It looks like the parboiled2 version is returning a decoded quoted string, while the cats-parse version is returning the raw quoted string.

core/src/main/scala/org/http4s/headers/Content-Type.scala Outdated Show resolved Hide resolved
@novakov-alexey-zz
Copy link
Contributor Author

It looks like the parboiled2 version is returning a decoded quoted string, while the cats-parse version is returning the raw quoted string.

We can try to unescape it only for MediaType parser.

@rossabaker
Copy link
Member

I don't think Scala 3.0.0-M1 ever ran on JDK >= 14. We can ignore that error. While we're in flux, it would be nice to turn off the fail fast on CI. We're going to have a hard time getting any results here until we fix that.

Copy link
Member

@rossabaker rossabaker left a comment

Choose a reason for hiding this comment

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

I think I'd probably rather have the quotedString helper take care of unescaping, but I'd need to spend more time making sure they always get interpreted the same way.

Hard to say given the current state of CI on these port branches, but I think this unblocks us? I'm 👍, and it's not going to a prod branch yet, so there's time to clean up if we forgot something.

@novakov-alexey-zz
Copy link
Contributor Author

Yes, I agree that unquoting could be done nicely via separate parser.

@rossabaker rossabaker merged commit a09bbc7 into http4s:dotty Dec 26, 2020
@novakov-alexey-zz novakov-alexey-zz deleted the dotty-alexey-novakov branch January 6, 2021 21:52
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

3 participants