Skip to content

Conversation

ioannakok
Copy link
Contributor

@ioannakok ioannakok commented Jan 10, 2024

What does this change?

Upgrades scala version to 2.13.12.

What is the value of this and can you measure success?

  • Keeping up-to-date with the latest Scala 2.13 version
  • Addresses "Implicit definition should have explicit type" warnings which in our case become errors because we have -Xfatal-warnings flag enabled. This also helps to ease migration to Scala 3.
  • Unblocks:

Screenshots

Before After
image image

Instead of crashing the compiler is producing a compilation error which we can now work on fixing in #26783

Checklist

@ioannakok ioannakok changed the title Scala 2.13 latest Upgrade to Scala 2.13.12 Jan 10, 2024
This commit also fixes "Implicit definition should have explicit type" errors. These are warnings to ease migration to Scala 3 but we have `-Xfatal-warnings` enabled in our `scalacOptions` in `ProjectSettings.scala`.
It looks like in Scala 2.13.12 the Map.get method becomes case sensitive.
@@ -31,7 +31,7 @@ case class RequestLoggerFields(request: RequestHeader, response: Option[Result],

val allowListedHeaders = (for {
headerName <- allowListedHeaderNames
value <- allHeadersFields.get(headerName)
value <- allHeadersFields.map { case (key, value) => (key.toLowerCase, value) }.get(headerName.toLowerCase)
Copy link
Contributor Author

@ioannakok ioannakok Jan 10, 2024

Choose a reason for hiding this comment

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

It looks like in Scala 2.13.12 the get method of a Map becomes case sensitive, so I had to add this to fix this test that was failing in this line

Copy link
Contributor

Choose a reason for hiding this comment

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

I was looking into that because this seemed a bit strange to me, to have that code failing solely by upgrading to Scala 2.13.12. (Sorry for the long comment, I just wanted to know myself what was going on here)

TLDR: Turns out this is not a bug in Scala and also they did not really change expected behaviour (tough they did change some internals which causes your code to fail). The actual problem with this code was that you assumed that map.get here is case insensitive, which isn't defined somewhere and you can not expect that.

If you look at the code:

val allHeadersFields = request.headers.toMap.mapV(_.mkString(","))
val allowListedHeaders = (for {
headerName <- allowListedHeaderNames
value <- allHeadersFields.get(headerName)
} yield headerName -> value).toMap

allHeadersFields is a Map, not a SortedMap or anything concrete, but a "stupid" Map. Calling map.get usually checks for equality to retrieve an entry. Since you do not know which concrete Map implemention is used to back allHeadersFields, you can not assume that retrieving elements is case sensitive, since you don't know how elements are compared internally, the Map does not guarantee anything here.

The reason your code worked before was pure coincidence. Calling toMap

request.headers.toMap

here again just gives you a "stupid" map. Turns out, in Play this gives you a TreeMap implemention with a CaseInsensitiveOrdered ordering, see here:

val builder = TreeMap.newBuilder[String, Seq[String]](CaseInsensitiveOrdered)

Now, calling mapV on this method "transforms" this map, you can see that here:

  def mapV[U](f: map.V => U): Map[map.K, U] = map(coll).toMap.transform { case (_,v) => f(v) }

The toMap call here (which basically is toMap on the given map) in that line is calling Scala's internal code which in Scala 2.13.11 would return just the same instance that was given already, meaning it just returns the exact same map with the internal CaseInsensitiveOrdered ordering.
In Scala 2.13.12 however they changed some internals (because of other reasons) you get back a new instance of Map which however does not have the same ordering anymore. So the case-insensitive ordering from Play's internal code was lost and that is why your code breaks. Actually this is totally ok, because toMap does not make any promises which map it returns, not does it make promises on ordering.

The internal code they changed in Scala can be seen here, it's the from method:
In Scala 2.13.11 the matching case was:

case m: Map[K, V] => m

which just returns the same instance again (which was the same TreeMap with the ordering)
In Scala 2.13.12 however that case was removed and the code falls back to:

case _ => newBuilder[K, V].addAll(it).result()

which creates a new map, but as you can see the newBuilder does not get an ordering passed (turns out it then returns an Map2).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the comment! This makes sense

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cc @rtyley I think you will be interested in this!

@ioannakok ioannakok mentioned this pull request Jan 11, 2024
7 tasks
@ioannakok ioannakok marked this pull request as ready for review January 11, 2024 11:31
@ioannakok ioannakok requested review from a team as code owners January 11, 2024 11:31
@@ -32,7 +32,7 @@ object ProjectSettings {
Compile / packageDoc / publishArtifact := false,
Compile / doc / sources := Seq.empty,
Compile / doc := target.map(_ / "none").value,
scalaVersion := "2.13.10",
scalaVersion := "2.13.12",
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks fine!

@prout-bot
Copy link

Seen on ADMIN-PROD (merged by @ioannakok 12 minutes and 35 seconds ago)

@prout-bot
Copy link

Seen on FRONTS-PROD (merged by @ioannakok 14 minutes and 23 seconds ago)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants