UrlForm decoding support #1113

dhpiggott · 2023-07-21T08:57:24Z

Follow up to #1120.

Related: disneystreaming/alloy#110

Changes:

Add URL form parsing and decoding support.
Update tests to cover parsing and decoding.
Handle UrlForm model renaming TODOs.
Simplify UrlForm model by making encoder and decoder work with lists.
Fix bug in Blob and add regression test.
Make URL form schema visitors work using urlFormFlattened and urlFormName from alloy so they're as AWS protocol agnostic as possible. AWS protocol codecs now translate xmlFlattened to urlFormFlattened and xmlName to urlFormName before passing schemas to the visitors.
Make sandbox AWS specific.

modules/core/src/smithy4s/http/internals/UrlFormDataDecoderSchemaVisitor.scala

modules/core/src/smithy4s/http/internals/UrlFormDataEncoderSchemaVisitor.scala

This reads more easily and makes redundancy more obvious.

…lists

Same idea as the last commit, it makes reasoning about the operations that occur on the data easier.

It's not necessary.

Everything that this covered is now covered by UrlFormDataEncoderDecoderSchemaVisitorSpec.

There's not enough in common between that function and what OAuthCodecs does for lifting it out to have benefit.

PayloadCodec is used for Jsoniter based reading/writing because Jsonitor has a single trait for the read and write behaviour, and that's why PayloadCodec is used in those code paths. But for cases like this where reading and writing code paths are relatively separate, there's no benefit to combining them in a PayloadCodec.

It's not clear there is benefit to pushing string encoding into Blob. There probably isn't. The bug isn't something I've seen any impact from, I just noticed it while exploring the feasibility/benefit of handling the TODO.

dhpiggott · 2023-08-11T16:41:24Z

project/Dependencies.scala

@@ -29,7 +29,7 @@ object Dependencies {

  val Alloy = new {
    val org = "com.disneystreaming.alloy"
-    val alloyVersion = "0.2.5"
+    val alloyVersion = "0.2.6"


This brings in urlFormFlattened and urlFormName from disneystreaming/alloy#110.

dhpiggott · 2023-08-11T16:42:06Z

modules/xml/test/src/smithy4s/xml/XmlCodecSpec.scala

@@ -621,27 +607,20 @@ object XmlCodecSpec extends SimpleIOSuite {
      .map(result => expect.same(result, expected))
  }

-  // Decode document differs from decode content in that the top-level
-  // tag is checked against the ShapeId
-  private def decodeDocument[A: Schema](document: XmlDocument): IO[A] = {


I couldn't figure out why this was necessary. I believe I've been able to simplify things without losing anything, but might be worth a second opinion.

dhpiggott · 2023-08-11T16:57:37Z

build.sbt

@@ -900,14 +900,14 @@ lazy val sandbox = projectMatrix
    // Ignore deprecation warnings here - it's all generated code, anyway.
    scalacOptions ++= Seq(
      "-Wconf:cat=deprecation:silent"
-    ) ++ scala3MigrationOption(scalaVersion.value),


I can't remember what this was doing now but I think it started causing a problem, and didn't seem to be necessary.

dhpiggott · 2023-08-11T16:59:48Z

modules/core/src/smithy4s/http/UrlForm.scala

-  }
-  object FormData {
-    case object Empty extends FormData {
+  final case class FormData(path: PayloadPath, maybeValue: Option[String]) {


Significant model simplification here. FormData is no longer a coproduct. The various places that need to handle it can do so with less code that's also more readable.

Baccata · 2023-08-14T12:29:09Z

modules/core/src/smithy4s/http/internals/UrlFormDataDecoderSchemaVisitor.scala

+    val altMap = alternatives
+      .map(altDecoder(_))
+      .toMap[PayloadPath.Segment, UrlFormDataDecoder[U]]
+    ({


locally is your friend. The parenthesis made me think this was a function application

Baccata · 2023-08-14T12:39:37Z

modules/core/src/smithy4s/http/internals/UrlFormDataDecoderSchemaVisitor.scala

+            .groupBy { case (index, _) =>
+              index
+            }
+            .toVector
+            .sortBy { case (index, _) =>
+              index
+            }
+            .map { case (index, indicesAndValues) =>
+              UrlFormCursor(
+                history,
+                indicesAndValues.map { case (_, value) => value }
+              )
+                .down(PayloadPath.Segment.Index(index))
+            }


you could use groupMap to make it more efficient. It's available in 2.12 thanks to scala-collection-compat

👍 Done in e28d74d.

And for 2.12 in b0ad4eb.

Baccata · 2023-08-14T13:12:11Z

modules/core/src/smithy4s/http/internals/UrlFormDataDecoderSchemaVisitor.scala

+                .down(PayloadPath.Segment.Index(index))
+            }
+          groupedAndSortedCursors
+            .traverse[UrlFormDecodeError, A](memberDecoder.decode(_))


if the toVector is only there to satisfy the traverse, we can change/overload our traverse to accommodate the data structure and save an un-necessary .toVector call

It is and it isn't. The result of groupBy (and groupMap) is a Map, but we have to sort things before producing a result so that we're not making assumptions about input order. Map isn't sortable. I did initially write toList and puzzle over the compile error, before changing it to toVector. So it's toVector rather than toList to satisfy the traverse, but not toVector rather nothing to satisfy the traverse.

Baccata · 2023-08-14T13:25:30Z

modules/aws-http4s/src/smithy4s/aws/internals/AwsQueryCodecs.scala

+        // be completely agnostic of AWS protocol details, they work with their
+        // own more appropriately named hints - which is what necessitates the
+        // translation here.
+        Schema.transformHintsTransitivelyK { hints =>


👍 (and thanks for the comment)

Baccata · 2023-08-14T13:26:25Z

Good stuff overall, just a couple low-hanging fruits to address related to efficiency of list decoding

Also remove unused ScalaCompat conversion and references.

dhpiggott · 2023-08-14T17:26:29Z

modules/core/src-2.12/ScalaCompat.scala

@@ -28,9 +28,4 @@ private[smithy4s] trait ScalaCompat {
    private def opt[A](a: => A): Option[A] = try { Some(a) }
    catch { case scala.util.control.NonFatal(_) => None }
  }
-
-  private[smithy4s] implicit final class MapOps[K, V](val map: Map[K, V]) {


Couldn't see this being used anywhere.

dhpiggott · 2023-08-14T17:26:37Z

modules/core/src-2.13/ScalaCompat.scala

@@ -16,8 +16,4 @@

 package smithy4s

-private[smithy4s] trait ScalaCompat {
-  implicit final class MapOps[K, V](val map: Map[K, V]) {


Couldn't see this being used anywhere.

Should I merge the 2.13 and 3 ScalaCompat versions under src?

dhpiggott · 2023-08-14T17:26:49Z

modules/core/src-3/ScalaCompat.scala

@@ -16,8 +16,4 @@

 package smithy4s

-private[smithy4s] trait ScalaCompat {
-  implicit final class MapOps[K, V](val map: Map[K, V]) {


Couldn't see this being used anywhere.

dhpiggott · 2023-08-14T17:27:24Z

modules/xml/src/smithy4s/xml/internals/XmlEncoderSchemaVisitor.scala

@@ -30,8 +30,7 @@ import cats.kernel.Monoid

 private[smithy4s] class XmlEncoderSchemaVisitor(
    val cache: CompilationCache[XmlEncoder]
-) extends SchemaVisitor.Cached[XmlEncoder]
-    with smithy4s.ScalaCompat { compile =>


Baccata

Good work !

dhpiggott mentioned this pull request Jul 21, 2023

UrlForm and AWS query support #1101

Closed

dhpiggott commented Aug 3, 2023

View reviewed changes

modules/core/src/smithy4s/http/internals/UrlFormDataDecoderSchemaVisitor.scala Outdated Show resolved Hide resolved

dhpiggott commented Aug 3, 2023

View reviewed changes

modules/core/src/smithy4s/http/internals/UrlFormDataDecoderSchemaVisitor.scala Outdated Show resolved Hide resolved

dhpiggott commented Aug 3, 2023

View reviewed changes

modules/core/src/smithy4s/http/internals/UrlFormDataEncoderSchemaVisitor.scala Outdated Show resolved Hide resolved

dhpiggott added 26 commits August 7, 2023 10:35

Update tests to cover decoding, fix various bugs uncovered by them

7ab3c8d

Consolidate cursor behaviour within UrlFormCursor

e20d0d3

This reads more easily and makes redundancy more obvious.

Remove redundant FailedValue cursor type

5d0e251

Use headOption, not head

7accb71

Further cursor simplification

c4d1ae6

Handle renaming TODOs

bc174d2

Flatten UrlForm types by making encoder and decoder cursor work with …

a5fc091

…lists

Flatten UrlFormCursor

5ceae82

Same idea as the last commit, it makes reasoning about the operations that occur on the data easier.

Handle TODO re. DRYing error messages

e2466f5

Remove TODO

679c682

It's not necessary.

Make spec test primitive decoding too

c100080

Remove redundant UrlFormParserSpec

e99c5cb

Everything that this covered is now covered by UrlFormDataEncoderDecoderSchemaVisitorSpec.

Fix codecs

06cf841

First pass of OAuthCodecs

e1c8cd0

First pass of sandbox clients and servers

149f826

Fix copyright headers

b60496d

Apply suggestions from code review

83d6d3c

Scalafmt

a96480c

Remove redundant dependency

2701b0f

Convert oauth-sandbox apps to Cats Effect

5ac670f

Move OAuthCodecs to http4s module

1f43fd4

Remove redundant TODO

ddb8f30

There's not enough in common between that function and what OAuthCodecs does for lifting it out to have benefit.

Remove redundant TODO and fix bug in Blob

0e97a74

It's not clear there is benefit to pushing string encoding into Blob. There probably isn't. The bug isn't something I've seen any impact from, I just noticed it while exploring the feasibility/benefit of handling the TODO.

Revert previous bug fix, add test, verify it fails

9068eae

Reapply bug fix, verify new test passes

4a42267

dhpiggott added 5 commits August 10, 2023 14:05

Make URL form support work using urlFormFlattened and urlFormName

ed9b4dc

Fix OAuthCodecs

e4337de

Merge branch 'series/0.18' into url-form-decoding-support

841fa69

Fix enum handling

eccb3b7

Remove OAuthCodecs and oauth-sandbox

824c479

dhpiggott marked this pull request as ready for review August 11, 2023 16:28

Fix aws-sandbox directory

dfe3eb2

dhpiggott commented Aug 11, 2023

View reviewed changes

dhpiggott added 2 commits August 14, 2023 09:38

Merge branch 'series/0.18' into url-form-decoding-support

ffba6d7

Fix imports

e8d0a89

Baccata reviewed Aug 14, 2023

View reviewed changes

Use locally for readability

8c081b6

Baccata reviewed Aug 14, 2023

View reviewed changes

dhpiggott added 3 commits August 14, 2023 17:55

Use groupMap

e28d74d

s/list/vector/

18a723d

Fix use of groupMap for Scala 2.12

b0ad4eb

Also remove unused ScalaCompat conversion and references.

dhpiggott commented Aug 14, 2023

View reviewed changes

Oops, too hasty with "dead" code removal

033173c

Baccata approved these changes Aug 16, 2023

View reviewed changes

Baccata merged commit 65cc9c0 into disneystreaming:series/0.18 Aug 16, 2023
11 checks passed

dhpiggott deleted the url-form-decoding-support branch August 16, 2023 09:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

UrlForm decoding support #1113

UrlForm decoding support #1113

dhpiggott commented Jul 21, 2023 •

edited

dhpiggott Aug 11, 2023

dhpiggott Aug 11, 2023

dhpiggott Aug 11, 2023

dhpiggott Aug 11, 2023

Baccata Aug 14, 2023

Baccata Aug 14, 2023

dhpiggott Aug 14, 2023

dhpiggott Aug 14, 2023

Baccata Aug 14, 2023

dhpiggott Aug 14, 2023

Baccata Aug 14, 2023

Baccata commented Aug 14, 2023

dhpiggott Aug 14, 2023

dhpiggott Aug 14, 2023

dhpiggott Aug 14, 2023

dhpiggott Aug 14, 2023

dhpiggott Aug 14, 2023

Baccata left a comment

UrlForm decoding support #1113

UrlForm decoding support #1113

Conversation

dhpiggott commented Jul 21, 2023 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Baccata commented Aug 14, 2023

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 left a comment

Choose a reason for hiding this comment

dhpiggott commented Jul 21, 2023 •

edited