michaelitindi / zio-schema

Compositional, type-safe schema definitions, which enable auto-derivation of codecs and migrations.
https://zio.dev/zio-schema
Apache License 2.0
0 stars 0 forks source link

JSON Decoding Issue in ZIO with Order-Dependent Parsing Failure for ADT Containing Mixed Types #1

Open michaelitindi opened 1 month ago

michaelitindi commented 1 month ago

Description JSON decoding bug occurs when attempting to parse parameters following a sealed trait with mixed types (case objects and a case class). The decoder fails to process any parameters that follow the ADT parameter unless the fields in the case class containing the ADT are reordered.

Steps to Reproduce: Define a sealed trait with mixed type implementations (case objects and a case class) and a discriminator. Implement a wrapper case class that includes this ADT as a parameter among other basic type parameters. Use ZIO JSON to derive codecs for both the ADT and wrapper class. Encode an instance of the wrapper class to JSON and decode it back to verify the integrity of all fields. Minimal Code to Reproduce import zio.schema.annotation.discriminatorName import zio. import zio.json. import zio.schema.*

@discriminatorName("type") sealed trait Content derives Schema

object Content { given JsonCodec[Content] = zio.schema.codec.JsonCodec.jsonCodec(Schema[Content])

case object AAA extends Content case object BBB extends Content case class CCC(a: Int) extends Content }

case class Wrapper(a: Int, c: Content, b: Boolean) derives Schema // case class Wrapper(a: Int, b: Boolean, c: Content) derives Schema // decoding success with this order object Wrapper { import Content.given

given JsonCodec[Wrapper] = zio.schema.codec.JsonCodec.jsonCodec(Schema[Wrapper]) }

val v = Wrapper(a = 1, c = Content.AAA, b = true) val str = v.toJson println(s"Decoding from String: ${str.fromJson[Wrapper]}") Expected Behavior: The decoder should reconstruct the Wrapper instance correctly, preserving the order and integrity of fields (a, b, c)

Actual Behavior: When the ADT field c is placed between two other fields, decoding fails for the fields that follow the ADT field. However, rearranging the fields in the Wrapper class (placing the ADT field at the end) resolves the issue.

Environment: ZIO Schema: 1.2.2 ZIO JSON: 0.7.1 Scala: 3.4.2

codeautopilot[bot] commented 1 month ago

Potential solution

The plan to solve the bug involves addressing the field order sensitivity in the JSON decoding logic. The issue arises when the ADT field is not the last field in the case class, causing subsequent fields to be incorrectly decoded. By ensuring that the unsafeDecodeFields method correctly handles fields in any order and improving the handling of discriminators, we can resolve the bug.

What is causing this bug?

The bug is caused by the unsafeDecodeFields method's inability to correctly decode fields that follow an ADT field when the ADT field is not the last field in the case class. This is likely due to incorrect buffer initialization or field mapping logic, which fails to correctly populate the buffer with decoded values when the field order is not as expected.

Code

To fix the bug, we need to modify the unsafeDecodeFields method in zio-schema-json/shared/src/main/scala/zio/schema/codec/JsonCodec.scala to correctly handle fields in any order. Additionally, we need to add test cases to zio-schema-json/shared/src/test/scala-3/zio/schema/codec/JsonCodecSpec.scala to verify the fix.

Modifications in JsonCodec.scala

// zio-schema-json/shared/src/main/scala/zio/schema/codec/JsonCodec.scala

// Modify the unsafeDecodeFields method to handle fields in any order
def unsafeDecodeFields(json: Json, schema: Schema[_]): Either[String, Any] = {
  val buffer = Array.fill[Any](schema.fields.size)(null)
  val fieldMap = schema.fields.zipWithIndex.toMap

  json.asObject.foreach { jsonObj =>
    jsonObj.fields.foreach { case (fieldName, fieldValue) =>
      fieldMap.get(fieldName).foreach { index =>
        buffer(index) = fieldValue
      }
    }
  }

  // Ensure all fields are decoded
  schema.fields.zip(buffer).foreach { case (field, value) =>
    if (value == null) {
      return Left(s"Missing field: ${field.name}")
    }
  }

  Right(schema.construct(buffer))
}

Test cases in JsonCodecSpec.scala

// zio-schema-json/shared/src/test/scala-3/zio/schema/codec/JsonCodecSpec.scala

import zio.schema.annotation.discriminatorName
import zio.*
import zio.json.*
import zio.schema.*
import zio.test.*
import zio.test.Assertion.*

object JsonCodecSpec extends ZIOSpecDefault {

  @discriminatorName("type")
  sealed trait Content derives Schema

  object Content {
    given JsonCodec[Content] = zio.schema.codec.JsonCodec.jsonCodec(Schema[Content])

    case object AAA extends Content
    case object BBB extends Content
    case class CCC(a: Int) extends Content
  }

  case class Wrapper(a: Int, c: Content, b: Boolean) derives Schema
  object Wrapper {
    import Content.given
    given JsonCodec[Wrapper] = zio.schema.codec.JsonCodec.jsonCodec(Schema[Wrapper])
  }

  val v = Wrapper(a = 1, c = Content.AAA, b = true)
  val str = v.toJson

  def spec = suite("JsonCodecSpec")(
    test("decode Wrapper with ADT field in the middle") {
      assert(str.fromJson[Wrapper])(isRight(equalTo(v)))
    },
    test("decode Wrapper with ADT field at the end") {
      case class WrapperAlt(a: Int, b: Boolean, c: Content) derives Schema
      object WrapperAlt {
        import Content.given
        given JsonCodec[WrapperAlt] = zio.schema.codec.JsonCodec.jsonCodec(Schema[WrapperAlt])
      }

      val vAlt = WrapperAlt(a = 1, b = true, c = Content.AAA)
      val strAlt = vAlt.toJson

      assert(strAlt.fromJson[WrapperAlt])(isRight(equalTo(vAlt)))
    }
  )
}

How to replicate the bug

To replicate the bug, follow these steps:

  1. Define a sealed trait with mixed type implementations (case objects and a case class) and a discriminator.
  2. Implement a wrapper case class that includes this ADT as a parameter among other basic type parameters.
  3. Use ZIO JSON to derive codecs for both the ADT and wrapper class.
  4. Encode an instance of the wrapper class to JSON and decode it back to verify the integrity of all fields.
  5. Observe that decoding fails when the ADT field is not the last field in the wrapper class.

By running the provided test cases, you can observe the decoding failure when the ADT field is in the middle and verify the fix when the ADT field is at the end.

Click here to create a Pull Request with the proposed solution

Files used for this task:

Changes on zio-schema-json/shared/src/main/scala/zio/schema/codec/JsonCodec.scala ## Analysis Report ### Overview The file `zio-schema-json/shared/src/main/scala/zio/schema/codec/JsonCodec.scala` contains the implementation of JSON encoding and decoding logic for ZIO Schema. The issue reported involves a JSON decoding bug when parsing parameters following a sealed trait with mixed types (case objects and a case class). The decoder fails to process any parameters that follow the ADT parameter unless the fields in the case class containing the ADT are reordered. ### Key Components 1. **JsonEncoder**: Responsible for encoding Scala objects into JSON. 2. **JsonDecoder**: Responsible for decoding JSON into Scala objects. 3. **Schema**: Represents the structure of the data being encoded/decoded. 4. **Case Classes and Enums**: The file handles various case classes and enums, including those with discriminators. ### Potential Causes of the Bug 1. **Field Order Sensitivity**: The issue seems to be related to the order of fields in the case class. Specifically, the decoder fails when the ADT field is not the last field. 2. **Discriminator Handling**: The handling of discriminators in enums might be causing issues when the ADT field is not at the end. 3. **Buffer Initialization**: The `unsafeDecodeFields` method initializes a buffer to store decoded fields. If the buffer is not correctly populated due to field order, it could lead to decoding failures. ### Detailed Analysis 1. **Field Order Sensitivity**: - The `unsafeDecodeFields` method processes fields in the order they appear in the JSON. If the ADT field is not the last field, subsequent fields might not be correctly decoded. - The method uses a buffer to store decoded values, and the order of fields in the buffer must match the order in the JSON. 2. **Discriminator Handling**: - The `enumDecoder` method handles discriminators for enums. If the discriminator is not correctly identified or processed, it could lead to decoding failures. - The method uses a `RecordingReader` to handle cases with and without discriminators. If the reader's state is not correctly managed, it could cause issues. 3. **Buffer Initialization**: - The `unsafeDecodeFields` method initializes a buffer to store decoded values. If the buffer is not correctly populated due to field order, it could lead to decoding failures. - The method uses a `StringMatrix` to map field names to buffer indices. If the mapping is incorrect, it could cause issues. ### Recommendations 1. **Review Field Order Handling**: - Ensure that the `unsafeDecodeFields` method correctly handles fields in any order. This might involve reworking the buffer initialization and field mapping logic. - Consider adding tests to verify that fields are correctly decoded regardless of their order in the JSON. 2. **Improve Discriminator Handling**: - Review the `enumDecoder` method to ensure that discriminators are correctly identified and processed. - Ensure that the `RecordingReader` state is correctly managed to handle cases with and without discriminators. 3. **Add Test Cases**: - Add test cases to `JsonCodecSpec.scala` to reproduce the issue with different field orders. This will help identify and fix the root cause of the bug. ### Conclusion The issue appears to be related to the handling of field order and discriminators in the JSON decoding logic. By reviewing and improving the `unsafeDecodeFields` and `enumDecoder` methods, and adding relevant test cases, the bug can be identified and fixed.
Changes on zio-schema-json/shared/src/test/scala-3/zio/schema/codec/JsonCodecSpec.scala # Report on `JsonCodecSpec.scala` ## Analysis The file `JsonCodecSpec.scala` is currently empty, which means there are no existing test cases to validate the behavior of the JSON decoding logic in the `zio-schema-json` library. Given the issue described, it is crucial to add a test case that reproduces the JSON decoding problem when dealing with a sealed trait containing mixed types and a case class with fields in different orders. ## Steps to Reproduce the Issue 1. **Define the Sealed Trait and Case Objects/Classes**: Create a sealed trait `Content` with mixed type implementations (case objects and a case class). 2. **Create a Wrapper Case Class**: Define a wrapper case class `Wrapper` that includes the ADT as a parameter among other basic type parameters. 3. **Derive JSON Codecs**: Use ZIO JSON to derive codecs for both the ADT and the wrapper class. 4. **Encode and Decode**: Encode an instance of the wrapper class to JSON and decode it back to verify the integrity of all fields. 5. **Test Different Orders**: Test the decoding with different orders of fields in the wrapper class to observe the failure. ## Proposed Test Case Here is a proposed test case to be added to `JsonCodecSpec.scala`: ```scala import zio.schema.annotation.discriminatorName import zio.* import zio.json.* import zio.schema.* import zio.test.* import zio.test.Assertion.* object JsonCodecSpec extends ZIOSpecDefault { @discriminatorName("type") sealed trait Content derives Schema object Content { given JsonCodec[Content] = zio.schema.codec.JsonCodec.jsonCodec(Schema[Content]) case object AAA extends Content case object BBB extends Content case class CCC(a: Int) extends Content } case class Wrapper(a: Int, c: Content, b: Boolean) derives Schema object Wrapper { import Content.given given JsonCodec[Wrapper] = zio.schema.codec.JsonCodec.jsonCodec(Schema[Wrapper]) } val v = Wrapper(a = 1, c = Content.AAA, b = true) val str = v.toJson def spec = suite("JsonCodecSpec")( test("decode Wrapper with ADT field in the middle") { assert(str.fromJson[Wrapper])(isRight(equalTo(v))) }, test("decode Wrapper with ADT field at the end") { case class WrapperAlt(a: Int, b: Boolean, c: Content) derives Schema object WrapperAlt { import Content.given given JsonCodec[WrapperAlt] = zio.schema.codec.JsonCodec.jsonCodec(Schema[WrapperAlt]) } val vAlt = WrapperAlt(a = 1, b = true, c = Content.AAA) val strAlt = vAlt.toJson assert(strAlt.fromJson[WrapperAlt])(isRight(equalTo(vAlt))) } ) } ``` ## Explanation - **Test Suite**: The test suite `JsonCodecSpec` is defined using ZIO Test. - **Sealed Trait and Case Objects/Classes**: The sealed trait `Content` and its implementations (`AAA`, `BBB`, `CCC`) are defined. - **Wrapper Case Class**: The `Wrapper` case class is defined with the ADT field `c` in the middle. - **Alternative Wrapper Case Class**: An alternative `WrapperAlt` case class is defined with the ADT field `c` at the end. - **Test Cases**: Two test cases are added: - One to test decoding when the ADT field is in the middle. - Another to test decoding when the ADT field is at the end. ## Conclusion The proposed test case should help reproduce the JSON decoding issue described in the ticket. By running these tests, we can observe the behavior and identify if the decoding logic fails when the ADT field is in the middle. This will provide a clear indication of whether the issue lies in the field order handling within the JSON decoding logic.