Support `@default` on payloads?

[kubukoz]

This may be controversial...

I think @default should be used on payloads, i.e. if the HTTP request/response body is empty, we should attempt to decode it from something like defaultValue.orElse(emptyObject).

It appears that we're currently only trying to decode from an empty body:

(this is from a reproduction posted below)

[info] 400 Bad Request: Succeeded: {"payload":{"path":".","expected":"string","message":"expected '\"', offset: 0x00000000, buf:\n+----------+-------------------------------------------------+------------------+\n|          |  0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f | 0123456789abcdef |\n+----------+-------------------------------------------------+------------------+\n| 00000000 | 7b 7d                                           | {}               |\n+----------+-------------------------------------------------+------------------+"}}
[info] DEBUG: Errored: HttpPayloadError(., expected = string, message=expected '"', offset: 0x00000000, buf:
[info] +----------+-------------------------------------------------+------------------+
[info] |          |  0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f | 0123456789abcdef |
[info] +----------+-------------------------------------------------+------------------+
[info] | 00000000 | 7b 7d                                           | {}               |
[info] +----------+-------------------------------------------------+------------------+)

To reproduce:

$version: "2"

namespace hello

use alloy#simpleRestJson

@simpleRestJson
service WeatherService {
    operations: [PostWeather]
}

@http(method: "POST", uri: "/weather")
operation PostWeather {
    input := {
        @required
        @httpPayload
        city: String = "strange town"
    }
    output := {
        @httpPayload
        weather: String = "good weather"
    }
}

package hello

import cats.effect.IO
import cats.effect.IOApp
import hello.WeatherService
import org.http4s.HttpApp
import org.http4s.Method
import org.http4s.Request
import org.http4s.Response
import org.http4s.client.Client
import org.http4s.ember.client.EmberClientBuilder
import org.http4s.ember.server.EmberServerBuilder
import smithy4s.http4s.SimpleRestJsonBuilder

object Main extends IOApp.Simple {
  def serverDemo = SimpleRestJsonBuilder
    .routes(
      new WeatherService.Default[IO](IO.stub) {}: WeatherService[IO]
    )
    .resource
    .flatMap { routes =>
      EmberServerBuilder
        .default[IO]
        .withHttpApp(routes.orNotFound)
        .build
    }
    .use { server =>
      EmberClientBuilder.default[IO].build.use { client =>
        client
          .run(
            Request[IO](method = Method.POST, uri = server.baseUri / "weather")
          )
          .use { response =>
            response.bodyText.compile.string
              .debug(response.status.toString())
              .void
          }
      }
    }
    .void

  def clientDemo = SimpleRestJsonBuilder(WeatherService)
    .client(
      Client.fromHttpApp(HttpApp.pure[IO](Response[IO]()))
    )
    .resource
    .use(_.postWeather("NYC"))
    .debug()
    .attempt
    .void

  def run = serverDemo *> clientDemo
}

Full repro: https://github.com/kubukoz/demos/tree/smithy4s-required-payloads

[kubukoz]

I think this is the part responsible for the {} in the hexdump:

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

Line 86 in fc26f60

if (blob.isEmpty) readFromString("{}", jsoniterReaderConfig)(jcodec)

Update: a trivial "fix" proves that:

diff --git a/modules/json/src/smithy4s/json/internals/JsonPayloadCodecCompilerImpl.scala b/modules/json/src/smithy4s/json/internals/JsonPayloadCodecCompilerImpl.scala
index c02450ed8..b9b6c2f9b 100644
--- a/modules/json/src/smithy4s/json/internals/JsonPayloadCodecCompilerImpl.scala
+++ b/modules/json/src/smithy4s/json/internals/JsonPayloadCodecCompilerImpl.scala
@@ -71,19 +71,22 @@ private[json] case class JsonPayloadCodecCompilerImpl(
 
       def fromSchema[A](schema: Schema[A], cache: Cache): PayloadDecoder[A] = {
         val jcodec = jsoniterCodecCompiler.fromSchema(schema, cache)
-        new JsonPayloadDecoder(jcodec)
+        new JsonPayloadDecoder(jcodec, schema.getDefaultValue)
       }
 
       def fromSchema[A](schema: Schema[A]): PayloadDecoder[A] =
         fromSchema(schema, createCache())
     }
 
-  private class JsonPayloadDecoder[A](jcodec: JsonCodec[A])
+  private class JsonPayloadDecoder[A](jcodec: JsonCodec[A], default: Option[A])
       extends PayloadDecoder[A] {
     def decode(blob: Blob): Either[PayloadError, A] = {
       try {
         Right {
-          if (blob.isEmpty) readFromString("{}", jsoniterReaderConfig)(jcodec)
+          if (blob.isEmpty)
+            default.getOrElse(
+              readFromString("{}", jsoniterReaderConfig)(jcodec)
+            )
           else
             blob match {
               case b: Blob.ArraySliceBlob =>

so the question remains: is this something we should support?

[Baccata]

Why not. PR away

[ghostbuster91]

@kubukoz I will take a look at it. Can you assign me?