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.

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

Update meaning of required in both codegen and runtime. #1301

Merged
merged 2 commits into from
Nov 22, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,7 +1,11 @@
# 0.18.4

* Changes the behaviour of `Field#getUnlessDefault` and `Field#foreachUnlessDefault` to always take the value into consideration when the `smithy.api#required` trait
is present on the field. This leads to field values being always serialised even when their values match their defaults, as this abides by least-surprise-principle.

* Fix sbt `smithy4sUpdateLSPConfig` and mill `smithy4s.codegen.LSP/updateConfig` rendering of repositories.


# 0.18.3

* Support constraint traits on members targeting enums
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ object AStructure extends ShapeTag.Companion[AStructure] {
val hints: Hints = Hints.empty

implicit val schema: Schema[AStructure] = struct(
AString.schema.required[AStructure]("astring", _.astring).addHints(smithy.api.Default(smithy4s.Document.fromString("\"Hello World\" with \"quotes\""))),
AString.schema.field[AStructure]("astring", _.astring).addHints(smithy.api.Default(smithy4s.Document.fromString("\"Hello World\" with \"quotes\""))),
){
AStructure.apply
}.withId(id).addHints(hints)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ object DefaultInMixinUsageTest extends ShapeTag.Companion[DefaultInMixinUsageTes
val hints: Hints = Hints.empty

implicit val schema: Schema[DefaultInMixinUsageTest] = struct(
string.required[DefaultInMixinUsageTest]("one", _.one).addHints(smithy.api.Default(smithy4s.Document.fromString("test"))),
string.field[DefaultInMixinUsageTest]("one", _.one).addHints(smithy.api.Default(smithy4s.Document.fromString("test"))),
){
DefaultInMixinUsageTest.apply
}.withId(id).addHints(hints)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ object DefaultOrderingTest extends ShapeTag.Companion[DefaultOrderingTest] {

implicit val schema: Schema[DefaultOrderingTest] = struct(
string.required[DefaultOrderingTest]("three", _.three),
int.required[DefaultOrderingTest]("one", _.one).addHints(smithy.api.Default(smithy4s.Document.fromDouble(1.0d))),
int.field[DefaultOrderingTest]("one", _.one).addHints(smithy.api.Default(smithy4s.Document.fromDouble(1.0d))),
string.optional[DefaultOrderingTest]("two", _.two),
){
DefaultOrderingTest.apply
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,24 +28,24 @@ object DefaultTest extends ShapeTag.Companion[DefaultTest] {
val hints: Hints = Hints.empty

implicit val schema: Schema[DefaultTest] = struct(
int.required[DefaultTest]("one", _.one).addHints(smithy.api.Default(smithy4s.Document.fromDouble(1.0d))),
string.required[DefaultTest]("two", _.two).addHints(smithy.api.Default(smithy4s.Document.fromString("test"))),
StringList.underlyingSchema.required[DefaultTest]("three", _.three).addHints(smithy.api.Default(smithy4s.Document.array())),
StringList.underlyingSchema.required[DefaultTest]("four", _.four).addHints(smithy.api.Default(smithy4s.Document.array())),
string.required[DefaultTest]("five", _.five).addHints(smithy.api.Default(smithy4s.Document.fromString(""))),
int.required[DefaultTest]("six", _.six).addHints(smithy.api.Box(), smithy.api.Default(smithy4s.Document.fromDouble(0.0d))),
document.required[DefaultTest]("seven", _.seven).addHints(smithy.api.Default(smithy4s.Document.nullDoc)),
DefaultStringMap.underlyingSchema.required[DefaultTest]("eight", _.eight).addHints(smithy.api.Default(smithy4s.Document.obj())),
short.required[DefaultTest]("nine", _.nine).addHints(smithy.api.Box(), smithy.api.Default(smithy4s.Document.fromDouble(0.0d))),
double.required[DefaultTest]("ten", _.ten).addHints(smithy.api.Box(), smithy.api.Default(smithy4s.Document.fromDouble(0.0d))),
float.required[DefaultTest]("eleven", _.eleven).addHints(smithy.api.Box(), smithy.api.Default(smithy4s.Document.fromDouble(0.0d))),
long.required[DefaultTest]("twelve", _.twelve).addHints(smithy.api.Box(), smithy.api.Default(smithy4s.Document.fromDouble(0.0d))),
timestamp.required[DefaultTest]("thirteen", _.thirteen).addHints(smithy.api.Default(smithy4s.Document.fromDouble(0.0d))),
timestamp.required[DefaultTest]("fourteen", _.fourteen).addHints(smithy.api.TimestampFormat.HTTP_DATE.widen, smithy.api.Default(smithy4s.Document.fromString("Thu, 01 Jan 1970 00:00:00 GMT"))),
timestamp.required[DefaultTest]("fifteen", _.fifteen).addHints(smithy.api.TimestampFormat.DATE_TIME.widen, smithy.api.Default(smithy4s.Document.fromString("1970-01-01T00:00:00.00Z"))),
byte.required[DefaultTest]("sixteen", _.sixteen).addHints(smithy.api.Box(), smithy.api.Default(smithy4s.Document.fromDouble(0.0d))),
bytes.required[DefaultTest]("seventeen", _.seventeen).addHints(smithy.api.Default(smithy4s.Document.array())),
boolean.required[DefaultTest]("eighteen", _.eighteen).addHints(smithy.api.Box(), smithy.api.Default(smithy4s.Document.fromBoolean(false))),
int.field[DefaultTest]("one", _.one).addHints(smithy.api.Default(smithy4s.Document.fromDouble(1.0d))),
string.field[DefaultTest]("two", _.two).addHints(smithy.api.Default(smithy4s.Document.fromString("test"))),
StringList.underlyingSchema.field[DefaultTest]("three", _.three).addHints(smithy.api.Default(smithy4s.Document.array())),
StringList.underlyingSchema.field[DefaultTest]("four", _.four).addHints(smithy.api.Default(smithy4s.Document.array())),
string.field[DefaultTest]("five", _.five).addHints(smithy.api.Default(smithy4s.Document.fromString(""))),
int.field[DefaultTest]("six", _.six).addHints(smithy.api.Box(), smithy.api.Default(smithy4s.Document.fromDouble(0.0d))),
document.field[DefaultTest]("seven", _.seven).addHints(smithy.api.Default(smithy4s.Document.nullDoc)),
DefaultStringMap.underlyingSchema.field[DefaultTest]("eight", _.eight).addHints(smithy.api.Default(smithy4s.Document.obj())),
short.field[DefaultTest]("nine", _.nine).addHints(smithy.api.Box(), smithy.api.Default(smithy4s.Document.fromDouble(0.0d))),
double.field[DefaultTest]("ten", _.ten).addHints(smithy.api.Box(), smithy.api.Default(smithy4s.Document.fromDouble(0.0d))),
float.field[DefaultTest]("eleven", _.eleven).addHints(smithy.api.Box(), smithy.api.Default(smithy4s.Document.fromDouble(0.0d))),
long.field[DefaultTest]("twelve", _.twelve).addHints(smithy.api.Box(), smithy.api.Default(smithy4s.Document.fromDouble(0.0d))),
timestamp.field[DefaultTest]("thirteen", _.thirteen).addHints(smithy.api.Default(smithy4s.Document.fromDouble(0.0d))),
timestamp.field[DefaultTest]("fourteen", _.fourteen).addHints(smithy.api.TimestampFormat.HTTP_DATE.widen, smithy.api.Default(smithy4s.Document.fromString("Thu, 01 Jan 1970 00:00:00 GMT"))),
timestamp.field[DefaultTest]("fifteen", _.fifteen).addHints(smithy.api.TimestampFormat.DATE_TIME.widen, smithy.api.Default(smithy4s.Document.fromString("1970-01-01T00:00:00.00Z"))),
byte.field[DefaultTest]("sixteen", _.sixteen).addHints(smithy.api.Box(), smithy.api.Default(smithy4s.Document.fromDouble(0.0d))),
bytes.field[DefaultTest]("seventeen", _.seventeen).addHints(smithy.api.Default(smithy4s.Document.array())),
boolean.field[DefaultTest]("eighteen", _.eighteen).addHints(smithy.api.Box(), smithy.api.Default(smithy4s.Document.fromBoolean(false))),
){
DefaultTest.apply
}.withId(id).addHints(hints)
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
package smithy4s.example

import smithy4s.Hints
import smithy4s.Schema
import smithy4s.ShapeId
import smithy4s.ShapeTag
import smithy4s.schema.Schema.string
import smithy4s.schema.Schema.struct

final case class DefaultVariants(req: String, reqDef: String = "default", optDef: String = "default", opt: Option[String] = None)

object DefaultVariants extends ShapeTag.Companion[DefaultVariants] {
val id: ShapeId = ShapeId("smithy4s.example", "DefaultVariants")

val hints: Hints = Hints.empty

implicit val schema: Schema[DefaultVariants] = struct(
string.required[DefaultVariants]("req", _.req),
string.required[DefaultVariants]("reqDef", _.reqDef).addHints(smithy.api.Default(smithy4s.Document.fromString("default"))),
string.field[DefaultVariants]("optDef", _.optDef).addHints(smithy.api.Default(smithy4s.Document.fromString("default"))),
string.optional[DefaultVariants]("opt", _.opt),
Comment on lines +18 to +21
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure I fully understand why field is used here rather than optional and it is also used in other places where required was used prior. Is field only used when a default value is present and the field is not required?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is field only used when a default value is present and the field is not required

Pretty much.

  • optional forces the type of the getter to be S => Option[A]
  • required lets you do S => A, but automatically enriches the hints with smithy.api.Required
  • field lets you do S => A, and doesn't enrich the hints with anything

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it yeah that makes sense

){
DefaultVariants.apply
}.withId(id).addHints(hints)
}
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ object HeadersWithDefaults extends ShapeTag.Companion[HeadersWithDefaults] {
val hints: Hints = Hints.empty

implicit val schema: Schema[HeadersWithDefaults] = struct(
string.required[HeadersWithDefaults]("dflt", _.dflt).addHints(smithy.api.Default(smithy4s.Document.fromString("test")), smithy.api.HttpHeader("dflt")),
string.field[HeadersWithDefaults]("dflt", _.dflt).addHints(smithy.api.Default(smithy4s.Document.fromString("test")), smithy.api.HttpHeader("dflt")),
){
HeadersWithDefaults.apply
}.withId(id).addHints(hints)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ object MixinOptionalMemberDefaultAdded extends ShapeTag.Companion[MixinOptionalM
val hints: Hints = Hints.empty

implicit val schema: Schema[MixinOptionalMemberDefaultAdded] = struct(
string.required[MixinOptionalMemberDefaultAdded]("a", _.a).addHints(smithy.api.Default(smithy4s.Document.fromString("test"))),
string.field[MixinOptionalMemberDefaultAdded]("a", _.a).addHints(smithy.api.Default(smithy4s.Document.fromString("test"))),
){
MixinOptionalMemberDefaultAdded.apply
}.withId(id).addHints(hints)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,13 +21,13 @@ object Numeric extends ShapeTag.Companion[Numeric] {
val hints: Hints = Hints.empty

implicit val schema: Schema[Numeric] = struct(
int.required[Numeric]("i", _.i).addHints(smithy.api.Default(smithy4s.Document.fromDouble(1.0d))),
float.required[Numeric]("f", _.f).addHints(smithy.api.Default(smithy4s.Document.fromDouble(1.0d))),
double.required[Numeric]("d", _.d).addHints(smithy.api.Default(smithy4s.Document.fromDouble(1.0d))),
short.required[Numeric]("s", _.s).addHints(smithy.api.Default(smithy4s.Document.fromDouble(1.0d))),
long.required[Numeric]("l", _.l).addHints(smithy.api.Default(smithy4s.Document.fromDouble(1.0d))),
bigint.required[Numeric]("bi", _.bi).addHints(smithy.api.Default(smithy4s.Document.fromDouble(1.0d))),
bigdecimal.required[Numeric]("bd", _.bd).addHints(smithy.api.Default(smithy4s.Document.fromDouble(1.0d))),
int.field[Numeric]("i", _.i).addHints(smithy.api.Default(smithy4s.Document.fromDouble(1.0d))),
float.field[Numeric]("f", _.f).addHints(smithy.api.Default(smithy4s.Document.fromDouble(1.0d))),
double.field[Numeric]("d", _.d).addHints(smithy.api.Default(smithy4s.Document.fromDouble(1.0d))),
short.field[Numeric]("s", _.s).addHints(smithy.api.Default(smithy4s.Document.fromDouble(1.0d))),
long.field[Numeric]("l", _.l).addHints(smithy.api.Default(smithy4s.Document.fromDouble(1.0d))),
bigint.field[Numeric]("bi", _.bi).addHints(smithy.api.Default(smithy4s.Document.fromDouble(1.0d))),
bigdecimal.field[Numeric]("bd", _.bd).addHints(smithy.api.Default(smithy4s.Document.fromDouble(1.0d))),
){
Numeric.apply
}.withId(id).addHints(hints)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ object QueriesWithDefaults extends ShapeTag.Companion[QueriesWithDefaults] {
val hints: Hints = Hints.empty

implicit val schema: Schema[QueriesWithDefaults] = struct(
string.required[QueriesWithDefaults]("dflt", _.dflt).addHints(smithy.api.Default(smithy4s.Document.fromString("test")), smithy.api.HttpQuery("dflt")),
string.field[QueriesWithDefaults]("dflt", _.dflt).addHints(smithy.api.Default(smithy4s.Document.fromString("test")), smithy.api.HttpQuery("dflt")),
){
QueriesWithDefaults.apply
}.withId(id).addHints(hints)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,17 +6,17 @@ import smithy4s.ShapeId
import smithy4s.ShapeTag
import smithy4s.schema.Schema.struct

final case class StructureWithRefinedTypes(requiredAge: Age, age: Option[Age] = None, personAge: Option[PersonAge] = None, fancyList: Option[smithy4s.example.FancyList] = None, unwrappedFancyList: Option[smithy4s.refined.FancyList] = None, name: Option[smithy4s.example.Name] = None, dogName: Option[smithy4s.refined.Name] = None)
final case class StructureWithRefinedTypes(age: Age, personAge: PersonAge, requiredAge: Age, fancyList: Option[smithy4s.example.FancyList] = None, unwrappedFancyList: Option[smithy4s.refined.FancyList] = None, name: Option[smithy4s.example.Name] = None, dogName: Option[smithy4s.refined.Name] = None)

object StructureWithRefinedTypes extends ShapeTag.Companion[StructureWithRefinedTypes] {
val id: ShapeId = ShapeId("smithy4s.example", "StructureWithRefinedTypes")

val hints: Hints = Hints.empty

implicit val schema: Schema[StructureWithRefinedTypes] = struct(
Age.schema.field[StructureWithRefinedTypes]("age", _.age).addHints(smithy.api.Default(smithy4s.Document.fromDouble(0.0d))),
PersonAge.schema.field[StructureWithRefinedTypes]("personAge", _.personAge).addHints(smithy.api.Default(smithy4s.Document.fromDouble(0.0d))),
Age.schema.required[StructureWithRefinedTypes]("requiredAge", _.requiredAge).addHints(smithy.api.Default(smithy4s.Document.fromDouble(0.0d))),
Age.schema.optional[StructureWithRefinedTypes]("age", _.age).addHints(smithy.api.Default(smithy4s.Document.fromDouble(0.0d))),
PersonAge.schema.optional[StructureWithRefinedTypes]("personAge", _.personAge).addHints(smithy.api.Default(smithy4s.Document.fromDouble(0.0d))),
smithy4s.example.FancyList.schema.optional[StructureWithRefinedTypes]("fancyList", _.fancyList),
UnwrappedFancyList.underlyingSchema.optional[StructureWithRefinedTypes]("unwrappedFancyList", _.unwrappedFancyList),
smithy4s.example.Name.schema.optional[StructureWithRefinedTypes]("name", _.name),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ object World extends ShapeTag.Companion[World] {
val hints: Hints = Hints.empty

implicit val schema: Schema[World] = struct(
string.required[World]("message", _.message).addHints(smithy.api.Default(smithy4s.Document.fromString("World !"))),
string.field[World]("message", _.message).addHints(smithy.api.Default(smithy4s.Document.fromString("World !"))),
){
World.apply
}.withId(id).addHints(hints)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ object World extends ShapeTag.Companion[World] {
val hints: Hints = Hints.empty

implicit val schema: Schema[World] = struct(
string.required[World]("message", _.message).addHints(smithy.api.Default(smithy4s.Document.fromString("World !"))),
string.field[World]("message", _.message).addHints(smithy.api.Default(smithy4s.Document.fromString("World !"))),
){
World.apply
}.withId(id).addHints(hints)
Expand Down
44 changes: 44 additions & 0 deletions modules/bootstrapped/test/src/smithy4s/DefaultSmokeSpec.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
/*
* Copyright 2021-2023 Disney Streaming
*
* Licensed under the Tomorrow Open Source Technology License, Version 1.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* https://disneystreaming.github.io/TOST-1.0.txt
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package smithy4s

import munit._
import smithy4s.example._

class DefaultSmokeSpec() extends FunSuite {

test(
"Fields with defaults do not get implicitly `smithy.api.Required` hints"
) {

val document = Document.encode(
DefaultVariants("default", "default", "default", Some("default"))
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 parameters would be very helpful

)

import Document._
// optDef field gets eluded because it matches the default value
val expected = obj(
"req" -> fromString("default"),
"reqDef" -> fromString("default"),
"opt" -> fromString("default")
)

assertEquals(document, expected)

}

}
37 changes: 37 additions & 0 deletions modules/bootstrapped/test/src/smithy4s/DocumentSpec.scala
Original file line number Diff line number Diff line change
Expand Up @@ -358,4 +358,41 @@ class DocumentSpec() extends FunSuite {
expect.same(decoded, Right(None))

}

test(
"fields marked with @required and @default should always be encoded"
) {
val requiredFieldSchema =
Schema
.struct[String](
Schema.string
.required[String]("test", identity)
.addHints(smithy.api.Default(Document.fromString("default")))
)(identity)

val encoded = Document.Encoder
.fromSchema(requiredFieldSchema)
.encode("default")

expect.same(encoded, Document.obj("test" -> Document.fromString("default")))
}

test(
"fields marked with @default but not @required should be skipped during encoding when matching default"
) {
val fieldSchema =
Schema
.struct[String](
Schema.string
.field[String]("test", identity)
.addHints(smithy.api.Default(Document.fromString("default")))
)(identity)

val encoded = Document.Encoder
.fromSchema(fieldSchema)
.encode("default")

expect.same(encoded, Document.obj())
}

}
30 changes: 28 additions & 2 deletions modules/bootstrapped/test/src/smithy4s/schema/FieldSpec.scala
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,9 @@ import scala.collection.mutable.ListBuffer

final class FieldSpec extends FunSuite {

test("foreachUnlessDefault") {
test(
"foreachUnlessDefault always applies the lambda when fields are required"
) {
case class Foo(a: String)
val field = string
.required[Foo]("a", _.a)
Expand All @@ -23,7 +25,31 @@ final class FieldSpec extends FunSuite {
result += foo
}

assertEquals(result.toList, List("test2"))
assertEquals(result.toList, List("test", "test2"))
}

test(
"foreachUnlessDefault skips the lambda if an optional field has a default value"
) {
case class Foo(a: Option[String])
val field = string
.optional[Foo]("a", _.a)
.addHints(Default(Document.fromString("test")))

val result = ListBuffer.empty[Option[String]]
field.foreachUnlessDefault(Foo(Some("test"))) { foo =>
result += foo
}

field.foreachUnlessDefault(Foo(Some("test2"))) { foo =>
result += foo
}

field.foreachUnlessDefault(Foo(None)) { foo =>
result += foo
}

assertEquals(result.toList, List(Some("test2"), None))
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,7 @@ private[internals] object CollisionAvoidance {
protectKeyword(uncapitalise(field.name)),
field.name,
modType(field.tpe),
field.required,
field.modifier,
field.hints.map(modHint)
)
}
Expand Down
30 changes: 27 additions & 3 deletions modules/codegen/src/smithy4s/codegen/internals/IR.scala
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,7 @@ private[internals] case class Field(
name: String,
realName: String,
tpe: Type,
required: Boolean,
modifier: Field.Modifier,
hints: List[Hint]
)

Expand All @@ -141,13 +141,37 @@ private[internals] case class StreamingField(

private[internals] object Field {

sealed trait Modifier {
Copy link
Contributor

Choose a reason for hiding this comment

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

great improvement

def default: Option[Node] = this match {
case Modifier.RequiredDefaultMod(node, _) => Some(node)
case Modifier.DefaultMod(node, _) => Some(node)
case Modifier.RequiredMod => None
case Modifier.NoModifier => None
}

def none: Boolean = this match {
case Modifier.RequiredDefaultMod(_, _) => false
case Modifier.DefaultMod(_, _) => false
case Modifier.RequiredMod => false
case Modifier.NoModifier => true
}
}
object Modifier {
case object NoModifier extends Modifier
case object RequiredMod extends Modifier
case class RequiredDefaultMod(node: Node, typedNode: Option[Fix[TypedNode]])
extends Modifier
case class DefaultMod(node: Node, typedNode: Option[Fix[TypedNode]])
extends Modifier
}

def apply(
name: String,
tpe: Type,
required: Boolean = true,
modifier: Modifier,
hints: List[Hint] = Nil
): Field =
Field(name, name, tpe, required, hints)
Field(name, name, tpe, modifier, hints)

}

Expand Down
Loading