Support customizing $type tag with key annotation

[mrdziuban]

Closes #578

Adds support for customizing the key used a discriminator for sealed hierarchies. The default is still $type, but can be customized by annotating the sealed trait with @key("customKey").

The code changes necessary to support this in Writer were:

• Adding a new tagKey parameter to TaggedWriter.Leaf
• Adding a new TaggedWriter#findWriterWithKey method that includes the tagKey in its return value
• Updating TaggedWriter#write0 to call findWriterWithKey and pass the tagKey through to taggedWrite
• Updating AttributeTagged#taggedWrite to use the given tagKey in place of the old default key

The code changes necessary to support this in Reader were:

• Adding a new tagKey method to TaggedReader, which defaults to the old default key
• Adding a tagKey parameter to TaggedReader.Leaf, TaggedReader.Node, TaggedReadWriter.Leaf, and TaggedReadWriter.Node to override the default value in TaggedReader
• Updating AttributeTagged#taggedObjectContext to use the given TaggedReader's tagKey in place of the old default key

I tried instead to pass the tagKey through to findReader, but findReader is called in a number of contexts where it wasn't possible to get the tagKey value, for example in TaggedReader#visitString and the visitValue method within the ObjVisitor returned by AttributeTagged#taggedObjectContext.

The one limitation of the customization is when a member of a sealed hierarchy inherits from multiple parents that would result in different discriminators, e.g. if two parents have @key annotations with different arguments, or if one parent has a @key annotation and another doesn't. In these cases, the macros will fail with an error telling the user what went wrong and suggesting ways to fix it:

@key("customKey1")
sealed trait MultiKeyedADT1
@key("customKey2")
sealed trait MultiKeyedADT2
case object MultiKeyedObj extends MultiKeyedADT1 with MultiKeyedADT2 {
  implicit val rw: ReadWriter[MultiKeyedObj.type] = macroRW
}
-- Error: ----------------------------------------------------------------------
 6 |  implicit val rw: ReadWriter[MultiKeyedObj.type] = macroRW
   |                                                    ^^^^^^^
   |Type MultiKeyedObj.type inherits from multiple parent types with different discriminator keys:
   |
   |  parents: MultiKeyedADT1, MultiKeyedADT2
   |  keys: customKey1, customKey2
   |
   |To resolve this, either remove the `@key` annotations from all parents of the type,
   |or make sure all the parents pass the same value to `@key`

[lihaoyi]

@mrdziuban I think the approach looks great.

Determine how to handle cases where a type inherits from multiple sealed traits

In the current codebase, I suppose it doesn't matter, because the parent sealed trait that a case class inherits from does not change the serialization format. With this change, that is no longer true.

I think a reasonably first pass would be to just fail loudly if a case class inherits from multiple sealed traits with different @keys configured (or some configured and others not configured). The "different parent sealed traits have different discriminator fields" use case is probably obscure enough most people won't notice.

Another alternative would be to make the case class have multiple duplicate discriminator fields if its parents have inconsistent @keys, e.g. {"$type": "foo.Bar", "$type2": "foo.Baz", "data", "blah", ...}. But I think that can be left to a follow PR, and for now just erroring out gives us the flexibility to adjust the design in future if we learn more

Consider if there's a more ergonomic way to get tagKey in taggedObjectContext

Could you describe what you think is un-ergonomic about the current approach? That would help me give a proper answer

[mrdziuban]

fail loudly if a case class inherits from multiple sealed traits with different @keys configured (or some configured and others not configured)

Good idea, updated to do so.

Could you describe what you think is un-ergonomic about the current approach?

I wasn't sure if adding def tagKey to TaggedReader was the best approach, since it proliferates to everywhere that constructs one, like Reader.merge and Writer.merge. That said, I tried some other approaches, like passing the tagKey to findReader, but ran into limitations where I didn't have the value available where findReader was called.

[mrdziuban]

Re: binary compatibility, I've made progress on it but am still having trouble with 5 problems:

 * abstract method annotate(upickle.core.Types#Reader,java.lang.String,java.lang.String)upickle.core.Types#TaggedReader in interface upickle.core.Annotator is present only in current version
   filter with: ProblemFilter.exclude[ReversedMissingMethodProblem]("upickle.core.Annotator.annotate")

 * abstract method annotate(upickle.core.Types#ObjectWriter,java.lang.String,java.lang.String,upickle.core.Annotator#Checker)upickle.core.Types#TaggedWriter in interface upickle.core.Annotator is present only in current version
   filter with: ProblemFilter.exclude[ReversedMissingMethodProblem]("upickle.core.Annotator.annotate")

 * abstract method taggedWrite(upickle.core.Types#ObjectWriter,java.lang.String,java.lang.String,upickle.core.Visitor,java.lang.Object)java.lang.Object in interface upickle.core.Types is present only in current version
   filter with: ProblemFilter.exclude[ReversedMissingMethodProblem]("upickle.core.Types.taggedWrite")

 * private[..] abstract method tagKey()java.lang.String in interface upickle.core.Types#TaggedReader is present only in current version
   filter with: ProblemFilter.exclude[ReversedMissingMethodProblem]("upickle.core.Types#TaggedReader.tagKey")
   
 * abstract method findWriterWithKey(java.lang.Object)scala.Tuple3 in interface upickle.core.Types#TaggedWriter is present only in current version
   filter with: ProblemFilter.exclude[ReversedMissingMethodProblem]("upickle.core.Types#TaggedWriter.findWriterWithKey")

I can fix the first two by implementing the annotate methods in Types.scala to construct the TaggedReader.Leaf and TaggedWriter.Leaf instances like they do in the LegacyApi and AttributeTagged in Api.scala, but I'm not sure if there's any drawback to this and/or if they need to stay abstract in Types.scala. These are the methods I'm talking about:

upickle/upickle/core/src/upickle/core/Types.scala

Lines 287 to 288 in d050f72

	def annotate[V](rw: Reader[V], key: String, value: String): TaggedReader[V]
	def annotate[V](rw: ObjectWriter[V], key: String, value: String, checker: Annotator.Checker): TaggedWriter[V]

I'm stumped on the others. Do you have any idea how to handle them?

[lihaoyi]

The abstracy method present only in current version errors generally can be resolved by giving the method some default implementation. e.g.

• if the new abstract method has an additional argument, make it instead a concrete method that forwards to the original abstract method.

• If the new abstract method is a variant of a previousl6 existing abstract method, just make it return some reasonable default value

[mrdziuban]

Thanks for the tips, all the changes are binary compatible now 🎉

I'll hopefully be able to add some tests over the next couple of days

[mrdziuban]

Added a couple tests, I think this is ready for review now

[lihaoyi]

For backwards compatibility, could we we leave the existing method un-deprecated?

I think it's reasonable to leave the existing functionality with the default in place rather than forcing people to explicitly pass in a defaultTagKey every time they call merge, and because of the use of varargs we can't give defaultTagKey a default value. So having both methods available (with and without passing in the defaultTagKey explicitly) seems like the way to go. Whether they have the same name or different names is fine either way for me

That would let all our tests remain unchanged, which is probably what we want since it ensures the user-facing API (as far as is tested) to remain stable

[mrdziuban]

Definitely, updated

[lihaoyi]

Left one code comment. One more request, could you update the documentation site to include the new functionality? You can use sbt upickleReadme/run to re-generate it in the upickleReadme/target/scalatex folder

[lihaoyi]

Also, could you update the PR description? A few lines summarizing what code changes are necessary, any alternatives you considered, and any other concerns you thought about, should be enough

[mrdziuban]

could you update the documentation site to include the new functionality?

👍 done

could you update the PR description?

For sure, updated to add these details

[lihaoyi]

Looks good to me. I kicked off CI and will merge it when green. Send me your bank details via email to [email protected] and i will close out the bounty

[mrdziuban]

Whoops sorry about the test failures, fixing in a minute

[lihaoyi]

Thanks!