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

Produce &[T] instead of Option<&[T]> for list fields #2995

Merged
merged 6 commits into from
Sep 28, 2023
Merged

Produce &[T] instead of Option<&[T]> for list fields #2995

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
e7424f0
Produce `&[T]` instead of `Option<&[T]>` for list fields
rcoh Sep 21, 2023
103f35e
Fix various examples
rcoh Sep 21, 2023
ada8fcb
Make list flattening configurable
rcoh Sep 22, 2023
83ea856
Fix defaults for servers
rcoh Sep 22, 2023
7a939a9
Remove default for parameter and ensure its specified everywhere
rcoh Sep 22, 2023
ed5a19d
Merge branch 'main' into auto-unwrap-or-default
rcoh Sep 26, 2023
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
22 changes: 22 additions & 0 deletions CHANGELOG.next.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -178,6 +178,28 @@ references = ["smithy-rs#2985"]
meta = { "breaking" = false, "tada" = false, "bug" = true, "target" = "client" }
author = "rcoh"

[[smithy-rs]]
message = """
Structure members with the type `Option<Vec<T>>` now produce an accessor with the type `&[T]` instead of `Option<&[T]>`. This is enabled by default for clients and can be disabled by updating your smithy-build.json with the following setting:
```json
{
"codegen": {
"flattenCollectionAccessors": false,
...
}
}
```
"""
references = ["smithy-rs#2995"]
author = "rcoh"
meta = { "breaking" = true, "tada" = false, "bug" = false, "target" = "all" }

[[aws-sdk-rust]]
message = "Structure members with the type `Option<Vec<T>>` now produce an accessor with the type `&[T]` instead of `Option<&[T]>`. To determine if the field was actually set use `.<field_name>.is_some()`."
references = ["smithy-rs#2995"]
author = "rcoh"
meta = { "breaking" = true, "tada" = false, "bug" = false }

[[smithy-rs]]
message = "Produce better docs when items are marked @required"
references = ["smithy-rs#2996"]
Expand Down
8 changes: 4 additions & 4 deletions aws/sdk/integration-tests/dynamodb/tests/retries-with-client-rate-limiting.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,14 +86,14 @@ async fn test_adaptive_retries_with_no_throttling_errors() {
let client = aws_sdk_dynamodb::Client::from_conf(config.clone());
let res = client.list_tables().send().await.unwrap();
assert_eq!(sleep_impl.total_duration(), Duration::from_secs(3));
assert_eq!(res.table_names(), Some(expected_table_names.as_slice()));
assert_eq!(res.table_names(), expected_table_names.as_slice());
// Three requests should have been made, two failing & one success
assert_eq!(conn.requests().len(), 3);

let client = aws_sdk_dynamodb::Client::from_conf(config.clone());
let res = client.list_tables().send().await.unwrap();
assert_eq!(sleep_impl.total_duration(), Duration::from_secs(3 + 1));
assert_eq!(res.table_names(), Some(expected_table_names.as_slice()));
assert_eq!(res.table_names(), expected_table_names.as_slice());
// Two requests should have been made, one failing & one success (plus previous requests)
assert_eq!(conn.requests().len(), 5);

Expand Down Expand Up @@ -141,15 +141,15 @@ async fn test_adaptive_retries_with_throttling_errors() {
let client = aws_sdk_dynamodb::Client::from_conf(config.clone());
let res = client.list_tables().send().await.unwrap();
assert_eq!(sleep_impl.total_duration(), Duration::from_secs(40));
assert_eq!(res.table_names(), Some(expected_table_names.as_slice()));
assert_eq!(res.table_names(), expected_table_names.as_slice());
// Three requests should have been made, two failing & one success
assert_eq!(conn.requests().len(), 3);

let client = aws_sdk_dynamodb::Client::from_conf(config.clone());
let res = client.list_tables().send().await.unwrap();
assert!(Duration::from_secs(48) < sleep_impl.total_duration());
assert!(Duration::from_secs(49) > sleep_impl.total_duration());
assert_eq!(res.table_names(), Some(expected_table_names.as_slice()));
assert_eq!(res.table_names(), expected_table_names.as_slice());
// Two requests should have been made, one failing & one success (plus previous requests)
assert_eq!(conn.requests().len(), 5);
}
2 changes: 1 addition & 1 deletion aws/sdk/integration-tests/webassembly/src/list_buckets.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ pub async fn s3_list_buckets() {
let shared_config = get_default_config().await;
let client = Client::new(&shared_config);
let result = client.list_buckets().send().await.unwrap();
assert_eq!(result.buckets().unwrap().len(), 2)
assert_eq!(result.buckets().len(), 2)
}

#[tokio::test]
Expand Down
2 changes: 2 additions & 0 deletions ...src/main/kotlin/software/amazon/smithy/rust/codegen/client/smithy/ClientCodegenVisitor.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -223,6 +223,7 @@ class ClientCodegenVisitor(
this,
shape,
codegenDecorator.structureCustomizations(codegenContext, emptyList()),
structSettings = codegenContext.structSettings(),
).render()

implBlock(symbolProvider.toSymbol(shape)) {
Expand All @@ -246,6 +247,7 @@ class ClientCodegenVisitor(
shape,
errorTrait,
codegenDecorator.errorImplCustomizations(codegenContext, emptyList()),
codegenContext.structSettings(),
)
errorGenerator::renderStruct to errorGenerator::renderBuilder
}
Expand Down
7 changes: 6 additions & 1 deletion ...t/src/main/kotlin/software/amazon/smithy/rust/codegen/client/smithy/ClientRustSettings.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,7 @@ data class ClientRustSettings(
data class ClientCodegenConfig(
override val formatTimeoutSeconds: Int = defaultFormatTimeoutSeconds,
override val debugMode: Boolean = defaultDebugMode,
override val flattenCollectionAccessors: Boolean = defaultFlattenAccessors,
val nullabilityCheckMode: NullableIndex.CheckMode = NullableIndex.CheckMode.CLIENT,
val renameExceptions: Boolean = defaultRenameExceptions,
val includeFluentClient: Boolean = defaultIncludeFluentClient,
Expand All @@ -92,7 +93,7 @@ data class ClientCodegenConfig(
val includeEndpointUrlConfig: Boolean = defaultIncludeEndpointUrlConfig,
val enableUserConfigurableRuntimePlugins: Boolean = defaultEnableUserConfigurableRuntimePlugins,
) : CoreCodegenConfig(
formatTimeoutSeconds, debugMode,
formatTimeoutSeconds, debugMode, defaultFlattenAccessors,
) {
companion object {
private const val defaultRenameExceptions = true
Expand All @@ -103,10 +104,14 @@ data class ClientCodegenConfig(
private const val defaultEnableUserConfigurableRuntimePlugins = true
private const val defaultNullabilityCheckMode = "CLIENT"

// Note: only clients default to true, servers default to false
private const val defaultFlattenAccessors = true

fun fromCodegenConfigAndNode(coreCodegenConfig: CoreCodegenConfig, node: Optional<ObjectNode>) =
if (node.isPresent) {
ClientCodegenConfig(
formatTimeoutSeconds = coreCodegenConfig.formatTimeoutSeconds,
flattenCollectionAccessors = node.get().getBooleanMemberOrDefault("flattenCollectionAccessors", defaultFlattenAccessors),
debugMode = coreCodegenConfig.debugMode,
eventStreamAllowList = node.get().getArrayMember("eventStreamAllowList").map { array ->
array.toList().mapNotNull { node ->
Expand Down
3 changes: 3 additions & 0 deletions ...tlin/software/amazon/smithy/rust/codegen/client/smithy/generators/error/ErrorGenerator.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import software.amazon.smithy.rust.codegen.core.smithy.RustSymbolProvider
import software.amazon.smithy.rust.codegen.core.smithy.generators.BuilderCustomization
import software.amazon.smithy.rust.codegen.core.smithy.generators.BuilderGenerator
import software.amazon.smithy.rust.codegen.core.smithy.generators.BuilderSection
import software.amazon.smithy.rust.codegen.core.smithy.generators.StructSettings
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: which namespace do the rest of the settings lie in? For example, we don't have a software.amazon.smithy.rust.codegen.core.smithy.generators.settings namespace?

import software.amazon.smithy.rust.codegen.core.smithy.generators.StructureCustomization
import software.amazon.smithy.rust.codegen.core.smithy.generators.StructureGenerator
import software.amazon.smithy.rust.codegen.core.smithy.generators.StructureSection
Expand All @@ -34,6 +35,7 @@ class ErrorGenerator(
private val shape: StructureShape,
private val error: ErrorTrait,
private val implCustomizations: List<ErrorImplCustomization>,
private val structSettings: StructSettings,
) {
private val runtimeConfig = symbolProvider.config.runtimeConfig
private val symbol = symbolProvider.toSymbol(shape)
Expand All @@ -59,6 +61,7 @@ class ErrorGenerator(
}
},
),
structSettings,
).render()

ErrorImplGenerator(
Expand Down
3 changes: 3 additions & 0 deletions ...en-core/src/main/kotlin/software/amazon/smithy/rust/codegen/core/smithy/CodegenContext.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import software.amazon.smithy.model.Model
import software.amazon.smithy.model.shapes.ServiceShape
import software.amazon.smithy.model.shapes.ShapeId
import software.amazon.smithy.rust.codegen.core.smithy.generators.BuilderInstantiator
import software.amazon.smithy.rust.codegen.core.smithy.generators.StructSettings

/**
* [CodegenContext] contains code-generation context that is _common to all_ smithy-rs plugins.
Expand Down Expand Up @@ -91,5 +92,7 @@ abstract class CodegenContext(
"A ModuleDocProvider must be set on the CodegenContext"
}

fun structSettings() = StructSettings(settings.codegenConfig.flattenCollectionAccessors)

abstract fun builderInstantiator(): BuilderInstantiator
}
3 changes: 3 additions & 0 deletions ...-core/src/main/kotlin/software/amazon/smithy/rust/codegen/core/smithy/CoreRustSettings.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,16 +41,19 @@ const val CODEGEN_SETTINGS = "codegen"
open class CoreCodegenConfig(
open val formatTimeoutSeconds: Int = defaultFormatTimeoutSeconds,
open val debugMode: Boolean = defaultDebugMode,
open val flattenCollectionAccessors: Boolean = defaultFlattenMode,
drganjoo marked this conversation as resolved.
Show resolved Hide resolved
) {
companion object {
const val defaultFormatTimeoutSeconds = 20
const val defaultDebugMode = false
const val defaultFlattenMode = false

fun fromNode(node: Optional<ObjectNode>): CoreCodegenConfig =
if (node.isPresent) {
CoreCodegenConfig(
formatTimeoutSeconds = node.get().getNumberMemberOrDefault("formatTimeoutSeconds", defaultFormatTimeoutSeconds).toInt(),
debugMode = node.get().getBooleanMemberOrDefault("debugMode", defaultDebugMode),
flattenCollectionAccessors = node.get().getBooleanMemberOrDefault("flattenCollectionAccessors", defaultFlattenMode),
Copy link
Contributor

Choose a reason for hiding this comment

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

Will this change allow someone on the server side to override how we generate accessor methods for collection types by setting "flattenCollectionAccessor" in smithy-build.json?

If the user cannot set this in the settings, then disregard the following:

Currently, on the server side that behavior is controlled by whether the field is marked as @required in the model.

This approach is fine for optional server inputs, where not sending a value for a collection (resulting in None) and sending an empty collection are treated the same by the business logic. However, there are scenarios where the server's output could make a meaningful distinction between an empty list and a None list. Additionally, using $memberName.is_none() on the client side to check whether the server sent an output will be unreliable if the server sends an empty list instead of None.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this is only the accessor method—it has absolutely no impact on serialization, it's just for convenience when reading the field.

Additionally, using $memberName.is_none() on the client side to check whether the server sent an output will be unreliable if the server sends an empty list instead of None.

The only reason to do that is explicitly if you are trying to tell the difference.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh yes! only the accessor gets changed. The underlying serialization format and the field remains the same.

)
} else {
CoreCodegenConfig(
Expand Down
23 changes: 21 additions & 2 deletions ...n/kotlin/software/amazon/smithy/rust/codegen/core/smithy/generators/StructureGenerator.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,12 +16,14 @@ import software.amazon.smithy.rust.codegen.core.rustlang.RustWriter
import software.amazon.smithy.rust.codegen.core.rustlang.asDeref
import software.amazon.smithy.rust.codegen.core.rustlang.asRef
import software.amazon.smithy.rust.codegen.core.rustlang.deprecatedShape
import software.amazon.smithy.rust.codegen.core.rustlang.docs
import software.amazon.smithy.rust.codegen.core.rustlang.documentShape
import software.amazon.smithy.rust.codegen.core.rustlang.isCopy
import software.amazon.smithy.rust.codegen.core.rustlang.isDeref
import software.amazon.smithy.rust.codegen.core.rustlang.render
import software.amazon.smithy.rust.codegen.core.rustlang.rust
import software.amazon.smithy.rust.codegen.core.rustlang.rustBlock
import software.amazon.smithy.rust.codegen.core.rustlang.stripOuter
import software.amazon.smithy.rust.codegen.core.smithy.RuntimeType
import software.amazon.smithy.rust.codegen.core.smithy.RustSymbolProvider
import software.amazon.smithy.rust.codegen.core.smithy.customize.NamedCustomization
Expand Down Expand Up @@ -53,12 +55,15 @@ sealed class StructureSection(name: String) : Section(name) {
/** Customizations for StructureGenerator */
abstract class StructureCustomization : NamedCustomization<StructureSection>()

data class StructSettings(val flattenVecAccessors: Boolean)

open class StructureGenerator(
val model: Model,
private val symbolProvider: RustSymbolProvider,
private val writer: RustWriter,
private val shape: StructureShape,
private val customizations: List<StructureCustomization>,
private val structSettings: StructSettings,
) {
companion object {
/** Reserved struct member names */
Expand Down Expand Up @@ -131,15 +136,26 @@ open class StructureGenerator(
writer.rustBlock("impl $name") {
// Render field accessor methods
forEachMember(accessorMembers) { member, memberName, memberSymbol ->
writer.renderMemberDoc(member, memberSymbol)
writer.deprecatedShape(member)
val memberType = memberSymbol.rustType()
var unwrapOrDefault = false
val returnType = when {
// Automatically flatten vecs
structSettings.flattenVecAccessors && memberType is RustType.Option && memberType.stripOuter<RustType.Option>() is RustType.Vec -> {
unwrapOrDefault = true
memberType.stripOuter<RustType.Option>().asDeref().asRef()
}
memberType.isCopy() -> memberType
memberType is RustType.Option && memberType.member.isDeref() -> memberType.asDeref()
memberType.isDeref() -> memberType.asDeref().asRef()
else -> memberType.asRef()
}
writer.renderMemberDoc(member, memberSymbol)
if (unwrapOrDefault) {
// Add a newline
writer.docs("")
writer.docs("If no value was sent for this field, a default will be set. If you want to determine if no value was sent, use `.$memberName.is_none()`.")
drganjoo marked this conversation as resolved.
Show resolved Hide resolved
}
writer.deprecatedShape(member)
writer.rustBlock("pub fn $memberName(&self) -> ${returnType.render()}") {
when {
memberType.isCopy() -> rust("self.$memberName")
Expand All @@ -148,6 +164,9 @@ open class StructureGenerator(
memberType.isDeref() -> rust("use std::ops::Deref; self.$memberName.deref()")
else -> rust("&self.$memberName")
}
if (unwrapOrDefault) {
rust(".unwrap_or_default()")
}
}
}
}
Expand Down
3 changes: 2 additions & 1 deletion ...gen-core/src/main/kotlin/software/amazon/smithy/rust/codegen/core/testutil/TestHelpers.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ import software.amazon.smithy.rust.codegen.core.smithy.RustSymbolProviderConfig
import software.amazon.smithy.rust.codegen.core.smithy.SymbolVisitor
import software.amazon.smithy.rust.codegen.core.smithy.generators.BuilderGenerator
import software.amazon.smithy.rust.codegen.core.smithy.generators.BuilderInstantiator
import software.amazon.smithy.rust.codegen.core.smithy.generators.StructSettings
import software.amazon.smithy.rust.codegen.core.smithy.generators.StructureGenerator
import software.amazon.smithy.rust.codegen.core.smithy.module
import software.amazon.smithy.rust.codegen.core.smithy.traits.SyntheticInputTrait
Expand Down Expand Up @@ -194,7 +195,7 @@ fun StructureShape.renderWithModelBuilder(
) {
val struct = this
rustCrate.withModule(symbolProvider.moduleForShape(struct)) {
StructureGenerator(model, symbolProvider, this, struct, emptyList()).render()
StructureGenerator(model, symbolProvider, this, struct, emptyList(), StructSettings(true)).render()
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we have a test someplace for StructSettings(false)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

actually no! let me add one, good catch. (Although all existing server tests are testing this)

implBlock(symbolProvider.toSymbol(struct)) {
BuilderGenerator.renderConvenienceMethod(this, symbolProvider, struct)
}
Expand Down
19 changes: 12 additions & 7 deletions ...kotlin/software/amazon/smithy/rust/codegen/core/smithy/generators/BuilderGeneratorTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,14 +7,17 @@ package software.amazon.smithy.rust.codegen.core.smithy.generators

import org.junit.jupiter.api.Test
import software.amazon.smithy.codegen.core.Symbol
import software.amazon.smithy.model.Model
import software.amazon.smithy.model.knowledge.NullableIndex
import software.amazon.smithy.model.shapes.Shape
import software.amazon.smithy.model.shapes.StructureShape
import software.amazon.smithy.rust.codegen.core.rustlang.Attribute.Companion.AllowDeprecated
import software.amazon.smithy.rust.codegen.core.rustlang.RustWriter
import software.amazon.smithy.rust.codegen.core.rustlang.implBlock
import software.amazon.smithy.rust.codegen.core.rustlang.rust
import software.amazon.smithy.rust.codegen.core.rustlang.rustTemplate
import software.amazon.smithy.rust.codegen.core.smithy.Default
import software.amazon.smithy.rust.codegen.core.smithy.RustSymbolProvider
import software.amazon.smithy.rust.codegen.core.smithy.WrappingSymbolProvider
import software.amazon.smithy.rust.codegen.core.smithy.setDefault
import software.amazon.smithy.rust.codegen.core.testutil.TestWorkspace
Expand All @@ -37,8 +40,8 @@ internal class BuilderGeneratorTest {
val project = TestWorkspace.testProject(provider)
project.moduleFor(inner) {
rust("##![allow(deprecated)]")
StructureGenerator(model, provider, this, inner, emptyList()).render()
StructureGenerator(model, provider, this, struct, emptyList()).render()
generator(model, provider, this, inner).render()
generator(model, provider, this, struct).render()
implBlock(provider.toSymbol(struct)) {
BuilderGenerator.renderConvenienceMethod(this, provider, struct)
}
Expand Down Expand Up @@ -73,8 +76,8 @@ internal class BuilderGeneratorTest {

project.moduleFor(StructureGeneratorTest.struct) {
AllowDeprecated.render(this)
StructureGenerator(model, provider, this, inner, emptyList()).render()
StructureGenerator(model, provider, this, struct, emptyList()).render()
generator(model, provider, this, inner).render()
generator(model, provider, this, struct).render()
implBlock(provider.toSymbol(struct)) {
BuilderGenerator.renderConvenienceMethod(this, provider, struct)
}
Expand All @@ -94,12 +97,14 @@ internal class BuilderGeneratorTest {
project.compileAndTest()
}

private fun generator(model: Model, provider: RustSymbolProvider, writer: RustWriter, shape: StructureShape) = StructureGenerator(model, provider, writer, shape, emptyList(), StructSettings(flattenVecAccessors = true))
drganjoo marked this conversation as resolved.
Show resolved Hide resolved

@Test
fun `builder for a struct with sensitive fields should implement the debug trait as such`() {
val provider = testSymbolProvider(model)
val project = TestWorkspace.testProject(provider)
project.moduleFor(credentials) {
StructureGenerator(model, provider, this, credentials, emptyList()).render()
generator(model, provider, this, credentials).render()
implBlock(provider.toSymbol(credentials)) {
BuilderGenerator.renderConvenienceMethod(this, provider, credentials)
}
Expand All @@ -126,7 +131,7 @@ internal class BuilderGeneratorTest {
val provider = testSymbolProvider(model)
val project = TestWorkspace.testProject(provider)
project.moduleFor(secretStructure) {
StructureGenerator(model, provider, this, secretStructure, emptyList()).render()
generator(model, provider, this, secretStructure).render()
implBlock(provider.toSymbol(secretStructure)) {
BuilderGenerator.renderConvenienceMethod(this, provider, secretStructure)
}
Expand Down Expand Up @@ -188,7 +193,7 @@ internal class BuilderGeneratorTest {
val project = TestWorkspace.testProject(provider)
val shape: StructureShape = model.lookup("com.test#MyStruct")
project.useShapeWriter(shape) {
StructureGenerator(model, provider, this, shape, listOf()).render()
generator(model, provider, this, shape).render()
BuilderGenerator(model, provider, shape, listOf()).render(this)
unitTest("test_defaults") {
rustTemplate(
Expand Down
Loading
Loading