Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Retain original field ordering in schemas #1427

Merged
merged 7 commits into from Mar 4, 2024
Merged

Retain original field ordering in schemas #1427

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
7137905
Retain the original ordering of fields in schemas
Baccata Mar 1, 2024
e8bd027
Regenerated bootstrapped
Baccata Mar 1, 2024
831c750
Update CHANGELOG
Baccata Mar 1, 2024
f0b79af
Avoid using curly brackets for `make` invocation
Baccata Mar 4, 2024
5bc7c6d
Regenerate bootstrapped code
Baccata Mar 4, 2024
ee99933
Fix rendering tests
Baccata Mar 4, 2024
ad23001
Merge remote-tracking branch 'origin/series/0.18' into retain-origina…
Baccata Mar 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions modules/codegen/src/smithy4s/codegen/internals/CollisionAvoidance.scala
View file
Open in desktop
Expand Up @@ -147,6 +147,7 @@ private[internals] object CollisionAvoidance {
field.name,
modType(field.tpe),
field.modifier,
field.originalIndex,
field.hints.map(modHint)
)
}
Expand Down
4 changes: 3 additions & 1 deletion modules/codegen/src/smithy4s/codegen/internals/IR.scala
View file
Open in desktop
Expand Up @@ -132,6 +132,7 @@ private[internals] case class Field(
realName: String,
tpe: Type,
modifier: Field.Modifier,
originalIndex: Int,
hints: List[Hint]
)

Expand Down Expand Up @@ -193,9 +194,10 @@ private[internals] object Field {
name: String,
tpe: Type,
modifier: Modifier,
originalIndex: Int,
hints: List[Hint] = Nil
): Field =
Field(name, name, tpe, modifier, hints)
Field(name, name, tpe, modifier, originalIndex, hints)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: named params


}

Expand Down
16 changes: 13 additions & 3 deletions modules/codegen/src/smithy4s/codegen/internals/Renderer.scala
View file
Open in desktop
Expand Up @@ -698,9 +698,19 @@ private[internals] class Renderer(compilationUnit: CompilationUnit) { self =>
renderProtocol(product.nameRef, hints),
newline,
renderLenses(product, hints),
locally {
val params =
fields.sortBy(_.originalIndex).map(fieldToRenderLine(_, noDefault = true)).intercalate(Line.comma)
val args = fields.map(f => Line(f.name)).intercalate(Line.comma)
lines(
line"// constructor using the original order from the spec",
line"private def make($params): ${product.nameRef} = ${product.nameRef}($args)"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question: I think it'd be valuable to have this in the public API, e.g. if someone really really wants to avoid getting bit by a default parameter (I've seen it happen). WDYT?

Copy link
Contributor Author

@Baccata Baccata Mar 4, 2024 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If people care about it, we have rendering options to prevent default parameters from being generated : https://disneystreaming.github.io/smithy4s/docs/codegen/customisation/default-rendering

I think the "correct" course of action would be to generate the case-class without default parameters, retaining the original order, and to create a custom apply method that would have the default parameters. The problem is that it'd also impact extractors, so I don't want to pull that trigger just yet. We could potentially roll it in 0.19, as people should be more careful about reading release notes.

I do not however want to expose this make method. I've toyed with the idea of actually turning it into a IndexedSeq[Any] => A, as it's general and could be captured in a potential GeneratedStructureCompanion interface, which could potentially help the Spark usecase. I don't want to make the generated make public because we may change it later on.

).when(fields.nonEmpty)
},
newline,
if (fields.nonEmpty) {
val renderedFields =
fields.map { case Field(fieldName, realName, tpe, modifier, hints) =>
fields.sortBy(_.originalIndex).map { case Field(fieldName, realName, tpe, modifier, _, hints) =>
val fieldBuilder = modifier.typeMod match {
case Field.TypeModification.None if modifier.required => "required"
case Field.TypeModification.None => "field"
Expand All @@ -727,7 +737,7 @@ private[internals] class Renderer(compilationUnit: CompilationUnit) { self =>
if (recursive) line"$recursive_($struct_" else line"$struct_"
line"${schemaImplicit}val schema: $Schema_[${product.nameRef}] = $definition"
.args(renderedFields)
.block(line"${product.nameRef}.apply")
.block(line"make")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick: I'd try to avoid the block here and pass this in the same line

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

f0b79af

.appendToLast(".withId(id).addHints(hints)")
.appendToLast(if (recursive) ")" else "")
} else {
Expand All @@ -737,7 +747,7 @@ private[internals] class Renderer(compilationUnit: CompilationUnit) { self =>
line"${schemaImplicit}val schema: $Schema_[${product.nameRef}] = $definition"
.args(renderedFields)
.block(
line"arr => new ${product.nameRef}".args(
line"arr => make".args(
fields.zipWithIndex.map { case (field, idx) =>
line"arr($idx).asInstanceOf[${Line.fieldType(field)}]"
}
Expand Down
13 changes: 7 additions & 6 deletions modules/codegen/src/smithy4s/codegen/internals/SmithyToIR.scala
View file
Open in desktop
Expand Up @@ -1035,12 +1035,13 @@ private[codegen] class SmithyToIR(model: Model, namespace: String) {
hintsExtractor(member) ++ default ++ noDefault
)
}
.zipWithIndex
.collect {
case (name, Some(tpe: Type.ExternalType), modifier, hints) =>
case ((name, Some(tpe: Type.ExternalType), modifier, hints), index) =>
val newHints = hints.filterNot(_ == tpe.refinementHint)
Field(name, tpe, modifier, newHints)
case (name, Some(tpe), modifier, hints) =>
Field(name, tpe, modifier, hints)
Field(name, tpe, modifier, index, newHints)
case ((name, Some(tpe), modifier, hints), index) =>
Field(name, tpe, modifier, index, hints)
}
.toList

Expand Down Expand Up @@ -1234,13 +1235,13 @@ private[codegen] class SmithyToIR(model: Model, namespace: String) {
val structFields = struct.getFieldsPlain
val fieldNames = struct.getFieldsPlain.map(_.name)
val fields: List[TypedNode.FieldTN[NodeAndType]] = structFields.map {
case Field(_, realName, tpe, mod, _)
case Field(_, realName, tpe, mod, _, _)
if mod.typeMod == Field.TypeModification.None =>
val node = map.get(realName).getOrElse {
mod.default.get.node
} // value or default must be present if type is not wrapped
TypedNode.FieldTN.RequiredTN(NodeAndType(node, tpe))
case Field(_, realName, tpe, _, _) =>
case Field(_, realName, tpe, _, _, _) =>
map.get(realName) match {
case Some(node) =>
TypedNode.FieldTN.OptionalSomeTN(NodeAndType(node, tpe))
Expand Down
27 changes: 18 additions & 9 deletions modules/codegen/test/src/smithy4s/codegen/internals/DefaultRenderModeSpec.scala
View file
Open in desktop
Expand Up @@ -64,6 +64,9 @@ final class DefaultRenderModeSpec extends munit.FunSuite {
|
| val hints: Hints = Hints.empty
|
| // constructor using the original order from the spec
| private def make(one: Option[String], two: String, three: String, four: String, five: Option[Nullable[String]], six: Nullable[String], seven: Nullable[String], eight: Nullable[String]): Test = Test(one, two, three, four, five, six, seven, eight)
|
| implicit val schema: Schema[Test] = struct(
| string.optional[Test]("one", _.one),
| string.field[Test]("two", _.two).addHints(smithy.api.Default(smithy4s.Document.fromString("test"))),
Expand All @@ -74,7 +77,7 @@ final class DefaultRenderModeSpec extends munit.FunSuite {
| string.nullable.field[Test]("seven", _.seven).addHints(smithy.api.Default(smithy4s.Document.nullDoc)),
| string.nullable.required[Test]("eight", _.eight),
| ){
| Test.apply
| make
| }.withId(id).addHints(hints)
|}""".stripMargin

Expand Down Expand Up @@ -127,17 +130,20 @@ final class DefaultRenderModeSpec extends munit.FunSuite {
|
| val hints: Hints = Hints.empty
|
| // constructor using the original order from the spec
| private def make(one: Option[String], two: String, three: String, four: String, five: Option[Nullable[String]], six: Nullable[String], seven: Nullable[String], eight: Nullable[String]): Test = Test(two, three, four, six, seven, eight, one, five)
|
| implicit val schema: Schema[Test] = struct(
| string.optional[Test]("one", _.one),
| string.field[Test]("two", _.two).addHints(smithy.api.Default(smithy4s.Document.fromString("test"))),
| string.required[Test]("three", _.three),
| string.required[Test]("four", _.four).addHints(smithy.api.Default(smithy4s.Document.fromString("test"))),
| string.nullable.optional[Test]("five", _.five),
| string.nullable.field[Test]("six", _.six).addHints(smithy.api.Default(smithy4s.Document.fromString("test"))),
| string.nullable.field[Test]("seven", _.seven).addHints(smithy.api.Default(smithy4s.Document.nullDoc)),
| string.nullable.required[Test]("eight", _.eight),
| string.optional[Test]("one", _.one),
| string.nullable.optional[Test]("five", _.five),
| ){
| Test.apply
| make
| }.withId(id).addHints(hints)
|}""".stripMargin

Expand Down Expand Up @@ -192,17 +198,20 @@ final class DefaultRenderModeSpec extends munit.FunSuite {
|
| val hints: Hints = Hints.empty
|
| // constructor using the original order from the spec
| private def make(one: Option[String], two: String, three: String, four: String, five: Option[Nullable[String]], six: Nullable[String], seven: Nullable[String], eight: Nullable[String]): Test = Test(three, eight, two, four, six, seven, one, five)
|
| implicit val schema: Schema[Test] = struct(
| string.required[Test]("three", _.three),
| string.nullable.required[Test]("eight", _.eight),
| string.optional[Test]("one", _.one),
| string.field[Test]("two", _.two).addHints(smithy.api.Default(smithy4s.Document.fromString("test"))),
| string.required[Test]("three", _.three),
| string.required[Test]("four", _.four).addHints(smithy.api.Default(smithy4s.Document.fromString("test"))),
| string.nullable.optional[Test]("five", _.five),
| string.nullable.field[Test]("six", _.six).addHints(smithy.api.Default(smithy4s.Document.fromString("test"))),
| string.nullable.field[Test]("seven", _.seven).addHints(smithy.api.Default(smithy4s.Document.nullDoc)),
| string.optional[Test]("one", _.one),
| string.nullable.optional[Test]("five", _.five),
| string.nullable.required[Test]("eight", _.eight),
| ){
| Test.apply
| make
| }.withId(id).addHints(hints)
|}
|""".stripMargin
Expand Down
15 changes: 12 additions & 3 deletions modules/codegen/test/src/smithy4s/codegen/transformers/AwsStandardTypesTransformerSpec.scala
View file
Open in desktop
Expand Up @@ -87,12 +87,15 @@ final class AwsStandardTypesTransformerSpec extends munit.FunSuite {
|
| val hints: Hints = Hints.empty
|
| // constructor using the original order from the spec
| private def make(i: Int, d: Option[Date], l: Option[Long]): TestStructure = TestStructure(i, d, l)
|
| implicit val schema: Schema[TestStructure] = struct(
| int.required[TestStructure]("i", _.i),
| Date.schema.optional[TestStructure]("d", _.d),
| Long.schema.optional[TestStructure]("l", _.l),
| ){
| TestStructure.apply
| make
| }.withId(id).addHints(hints)
|}""".stripMargin
)
Expand Down Expand Up @@ -149,10 +152,13 @@ final class AwsStandardTypesTransformerSpec extends munit.FunSuite {
|
| val hints: Hints = Hints.empty
|
| // constructor using the original order from the spec
| private def make(s: Option[String]): TestStructure = TestStructure(s)
|
| implicit val schema: Schema[TestStructure] = struct(
| string.validated(smithy.api.Length(min = Some(5L), max = Some(10L))).optional[TestStructure]("s", _.s),
| ){
| TestStructure.apply
| make
| }.withId(id).addHints(hints)
|}""".stripMargin
)
Expand Down Expand Up @@ -211,10 +217,13 @@ final class AwsStandardTypesTransformerSpec extends munit.FunSuite {
|
| val hints: Hints = Hints.empty
|
| // constructor using the original order from the spec
| private def make(i: Int): TestStructure = TestStructure(i)
|
| implicit val schema: Schema[TestStructure] = struct(
| int.field[TestStructure]("i", _.i).addHints(smithy.api.Default(smithy4s.Document.fromDouble(5.0d))),
| ){
| TestStructure.apply
| make
| }.withId(id).addHints(hints)
|}""".stripMargin
)
Expand Down