add support for jsonUnknown trait #1574
Conversation
| @@ -1445,14 +1480,21 @@ private[smithy4s] class SchemaVisitorJCodec( | |||
| cursor: Cursor, | |||
| in: JsonReader | |||
| ): scala.collection.Map[String, Any] => Z = { | |||
| val unknownValues = | |||
| if (keepUnknown) ListBuffer[(String, Document)]() else null | |||
There was a problem hiding this comment.
I appreciate the effort to minimise allocations 👍 . I would personally have gone for distinct JCodec implementations based on whether the structure has a @jsonUnknown field or not, which does induce some copy/pasting, but completely shaves off some checks on the critical path.
There was a problem hiding this comment.
There are now 2 JCodec implementations to separately handle structs with a jsonUnknown member and ones without.
| else None | ||
| } else default | ||
| } else { | ||
| if (f.isRequired) unknownValue else Some(unknownValue) |
There was a problem hiding this comment.
you cannot make the assumption that !required implies scala.Option : we're planning on relaxing our Schema model to allow for other ways of modelling optionality (to cater for java.util.Optional, for instance).
Moreover, maybe the user applies other type of customisations to what ends up annotated with jsonUnknown. Maybe they have a bijection from Document to circe's Json in place, etc.
So what you need to do instead is to compile the Document.Decoder associated to the field that is annotated with @jsonUnkown, in order to run the unknownValue (converted to Document) through it to get a value that'll be guaranteed to be of the correct type, without having to make any assumption.
There was a problem hiding this comment.
This has been addressed and the decoding now uses what you described.
| if (unknownValue == null) { | ||
| if (default == null) { | ||
| if (f.isRequired) Map.empty | ||
| else None |
| .decode(Document.obj()) | ||
| .getOrElse( | ||
| throw new RuntimeException( | ||
| s"${cursor.getPath(Nil)} Failed translating a Document.DObject to the type targeted by ${f.label}." |
There was a problem hiding this comment.
use in.decodeError instead, to throw the exception
| .decode(unknownValue) | ||
| .getOrElse( | ||
| throw new RuntimeException( | ||
| s"${cursor.getPath(Nil)} Failed translating a Document.DObject to the type targeted by ${f.label}." |
| case m: Map[_, _] => | ||
| m.foreach { case (label: String, value: Document) => | ||
| writeLabel(label, out) | ||
| documentJCodec.encodeValue(value, out) | ||
| } | ||
| case Some(m: Map[_, _]) => | ||
| m.foreach { case (label: String, value: Document) => | ||
| writeLabel(label, out) | ||
| documentJCodec.encodeValue(value, out) | ||
| } |
There was a problem hiding this comment.
Sorry, I should have signposted this bit as well, but this is falling under the similar notion to what I was describing here :
- You cannot make the assumption that a smithy map necessarily translates to a Scala map
- You cannot make the assumption that the absence of
requiredtranslates to Scala Option - You cannot even make the assumption that at this level, you're dealing with
Documents
Therefore you need to pre-compile the Document.Encoder and encode the field value against it, and pattern-match against the produced document to check it's a DObject, in which case you run the exact same foreach you have there
There was a problem hiding this comment.
This makes sense, thanks for the directions.
|
This is shaping up well ! This code is quite tricky and it's a very valuable PR, thank you very much for taking this on 👍 |
Implement support for
jsonUnknowntrait defined in disneystreaming/alloy#180