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

refactor: cleanup public symbols and rename transform package #971

Merged
merged 9 commits into from
Oct 5, 2023
Merged
Show file tree
Hide file tree
Changes from 7 commits
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
5 changes: 5 additions & 0 deletions .changes/2652a0e3-6588-4542-a35d-eafd5adf8c77.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
{
"id": "2652a0e3-6588-4542-a35d-eafd5adf8c77",
"type": "misc",
"description": "Refactor codegen to place serialization and serialization into the `serde` package rather than the `transform` package. Relocate `TlsVersion` to the `net` package. Make `ByteArrayContent` and friends `internal` and force consumers to go through companion object convenience functions."
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd vote for splitting these unrelated changes out into separate changelog entries

}
5 changes: 5 additions & 0 deletions .changes/bd8a575c-ecec-4744-bde6-44f8900d573d.json
Copy link
Contributor

Choose a reason for hiding this comment

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

Duplicate changelog

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm wonder how that happened 🤔 , good catch.

Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
{
"id": "bd8a575c-ecec-4744-bde6-44f8900d573d",
"type": "misc",
"description": "Refactor codegen to place serialization and serialization into the `serde` package rather than the `transform` package. Relocate `TlsVersion` to the package. Make `ByteArrayContent` and friends `internal` and force consumers to go through companion object convenience functions."
}
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,12 @@ data class KotlinSettings(
* Derive a subpackage namespace from the root package name
*/
fun subpackage(subpackageName: String): String = "$name.$subpackageName"

/**
* The package namespace for generated serialization/deserialization support
*/
val serde: String
get() = subpackage("serde")
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ object RuntimeTypes {
val HttpBody = symbol("HttpBody")
val HttpCall = symbol("HttpCall")
val HttpMethod = symbol("HttpMethod")
val ByteArrayContent = symbol("ByteArrayContent", subpackage = "content")
val readAll = symbol("readAll")
val toByteStream = symbol("toByteStream")
val toHttpBody = symbol("toHttpBody")
Expand Down Expand Up @@ -109,7 +108,6 @@ object RuntimeTypes {
val buildDocument = symbol("buildDocument")
val decodeToString = symbol("decodeToString")
val Document = symbol("Document")
val StringContent = symbol("StringContent")
val toByteArray = symbol("toByteArray")
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -97,12 +97,6 @@ class ShapeValueGenerator(
}

private fun primitiveDeclaration(writer: KotlinWriter, shape: Shape, block: () -> Unit) {
val blobHandlingSymbols = listOf(
RuntimeTypes.Core.Content.ByteArrayContent,
RuntimeTypes.Core.Content.ByteStream,
RuntimeTypes.Core.Content.StringContent,
RuntimeTypes.Core.Content.toByteArray,
)
val suffix = when {
shape.isEnum -> {
val symbol = symbolProvider.toSymbol(shape)
Expand All @@ -112,8 +106,7 @@ class ShapeValueGenerator(

shape.type == ShapeType.BLOB -> {
if (shape.hasTrait<StreamingTrait>()) {
writer.addImport(blobHandlingSymbols)
writer.writeInline("StringContent(")
writer.writeInline("#T.fromString(", RuntimeTypes.Core.Content.ByteStream)
")"
} else {
// blob params are spit out as strings
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -155,15 +155,14 @@ abstract class HttpBindingProtocolGenerator : ProtocolGenerator {
val serializerSymbol = buildSymbol {
definitionFile = "${op.serializerName()}.kt"
name = op.serializerName()
namespace = "${ctx.settings.pkg.name}.transform"
namespace = ctx.settings.pkg.serde

reference(inputSymbol, SymbolReference.ContextOption.DECLARE)
}
val operationSerializerSymbols = setOf(
RuntimeTypes.Http.HttpBody,
RuntimeTypes.Http.HttpMethod,
RuntimeTypes.HttpClient.Operation.HttpSerialize,
RuntimeTypes.Http.ByteArrayContent,
RuntimeTypes.Http.Request.HttpRequestBuilder,
RuntimeTypes.Http.Request.url,
)
Expand Down Expand Up @@ -265,7 +264,7 @@ abstract class HttpBindingProtocolGenerator : ProtocolGenerator {
val sdg = structuredDataSerializer(ctx)
val opBodySerializerFn = sdg.operationSerializer(ctx, op, documentMembers)
writer.write("val payload = #T(context, input)", opBodySerializerFn)
writer.write("builder.body = #T(payload)", RuntimeTypes.Http.ByteArrayContent)
writer.write("builder.body = #T.fromBytes(payload)", RuntimeTypes.Http.HttpBody)
}

resolver.determineRequestContentType(op)?.let { contentType ->
Expand Down Expand Up @@ -428,7 +427,7 @@ abstract class HttpBindingProtocolGenerator : ProtocolGenerator {
if (isBinaryStream) {
writer.write("builder.body = input.#L.#T() ?: #T.Empty", memberName, RuntimeTypes.Http.toHttpBody, RuntimeTypes.Http.HttpBody)
} else {
writer.write("builder.body = #T(input.#L)", RuntimeTypes.Http.ByteArrayContent, memberName)
writer.write("builder.body = #T.fromBytes(input.#L)", RuntimeTypes.Http.HttpBody, memberName)
}
}

Expand All @@ -438,20 +437,20 @@ abstract class HttpBindingProtocolGenerator : ProtocolGenerator {
} else {
memberName
}
writer.write("builder.body = #T(input.#L.#T())", RuntimeTypes.Http.ByteArrayContent, contents, KotlinTypes.Text.encodeToByteArray)
writer.write("builder.body = #T.fromBytes(input.#L.#T())", RuntimeTypes.Http.HttpBody, contents, KotlinTypes.Text.encodeToByteArray)
}

ShapeType.ENUM ->
writer.write(
"builder.body = #T(input.#L.value.#T())",
RuntimeTypes.Http.ByteArrayContent,
"builder.body = #T.fromBytes(input.#L.value.#T())",
RuntimeTypes.Http.HttpBody,
memberName,
KotlinTypes.Text.encodeToByteArray,
)
ShapeType.INT_ENUM ->
writer.write(
"builder.body = #T(input.#L.value.toString().#T())",
RuntimeTypes.Http.ByteArrayContent,
"builder.body = #T.fromBytes(input.#L.value.toString().#T())",
RuntimeTypes.Http.HttpBody,
memberName,
KotlinTypes.Text.encodeToByteArray,
)
Expand All @@ -460,7 +459,7 @@ abstract class HttpBindingProtocolGenerator : ProtocolGenerator {
val sdg = structuredDataSerializer(ctx)
val payloadSerializerFn = sdg.payloadSerializer(ctx, binding.member)
writer.write("val payload = #T(input.#L)", payloadSerializerFn, memberName)
writer.write("builder.body = #T(payload)", RuntimeTypes.Http.ByteArrayContent)
writer.write("builder.body = #T.fromBytes(payload)", RuntimeTypes.Http.HttpBody)
}

else ->
Expand All @@ -487,7 +486,7 @@ abstract class HttpBindingProtocolGenerator : ProtocolGenerator {
val definitionFileName = op.deserializerName().replaceFirstChar(Char::uppercaseChar)
definitionFile = "$definitionFileName.kt"
name = op.deserializerName()
namespace = ctx.settings.pkg.subpackage("transform")
namespace = ctx.settings.pkg.serde

reference(outputSymbol, SymbolReference.ContextOption.DECLARE)
}
Expand Down Expand Up @@ -542,7 +541,7 @@ abstract class HttpBindingProtocolGenerator : ProtocolGenerator {
val deserializerName = "${outputSymbol.name}Deserializer"
definitionFile = "$deserializerName.kt"
name = deserializerName
namespace = ctx.settings.pkg.subpackage("transform")
namespace = ctx.settings.pkg.serde
reference(outputSymbol, SymbolReference.ContextOption.DECLARE)
}

Expand Down Expand Up @@ -996,7 +995,7 @@ fun OperationShape.errorHandlerName(): String = "throw${capitalizedDefaultName()
*/
fun OperationShape.errorHandler(settings: KotlinSettings, block: SymbolRenderer): Symbol = buildSymbol {
name = errorHandlerName()
namespace = "${settings.pkg.name}.transform"
namespace = settings.pkg.serde
// place error handler in same file as operation deserializer
definitionFile = "${deserializerName()}.kt"
renderBy = block
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -127,9 +127,9 @@ abstract class HttpProtocolClientGenerator(
}

protected open fun importSymbols(writer: KotlinWriter) {
writer.addImport("${ctx.settings.pkg.name}.model", "*")
writer.addImport(ctx.settings.pkg.subpackage("model"), "*")
if (TopDownIndex(ctx.model).getContainedOperations(ctx.service).isNotEmpty()) {
writer.addImport("${ctx.settings.pkg.name}.transform", "*")
writer.addImport(ctx.settings.pkg.serde, "*")
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't this still reference the package name somewhere? I was thinking it would be ${ctx.settings.pkg.name}.serde

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ctx.settings.pkg.serde effectively resolves to ${ctx.settings.pkg.name}.serde

}

val defaultClientSymbols = setOf(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,8 +42,8 @@ fun OperationShape.bodySerializer(
block: SymbolRenderer,
): Symbol = buildSymbol {
name = bodySerializerName()
namespace = "${settings.pkg.name}.transform"
// place body serializer in same file as operation serializer implementaiton
namespace = settings.pkg.serde
// place body serializer in same file as operation serializer implementation
definitionFile = "${serializerName()}.kt"
renderBy = block
}
Expand All @@ -68,7 +68,7 @@ fun OperationShape.bodyDeserializer(
block: SymbolRenderer,
): Symbol = buildSymbol {
name = bodyDeserializerName()
namespace = "${settings.pkg.name}.transform"
namespace = settings.pkg.serde
// place body serializer in same file as operation serializer implementation
definitionFile = "${deserializerName()}.kt"
renderBy = block
Expand All @@ -95,7 +95,7 @@ fun Shape.documentSerializer(

return buildSymbol {
name = "$base$suffix"
namespace = settings.pkg.subpackage("transform")
namespace = settings.pkg.serde
definitionFile = "${symbol.name}DocumentSerializer.kt"
reference(symbol, SymbolReference.ContextOption.DECLARE)
renderBy = block
Expand Down Expand Up @@ -123,7 +123,7 @@ fun Shape.documentDeserializer(

return buildSymbol {
name = "$base$suffix"
namespace = settings.pkg.subpackage("transform")
namespace = settings.pkg.serde
definitionFile = "${symbol.name}DocumentDeserializer.kt"
reference(symbol, SymbolReference.ContextOption.DECLARE)
renderBy = block
Expand All @@ -141,7 +141,7 @@ fun Symbol.errorDeserializerName(): String = "deserialize" + StringUtils.capital
*/
fun Symbol.errorDeserializer(settings: KotlinSettings, block: SymbolRenderer): Symbol = buildSymbol {
name = errorDeserializerName()
namespace = "${settings.pkg.name}.transform"
namespace = settings.pkg.serde
val symbol = this@errorDeserializer
// place it in the same file as the exception deserializer, e.g. for HTTP protocols this will be in
// same file as HttpDeserialize
Expand All @@ -163,7 +163,7 @@ fun Shape.payloadDeserializer(
val suffix = mangledSuffix(members)
return buildSymbol {
name = "$base$suffix"
namespace = settings.pkg.subpackage("transform")
namespace = settings.pkg.serde
definitionFile = "${symbol.name}PayloadDeserializer.kt"
reference(symbol, SymbolReference.ContextOption.DECLARE)
renderBy = block
Expand All @@ -183,7 +183,7 @@ fun Shape.payloadSerializer(
val suffix = mangledSuffix(members)
return buildSymbol {
name = "$base$suffix"
namespace = "${settings.pkg.name}.transform"
namespace = settings.pkg.serde
definitionFile = "${symbol.name}PayloadSerializer.kt"
reference(symbol, SymbolReference.ContextOption.DECLARE)
renderBy = block
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,22 +13,22 @@ import kotlin.test.Test
class IdempotentTokenGeneratorTest {
private val defaultModel: Model = javaClass.getResource("idempotent-token-test-model.smithy").toSmithyModel()

private fun getTransformFileContents(filename: String): String {
private fun getSerdeFileContents(filename: String): String {
val (ctx, manifest, generator) = defaultModel.newTestContext()
generator.generateProtocolClient(ctx)
ctx.delegator.flushWriters()
return manifest.getTransformFileContents(filename)
return manifest.getSerdeFileContents(filename)
}

// Assume a specific file path to retrieve a file from the manifest
private fun MockManifest.getTransformFileContents(filename: String, packageNamespace: String = TestModelDefault.NAMESPACE): String {
private fun MockManifest.getSerdeFileContents(filename: String, packageNamespace: String = TestModelDefault.NAMESPACE): String {
val packageNamespaceExpr = packageNamespace.replace('.', '/')
return expectFileString("src/main/kotlin/$packageNamespaceExpr/transform/$filename")
return expectFileString("src/main/kotlin/$packageNamespaceExpr/serde/$filename")
}

@Test
fun `it serializes operation payload inputs with idempotency token trait`() {
val contents = getTransformFileContents("AllocateWidgetOperationSerializer.kt")
val contents = getSerdeFileContents("AllocateWidgetOperationSerializer.kt")
contents.assertBalancedBracesAndParens()
val expectedContents = """
internal class AllocateWidgetOperationSerializer: HttpSerialize<AllocateWidgetRequest> {
Expand All @@ -41,7 +41,7 @@ class IdempotentTokenGeneratorTest {
}

val payload = serializeAllocateWidgetOperationBody(context, input)
builder.body = ByteArrayContent(payload)
builder.body = HttpBody.fromBytes(payload)
if (builder.body !is HttpBody.Empty) {
builder.headers.setMissing("Content-Type", "application/json")
}
Expand All @@ -54,7 +54,7 @@ class IdempotentTokenGeneratorTest {

@Test
fun `it serializes operation query inputs with idempotency token trait`() {
val contents = getTransformFileContents("AllocateWidgetQueryOperationSerializer.kt")
val contents = getSerdeFileContents("AllocateWidgetQueryOperationSerializer.kt")
contents.assertBalancedBracesAndParens()
val expectedContents = """
internal class AllocateWidgetQueryOperationSerializer: HttpSerialize<AllocateWidgetQueryRequest> {
Expand All @@ -78,7 +78,7 @@ internal class AllocateWidgetQueryOperationSerializer: HttpSerialize<AllocateWid

@Test
fun `it serializes operation header inputs with idempotency token trait`() {
val contents = getTransformFileContents("AllocateWidgetHeaderOperationSerializer.kt")
val contents = getSerdeFileContents("AllocateWidgetHeaderOperationSerializer.kt")
contents.assertBalancedBracesAndParens()
val expectedContents = """
internal class AllocateWidgetHeaderOperationSerializer: HttpSerialize<AllocateWidgetHeaderRequest> {
Expand Down
Loading