Support unknown field retention trait #1409

dhpiggott · 2024-02-20T16:55:55Z

PR Checklist (not all items are relevant to all PRs)

Added unit-tests (for runtime code)
Added bootstrapped code + smoke tests (when the rendering logic is modified)
Added build-plugins integration tests (when reflection loading is required at codegen-time)
Added alloy compliance tests (when simpleRestJson protocol behaviour is expanded/updated)
Updated dynamic module to match generated-code behaviour
Added documentation
Updated changelog

Baccata · 2024-03-11T12:45:56Z

modules/json/src/smithy4s/json/internals/SchemaVisitorJCodec.scala

    field.hints.get(JsonName) match {
      case None    => field.label
      case Some(x) => x.value
    }

+  trait UnknownFieldsDecoder[A] { self =>


rather than creating a bespoke trait (which implies having to create a schema visitor for it), I'd just delegate to Document.Decoder, turning the Hashmap into a Document.DObject

Done in 59e0d22.

Baccata · 2024-03-11T12:51:18Z

modules/json/src/smithy4s/json/internals/SchemaVisitorJCodec.scala

@@ -1299,35 +1336,81 @@ private[smithy4s] class SchemaVisitorJCodec(
      )
  }

+  trait UnknownFieldsEncoder[A] {


Rather than creating a bespoke trait, I'd delegate to the Document.Encoder to produce a document representation. I'd then match the document to check if it's a DObject and would call upon a function to inline its fields, that I'd factor out of the encoder implementation for documents.

Done in 59e0d22. Wasn't sure what you meant by this:

I'd factor out of the encoder implementation for documents

I think you're referring to this:

case Document.DObject(values) => values.foreach { case (key, value) => out.writeKey(key) documentJCodec.encodeValue(value, out) }

but it seems too small to be worth factoring out to me - can you confirm?

Baccata · 2024-03-11T12:53:16Z

modules/core/src/smithy4s/internals/DocumentEncoderSchemaVisitor.scala

@@ -181,6 +182,42 @@ class DocumentEncoderSchemaVisitor(
        from(e => DString(total(e).stringValue))
    }

+  trait UnknownFieldsEncoder[A] {


No need for a bespoke trait : just compile the DocumentEncoder for A, and inline its fields if the result is a DObject

Done in 59e0d22.

Baccata · 2024-03-11T12:54:57Z

modules/core/src/smithy4s/internals/DocumentDecoderSchemaVisitor.scala

@@ -451,16 +506,16 @@ class DocumentDecoderSchemaVisitor(
      alternatives: Vector[Alt[U, _]],
      dispatch: Alt.Dispatcher[U]
  ): DocumentDecoder[U] = {
-    def jsonLabel[A](alt: Alt[U, A]): String =
+    def jsonLabelOrLabel[A](alt: Alt[U, A]): String =


... I mean ... why ?

Baccata · 2024-03-11T12:55:55Z

modules/core/src/smithy4s/internals/DocumentDecoderSchemaVisitor.scala

+        val unknownFieldsDecoder = UnknownFieldsDecoder(field.schema)
+        (
+            pp: List[PayloadPath.Segment],
+            buffer: Any => Unit,
+            fields: Map[String, Document]
+        ) => {
+          val unknownFields = fields -- knownFieldLabels
+          buffer(unknownFieldsDecoder(pp, unknownFields))
+        }


I'd like this bit to be extracted in a private method.

Baccata · 2024-03-11T12:58:01Z

modules/core/src/smithy4s/internals/DocumentDecoderSchemaVisitor.scala

@@ -307,66 +308,120 @@ class DocumentDecoderSchemaVisitor(
    }
  }

+  trait UnknownFieldsDecoder[A] { self =>


This one could be removed too : just compile the document decoder for A and pass it a DObject containing the unknownFields

Done in 59e0d22.

dhpiggott · 2024-03-11T15:41:23Z

modules/json/src/smithy4s/json/internals/SchemaVisitorJCodec.scala

+        fields.foreach { case (field, jsonLabel, default) =>
+          values += {
+            if (isForUnknownFieldRetention(field)) {
+              // TODO: Lift out.


Currently working on a refactor to avoid re-creating this decoder on each call. Will move on to the remaining feedback actions once I've done this.

Baccata · 2024-03-11T16:20:54Z

Please refrain from force-pushing once you've opened a PR. It prevents the review of the latest changes, and also makes it extremely annoying if someone was branching out from your work to help out or not. We tend to squash PRs anyway at the end.

dhpiggott · 2024-03-11T18:06:51Z

Please refrain from force-pushing once you've opened a PR. It prevents the review of the latest changes, and also makes it extremely annoying if someone was branching out from your work to help out or not. We tend to squash PRs anyway at the end.

I thought you were OK with it so long as the change is in draft state, not PR. I wouldn't have done it it with PRs. I'll try to remember not to with drafts now, apologies if I forget occasionally while I adjust.

Baccata · 2024-03-12T08:53:43Z

I thought you were OK with it so long as the change is in draft state, not PR

It's not about whether it's a draft or not. I switch to not being OK with it at the point when you ask me to have a look at it. Generally speaking I'll avoid looking at drafts unless asked to, but once you ask, you're essentially opening the Schrödinger box.

dhpiggott · 2024-03-12T09:08:07Z

I switch to not being OK with it at the point when you ask me to have a look at it.

Ok yeah I get that.

dhpiggott added 4 commits February 15, 2024 17:41

Build against alloy 0.2.8-27-e53667-SNAPSHOT

aa08e4a

First pass of Document codec support

f1cf193

Second pass of Document codec support

f3d9be9

Merge branch 'series/0.18' into support-unknown-field-retention-trait

485a43a

dhpiggott force-pushed the support-unknown-field-retention-trait branch from 0e907a9 to 485a43a Compare March 6, 2024 11:33

dhpiggott added 2 commits March 7, 2024 11:09

Bump to alloy snapshot with unknownJsonFieldRetention trait

dd7b5ec

Add initial JSON tests

8229575

dhpiggott force-pushed the support-unknown-field-retention-trait branch from a7de7a5 to 6728eb9 Compare March 7, 2024 17:14

JSON encoder support

6467c95

dhpiggott force-pushed the support-unknown-field-retention-trait branch 2 times, most recently from 8f48bc4 to 44b76f3 Compare March 11, 2024 11:18

Json decoder support

0430424

dhpiggott force-pushed the support-unknown-field-retention-trait branch from 44b76f3 to 0430424 Compare March 11, 2024 11:55

dhpiggott added 5 commits March 11, 2024 11:55

Remove redundant code

4f07a62

Handle naming TODO

b3a6d2a

Merge branch 'series/0.18' into support-unknown-field-retention-trait

70c313e

Regenerate bootstrap

10e5985

Remove redundant types

2695543

Baccata reviewed Mar 11, 2024

View reviewed changes

Remove redundant schema visitors

59e0d22

dhpiggott force-pushed the support-unknown-field-retention-trait branch from 3942bb2 to 59e0d22 Compare March 11, 2024 15:32

Remove redundant TODOs

b7dbe27

dhpiggott commented Mar 11, 2024

View reviewed changes

Remove redunant types

50038a6

dhpiggott added 3 commits March 11, 2024 18:02

wip

a42616f

Prep for avoiding reallocating document decoder on each decode

6bdf681

Add TODO

521d534

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support unknown field retention trait #1409

Support unknown field retention trait #1409

dhpiggott commented Feb 20, 2024

Baccata Mar 11, 2024

dhpiggott Mar 11, 2024

Baccata Mar 11, 2024

dhpiggott Mar 11, 2024 •

edited

Loading

Baccata Mar 11, 2024

dhpiggott Mar 11, 2024

Baccata Mar 11, 2024

Baccata Mar 11, 2024

Baccata Mar 11, 2024

dhpiggott Mar 11, 2024

dhpiggott Mar 11, 2024

Baccata commented Mar 11, 2024

dhpiggott commented Mar 11, 2024

Baccata commented Mar 12, 2024

dhpiggott commented Mar 12, 2024

Support unknown field retention trait #1409

Are you sure you want to change the base?

Support unknown field retention trait #1409

Conversation

dhpiggott commented Feb 20, 2024

PR Checklist (not all items are relevant to all PRs)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhpiggott Mar 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Baccata commented Mar 11, 2024

dhpiggott commented Mar 11, 2024

Baccata commented Mar 12, 2024

dhpiggott commented Mar 12, 2024

dhpiggott Mar 11, 2024 •

edited

Loading