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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow $type tag to be replaced by some other string (500USD Bounty) #578

Closed
lihaoyi opened this issue May 14, 2024 · 3 comments · Fixed by #579
Closed

Allow $type tag to be replaced by some other string (500USD Bounty) #578

lihaoyi opened this issue May 14, 2024 · 3 comments · Fixed by #579

Comments

@lihaoyi
Copy link
Member

lihaoyi commented May 14, 2024

Given a "$type": "foo.Bar" sealed trait discriminator, Custom Keys's @key annotation allows you to replace the right hand side "foo.Bar" of a particular case class by annotating the case class, but it does not allow you to replace to left hand side "$type" string with something else.

This ticket is to implement the ability for a user to replace $type" with some other string, by annotating the sealed trait itself with @key("myType") annotation that would replace "$type" with "myType" for all sub-classes of the sealed trait.

To incentivize contribution, I'm putting a 500USD bounty on resolving this ticket. This is payable via bank transfer, and at my discretion in case of ambiguity.

@mrdziuban
Copy link
Contributor

I'd be happy to take a stab at this @lihaoyi

Is it okay if the LegacyApi continues to use $type for the discriminator and only upickle.default uses the new behavior?

@lihaoyi
Copy link
Member Author

lihaoyi commented May 14, 2024

IIRC the legacy one doesnt use a key value discriminator at all; instead, it wraps things jn two element arrays

@lihaoyi
Copy link
Member Author

lihaoyi commented May 14, 2024

But yeah, I think in general focusing on making it work for upickle.default is fine.

lihaoyi pushed a commit that referenced this issue May 17, 2024
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:

```scala
@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`
```
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 a pull request may close this issue.

2 participants