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

Support multi-level sealed trait in macros #512

Merged
merged 2 commits into from
Nov 24, 2020

Conversation

cchantep
Copy link
Member

@cchantep cchantep commented Sep 11, 2020

Considering:

  sealed trait TreeValue

  sealed trait SubLevel extends TreeValue

  case class Leaf1(value: String) extends TreeValue
  case class Leaf2(value: Int) extends SubLevel

  object TreeValue {
    private implicit val leaf1: OFormat[Leaf1] = Json.format
    private implicit val leaf2: OFormat[Leaf2] = Json.format
    private implicit val subLevel: OFormat[SubLevel] = Json.format

    implicit val format: OFormat[TreeValue] = Json.format
  }

For now the discriminator from the top level overwrite the one from the sub one:

// So:
Json.toJson[TreeValue](Leaf2(1)) == Json.obj("_type" -> "SubLevel", "value" -> 1)

// Instead:
Json.toJson[TreeValue](Leaf2(2)) == Json.obj("_type" -> "Leaf2", "value" -> 2)

See failing test

@@ -676,7 +676,7 @@ class JsMacroImpl(val c: blackbox.Context) {
case jsv => $json.JsObject(Seq("_value" -> jsv))
}

jso + ($config.discriminator -> $json.JsString($config.typeNaming(${t.typeSymbol.fullName})))
$json.JsObject(Map($config.discriminator -> $json.JsString($config.typeNaming(${t.typeSymbol.fullName})))) ++ jso
Copy link
Member Author

Choose a reason for hiding this comment

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

If the sub-writer use the same discriminator name, let it overwrite the value with the more specific type

@cchantep cchantep requested a review from ennru September 11, 2020 15:27
Copy link
Member

@ignasi35 ignasi35 left a comment

Choose a reason for hiding this comment

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

I like the improvement but I think it has the potential of breaking some existing code that relies on this behavior.
Should this be released on a new major or maybe behind a setting?

@cchantep
Copy link
Member Author

cchantep commented Nov 2, 2020

I like the improvement but I think it has the potential of breaking some existing code that relies on this behavior.
Should this be released on a new major or maybe behind a setting?

For me the fix make sure the already expected behaviour, so that's not breaking.

Anyway if we think about a setting, it would mean exposing a setting to keep an invalid discriminator ?

@cchantep
Copy link
Member Author

ping

@ignasi35
Copy link
Member

Yeah, I think you're right. The improvement on the PR makes everything work as one would expect. If someone relies on the current (unexpected) behavior, then they can force their way out of the correct implementation by tunning the Format's.

@ignasi35 ignasi35 merged commit 0215ac6 into playframework:master Nov 24, 2020
@cchantep cchantep deleted the fix/sub-trait branch November 24, 2020 16:19
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.

2 participants