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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: 馃殌 Support parameterized types in ClassTagExtensions #518

Merged
merged 1 commit into from
Jun 3, 2021

Conversation

gaeljw
Copy link
Contributor

@gaeljw gaeljw commented May 19, 2021

Aims to solve #503 by enriching ClassTagExtensions as ScalaObjectMapper is being deprecated/removed for Scala 3.

The idea is to use a home-made typeclass JavaTypeable in combination with ClassTags.
An instance of JavaTypeable being provided for all non-generic types and generic types up to 5 type parameters.

This compiles and pass the tests in ClassTagExtensionsTest.


However I see at least one caveat to this implementation: if using a generic class with more type parameters than "allowed" (3 for now in this MR), the error provided to the user is not very clean:

private case class Uber[A,B,C,D](a: A, b: B, c: C, d: D)

mapper.readValue[Uber[Int, String, Double, Float]](...)

// Gives:
java.lang.IllegalArgumentException: Cannot create TypeBindings for class com.fasterxml.jackson.module.scala.ScalaObjectMapperTest$Uber with 3 type parameters: class expects 4
   at com.fasterxml.jackson.databind.type.TypeBindings.create(TypeBindings.java:113)

This is because a JavaTypeable[Uber[B,C,D]] is provided instead of a more regular "implicit not found" error. I don't know if there's a way around that 馃し

This might be acceptable though if we provide the typeclass for up to 10 or even 22 type parameters as it would occur on a very rare basis?

@pjfanning
Copy link
Member

pjfanning commented May 19, 2021

@gaeljw thanks - would it be possible to retarget this PR to the 2.13 branch? - the master branch is for a future unscheduled release

@pjfanning pjfanning closed this May 19, 2021
@pjfanning pjfanning reopened this May 19, 2021
@pjfanning
Copy link
Member

I'm not sure if this won't break binary compatibility for existing ScalaObjectMapper users. We don't make any promises about binary compatibility in jackson-module-scala v2 but we've recently started doing MIMA checks.

One possibility would be to put the new code in an existing class called ClassTagExtensions - that class is not very useful as is so I'd happily replace it - so users can opt in to using your new code but would have to make an explicit change to their code to use it.

@gaeljw gaeljw force-pushed the scalaobjectmapperforscala3 branch from f7d045f to d06e656 Compare May 19, 2021 16:23
@gaeljw gaeljw changed the base branch from master to 2.13 May 19, 2021 16:25
@gaeljw gaeljw force-pushed the scalaobjectmapperforscala3 branch from d06e656 to a61c163 Compare May 19, 2021 16:31
@gaeljw
Copy link
Contributor Author

gaeljw commented May 19, 2021

would it be possible to retarget this PR to the 2.13 branch? - the master branch is for a future unscheduled release

Done :)

I'm not sure if this won't break binary compatibility for existing ScalaObjectMapper users.

I'm 100% sure it breaks binary compatibility indeed.

One possibility would be to put the new code in an existing class called ClassTagExtensions

Indeed, I had not see this class, my proposal makes more sense in ClassTagExtensions then and keep the removal of ScalaObjectMapper for Scala 3 users If I understand correctly.

@pjfanning
Copy link
Member

pjfanning commented May 19, 2021

@gaeljw that's what I am proposing - it's awkward to balance the expectations of users who want binary compatibility and those who want to continue to support ScalaObjectMapper for Scala 3 users.

One other option (that I would prefer slightly less than the existing proposal) is that we already have scala version specific src dirs and could keep ScalaObjectMapper as is in scala 2.x source but use your version in scala 3.x source.

@gaeljw
Copy link
Contributor Author

gaeljw commented May 20, 2021

I'll think about the different options in the coming days and will update the PR next week probably :)

@gaeljw gaeljw force-pushed the scalaobjectmapperforscala3 branch from a61c163 to 6f6edfb Compare June 3, 2021 06:46
@gaeljw gaeljw changed the title feat: 馃殌 Remove usage of Manifest in ScalaObjectMapper feat: 馃殌 Support parameterized types in ClassTagExtensions Jun 3, 2021
@gaeljw gaeljw marked this pull request as ready for review June 3, 2021 06:48
@gaeljw
Copy link
Contributor Author

gaeljw commented Jun 3, 2021

Finally had some time to update the PR.

The suggested code now lives in ClassTagExtensions.

@gaeljw
Copy link
Contributor Author

gaeljw commented Jun 3, 2021

Not sure why the build is failing for Scala 2.13. Output is incomplete on TravisCI 馃し

@pjfanning pjfanning merged commit 2dab7c3 into FasterXML:2.13 Jun 3, 2021
@pjfanning
Copy link
Member

Thanks very much @gaeljw - I'll merge and see what happens with the build

pjfanning pushed a commit that referenced this pull request Jun 3, 2021
Thanks very much @gaeljw - I'll merge this and see what happens with build - it was probably just some travis issue that caused the build failure
@gaeljw gaeljw deleted the scalaobjectmapperforscala3 branch June 3, 2021 10:43
@tmunk123
Copy link

tmunk123 commented Jun 7, 2022

@pjfanning I'm currently using Scala 2.11.12 and fasterXML 2.13.1. When I compile, I see the following:

C:\temp\IdeaProjects\dataservices\Db2ZAI\src\com\ibm\analytics\db2zai\utils\Db2ZAIJSONUtil.scala:15:40
trait ScalaObjectMapper in package scala is deprecated: ScalaObjectMapper is deprecated because Manifests are not supported in Scala3, you might want to use ClassTagExtensions as a replacement
    val mapr = new ObjectMapper() with ScalaObjectMapper

Will this deprecation warning be resolved/fixed once we move to Scala 2.12.x or later?

@gaeljw
Copy link
Contributor Author

gaeljw commented Jun 7, 2022

@tmunk123 please read the documentation: https://github.com/FasterXML/jackson-module-scala#classtagextensions

The deprecation will not disappear, you have to change your code.

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