disneystreaming / smithy4s

https://disneystreaming.github.io/smithy4s/
Other
351 stars 72 forks source link

Enabling lenient union decoding changes the error message #1617

Closed ghostbuster91 closed 3 weeks ago

ghostbuster91 commented 4 weeks ago

Problem description

Using following schema for the union:

val schema = Schema.either(
  Schema
    .struct[String](
      Schema.string
        .required[String]("test1", identity)
    )(identity),
  Schema
    .struct[String](
      Schema.string
        .required[String]("test2", identity)
    )(identity)
)

the json:

val json2 = """|{
               |  "left" : {"_test1_": "b"}
               |}
               |""".stripMargin

fails with error:

a) Missing required field (path: .test1) when withLenientTaggedUnionDecoding is used b) Missing required field (path: .left.test1) when withLenientTaggedUnionDecoding is not used

Full repro: https://github.com/disneystreaming/smithy4s/compare/series/0.18...ghostbuster91:smithy4s:test-lenient-codec-error-msg?expand=1

Expected behavior

In both cases the error should be the same (Missing required field (path: .left.test1))

kubukoz commented 4 weeks ago

The expectation sounds reasonable to me - these are still supposed to be tagged unions.

What happens if you try to decode {"test1": "b"} with the lenient option enabled?

ghostbuster91 commented 4 weeks ago

What happens if you try to decode {"test1": "b"} with the lenient option enabled?

It fails as it should:

Left(Expected a single non-null value, offset: 0x00000011, buf:
+----------+-------------------------------------------------+------------------+
|          |  0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f | 0123456789abcdef |
+----------+-------------------------------------------------+------------------+
| 00000000 | 7b 0a 20 20 22 74 65 73 74 31 22 3a 20 22 62 22 | {.  "test1": "b" |
| 00000010 | 0a 7d 0a                                        | .}.              |
+----------+-------------------------------------------------+------------------+ (path: .))
Baccata commented 3 weeks ago

Agreed, this is a bug

msosnicki commented 3 weeks ago

I can have a look into it, can I have it assigned?