Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The new generated Config classes seem to be missing docstrings for the new descriptors config options. We should make sure that gets addressed in the next revision of this PR.

Original file line number Diff line number Diff line change
Expand Up @@ -20,5 +20,13 @@ private AwsConfiguration() {}
.type(Symbol.builder().name("str").build())
.documentation(" The AWS region to connect to. The configured region is used to "
+ "determine the service endpoint.")
.nullable(false)
.useDescriptor(true)
.validator(Symbol.builder()
.name("validate_host")
.namespace("smithy_aws_core.config.validators", ".")
.addDependency(AwsPythonDependency.SMITHY_AWS_CORE)
.build())
.defaultValue("'us-east-1'")
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Where is this requirement coming from? I think setting a default region on behalf of customers isn't a good idea.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Noted. Decided to remove the default value for region after discussion.

.build();
}
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,10 @@ public final class ConfigProperty implements ToSmithyBuilder<ConfigProperty> {
private final boolean nullable;
private final String documentation;
private final Consumer<PythonWriter> initialize;
private final Symbol validator;
private final Symbol customResolver;
private final boolean useDescriptor;
private final String defaultValue;

/**
* Constructor.
Expand All @@ -33,6 +37,10 @@ private ConfigProperty(Builder builder) {
this.nullable = builder.nullable;
this.documentation = Objects.requireNonNull(builder.documentation);
this.initialize = Objects.requireNonNull(builder.initialize);
this.validator = builder.validator;
this.customResolver = builder.customResolver;
this.useDescriptor = builder.useDescriptor;
this.defaultValue = builder.defaultValue;
}

/**
Expand Down Expand Up @@ -63,6 +71,34 @@ public String documentation() {
return documentation;
}

/**
* @return Returns the validator symbol for this property, if any.
*/
public java.util.Optional<Symbol> validator() {
return java.util.Optional.ofNullable(validator);
}

/**
* @return Returns the custom resolver symbol for this property, if any.
*/
public java.util.Optional<Symbol> customResolver() {
return java.util.Optional.ofNullable(customResolver);
}

/**
* @return Returns whether this property uses the ConfigProperty descriptor.
*/
public boolean useDescriptor() {
return useDescriptor;
}

/**
* @return Returns the default value for this property, if any.
*/
public java.util.Optional<String> defaultValue() {
return java.util.Optional.ofNullable(defaultValue);
}

/**
* Initializes the config field on the config object.
*
Expand Down Expand Up @@ -94,7 +130,11 @@ public SmithyBuilder<ConfigProperty> toBuilder() {
.type(type)
.nullable(nullable)
.documentation(documentation)
.initialize(initialize);
.initialize(initialize)
.validator(validator)
.customResolver(customResolver)
.useDescriptor(useDescriptor)
.defaultValue(defaultValue);
}

/**
Expand All @@ -107,6 +147,11 @@ public static final class Builder implements SmithyBuilder<ConfigProperty> {
private String documentation;
private Consumer<PythonWriter> initialize = writer -> writer.write("self.$1L = $1L", name);

private Symbol validator;
private Symbol customResolver;
private boolean useDescriptor = false;
private String defaultValue;

@Override
public ConfigProperty build() {
return new ConfigProperty(this);
Expand Down Expand Up @@ -182,5 +227,49 @@ public Builder initialize(Consumer<PythonWriter> initialize) {
this.initialize = initialize;
return this;
}

/**
* Sets the validator symbol for the config property.
*
* @param validator The validator function symbol.
* @return Returns the builder.
*/
public Builder validator(Symbol validator) {
this.validator = validator;
return this;
}

/**
* Sets the custom resolver symbol for the config property.
*
* @param customResolver The custom resolver function symbol.
* @return Returns the builder.
*/
public Builder customResolver(Symbol customResolver) {
this.customResolver = customResolver;
return this;
}

/**
* Sets whether the config property uses the ConfigProperty descriptor.
*
* @param useDescriptor Whether to use the descriptor pattern.
* @return Returns the builder.
*/
public Builder useDescriptor(boolean useDescriptor) {
this.useDescriptor = useDescriptor;
return this;
}

/**
* Sets the default value for the config property.
*
* @param defaultValue The default value as a Python expression string.
* @return Returns the builder.
*/
public Builder defaultValue(String defaultValue) {
this.defaultValue = defaultValue;
return this;
}
}
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

General Question: How are we distinguishing between values set by the user and values not set by the user? For example if we ever have a default value set to "my_default_value", how will we know this was set by us or the user?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We don't have a way to that now. We discussed about using a sentinel value to differentiate it when necessary.

Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,17 @@ public final class ConfigGenerator implements Runnable {
.build())
.documentation(
"The retry strategy or options for configuring retry behavior. Can be either a configured RetryStrategy or RetryStrategyOptions to create one.")
.nullable(false)
.useDescriptor(true)
.validator(Symbol.builder()
.name("validate_retry_strategy")
.namespace("smithy_aws_core.config.validators", ".")
.build())
.customResolver(Symbol.builder()
.name("resolve_retry_strategy")
.namespace("smithy_aws_core.config.custom_resolvers", ".")
.build())
.defaultValue("RetryStrategyOptions(retry_mode=\"standard\", max_attempts=3)")
.build(),
ConfigProperty.builder()
.name("endpoint_uri")
Expand Down Expand Up @@ -322,6 +333,8 @@ private void generateConfig(GenerationContext context, PythonWriter writer) {
writer.onSection(new AddAuthHelper());
}

writer.onSection(new AddGetSourceHelper());

var model = context.model();
var service = context.settings().service(model);

Expand All @@ -335,6 +348,37 @@ private void generateConfig(GenerationContext context, PythonWriter writer) {
}

var finalProperties = List.copyOf(properties);

// Add imports for config resolution
writer.addDependency(SmithyPythonDependency.SMITHY_CORE);
writer.addImport("smithy_core.config.property", "ConfigProperty");
writer.addImport("smithy_core.config.resolver", "ConfigResolver");
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is adding an AWS resolver to generic clients that never gets used, please make sure this only goes in AWS clients

writer.addImport("smithy_aws_core.config.sources", "EnvironmentSource");

// Add validator and resolver imports for properties that use descriptors
// Also add imports for types used in default values
for (ConfigProperty property : finalProperties) {
if (property.useDescriptor()) {
if (property.validator().isPresent()) {
var validatorSymbol = property.validator().get();
writer.addImport(validatorSymbol.getNamespace(), validatorSymbol.getName());
}
if (property.customResolver().isPresent()) {
var resolverSymbol = property.customResolver().get();
writer.addImport(resolverSymbol.getNamespace(), resolverSymbol.getName());
}
// Add imports for types referenced in default values
if (property.defaultValue().isPresent()) {
var defaultValue = property.defaultValue().get();
// Check if default value uses RetryStrategyOptions
if (defaultValue.contains("RetryStrategyOptions")) {
writer.addDependency(SmithyPythonDependency.SMITHY_CORE);
writer.addImport("smithy_core.retries", "RetryStrategyOptions");
}
}
}
}

final String serviceId = context.settings()
.service(context.model())
.getTrait(ServiceTrait.class)
Expand All @@ -349,6 +393,8 @@ class $L:

${C|}

${C|}

def __init__(
self,
*,
Expand All @@ -358,21 +404,65 @@ def __init__(
""",
configSymbol.getName(),
serviceId,
writer.consumer(w -> writeDescriptorDeclarations(w, finalProperties)),
writer.consumer(w -> writePropertyDeclarations(w, finalProperties)),
writer.consumer(w -> writeInitParams(w, finalProperties)),
writer.consumer(w -> initializeProperties(w, finalProperties)));
writer.popState();
}

// Write descriptor declarations for properties using ConfigProperty descriptor
private void writeDescriptorDeclarations(PythonWriter writer, Collection<ConfigProperty> properties) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If we include the type annotations like this:

region: str = ConfigProperty(
    "region", validator=validate_region, default_value="us-east-1"
)

Then we will give the type hint for IDEs. We could also use a pyi file for this.

boolean hasDescriptors = properties.stream().anyMatch(ConfigProperty::useDescriptor);

if (!hasDescriptors) {
return;
}

writer.write("# Config properties using descriptors (lazy resolution with caching)");
for (ConfigProperty property : properties) {
if (property.useDescriptor()) {
writer.writeInline("$L = ConfigProperty('$L'",
property.name(),
property.name());

if (property.validator().isPresent()) {
writer.writeInline(", validator=$L", property.validator().get().getName());
}

if (property.customResolver().isPresent()) {
writer.writeInline(", resolver_func=$L", property.customResolver().get().getName());
}

if (property.defaultValue().isPresent()) {
writer.writeInline(", default_value=$L", property.defaultValue().get());
}

writer.write(")");
}
}
writer.write("");
}

private void writePropertyDeclarations(PythonWriter writer, Collection<ConfigProperty> properties) {
for (ConfigProperty property : properties) {
// Skip descriptor properties - they're declared above
if (property.useDescriptor()) {
continue;
}

var formatString = property.isNullable()
? "$L: $T | None"
: "$L: $T";
writer.write(formatString, property.name(), property.type());
writer.writeDocs(property.documentation(), context);
writer.write("");
}

// Add _resolver declaration
writer.write("_resolver: ConfigResolver");
writer.writeDocs("Shared resolver for all Config instances.", context);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
writer.writeDocs("Shared resolver for all Config instances.", context);
writer.writeDocs("Resolver for iterating through config sources to retrieve keys.", context);

This is no longer shared, right?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah, its not shared. I realized it after publishing this PR and decided to wait until the first round of review is completed.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It looks like we have a double comment here? Did you intend to delete line 435?

writer.write("");
}

private void writeInitParams(PythonWriter writer, Collection<ConfigProperty> properties) {
Expand All @@ -381,9 +471,48 @@ private void writeInitParams(PythonWriter writer, Collection<ConfigProperty> pro
}
}

// Handle descriptor properties efficiently with a loop
private void initializeProperties(PythonWriter writer, Collection<ConfigProperty> properties) {
// First, initialize the resolver
writer.write("# Create resolver with environment source");
writer.write("self._resolver = ConfigResolver(sources=[EnvironmentSource()])");
writer.write("");

// Then, handle descriptor properties efficiently with a loop
var descriptorProperties = properties.stream()
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is in the generic codegen and is not aws specific - are we adding descriptors and everything to white label SDKs?

.filter(ConfigProperty::useDescriptor)
.map(ConfigProperty::name)
.toList();

if (!descriptorProperties.isEmpty()) {
writer.write("# Set instance values for descriptor properties");

// Generate the list of descriptor property names
writer.writeInline("descriptor_keys = [");
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It's a little odd that we are duplicating the descriptor keys here. Did you consider making the descriptors into a dictionary like so:

descriptors = {"region": ConfigProperty(...), "retry_strategy":...}

That way we could use descriptors.keys() instead of two lists.

var iter = descriptorProperties.iterator();
while (iter.hasNext()) {
writer.writeInline("'$L'", iter.next());
if (iter.hasNext()) {
writer.writeInline(", ");
}
}
writer.write("]");
writer.write("for key in descriptor_keys:");
writer.indent();
writer.write("value = locals().get(key)");
writer.write("if value is not None:");
writer.indent();
writer.write("setattr(self, key, value)");
writer.dedent();
writer.dedent();
writer.write("");
}

// Finally, initialize non-descriptor properties normally
for (ConfigProperty property : properties) {
property.initialize(writer);
if (!property.useDescriptor()) {
property.initialize(writer);
}
}
}

Expand Down Expand Up @@ -418,4 +547,46 @@ def set_auth_scheme(self, scheme: AuthScheme[Any, Any, Any, Any]) -> None:
""");
}
}

// Helper to add get_source method for descriptor properties
private static final class AddGetSourceHelper implements CodeInterceptor<ConfigSection, PythonWriter> {
@Override
public Class<ConfigSection> sectionType() {
return ConfigSection.class;
}

@Override
public void write(PythonWriter writer, String previousText, ConfigSection section) {
// Check if there are any descriptor properties
boolean hasDescriptors = section.properties()
.stream()
.anyMatch(ConfigProperty::useDescriptor);

if (!hasDescriptors) {
// No descriptor properties, just write previous text
writer.write(previousText);
return;
}

// First write the previous text
writer.write(previousText);

// Add the get_source helper method
writer.write("""

def get_source(self, key: str) -> str | None:
\"""Get the source that provided a configuration value.

Args:
key: The configuration key (e.g., 'region', 'retry_strategy')

Returns:
The source name ('instance', 'environment', etc.),
or None if the key hasn't been resolved yet.
\"""
cached = self.__dict__.get(f'_cache_{key}')
return cached[1] if cached else None
""");
}
}
}
Loading