Change CORSConfig to builder pattern

[qwbarch]

Fixes #1717. I wasn't sure how to use Method's generics so I left it as a String for now.

[hamnis]

Hello, and thanks for your contribution.

Needs a sbt scalafmtAll

I would expect a builder to have a build, method or something that produces the final result. Since this builder is immutable, i guess that is not really a problem.

Now we just really have the same as we did, but not able to do equality checks and other useful operations.

Perhaps we should keep the old type around, but make it a non-case class, but with equals, hashCode and toString implemented. Not sure.

[qwbarch]

I wasn't sure what to do with allowedOrigins: String => Boolean, so I left it out for equals, and hashCode.
How do I go about checking for equality and calculating a function's hash?

[rossabaker]

Yeah, we call this "the builder pattern", but maybe "the contraband pattern" is more accurate. I don't think it needs build or result method. We're just trying to promote binary compatibility moving forward.

Function equality is hard. We'd like two functions A => B to be equal if and only if for each A the two functions each return the same B. But our A is String, with infinite inhabitants, so we can't possibly test this! The default definition is reference equality, and it's really the best we can do. I'd just use == and ##. The alternative is to redesign that function into some more constrained AST, but that's not really worth it to recover equality.

[rossabaker]

We should cherry-pick this to series/0.22, if it's ready before we release it.

[qwbarch]

I cherry-picked my latest commit and brought it over to series/0.22, and then changed this PR's base to that branch as well.
This doesn't look quite right though, did I do it wrong?

[qwbarch]

Whoops, I just realized this isn't even merged properly when I was cherry-picking. Going to revert it and be more careful next time

[qwbarch]

I've also realized I can't switch the source branch, only the target branch (which I don't think makes a difference now that I think about it).
I've switched the target branch back to main for now since it brought in a ton of participants, I don't want to end up giving everyone a notification email 😅

[rossabaker]

Don't worry about the targeting. I can cherry-pick it so it shows up as you and gets to the right place. 😄

2.12 compilation issue

[rossabaker]

Looks like that Objects.hash doesn't work on primitives on Scala 2.12 due to an implicit conversion.

I can dig up an example of MurmurHash3 if we want to do it the way it's generated for Scala case classes. Or there are a few examples in our codebase.

[qwbarch]

Ah that's unfortunate. I'll look into MurmurHash3 from the other examples

[qwbarch]

Looks like that Objects.hash doesn't work on primitives on Scala 2.12 due to an implicit conversion.

I'm curious though, where does the error show up? I want to avoid running into these issues myself but there's no compiler error nor is there a failed test.

Edit: Nevermind I think I know what you meant, since there's an implicit conversion, an incorrect hash is generated I think right?

[rossabaker]

since there's an implicit conversion, an incorrect hash is generated I think right?

It hit this error in Scala 2.12. Something that was fine in 2.13 and 3:

[error] /home/runner/work/http4s/http4s/server/src/main/scala/org/http4s/server/middleware/CORS.scala:102:7: the result type of an implicit conversion must be more specific than Object
[error]       anyOrigin,
[error]       ^
[error] /home/runner/work/http4s/http4s/server/src/main/scala/org/http4s/server/middleware/CORS.scala:103:7: the result type of an implicit conversion must be more specific than Object
[error]       allowCredentials,
[error]       ^
[error] /home/runner/work/http4s/http4s/server/src/main/scala/org/http4s/server/middleware/CORS.scala:105:7: the result type of an implicit conversion must be more specific than Object
[error]       anyMethod,
[error]       

[rossabaker]

LGTM. I'm going to cherry-pick it all back to series/0.22, because there's no CE work to backport.

[qwbarch]

Thanks! Just wondering since I didn't see anything about this on the contribution guide, is there a limit to how many one can work on Good First Issue labeled issues?
The other issues are a bit advanced for my current skill-level so I'm looking to work on some easier ones until I'm more familiar with the library

[rossabaker]

Nope, absolutely no limit! Just please leave a comment when you start to look at one, so we know we're not duplicating effort. And thanks for the great work on this one!

[qwbarch]

Good to hear, thanks for helping me out with the issue. Looking forward to working on other issues!