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.

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

Add support for endpoints.json #468

Merged
merged 10 commits into from
Jun 8, 2021
Merged

Add support for endpoints.json #468

merged 10 commits into from
Jun 8, 2021

Conversation

rcoh
Copy link
Collaborator

@rcoh rcoh commented Jun 7, 2021

Issue #, if available: #459
Description of changes: Add support for endpoint.json based endpoint resolution. This takes a slightly different approach to previous work. Rather than resolving endpoint merging at runtime in Rust, endpoint merging is handled at the code generation level. This enables keeping the Rust code simple (and since the Kotlin side already needed to support endpoint merging, this doesn't increase complexity there either)

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.


/// Endpoint metadata
///
/// T & P exist to support optional vs. non optional values for Protocol and URI template during
Copy link
Collaborator

Choose a reason for hiding this comment

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

Outdated comment?

// We use a stripped down version of the regex crate without unicode support
// To support `\d` and `\w`, we need to explicitly opt into the ascii-only version.
let ascii_only = regex
.replace("\\d", "(?-u:\\d)")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Any reason not to do this replacement at Kotlin codegen time?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I considered it, but it's more of a Rust implementation detail, so I wanted to allow Kotlin to just send the raw regexes. I could be convinced though.

Comment on lines 159 to 169
val comparePartitions = object : Comparator<PartitionNode> {
override fun compare(x: PartitionNode, y: PartitionNode): Int {
// always sort standard aws partition first
if (x.id == "aws") return -1
return x.id.compareTo(y.id)
}
}

val partitions = partitionsData.map {
PartitionNode(endpointPrefix, it)
}.sortedWith(comparePartitions)
Copy link
Collaborator

Choose a reason for hiding this comment

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

There is Kotlin syntax to allow passing in the Comparator with lambda syntax:

.sortedWith(Comparator { x, y ->
   ...
})

val rest = partitions.drop(1)
val fnName = "endpoint_resolver"
return RuntimeType.forInlineFun(fnName, "aws_endpoint") {
it.rustBlock("pub fn $fnName() -> impl #T", awsEndpoint.member("ResolveAwsEndpoint")) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: I think these will be more readable if you just use rustBlockTemplate, name the types, and pass in *codegenScope.

@@ -41,6 +41,7 @@ class FluentClientDecorator : RustCodegenDecorator {
override val order: Byte = 0

override fun extras(protocolConfig: ProtocolConfig, rustCrate: RustCrate) {
protocolConfig.symbolProvider.config().codegenConfig
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this have a side effect?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

heh, no good catch.

return template.replace(Regex("""#\{([a-zA-Z_0-9]+)\}""")) { matchResult ->
val keyName = matchResult.groupValues[1]
if (!scope.toMap().keys.contains(keyName)) {
throw CodegenException(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice!

@@ -346,7 +346,7 @@ class XmlBindingTraitParserGenerator(
*codegenScope, "Shape" to symbol
) {
val members = shape.members()
rustTemplate("let mut base: Option<#{Shape}> = None;", *codegenScope)
rustTemplate("let mut base: Option<#{Shape}> = None;", *codegenScope, "Shape" to symbol)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Wait, how did this ever work?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

because rustBlockTemplate actually applies the codegen scope to the entire block, but now it fails because of the new assert

@rcoh rcoh requested a review from jdisanti June 7, 2021 22:54
@rcoh rcoh merged commit 460ef53 into main Jun 8, 2021
@rcoh rcoh deleted the endpoints-dot-json branch June 8, 2021 01:00
@rcoh rcoh mentioned this pull request Jun 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants