disneystreaming / smithy4s

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

Respect original ordering of fields in code-generated schemas #1407

Open Baccata opened 4 months ago

Baccata commented 4 months ago

@kubukoz was right (... for once 😉).

During code-generation and by default, we re-order case class fields in order to improve the UX of the generated case classes : optional fields go to the end of the parameter-set, required-fields with defaults go before optional fields, required fields go at the front.

Thought this is good from a user's perspective, it loses potentially important information in the schema (in particular in binary protocols where ordering of fields matter). It is therefore important to try and retain the original ordering of fields in the generated smithy4s.schema.Schema instances that go in the companion object of case-classes.

In order to keep the rendering reasonably sane, I think a right course of action should be to :

  1. generate a private factory method in the companion object of case classes, that retains original ordering.
  2. call that factory method in the schema (instead of the case class's .apply)

For instance, assuming :

structure Foo {
  x: Integer 
  @required
  y: Integer
  z: Integer = 1
} 

we ought to generate something like :

case class Foo(y: Int, z: Int = 1, x: Option[Int] = None)
object Foo {
  private def make(x: Option[Int], y: Int, z: Int) = new Foo(y, z, x)   

  implicit val schema: Schema[Foo] = struct( 
    int.optional[Foo]("x", _.x),
    int.required[Foo]("y", _.y),
    int.field[Foo]("z", _.z).addHints(smithy.api.Default(...))) 
  )(make)
} 

For large structures (arity > 22), we should generate something like :

case class Foo(y: Int, z: Int = 1, x: Option[Int] = None)
object Foo {
  private def makeUnsafe(seq: IndexedSeq[Any]) : Foo = new Foo(
    seq(1).asInstanceOf[Int], 
    seq(2).asInstanceOf[Int], 
    seq(0).asInstanceOf[Option[Int]]
  )

  implicit val schema: Schema[Foo] = struct.genericArity(...)(makeUnsafe)
} 

Obviously, all of this needs to be aware of user-land settings related to the generation of default-parameters.

daddykotex commented 4 months ago

Friday, I also noticed something, unrelated to this particular issue but related to ordering of members: when we reorder, we don't reorder the scaladoc entry

I'm not sure it matters as entries for parameters are annotated with the member's name but it's confusing:

/**
* @param x ...
* @param y ...
* @param z ...
*/
case class Foo(y: Int, z: Int = 1, x: Option[Int] = None)

should we address this as well?

Baccata commented 4 months ago

mmm maybe ?

kubukoz commented 4 months ago

In order to keep the rendering reasonably sane, I think a right course of action should be to :

1. generate a private factory method in the companion object of case classes, that retains original ordering.

2. call that factory method in the schema (instead of the case class's `.apply`)

I buy that.