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

Fix JSON decoding of empty objects #710

Merged
merged 3 commits into from
Jul 9, 2024

Conversation

guersam
Copy link
Contributor

@guersam guersam commented Jul 4, 2024

This PR fixes the decoding failure of discriminated case objects.

Example

@discriminatorName("type")
enum Foo derives Schema:
  case Bar(b: Int)
  case Baz

JsonCodec.jsonDecoder(Schema[Foo]).decodeJson("""{ "type": "Baz" }""")

It also makes case object decoding respect @rejectExtraFields.

However, currently @rejectExtraFields can be applied only to the leaf nodes of ADTs, not the sealed trait/enum itself. To fix this, we need to propagate the parent schema to every caseClassN decoder, but it's out of the scope of this PR. (#700)

/claim #711

@guersam guersam requested a review from a team as a code owner July 4, 2024 03:37
@guersam guersam force-pushed the fix-discriminated-caseclass0 branch from be4590d to 116b786 Compare July 4, 2024 03:50
- discriminated case object
- case object with @rejectExtraField
@guersam guersam force-pushed the fix-discriminated-caseclass0 branch from f9bab51 to d9a785c Compare July 4, 2024 04:00
@@ -991,7 +991,29 @@ object JsonCodec {
import JsonCodec.JsonDecoder.schemaDecoder

private[codec] def caseClass0Decoder[Z](discriminator: Int, schema: Schema.CaseClass0[Z]): ZJsonDecoder[Z] = { (trace: List[JsonError], in: RetractReader) =>
if (discriminator == -1) Codecs.unitDecoder.unsafeDecode(trace, in)
def skipField(): Unit = {
val rejectExtraFields = schema.annotations.collectFirst({ case _: rejectExtraFields => () }).isDefined
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Calculating this every time may seem redundant, but skipField() won't run at all in most cases.

Copy link
Contributor

@987Nabil 987Nabil Jul 4, 2024

Choose a reason for hiding this comment

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

I still think we can make this a val on Schema.Record. Maybe I should do this in my PR

@jdegoes jdegoes merged commit 975539a into zio:main Jul 9, 2024
25 checks passed
@guersam
Copy link
Contributor Author

guersam commented Aug 6, 2024

/claim #711

@jdegoes Can I claim a bounty with a PR merged in advance?

@987Nabil
Copy link
Contributor

987Nabil commented Aug 6, 2024

@guersam ask algora ppl in the discord

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.

3 participants