fix: paginator generator nullability bug caused by documents #1155
Conversation
This comment has been minimized.
This comment has been minimized.
0marperez
changed the title
0marperez
changed the title
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Correctness: Missing test coverage. Add unit tests as a minimum and preferably codegen tests as well.
| is MapShape -> { | ||
| val literal = ctx.symbolProvider.toSymbol(itemMember) | ||
| .expectProperty(SymbolProperty.ENTRY_EXPRESSION) as String + if (isSparse) "?" else "" | ||
| literal to itemMember | ||
| } |
There was a problem hiding this comment.
Question: Don't we also need to check if the symbol is nullable alongside the map being sparse?
There was a problem hiding this comment.
No, the nullability of documents in this case is handled by the KotlinSymbolProvider
https://github.com/smithy-lang/smithy-kotlin/blob/main/codegen/smithy-kotlin-codegen/src/main/kotlin/software/amazon/smithy/kotlin/codegen/core/KotlinSymbolProvider.kt#L168-L179
There was a problem hiding this comment.
Can you clarify a little bit? I'm confused why MapShape only checks isSparse while CollectionShape checks symbol.isNullable || isSparse
There was a problem hiding this comment.
Yeah, for MapShape the symbol provider provides the symbol via toSymbol and then we use the ENTRY_EXPRESSION system property from the symbol as the literal, and never look at the type that the map targets like in collections.
The ENTRY_EXPRESSION system property is set in the code I linked above. It uses val valueType which uses val valueSuffix to add the ? is the value is nullable.
There was a problem hiding this comment.
I don't think that's what we want. Your new unit test testRenderPaginatorWithSparseDocumentMap shows the problem: It takes a sparse map shape with Document values and codegens a paginator that returns Flow<Map.Entry<String, Document?>?>.
Note that there are two nullability modifiers:
- one on
DocumentbecauseDocumentis nullable, which is correct - one on
Map.Entry<String, Document?>because the map is sparse, which is incorrect
A sparse map does not have missing entries, only missing values. I believe the logic needs to coalesce the Document nullability and the map sparseness into the same spot. In other words, I believe the testRenderPaginatorWithSparseDocumentMap test should be expecting a Flow<Map.Entry<String, Document?>>.
There was a problem hiding this comment.
I noticed that too, we've been code-generating them like that so I wasn't 100% sure if it was wrong. The fix is in the PR now.
|
Yeah, this was my mistake I marked as ready for review before adding the tests! |
This comment has been minimized.
This comment has been minimized.
| literal to itemMember | ||
| } | ||
| is CollectionShape -> { | ||
| val symbol = ctx.symbolProvider.toSymbol(ctx.model.expectShape(itemMember.member.target)) |
There was a problem hiding this comment.
simplification/duplication: ctx.model.expectShape(itemMember.member.target) is used twice, refactor to local value
| { | ||
| "id": "f71f083b-6e5f-4a3c-9069-464b1f9f6d36", | ||
| "type": "bugfix", | ||
| "description": "Fix paginator generator `List<Document>` nullability" |
There was a problem hiding this comment.
nit/clarification: This change does not only affect List<Document>, right?
There was a problem hiding this comment.
It should only affect List<Document> types because they're the only type of list that can contain nullables without the sparse trait being set on the list. Looking at KotlinSymbolProvider, document is the only symbol that always returns asNullable()
| NextMarker: String | ||
| } | ||
|
|
||
| map FunctionConfigurationList { |
| is MapShape -> { | ||
| val literal = ctx.symbolProvider.toSymbol(itemMember) | ||
| .expectProperty(SymbolProperty.ENTRY_EXPRESSION) as String + if (isSparse) "?" else "" | ||
| literal to itemMember | ||
| } |
There was a problem hiding this comment.
Can you clarify a little bit? I'm confused why MapShape only checks isSparse while CollectionShape checks symbol.isNullable || isSparse
This comment has been minimized.
This comment has been minimized.
| @Test | ||
| fun testRenderPaginatorWithDocumentList() { |
There was a problem hiding this comment.
Nit: Let add a comment to each linking the issue that spawned them.
There was a problem hiding this comment.
I'll create one, we don't have an external issue for this
There was a problem hiding this comment.
Oh maybe not required if there wasn't an external issue...that's just busywork.
| is MapShape -> { | ||
| val literal = ctx.symbolProvider.toSymbol(itemMember) | ||
| .expectProperty(SymbolProperty.ENTRY_EXPRESSION) as String + if (isSparse) "?" else "" | ||
| literal to itemMember | ||
| } |
There was a problem hiding this comment.
I don't think that's what we want. Your new unit test testRenderPaginatorWithSparseDocumentMap shows the problem: It takes a sparse map shape with Document values and codegens a paginator that returns Flow<Map.Entry<String, Document?>?>.
Note that there are two nullability modifiers:
- one on
DocumentbecauseDocumentis nullable, which is correct - one on
Map.Entry<String, Document?>because the map is sparse, which is incorrect
A sparse map does not have missing entries, only missing values. I believe the logic needs to coalesce the Document nullability and the map sparseness into the same spot. In other words, I believe the testRenderPaginatorWithSparseDocumentMap test should be expecting a Flow<Map.Entry<String, Document?>>.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Affected ArtifactsNo artifacts changed size |
Issue #
The paginator generator is not code-generating nullable literals for item paginators of lists that have values that are nullable. It will only code-generate nullability if the list is marked as sparse and not check if the list has values that are nullable.
Lists are not supposed to contain nullable items unless marked as
sparse, so this is non-modeled behavior that we were not looking for but is necessary because documents are always nullable in smithy kotlin.Description of changes
The paginator generator now checks if the collection is made of nullable symbols and code generates nullability
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.