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.

Sign up for GitHub

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

Check type before using asInstanceOf #706

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Check type before using asInstanceOf #706

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions zio-schema-json/shared/src/main/scala/zio/schema/codec/JsonCodec.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -378,7 +378,9 @@ object JsonCodec {

private def enumEncoder[Z](schema: Schema.Enum[Z], cfg: Config, cases: Schema.Case[Z, _]*): ZJsonEncoder[Z] =
// if all cases are CaseClass0, encode as a String
if (schema.annotations.exists(_.isInstanceOf[simpleEnum])) {
if (schema.annotations.exists(_.isInstanceOf[simpleEnum]) && cases.forall(
Copy link
Contributor

@987Nabil 987Nabil Jun 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

since @simpleEnum is not added explicitly by the user, is this not an error in the macro?
And if the user would add it, the macro should fail.
But here, the user could have created a schema manually or added the annotation manually after the macro code.
So I think we should at least print a warning here, if the annotation is present but the cases are not parameter less. I would even prefer to fail.

Copy link
Member Author

Choose a reason for hiding this comment

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

I had thought that it made sense to use simpleEnum if the final leaves were case objects, but actually that should not be the macro behaviour. I think that I will close this PR and add this to #707. I agree that it's better to make it fail.

Adding fixes from now on!

Copy link
Contributor

Choose a reason for hiding this comment

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

@pablf I think it is okay, if all leaves are objects or case classes without fields, that it is a simple enum. But why would you need the check in the json codec for that? The check there makes sense to prevent errors in case of an annotation where no annotation belongs, to raise an error. But if leaves are simple cases and the json codec still can't handle it, it is a different error and this would not be the fix.
I think the goals should be

  • simple leaves = simple enum
  • simple leaves with intermediate traits should be handled by codecs as if the intermediate trait is missing
  • no simple leaves but annotated => macro fails
  • no simple leaves but annotated => codec fails

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we need more issues

Copy link
Member Author

@pablf pablf Jun 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@987Nabil I've just noticed a different behaviour in this aspect between Scala 2 and Scala 3 macros. Scala 2 doesn't derives the intermediate traits in the schemas while Scala 3 does. Maybe that's the only issue here. Are intermediate traits supposed to appear in the schema or not? That would also solve the issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it should not. At least I do not see any value in it. @jdegoes wdyt?

Copy link
Member

Choose a reason for hiding this comment

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

@987Nabil Why not derive intermediate traits??

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't say it's not good. I say, is there a use case for it? Is there value in it? Or does it just make the use case of simple enum harder without a clear benefit.

Anyhow, if we have intermediate traits derived, it should be the same for Scala 2 and 3 and leave traits (no subtypes) should be ignored for the question of "Is this a simple enum".

Copy link
Member

Choose a reason for hiding this comment

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

It seems like not deriving intermediates makes the system "lossy", which is something I like to avoid on principle even if I don't have a compelling use case for it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I am fine with that. Then the Scala 2 macro should do the same. And the structure should be interpreted correctly regarding simple enum

_.schema.isInstanceOf[Schema.CaseClass0[_]]
)) {
val caseMap: Map[Z, String] =
schema.nonTransientCases
.map(
Expand Down Expand Up @@ -699,7 +701,9 @@ object JsonCodec {
}.toMap

// if all cases are CaseClass0, decode as String
if (cases.forall(_.schema.isInstanceOf[Schema.CaseClass0[_]])) {
if (parentSchema.annotations.exists(_.isInstanceOf[simpleEnum]) && cases.forall(
_.schema.isInstanceOf[Schema.CaseClass0[_]]
)) {
val caseMap: Map[String, Z] =
cases.map(case_ => case_.id -> case_.schema.asInstanceOf[Schema.CaseClass0[Z]].defaultConstruct()).toMap
ZJsonDecoder.string.mapOrFail(
Expand Down
21 changes: 21 additions & 0 deletions zio-schema-json/shared/src/test/scala-2/zio/schema/codec/JsonCodecSpec.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -292,6 +292,13 @@ object JsonCodecSpec extends ZIOSpecDefault {
charSequenceToByteChunk("""{"oneOf":{"_type":"StringValue2","value":"foo2"}}""")
)
},
test("ADT with intermediate enumeration") {
assertEncodes(
Schema[IntermediateEnum],
IntermediateEnum.FinalElem,
charSequenceToByteChunk(""""FinalElem"""")
)
},
test("case class") {
assertEncodes(
searchRequestWithTransientFieldSchema,
Expand Down Expand Up @@ -1226,6 +1233,12 @@ object JsonCodecSpec extends ZIOSpecDefault {
Enumeration2(BooleanValue2(false))
)
},
test("ADT with intermediate enumeration") {
assertEncodesThenDecodes(
Schema[IntermediateEnum],
IntermediateEnum.FinalElem
)
},
test("ADT with noDiscriminator") {
assertEncodesThenDecodes(
Schema[Enumeration3],
Expand Down Expand Up @@ -1742,6 +1755,14 @@ object JsonCodecSpec extends ZIOSpecDefault {
implicit val schema: Schema[Enumeration3] = DeriveSchema.gen[Enumeration3]
}

sealed trait IntermediateEnum

object IntermediateEnum {
sealed trait IntermediateElem extends IntermediateEnum
case object FinalElem extends IntermediateEnum
implicit val schema: zio.schema.Schema[IntermediateEnum] = zio.schema.DeriveSchema.gen[IntermediateEnum]
}

sealed trait Color

object Color {
Expand Down
Loading