httpclient/jackson/http - Cross build for Scala 2.13

[chrisbenincasa]

Problem

httpclient, jackson, and http modules don't build for 2.13

Solution

Cross build these modules.

Notes on changes

The commit for the jackson module brings along some changes necessary for convert Scala 2.12 usages of Seq to Scala 2.13 compliant usages, as detailed here. There are performance considerations for these changes, as detailed in https://github.com/twitter/finagle/issues/818.

For now, the most important aspect here is ensuring that these changes don't regress 2.12 performance. For jackson, I've run the JsonBenchmark against 2.12.11 with and without the changes I've made here. There doesn't seem to be an impact in performance with the changes. The results are here: https://gist.github.com/chrisbenincasa/ff654689fb79ca6443e3b99900a3a824

---

The commit for http contains some of the Seq changes, but not on high volume code paths, from what I can tell.

I had to remove the mocking from RequestUtilsTest.scala, because Mockito was throwing errors when mocking HeaderMap due to some Map methods being made final in 2.13.

I also introduced 2.13+ and 2.13- directory support for the build in this commit.

Result

httpclient, jackson, and http modules are built for 2.13.

[cacoco]

@chrisbenincasa is mocking still a concern with the update to Mockito 3.x and mockito-scala (via util-mock)?

[chrisbenincasa]

@cacoco yeah that test is using the mixin you mentioned (and new versions of Mockito from what I can tell) but I don't think that changes anything w/r/t mocking HeaderMap here.

Here's what Mockito gives me:

[info]   Cause: org.mockito.exceptions.base.MockitoException: Mockito cannot mock this class: class com.twitter.finagle.http.HeaderMap.
[info]
[info] Mockito can only mock non-private & non-final classes.
[info] If you're not sure why you're getting this error, please report to the mailing list.
[info]

and further down:

[info]   Cause: java.lang.VerifyError: class com.twitter.finagle.http.HeaderMap$MockitoMock$686332027 overrides final method $plus$eq.(Ljava/lang/Object;)Lscala/collection/mutable/Growable;
[info]   at java.lang.ClassLoader.defineClass1(Native Method)

So I think this is solely related to 2.13 marking methods like += to be final.

[cacoco]

@chrisbenincasa hmm, is the issue with the ExceptionMapperCollection: https://github.com/twitter/finatra/pull/554/files#diff-a4e23f17bf4ae13f9449d92df45f730e404fd770690af415421ce722ca75fdbd? We may be able to rewrite this. I'd like to push to not have to have customized 2.13 directories if we don't have to, not if we can rewrite the code in such a way to avoid the issue.

[chrisbenincasa]

Sorry for the delay in response.

@cacoco I don't think the Mockito issue is related to ExceptionMapperCollection - where do you see that?

In terms of version-specific directories... I'm not sure there's a way around that if we wish to have ExceptionMapperCollection extend Scala's collection types. Scala 2.13 marks Traversable as deprecated and Iterable is at the top trait of the collection hierarchy. Perhaps we could change the existing implementation to extend Iterable instead, but it's unclear to me whether that's a breaking change or not.

[cacoco]

Sorry for the delay in response.

@cacoco I don't think the Mockito issue is related to ExceptionMapperCollection - where do you see that?

In terms of version-specific directories... I'm not sure there's a way around that if we wish to have ExceptionMapperCollection extend Scala's collection types. Scala 2.13 marks Traversable as deprecated and Iterable is at the top trait of the collection hierarchy. Perhaps we could change the existing implementation to extend Iterable instead, but it's unclear to me whether that's a breaking change or not.

I think our preference would be to rewrite the ExceptionMapperCollection. I don't think it'd be a breaking change and we need to move to Iterable anyway here. I asked about ExceptionMapperCollection because it was one file that got moved to the specific 2.13 directory.

[cacoco]

For the Mockito issue:

 Mockito can only mock non-private & non-final classes.

I think that's different and not necessarily 2.13 related. Did you see that with JDK11?

[chrisbenincasa]

@yufangong I've removed the forked directories in favor of just reimplementing ExceptionMapperCollection to conform to Iterable, instead of Traversable, per @cacoco's suggestion. This seems to work.

@cacoco I've tried with jdk8 and jdk11. It passes consistently with 2.12.11 and fails consistently on 2.13.1 for me. I get the same output on jdk11:

[info] - throw BadRequestException when missing host header for pathUrl *** FAILED ***
[info]   org.mockito.exceptions.base.MockitoException: Mockito cannot mock this class: class com.twitter.finagle.http.HeaderMap.
[info]
[info] Mockito can only mock non-private & non-final classes.
[info] If you're not sure why you're getting this error, please report to the mailing list.
[info]
[info]
[info] Java               : 11
[info] JVM vendor name    : Oracle Corporation
[info] JVM vendor version : 11.0.8+11
[info] JVM name           : OpenJDK 64-Bit Server VM
[info] JVM version        : 11.0.8+11
[info] JVM info           : mixed mode
[info] OS name            : Mac OS X
[info] OS version         : 10.15.7
[info]
[info]

with the cause:

[info]   Cause: java.lang.VerifyError: class com.twitter.finagle.http.HeaderMap$MockitoMock$1691613127 overrides final method com.twitter.finagle.http.HeaderMap.$plus$eq(Ljava/lang/Object;)Lscala/collection/mutable/Growable;

I believe this is distinctly Scala 2.13 related due to the += method changing to final from 2.12 -> 2.13. This is seen in Finagle as well, as they had to fork the HeaderMap into version specific interfaces in order to conform to that change: https://github.com/twitter/finagle/blob/develop/finagle-base-http/src/main/scala-2.12-/com/twitter/finagle/http/headers/HeaderMapVersionSpecific.scala#L8

[cacoco]

@chrisbenincasa I see, ok. It is what it is then. Thanks for the work here.

[codecov[bot]]

Codecov Report

Merging #554 into develop will increase coverage by 0.00%. The diff coverage is 92.30%.

@@           Coverage Diff            @@
##           develop     #554   +/-   ##
========================================
  Coverage    91.13%   91.14%           
========================================
  Files          269      269           
  Lines         4872     4877    +5     
  Branches       303      314   +11     
========================================
+ Hits          4440     4445    +5     
  Misses         432      432           

Impacted Files	Coverage Δ
...ra/http/exceptions/ExceptionMapperCollection.scala	80.00% <0.00%> (ø)
...nal/marshalling/DefaultMessageBodyWriterImpl.scala	90.90% <ø> (ø)
...atra/http/internal/routing/CallbackConverter.scala	95.27% <100.00%> (+0.11%)	:arrow_up:
.../com/twitter/finatra/http/routing/HttpRouter.scala	94.31% <100.00%> (+0.06%)	:arrow_up:
.../com/twitter/finatra/http/routing/HttpWarmup.scala	92.30% <100.00%> (ø)
...om/twitter/finatra/jackson/ScalaObjectMapper.scala	100.00% <100.00%> (ø)
...atra/jackson/caseclass/CaseClassDeserializer.scala	95.03% <100.00%> (ø)
...er/finatra/jackson/streaming/AsyncJsonParser.scala	98.14% <100.00%> (+0.03%)	:arrow_up:

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update b95b846...646ce88. Read the comment docs.

[yufangong]

@chrisbenincasa just a heads up, we are working on some canary and perf testing internally, the merge can be delayed a little bit. Thanks!

[chrisbenincasa]

@yufangong Thanks for the heads up and no problem! There seems to be a newly failing test in jackson on 2.13 that I was looking into briefly as well. Seems to be related to the ScalaObjectMapper.

This is the failure, if anybody has any inklings:

[info] ScalaObjectMapperTest:
JSON DIFF FAILED!
Received:
 [
  "month: null",
  "weekday: null"
]

Expected:
 [
  "month: key not found: feb",
  "weekday: key not found: sat"
]

                   *
Received: ["month: null","weekday: null"]
Expected: ["month: key not found: feb","weekday: key not found: sat"]
[info] - Scala enums *** FAILED ***
[info]   Json diff failed:
[info]                      *
[info]   Received: ["month: null","weekday: null"]
[info]   Expected: ["month: key not found: feb","weekday: key not found: sat"] (Option.scala:242)

[yufangong]

@yufangong Thanks for the heads up and no problem! There seems to be a newly failing test in jackson on 2.13 that I was looking into briefly as well. Seems to be related to the ScalaObjectMapper.

This is the failure, if anybody has any inklings:

[info] ScalaObjectMapperTest:
JSON DIFF FAILED!
Received:
 [
  "month: null",
  "weekday: null"
]

Expected:
 [
  "month: key not found: feb",
  "weekday: key not found: sat"
]

                   *
Received: ["month: null","weekday: null"]
Expected: ["month: key not found: feb","weekday: key not found: sat"]
[info] - Scala enums *** FAILED ***
[info]   Json diff failed:
[info]                      *
[info]   Received: ["month: null","weekday: null"]
[info]   Expected: ["month: key not found: feb","weekday: key not found: sat"] (Option.scala:242)

I believe this is because of the migration of MapLike: https://www.scala-lang.org/api/2.12.8/scala/collection/mutable/MapLike.html#default(key:K):V

  /** Defines the default value computation for the map,
   *  returned when a key is not found
   *  The method implemented here throws an exception,
   *  but it might be overridden in subclasses.
   *
   *  @param key the given key value for which a binding is missing.
   *  @throws NoSuchElementException
   */
  def default(key: K): V =
    throw new NoSuchElementException("key not found: " + key)

The NoSuchElementException is caught in the case class deserializer without this message in 2.13, this should only have message mismatch, I will alter the message to make the test pass for both 2.12 and 2.13. ty for reporting!