Add support for @nullable trait in order to support MergePatch usecases #1408
Conversation
The JSON codec will now check for a Nullable hint on the schema it is parsing and not immediately parse fields as None if present. Note that getting this to work with a new Nullable type does require adjusting the code checking whether a Schema is for an Option a little so that it can also include bijections from an option type.
This involves totally reworking the Field.Modifier logic, as the addition of Nullable makes the number of cases too complex to keep track of in an enum alone: the modifier logic now depends on * whether there is a @required annotation present * whether there is a @nullable annotation present * whether there is a default value present, and (for nullable) whether this value is null or a concrete value making for a total of ten theoretical cases resulting in four possible types in the Scala code. We model this in a case class and attempt to shift some of the logic, such as ordering or working out the wrapped type, to the class itself in order to avoid other parts of the code needing to work with its internals.
There is a mild tension here: on the one hand, we really want to test all the edge cases of different field modifier combinations to make sure the generation code works as wel expect. On the other, these combinations generally have only an extremely limited use or are just overly convoluted ways to specify the default behaviour, and so they shouldn't be in the sample specs people use to check usage. We handle this by providing two test cases: Patchable, which handles the merge patch behaviour this whole change is meant to support, and PatchableEdgeCases which is explicitly described as only there for testing purposes.
| @@ -25,7 +25,7 @@ object Endpoint extends ShapeTag.Companion[Endpoint] { | |||
|
|
|||
| implicit val schema: Schema[Endpoint] = struct( | |||
| string.required[Endpoint]("Address", _.address).addHints(smithy.api.Documentation("<p>IP address of the endpoint.</p>")), | |||
| long.required[Endpoint]("CachePeriodInMinutes", _.cachePeriodInMinutes).addHints(smithy.api.Default(smithy4s.Document.fromDouble(0.0d)), smithy.api.Documentation("<p>Endpoint cache time to live (TTL) value.</p>")), | |||
| long.required[Endpoint]("CachePeriodInMinutes", _.cachePeriodInMinutes).addHints(smithy.api.Documentation("<p>Endpoint cache time to live (TTL) value.</p>"), smithy.api.Default(smithy4s.Document.fromDouble(0.0d))), | |||
There was a problem hiding this comment.
I'm not exactly sure why the hint ordering changed on some of the generated code or whether that's a problem? If it is, I can dig into it.
There was a problem hiding this comment.
mmm it may depend on non-deterministic stuff (ordering of map and sets). Not a problem unless the CI starts shouting (which it might), in which case we'll force a ShapeId-based ordering or something.
| nullable: Boolean, | ||
| default: Option[Default] | ||
| ) { | ||
| def typeMod: TypeModification = |
There was a problem hiding this comment.
This case class is unfortunately complex, so I tried to make sure most places using it didn't actually need to destructure it. The typeMod field is part of that, since a bunch of consumers of this class only care about whether the resulting type is wrapped in an Option (or Nullable, or Option[Nullable[...]]).
I did look into converting the underlying type representation to a Nullable before reaching the point where the modifier was generated - in which case in theory nothing here should need to change - but I was scared off by the fact that nothing in that part of the code seemed to be doing modifications of the underlying shape's type. I can have a second look if people think it's better there, though - I'm still not happy with how complex the modifier logic is now. 🤷♀️
There was a problem hiding this comment.
I'll give it a think, but worst case scenario I'm not too bothered : the internal representation is implementation detail, and even though the complexity does increase with the "nullable" requirement, it's still acceptable imho
There was a problem hiding this comment.
I tried to think of alternative ways of modelling this but what potentially made the code look nicer (using composable binary flags, for instance) or custom extractors also lose us exhaustive pat-mat, so ... I'm fine with the seemingly ugliness if you can't find a better way of modelling it.
There was a problem hiding this comment.
Yeah, that was pretty much my conclusion too 😓 . At least typeMod got rid of a bunch of the places that would have needed to destructure this case class otherwise.
| line"override def getMessage(): $string_ = ${field.name}.value" | ||
| case field => | ||
| line"override def getMessage(): $string_ = ${field.name}.map(_.value).orNull" | ||
| val fetchLogic = field.modifier.typeMod match { |
There was a problem hiding this comment.
I still need to test that this works the way it should. (Even if part of me wants to say that anyone who annotates a field with @errorMessage @nullable @required @default or the like is getting what they deserve 🙈 )
There was a problem hiding this comment.
😆 yeah unfortunately this project is very much subject to Murphy's law, so I'm thankful with the additional patterns
There was a problem hiding this comment.
😂 OK, added another detailed smoke spec test that goes through the eight possible cases and makes sure it can retrieve the error message.
modules/codegen/src/smithy4s/codegen/internals/SmithyToIR.scala
Outdated
Show resolved
Hide resolved
| import smithy4s.schema.Schema.int | ||
| import smithy4s.schema.Schema.struct | ||
|
|
||
| final case class PatchableEdgeCases(required: Nullable[Int], requiredDefaultValue: Nullable[Int] = smithy4s.Nullable.Value(3), requiredDefaultNull: Nullable[Int] = Null, defaultValue: Nullable[Int] = smithy4s.Nullable.Value(5), defaultNull: Nullable[Int] = Null) |
There was a problem hiding this comment.
this is great, thank you !
| final def isOption: Boolean = this match { | ||
| case _: OptionSchema[_] => true | ||
| case BijectionSchema(underlying, _) => underlying.isOption // TODO AJ: does this have any side-effects in the other places .isOption is used? is it a breaking change? |
There was a problem hiding this comment.
I don't think .isOption is used anymore. It was probably used transiently, but I think the change is fine.
Probably worth adding a case RefinementSchema too, just in case.
There was a problem hiding this comment.
It shows up once in a place involving flexibleCollectionSupport in the JSON codec (
And good point on the RefinementSchema - will add that!
|
|
|
@astridej, one important aspect of this work is gonna be the addition of tests to this specification. Most encoders now use the I presume that this line may have to change in order to account for the |
Co-authored-by: Olivier Mélois <baccata64@gmail.com>
The error message rendering code for custom error message fields is different for every type modification and also for custom vs primitive types - we add a test case for every combination to be absolutely sure they're being rendered correctly.
This reverts commit 8edf430.
Although technically the FieldTN representations aren't correct, this part of the code should never use the @nullable annotation and it's considered acceptable to leave as is.
@Baccata I just had a look at this and wrote some tests. I can push them, but I should note that it looks like with the current implementation, the code is actually totally independent of
If I'm off-base here, let me know! |
This does not include every type class satisfied by Option but just the ones we think are most likely to be needed by a consumer.
We refactor foreachUnlessDefault so the logic is not duplicated across two methods, and add some tests to confirm that it works as expected on a nullable field.
I think you may be right, but I'd love for the assumption to be validated with a couple additional test cases. |
Including tests to make sure all traits and schemas behave as expected - this does unfortunately require trying to figure out whether a schema is Nullable at runtime, which is an awkward process.
|
I love the emojis in the checklist. Going to steal this for my own :P |
Not 100% sure why this compiled locally, but it looks like this should fix it?
It's acceptable for fields with a default value to be wrapped in Option in the dynamically generated code, and trying to align it with the static codegen behaviour is causing problems in the compliance tests. We remove that change and any tests that relied on it.
Apparently 2.13 is more picky about line length for scalafmt than the other Scala versions, and I hadn't formatted the test files right - this should fix it.
modules/docs/markdown/04-codegen/01-customisation/13-nullable-values.md
Outdated
Show resolved
Hide resolved
…values.md Co-authored-by: Olivier Mélois <baccata64@gmail.com>
There was a problem hiding this comment.
This is an amazing PR, both from the added value but also from the fact that it has not required much oversight, and also because it adds some pretty thorough tests.
I am extremely thankful for this 🥇
Before we merge, I'd appreciate if @kubukoz or @lewisjkl could offer a second pair of eyes for a review, considering it's a fairly big PR.
|
@Baccata thank you for the kind words! 😊 🥳 I'm glad the PR worked without major changes being needed myself - and that you appreciate my edge-case-driven testing approach :) And thank you for your quick review and good guidance! |
This PR is an update of #1151 to take into account the changes to default and required handling that have happened since and to also change the modelling a little in line with what was discussed in the Discord (in particular, modelling required vs nullable as independent of each other). The change was big enough that I ended up writing the code from scratch, but I definitely based it on the linked PR.
With the change, it is possible to mark a field as nullable in order to distinguish between absence and removal of a field. This is modelled in Scala via
Option[Nullable[A]]-Nullablebeing isomorphic toOptionand left as a separate class for clarity. Unfortunately, this opens up a can of worms regarding possible combinations with @required and @default, the vast majority of which make no sense but are still theoretically possible. I had to reworkField.Modifierto make sure all possible combinations were reflected and handled properly, and although I did my best to keep the resulting complexity from leaking out it does show up in some places 😬I think I've now got this in roughly the state the original PR was in, so opening a draft PR in order to get feedback while I work on the missing pieces.
PR Checklist (not all items are relevant to all PRs)