disneystreaming / smithy4s

https://disneystreaming.github.io/smithy4s/
Other
339 stars 68 forks source link

Non-required fields with default values not seriailizing the default value in a simpleRestJson #1405

Closed mattdavpir closed 5 months ago

mattdavpir commented 5 months ago

Hey, I'm not sure if this is intended behaviour or not, but it was unexpected from my end.

It seems that when I return a default value for a non-required field it is being omitted from the generated JSON, e.g. for this example:

MyService .smithy

$version: "2.0"

namespace com.test

use alloy#simpleRestJson

@simpleRestJson
@httpBearerAuth
service MyService {
    version: "1.1.0",
    operations: [Overview]
}

@http(method: "GET", uri: "/overview", code: 200)
operation Overview {
    output: OverviewTable
}

structure OverviewTable {
    items: OverviewResponses
}

list OverviewResponses {
    member: OverviewResponse
}

structure OverviewResponse {
    stringField: String,
    sortable: Boolean = true
}

build.sbt

ThisBuild / version := "0.1.0-SNAPSHOT"

ThisBuild / scalaVersion := "2.13.12"

lazy val root = (project in file("."))
  .settings(
    name := "smithy-test",
    libraryDependencies ++= Seq(
      Dependencies.smithy4sCore,
      Dependencies.smithy4sHttp4s,
      Dependencies.http4sDsl,
      Dependencies.http4sEmberServer,
      Dependencies.zio,
      Dependencies.zioInteropCats,
    ),
    addCompilerPlugin(("org.typelevel" % "kind-projector" % "0.13.2").cross(CrossVersion.full)),
  )
  .enablePlugins(Smithy4sCodegenPlugin)

Dependencies.scala

import sbt.*

object Dependencies {
  private val smithy4sV = "0.18.8"
  private val http4sV = "0.23.25"
  private val zioV = "2.0.21"
  private val zioInteropCatsV = "23.1.0.0"

  val smithy4sCore = "com.disneystreaming.smithy4s" %% "smithy4s-core" % smithy4sV
  val smithy4sHttp4s = "com.disneystreaming.smithy4s" %% "smithy4s-http4s" % smithy4sV
  val http4sDsl = "org.http4s" %% "http4s-dsl" % http4sV
  val http4sEmberServer = "org.http4s" %% "http4s-ember-server" % http4sV

  val zio = "dev.zio" %% "zio" % zioV
  val zioInteropCats = "dev.zio" %% "zio-interop-cats" % zioInteropCatsV

}

Main.scala


import com.comcast.ip4s.{Host, Port}
import com.test.{MyService, OverviewResponse, OverviewTable}
import org.http4s.ember.server.EmberServerBuilder
import smithy4s.Transformation
import smithy4s.http4s.SimpleRestJsonBuilder
import zio.interop.catz.asyncInstance
import zio.{RIO, Task, ZIO, ZIOAppDefault}

import scala.util.chaining.scalaUtilChainingOps

object Main extends ZIOAppDefault {
  override def run = {
    MyServiceImpl.routes.flatMap { routes =>
      EmberServerBuilder.default[Task]
        .withHost(Host.fromString("127.0.0.1").get)
        .withPort(Port.fromInt(8080).get)
        .withHttpApp(routes.orNotFound)
        .build
        .use(_ => ZIO.never)
    }
  }

  class MyServiceImpl extends MyService.ErrorAware[ZIO[Any, *, *]] {
    override def overview(): ZIO[Any, Nothing, OverviewTable] = ZIO.succeed(
      OverviewTable(
        Some(
          List(
            OverviewResponse(sortable = true, stringField = Some("Hooray")),
            OverviewResponse(sortable = true, stringField = None),
            OverviewResponse(sortable = false, stringField = Some("Boo")),
          )
        )
      )
    )
  }

  object MyServiceImpl {

    def routes = {
      val service = new MyServiceImpl().transform(absorbErrors)

      SimpleRestJsonBuilder
        .routes(service)
        .make
        .pipe(ZIO.fromEither(_).orDie)
    }

    private def absorbErrors = new Transformation.AbsorbError[ZIO[Any, *, *], RIO[Any, *]] {
      def apply[E, A](fa: ZIO[Any, E, A], injectError: E => Throwable): ZIO[Any, Throwable, A] =
        fa.mapError(injectError)
    }
  }
}

When I hit the endpoint the response is: {"items":[{"stringField":"Hooray"},{},{"sortable":false,"stringField":"Boo"}]}

I was expecting the "sortable": true to be included in the serialized objects, which was the behaviour in 0.17.8 (maybe an intentional change in v0.18.0, but I couldn't find a mention of it in the change logs). If I set @required on the sortable field it will serialize the default properly.

daddykotex commented 5 months ago

Hey @mattdavpir , hope you're great. You are right there were some changes in 0.18.x regarding default and optional fields during encoding.

You can configure the behaviour though:

SimpleRestJsonBuilder
        .routes(service)
        .withExplicitDefaultsEncoding(true)
        .make
        .pipe(ZIO.fromEither(_).orDie)

That should give you what you want.

daddykotex commented 5 months ago

Related https://github.com/disneystreaming/smithy4s/issues/1290 and https://github.com/disneystreaming/smithy4s/pull/1315

Baccata commented 5 months ago

The behaviour is indeed working as intended now: non-required fields are skipped during serialisation when they have the default value.

We ought to document the expected behaviour better, but I'm making the decision to close this issue.

That being said, thank you very much for the detailed issue, it is very much appreciated and I would love it if every issue opened by users was written with a similar level of attention.

mattdavpir commented 5 months ago

Ah brilliant, thanks for the quick responses!