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

Fixes #3328 Order[ContentCoding] should depend on compareToIgnoreCase #3501

Merged
merged 1 commit into from Jun 24, 2020
Merged

Fixes #3328 Order[ContentCoding] should depend on compareToIgnoreCase #3501

merged 1 commit into from Jun 24, 2020

Conversation

ashwinbhaskar
Copy link
Collaborator

@ashwinbhaskar ashwinbhaskar commented Jun 14, 2020

No description provided.

Order.from {
case (a, b) if a.coding.compareToIgnoreCase(b.coding) == 0 => a.qValue.compare(b.qValue)
case (a, b) => a.coding.compareToIgnoreCase(b.coding)
}
Copy link
Member

@rossabaker rossabaker Jun 16, 2020

Choose a reason for hiding this comment

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

I think this is right (I haven't rerun the test with the seed), but you could avoid repeating the comparison when codings aren't equal. I think this is also right:

Suggested change
Order.from {
case (a, b) if a.coding.compareToIgnoreCase(b.coding) == 0 => a.qValue.compare(b.qValue)
case (a, b) => a.coding.compareToIgnoreCase(b.coding)
}
val cmp = a.coding.compareToIgnoreCase(b.coding)
if (cmp == 0) a.qValue.compare(b.qValue) else cmp

Copy link
Collaborator Author

@ashwinbhaskar ashwinbhaskar Jun 17, 2020

Choose a reason for hiding this comment

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

@rossabaker this does not work. The test fails with that seed value. Will change the PR to WIP and investigate.

Copy link
Member

@rossabaker rossabaker Jun 17, 2020

Choose a reason for hiding this comment

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

Nuts. Maybe I misanalyzed it on the ticket. If we can figure out the exact inputs to that failing case, and not just the .toString representation, it should be much easier to spot that error.

Copy link
Member

@rossabaker rossabaker Jun 20, 2020

Choose a reason for hiding this comment

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

I opened a PR after investigating deeper myself, but after that, I think this might still be correct. We just don't have an example where it matters, because characters are restricted to token characters, none of which do weird things in case folding.

@ashwinbhaskar ashwinbhaskar changed the title Fixes #3328 Order[ContentCoding] should depend on compareToIgnoreCase WIP: Fixes #3328 Order[ContentCoding] should depend on compareToIgnoreCase Jun 17, 2020
@@ -114,7 +115,8 @@ object ContentCoding {
}

implicit val http4sOrderForContentCoding: Order[ContentCoding] =
Order.by(c => (c.coding.toLowerCase, c.qValue))
Order.by(c => (c.coding.toLowerCase(ju.Locale.ENGLISH), c.qValue))
Copy link
Member

@rossabaker rossabaker Jun 17, 2020

Choose a reason for hiding this comment

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

Hmm. Maybe. compareToIgnoreCase is character-by-character, like equalIgnoreCase, so I'm having a hard time conceptualizing why that wouldn't work. toLowerCase can change the lengths of the strings, and I fear is open to more corner cases. And the default locale on my machine is en_US anyway.

If we're going to use .toLowerCase, I agree with this change, to not rely on the default locale. It still doesn't feel right, but I don't have a counterexample.

Copy link
Collaborator Author

@ashwinbhaskar ashwinbhaskar Jun 17, 2020

Choose a reason for hiding this comment

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

@ashwinbhaskar ashwinbhaskar changed the title WIP: Fixes #3328 Order[ContentCoding] should depend on compareToIgnoreCase Fixes #3328 Order[ContentCoding] should depend on compareToIgnoreCase Jun 22, 2020
Copy link
Member

@rossabaker rossabaker left a comment

I would still use compareToIgnoreCase, but shouldn't matter for the legal codings, isn't significantly more performant at the length of codings we'll see in practice, and will be refactored to a CIString in 1.0. As is, this protects us against default locales that none of us have heard of.

@rossabaker rossabaker added this to the 0.21.5 milestone Jun 23, 2020
hamnis
hamnis approved these changes Jun 24, 2020
@rossabaker rossabaker merged commit 4bec3fa into http4s:series/0.21 Jun 24, 2020
10 checks passed
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