Skip to content

Commit

Permalink
Don't allow test-util feature in compile time dependencies (#2843)
Browse files Browse the repository at this point in the history
During the orchestrator implementation, a bug was introduced that always
enabled the `test-util` feature on `aws-smithy-runtime` in the generated
crates. This led to several pieces of non-test code relying on test-only
code, specifically around `SystemTime` implementing `TimeSource`.

This PR fixes that issue, and also fixes another issue where the
`RuntimeComponentsBuilder` was being initialized without a name for the
service config.

----

_By submitting this pull request, I confirm that you can use, modify,
copy, and redistribute this contribution, under the terms of your
choice._
  • Loading branch information
jdisanti authored Jul 13, 2023
1 parent 3d1c34d commit 26827f1
Show file tree
Hide file tree
Showing 23 changed files with 255 additions and 112 deletions.
3 changes: 2 additions & 1 deletion aws/rust-runtime/aws-config/src/provider_config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,7 @@ impl ProviderConfig {
/// Unlike [`ProviderConfig::empty`] where `env` and `fs` will use their non-mocked implementations,
/// this method will use an empty mock environment and an empty mock file system.
pub fn no_configuration() -> Self {
use aws_smithy_async::time::StaticTimeSource;
use std::collections::HashMap;
use std::time::UNIX_EPOCH;
let fs = Fs::from_raw_map(HashMap::new());
Expand All @@ -100,7 +101,7 @@ impl ProviderConfig {
profile_files: ProfileFiles::default(),
env,
fs,
time_source: SharedTimeSource::new(UNIX_EPOCH),
time_source: SharedTimeSource::new(StaticTimeSource::new(UNIX_EPOCH)),
connector: HttpConnector::Prebuilt(None),
sleep: None,
region: None,
Expand Down
3 changes: 1 addition & 2 deletions aws/rust-runtime/aws-config/src/sts/util.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ use aws_credential_types::provider::{self, error::CredentialsError};
use aws_credential_types::Credentials as AwsCredentials;
use aws_sdk_sts::types::Credentials as StsCredentials;

use aws_smithy_async::time::TimeSource;
use std::convert::TryFrom;
use std::time::{SystemTime, UNIX_EPOCH};

Expand Down Expand Up @@ -47,6 +46,6 @@ pub(crate) fn into_credentials(
/// provide a name for the session, the provider will choose a name composed of a base + a timestamp,
/// e.g. `profile-file-provider-123456789`
pub(crate) fn default_session_name(base: &str, ts: SystemTime) -> String {
let now = ts.now().duration_since(UNIX_EPOCH).expect("post epoch");
let now = ts.duration_since(UNIX_EPOCH).expect("post epoch");
format!("{}-{}", base, now.as_millis())
}
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@

package software.amazon.smithy.rustsdk

import software.amazon.smithy.rust.codegen.client.smithy.ClientRustModule
import software.amazon.smithy.rust.codegen.client.smithy.generators.client.CustomizableOperationCustomization
import software.amazon.smithy.rust.codegen.client.smithy.generators.client.CustomizableOperationSection
import software.amazon.smithy.rust.codegen.core.rustlang.CargoDependency
Expand All @@ -18,28 +19,70 @@ class CustomizableOperationTestHelpers(runtimeConfig: RuntimeConfig) :
CustomizableOperationCustomization() {
private val codegenScope = arrayOf(
*RuntimeType.preludeScope,
"AwsUserAgent" to AwsRuntimeType.awsHttp(runtimeConfig)
.resolve("user_agent::AwsUserAgent"),
"AwsUserAgent" to AwsRuntimeType.awsHttp(runtimeConfig).resolve("user_agent::AwsUserAgent"),
"BeforeTransmitInterceptorContextMut" to RuntimeType.beforeTransmitInterceptorContextMut(runtimeConfig),
"ConfigBag" to RuntimeType.configBag(runtimeConfig),
"ConfigBagAccessors" to RuntimeType.configBagAccessors(runtimeConfig),
"http" to CargoDependency.Http.toType(),
"InterceptorContext" to RuntimeType.interceptorContext(runtimeConfig),
"StaticRuntimePlugin" to RuntimeType.smithyRuntimeApi(runtimeConfig)
.resolve("client::runtime_plugin::StaticRuntimePlugin"),
"RuntimeComponentsBuilder" to RuntimeType.runtimeComponentsBuilder(runtimeConfig),
"SharedTimeSource" to CargoDependency.smithyAsync(runtimeConfig).withFeature("test-util").toType()
.resolve("time::SharedTimeSource"),
"SharedInterceptor" to RuntimeType.smithyRuntimeApi(runtimeConfig)
.resolve("client::interceptors::SharedInterceptor"),
"TestParamsSetterInterceptor" to CargoDependency.smithyRuntime(runtimeConfig).withFeature("test-util")
.toType().resolve("client::test_util::interceptors::TestParamsSetterInterceptor"),
"SharedInterceptor" to RuntimeType.smithyRuntimeApi(runtimeConfig).resolve("client::interceptors::SharedInterceptor"),
"SharedTimeSource" to CargoDependency.smithyAsync(runtimeConfig).toType().resolve("time::SharedTimeSource"),
"StaticRuntimePlugin" to RuntimeType.smithyRuntimeApi(runtimeConfig).resolve("client::runtime_plugin::StaticRuntimePlugin"),
"StaticTimeSource" to CargoDependency.smithyAsync(runtimeConfig).toType().resolve("time::StaticTimeSource"),
"TestParamsSetterInterceptor" to testParamsSetterInterceptor(),
)

// TODO(enableNewSmithyRuntimeCleanup): Delete this once test helpers on `CustomizableOperation` have been removed
private fun testParamsSetterInterceptor(): RuntimeType = RuntimeType.forInlineFun("TestParamsSetterInterceptor", ClientRustModule.Client.customize) {
rustTemplate(
"""
mod test_params_setter_interceptor {
use aws_smithy_runtime_api::box_error::BoxError;
use aws_smithy_runtime_api::client::interceptors::context::BeforeTransmitInterceptorContextMut;
use aws_smithy_runtime_api::client::interceptors::Interceptor;
use aws_smithy_runtime_api::client::runtime_components::RuntimeComponents;
use aws_smithy_types::config_bag::ConfigBag;
use std::fmt;
pub(super) struct TestParamsSetterInterceptor<F> { f: F }
impl<F> fmt::Debug for TestParamsSetterInterceptor<F> {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
write!(f, "TestParamsSetterInterceptor")
}
}
impl<F> TestParamsSetterInterceptor<F> {
pub fn new(f: F) -> Self { Self { f } }
}
impl<F> Interceptor for TestParamsSetterInterceptor<F>
where
F: Fn(&mut BeforeTransmitInterceptorContextMut<'_>, &mut ConfigBag) + Send + Sync + 'static,
{
fn modify_before_signing(
&self,
context: &mut BeforeTransmitInterceptorContextMut<'_>,
_runtime_components: &RuntimeComponents,
cfg: &mut ConfigBag,
) -> Result<(), BoxError> {
(self.f)(context, cfg);
Ok(())
}
}
}
use test_params_setter_interceptor::TestParamsSetterInterceptor;
""",
*codegenScope,
)
}

override fun section(section: CustomizableOperationSection): Writable =
writable {
if (section is CustomizableOperationSection.CustomizableOperationImpl) {
if (section.isRuntimeModeOrchestrator) {
// TODO(enableNewSmithyRuntimeCleanup): Delete these utilities
rustTemplate(
"""
##[doc(hidden)]
Expand All @@ -49,7 +92,7 @@ class CustomizableOperationTestHelpers(runtimeConfig: RuntimeConfig) :
#{StaticRuntimePlugin}::new()
.with_runtime_components(
#{RuntimeComponentsBuilder}::new("request_time_for_tests")
.with_time_source(Some(#{SharedTimeSource}::new(request_time)))
.with_time_source(Some(#{SharedTimeSource}::new(#{StaticTimeSource}::new(request_time))))
)
)
}
Expand Down Expand Up @@ -92,7 +135,9 @@ class CustomizableOperationTestHelpers(runtimeConfig: RuntimeConfig) :
##[doc(hidden)]
// This is a temporary method for testing. NEVER use it in production
pub fn request_time_for_tests(mut self, request_time: ::std::time::SystemTime) -> Self {
self.operation.properties_mut().insert(#{SharedTimeSource}::new(request_time));
self.operation.properties_mut().insert(
#{SharedTimeSource}::new(#{StaticTimeSource}::new(request_time))
);
self
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -216,10 +216,12 @@ class AwsInputPresignedMethod(
"""
// Change signature type to query params and wire up presigning config
let mut props = request.properties_mut();
props.insert(#{SharedTimeSource}::new(presigning_config.start_time()));
props.insert(#{SharedTimeSource}::new(#{StaticTimeSource}::new(presigning_config.start_time())));
""",
"SharedTimeSource" to RuntimeType.smithyAsync(runtimeConfig)
.resolve("time::SharedTimeSource"),
"StaticTimeSource" to RuntimeType.smithyAsync(runtimeConfig)
.resolve("time::StaticTimeSource"),
)
withBlock("props.insert(", ");") {
rustTemplate(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -80,14 +80,14 @@ class IntegrationTestDependencies(
override fun section(section: LibRsSection) = when (section) {
is LibRsSection.Body -> testDependenciesOnly {
if (hasTests) {
val smithyClient = CargoDependency.smithyClient(codegenContext.runtimeConfig)
.copy(features = setOf("test-util"), scope = DependencyScope.Dev)
val smithyAsync = CargoDependency.smithyAsync(codegenContext.runtimeConfig)
.copy(features = setOf("test-util"), scope = DependencyScope.Dev)
val smithyClient = CargoDependency.smithyClient(codegenContext.runtimeConfig)
.copy(features = setOf("test-util"), scope = DependencyScope.Dev)
val smithyTypes = CargoDependency.smithyTypes(codegenContext.runtimeConfig)
.copy(features = setOf("test-util"), scope = DependencyScope.Dev)
addDependency(smithyClient)
addDependency(smithyAsync)
addDependency(smithyClient)
addDependency(smithyTypes)
addDependency(CargoDependency.smithyProtocolTestHelpers(codegenContext.runtimeConfig))
addDependency(SerdeJson)
Expand Down
2 changes: 1 addition & 1 deletion aws/sdk/integration-tests/s3/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ aws-credential-types = { path = "../../build/aws-sdk/sdk/aws-credential-types",
aws-http = { path = "../../build/aws-sdk/sdk/aws-http" }
aws-sdk-s3 = { path = "../../build/aws-sdk/sdk/s3" }
aws-sdk-sts = { path = "../../build/aws-sdk/sdk/sts" }
aws-smithy-async = { path = "../../build/aws-sdk/sdk/aws-smithy-async", features = ["rt-tokio"] }
aws-smithy-async = { path = "../../build/aws-sdk/sdk/aws-smithy-async", features = ["test-util", "rt-tokio"] }
aws-smithy-client = { path = "../../build/aws-sdk/sdk/aws-smithy-client", features = ["test-util", "rustls"] }
aws-smithy-http = { path = "../../build/aws-sdk/sdk/aws-smithy-http" }
aws-smithy-protocol-test = { path = "../../build/aws-sdk/sdk/aws-smithy-protocol-test" }
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import software.amazon.smithy.rust.codegen.core.rustlang.docsOrFallback
import software.amazon.smithy.rust.codegen.core.rustlang.raw
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.rustBlockTemplate
import software.amazon.smithy.rust.codegen.core.rustlang.rustTemplate
import software.amazon.smithy.rust.codegen.core.rustlang.writable
import software.amazon.smithy.rust.codegen.core.smithy.RuntimeConfig
Expand Down Expand Up @@ -374,9 +375,10 @@ class ServiceConfigGenerator(
}

writer.docs("Builder for creating a `Config`.")
writer.raw("#[derive(Clone, Default)]")
if (runtimeMode.defaultToOrchestrator) {
Attribute(Attribute.derive(RuntimeType.Debug)).render(writer)
if (runtimeMode.defaultToMiddleware) {
writer.raw("#[derive(Clone, Default)]")
} else {
writer.raw("#[derive(Clone, Debug)]")
}
writer.rustBlock("pub struct Builder") {
if (runtimeMode.defaultToOrchestrator) {
Expand Down Expand Up @@ -406,6 +408,22 @@ class ServiceConfigGenerator(
""",
)
}
} else {
// Custom implementation of Default to give the runtime components builder a name
writer.rustBlockTemplate("impl #{Default} for Builder", *codegenScope) {
writer.rustTemplate(
"""
fn default() -> Self {
Self {
config: #{Default}::default(),
runtime_components: #{RuntimeComponentsBuilder}::new("service config"),
runtime_plugins: #{Default}::default(),
}
}
""",
*codegenScope,
)
}
}

writer.rustBlock("impl Builder") {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ import software.amazon.smithy.rust.codegen.core.testutil.integrationTest
class HttpAuthDecoratorTest {
private fun codegenScope(runtimeConfig: RuntimeConfig): Array<Pair<String, Any>> = arrayOf(
"TestConnection" to CargoDependency.smithyClient(runtimeConfig)
.withFeature("test-util").toType()
.toDevDependency().withFeature("test-util").toType()
.resolve("test_connection::TestConnection"),
"SdkBody" to RuntimeType.sdkBody(runtimeConfig),
)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ class MetadataCustomizationTest {
.resolve("client::runtime_components::RuntimeComponents"),
)
rustCrate.testModule {
addDependency(CargoDependency.Tokio.withFeature("test-util").toDevDependency())
addDependency(CargoDependency.Tokio.toDevDependency().withFeature("test-util"))
tokioTest("test_extract_metadata_via_customizable_operation") {
rustTemplate(
"""
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ internal class ConfigOverrideRuntimePluginGeneratorTest {
"RuntimePlugin" to RuntimeType.runtimePlugin(runtimeConfig),
)
rustCrate.testModule {
addDependency(CargoDependency.Tokio.withFeature("test-util").toDevDependency())
addDependency(CargoDependency.Tokio.toDevDependency().withFeature("test-util"))
tokioTest("test_operation_overrides_endpoint_resolver") {
rustTemplate(
"""
Expand Down Expand Up @@ -97,7 +97,7 @@ internal class ConfigOverrideRuntimePluginGeneratorTest {
"RuntimePlugin" to RuntimeType.runtimePlugin(runtimeConfig),
)
rustCrate.testModule {
addDependency(CargoDependency.Tokio.withFeature("test-util").toDevDependency())
addDependency(CargoDependency.Tokio.toDevDependency().withFeature("test-util"))
tokioTest("test_operation_overrides_http_connection") {
rustTemplate(
"""
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ class FluentClientGeneratorTest {
}
""",
"TestConnection" to CargoDependency.smithyClient(codegenContext.runtimeConfig)
.toDevDependency()
.withFeature("test-util").toType()
.resolve("test_connection::TestConnection"),
"SdkBody" to RuntimeType.sdkBody(codegenContext.runtimeConfig),
Expand Down Expand Up @@ -108,6 +109,7 @@ class FluentClientGeneratorTest {
}
""",
"TestConnection" to CargoDependency.smithyClient(codegenContext.runtimeConfig)
.toDevDependency()
.withFeature("test-util").toType()
.resolve("test_connection::TestConnection"),
"SdkBody" to RuntimeType.sdkBody(codegenContext.runtimeConfig),
Expand Down
3 changes: 1 addition & 2 deletions codegen-core/build.gradle.kts
Original file line number Diff line number Diff line change
Expand Up @@ -112,12 +112,11 @@ if (isTestingEnabled.toBoolean()) {
tasks.test {
useJUnitPlatform()
testLogging {
events("passed", "skipped", "failed")
events("failed")
exceptionFormat = TestExceptionFormat.FULL
showCauses = true
showExceptions = true
showStackTraces = true
showStandardStreams = true
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,11 @@ import software.amazon.smithy.rust.codegen.core.smithy.RuntimeType
import software.amazon.smithy.rust.codegen.core.util.dq
import java.nio.file.Path

sealed class DependencyScope {
object Dev : DependencyScope()
object Compile : DependencyScope()
object CfgUnstable : DependencyScope()
object Build : DependencyScope()
enum class DependencyScope {
Build,
CfgUnstable,
Compile,
Dev,
}

sealed class DependencyLocation
Expand Down Expand Up @@ -136,6 +136,8 @@ fun InlineDependency.toType() = RuntimeType(module.fullyQualifiedPath(), this)

data class Feature(val name: String, val default: Boolean, val deps: List<String>)

val DEV_ONLY_FEATURES = setOf("test-util")

/**
* A dependency on an internal or external Cargo Crate
*/
Expand All @@ -150,6 +152,12 @@ data class CargoDependency(
) : RustDependency(name) {
val key: Triple<String, DependencyLocation, DependencyScope> get() = Triple(name, location, scope)

init {
if (scope != DependencyScope.Dev && DEV_ONLY_FEATURES.any { features.contains(it) }) {
throw IllegalArgumentException("The `test-util` feature cannot be used outside of DependencyScope.Dev")
}
}

fun withFeature(feature: String): CargoDependency {
return copy(features = features.toMutableSet().apply { add(feature) })
}
Expand Down Expand Up @@ -205,7 +213,8 @@ data class CargoDependency(
attribs.add("features = [${joinToString(",") { it.dq() }}]")
}
}
return "$name = { ${attribs.joinToString(",")} }"
attribs.add("scope = $scope")
return "$name = { ${attribs.joinToString(", ")} }"
}

fun toType(): RuntimeType {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -498,6 +498,15 @@ class RustWriter private constructor(
}
}

fun toml(fileName: String, debugMode: Boolean = false): RustWriter =
RustWriter(
fileName,
namespace = "ignore",
commentCharacter = "#",
printWarning = false,
debugMode = debugMode,
)

private fun rawWriter(fileName: String, debugMode: Boolean): RustWriter =
RustWriter(
fileName,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import software.amazon.smithy.codegen.core.WriterDelegator
import software.amazon.smithy.model.Model
import software.amazon.smithy.model.shapes.Shape
import software.amazon.smithy.rust.codegen.core.rustlang.CargoDependency
import software.amazon.smithy.rust.codegen.core.rustlang.DEV_ONLY_FEATURES
import software.amazon.smithy.rust.codegen.core.rustlang.DependencyScope
import software.amazon.smithy.rust.codegen.core.rustlang.Feature
import software.amazon.smithy.rust.codegen.core.rustlang.InlineDependency
Expand Down Expand Up @@ -298,7 +299,9 @@ internal fun List<CargoDependency>.mergeIdenticalTestDependencies(): List<CargoD
val compileDeps =
this.filter { it.scope == DependencyScope.Compile }.toSet()

return this.filterNot {
it.scope == DependencyScope.Dev && compileDeps.contains(it.copy(scope = DependencyScope.Compile))
return this.filterNot { dep ->
dep.scope == DependencyScope.Dev &&
DEV_ONLY_FEATURES.none { devOnly -> dep.features.contains(devOnly) } &&
compileDeps.contains(dep.copy(scope = DependencyScope.Compile))
}
}
Loading

0 comments on commit 26827f1

Please sign in to comment.