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

misc: add visibility build setting #951

Merged
merged 18 commits into from
Sep 19, 2023
Merged
Show file tree
Hide file tree
Changes from 8 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/b6288fbe-a409-473c-a6ac-12c3c310b963.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
{
"id": "b6288fbe-a409-473c-a6ac-12c3c310b963",
"type": "misc",
"description": "Generate internal-only clients with `internal` visibility"
}
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ private const val PACKAGE_NAME = "name"
private const val PACKAGE_VERSION = "version"
private const val PACKAGE_DESCRIPTION = "description"
private const val BUILD_SETTINGS = "build"
private const val VISIBILITY_SETTINGS = "visibility"

// Optional specification of sdkId for models that provide them, otherwise Service's shape id name is used
private const val SDK_ID = "sdkId"
Expand Down Expand Up @@ -160,6 +161,7 @@ data class BuildSettings(
val generateDefaultBuildFiles: Boolean = true,
val optInAnnotations: List<String>? = null,
val generateMultiplatformProject: Boolean = false,
val visibility: VisibilitySettings = VisibilitySettings.Default,
Copy link
Contributor

Choose a reason for hiding this comment

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

These aren't really "build" settings since this type doc comments explicitly talk about gradle build settings. I think maybe we want a new type like "API" settings?

) {
companion object {
const val ROOT_PROJECT = "rootProject"
Expand All @@ -170,17 +172,17 @@ data class BuildSettings(
fun fromNode(node: Optional<ObjectNode>): BuildSettings = node.map {
val generateFullProject = node.get().getBooleanMemberOrDefault(ROOT_PROJECT, false)
val generateBuildFiles = node.get().getBooleanMemberOrDefault(GENERATE_DEFAULT_BUILD_FILES, true)
val generateMultiplatformProject =
node.get().getBooleanMemberOrDefault(GENERATE_MULTIPLATFORM_MODULE, false)
val generateMultiplatformProject = node.get().getBooleanMemberOrDefault(GENERATE_MULTIPLATFORM_MODULE, false)
val annotations = node.get().getArrayMember(ANNOTATIONS).map {
it.elements.mapNotNull { node ->
node.asStringNode().map { stringNode ->
stringNode.value
}.orNull()
}
}.orNull()
val visibility = VisibilitySettings.fromNode(node.get().getObjectMember(VISIBILITY_SETTINGS))

BuildSettings(generateFullProject, generateBuildFiles, annotations, generateMultiplatformProject)
BuildSettings(generateFullProject, generateBuildFiles, annotations, generateMultiplatformProject, visibility)
}.orElse(Default)

/**
Expand All @@ -193,3 +195,27 @@ data class BuildSettings(
class UnresolvableProtocolException(message: String) : CodegenException(message)

private fun <T> Optional<T>.orNull(): T? = if (isPresent) get() else null

data class VisibilitySettings(
val serviceClient: String = "public",
val structure: String = "public",
val error: String = "public",
) {
companion object {
const val SERVICE_CLIENT = "serviceClient"
const val STRUCTURE = "structure"
Copy link
Contributor

Choose a reason for hiding this comment

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

correctness

I'm not sure we want to model visibility of structure and error settings separate from the service client. If you set structure or error visibility to internal then the service client isn't going to compile anyway. Do we validate the combination of settings? What other use cases are we thinking about?

From where I'm sitting at the moment I think we can get away with the same visibility for all of these (which is either public (default) or internal because private doesn't make sense and will also not compile). I suggest we collapse these to just visibility and make them all the same from that (also validate fromNode settings in tests).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah this is one thing I expected discussion about. I started with SERVICE_CLIENT and then decided to add STRUCTURE/ERROR to cover the remaining codegenned portions, but I do think collapsing them into one config option makes sense.

Copy link
Contributor

Choose a reason for hiding this comment

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

I recommend using an enum too instead of a string.

const val ERROR = "error"

fun fromNode(node: Optional<ObjectNode>): VisibilitySettings = node.map {
val serviceClient = node.get().getStringMemberOrDefault(SERVICE_CLIENT, "public")
val structure = node.get().getStringMemberOrDefault(STRUCTURE, "public")
val error = node.get().getStringMemberOrDefault(ERROR, "public")
VisibilitySettings(serviceClient, structure, error)
}.orElse(Default)

/**
* Default visibility settings
*/
val Default: VisibilitySettings = VisibilitySettings()
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,9 @@ object ExceptionBaseClassGenerator {
val name = clientName(ctx.settings.sdkId)
writer.dokka("Base class for all service related exceptions thrown by the $name client")
writer.withBlock(
"public open class #T : #T {",
"#L open class #T : #T {",
"}",
ctx.settings.build.visibility.error,
serviceException,
baseException,
) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -67,11 +67,12 @@ class ServiceClientGenerator(private val ctx: RenderingContext<ServiceShape>) {
requireNotNull(ctx.shape) { "ServiceShape is required to render a service client" }
private val serviceSymbol = ctx.symbolProvider.toSymbol(service)
private val writer = ctx.writer
private val visibility = ctx.settings.build.visibility.serviceClient

fun render() {
writer.write("\n\n")
writer.write("public const val ServiceId: String = #S", ctx.settings.sdkId)
writer.write("public const val SdkVersion: String = #S", ctx.settings.pkg.version)
writer.write("$visibility const val ServiceId: String = #S", ctx.settings.sdkId)
writer.write("$visibility const val SdkVersion: String = #S", ctx.settings.pkg.version)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Mix templating styles makes codegen harder to read. I recommend using #L instead of $visibility. Applies to other changes in this PR.

writer.write("\n\n")

writer.putContext("service.name", ctx.settings.sdkId)
Expand All @@ -82,7 +83,7 @@ class ServiceClientGenerator(private val ctx: RenderingContext<ServiceShape>) {

writer.renderDocumentation(service)
writer.renderAnnotations(service)
writer.openBlock("public interface ${serviceSymbol.name} : #T {", RuntimeTypes.SmithyClient.SdkClient)
writer.openBlock("$visibility interface ${serviceSymbol.name} : #T {", RuntimeTypes.SmithyClient.SdkClient)
.call {
// allow access to client's Config
writer.dokka("${serviceSymbol.name}'s configuration")
Expand Down Expand Up @@ -198,7 +199,7 @@ class ServiceClientGenerator(private val ctx: RenderingContext<ServiceShape>) {
write("Any resources created on your behalf will be shared between clients, and will only be closed when ALL clients using them are closed.")
write("If you provide a resource (e.g. [HttpClientEngine]) to the SDK, you are responsible for managing the lifetime of that resource.")
}
writer.withBlock("public fun #1T.withConfig(block: #1T.Config.Builder.() -> Unit): #1T {", "}", serviceSymbol) {
writer.withBlock("$visibility fun #1T.withConfig(block: #1T.Config.Builder.() -> Unit): #1T {", "}", serviceSymbol) {
write("val newConfig = config.toBuilder().apply(block).build()")
write("return Default#L(newConfig)", serviceSymbol.name)
}
Expand All @@ -219,7 +220,7 @@ class ServiceClientGenerator(private val ctx: RenderingContext<ServiceShape>) {
writer.renderDocumentation(op)
writer.renderAnnotations(op)
writer.write(
"public suspend inline fun #T.#L(crossinline block: #T.Builder.() -> Unit): #T = #L(#T.Builder().apply(block).build())",
"$visibility suspend inline fun #T.#L(crossinline block: #T.Builder.() -> Unit): #T = #L(#T.Builder().apply(block).build())",
serviceSymbol,
operationName,
inputSymbol,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,11 @@ class StructureGenerator(
* Renders a normal (non-error) Smithy structure to a Kotlin class
*/
private fun renderStructure() {
writer.openBlock("public class #T private constructor(builder: Builder) {", symbol)
writer.openBlock(
"#L class #T private constructor(builder: Builder) {",
ctx.settings.build.visibility.structure,
symbol,
)
.call { renderImmutableProperties() }
.write("")
.call { renderCompanionObject() }
Expand Down Expand Up @@ -299,7 +303,11 @@ class StructureGenerator(
val exceptionBaseClass = ExceptionBaseClassGenerator.baseExceptionSymbol(ctx.settings)
writer.addImport(exceptionBaseClass)

writer.openBlock("public class #T private constructor(builder: Builder) : ${exceptionBaseClass.name}() {", symbol)
writer.openBlock(
"#L class #T private constructor(builder: Builder) : ${exceptionBaseClass.name}() {",
ctx.settings.build.visibility.error,
symbol,
)
.write("")
.call { renderImmutableProperties() }
.write("")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,12 @@ class AuthSchemeParametersGenerator : AbstractConfigGenerator() {
val implSymbol = getImplementationSymbol(ctx.settings)

ctx.delegator.useSymbolWriter(symbol) { writer ->
writer.withBlock("public interface #T {", "}", symbol) {
writer.withBlock(
"#L interface #T {",
"}",
ctx.settings.build.visibility.structure,
symbol,
) {
dokka("The name of the operation currently being invoked.")
write("public val operationName: String")
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,8 @@ open class AuthSchemeProviderGenerator {
write("See [#T] for the default SDK behavior of this interface.", getDefaultSymbol(ctx.settings))
}
writer.write(
"public interface #T : #T<#T>",
"#L interface #T : #T<#T>",
ctx.settings.build.visibility.structure,
symbol,
RuntimeTypes.Auth.Identity.AuthSchemeProvider,
paramsSymbol,
Expand All @@ -64,8 +65,9 @@ open class AuthSchemeProviderGenerator {
private fun renderDefaultImpl(ctx: ProtocolGenerator.GenerationContext, writer: KotlinWriter) {
// FIXME - probably can't remain an object
writer.withBlock(
"public object #T : #T {",
"#L object #T : #T {",
"}",
ctx.settings.build.visibility.structure,
getDefaultSymbol(ctx.settings),
getSymbol(ctx.settings),
) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ class DefaultEndpointProviderGenerator(
private val defaultProviderSymbol: Symbol,
private val interfaceSymbol: Symbol,
private val paramsSymbol: Symbol,
private val settings: KotlinSettings,
private val externalFunctions: Map<String, Symbol> = emptyMap(),
private val propertyRenderers: Map<String, EndpointPropertyRenderer> = emptyMap(),
) : ExpressionRenderer {
Expand All @@ -78,7 +79,13 @@ class DefaultEndpointProviderGenerator(

fun render() {
renderDocumentation()
writer.withBlock("public class #T: #T {", "}", defaultProviderSymbol, interfaceSymbol) {
writer.withBlock(
"#L class #T: #T {",
"}",
settings.build.visibility.structure,
defaultProviderSymbol,
interfaceSymbol,
) {
renderResolve()
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,12 +33,12 @@ interface EndpointDelegator {
val defaultProviderSymbol = DefaultEndpointProviderGenerator.getSymbol(ctx.settings)

ctx.delegator.useFileWriter(providerSymbol) {
EndpointProviderGenerator(it, providerSymbol, paramsSymbol).render()
EndpointProviderGenerator(it, ctx.settings, providerSymbol, paramsSymbol).render()
}

if (rules != null) {
ctx.delegator.useFileWriter(defaultProviderSymbol) {
DefaultEndpointProviderGenerator(it, rules, defaultProviderSymbol, providerSymbol, paramsSymbol).render()
DefaultEndpointProviderGenerator(it, rules, defaultProviderSymbol, providerSymbol, paramsSymbol, ctx.settings).render()
}
}
}
Expand All @@ -49,7 +49,7 @@ interface EndpointDelegator {
fun generateEndpointParameters(ctx: ProtocolGenerator.GenerationContext, rules: EndpointRuleSet?) {
val paramsSymbol = EndpointParametersGenerator.getSymbol(ctx.settings)
ctx.delegator.useFileWriter(paramsSymbol) {
EndpointParametersGenerator(it, rules, paramsSymbol).render()
EndpointParametersGenerator(it, ctx.settings, rules, paramsSymbol).render()
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ private const val DEFAULT_DEPRECATED_MESSAGE =
*/
class EndpointParametersGenerator(
private val writer: KotlinWriter,
private val settings: KotlinSettings,
rules: EndpointRuleSet?,
private val paramsSymbol: Symbol,
) {
Expand Down Expand Up @@ -51,7 +52,12 @@ class EndpointParametersGenerator(
fun render() {
renderDocumentation()
// FIXME - this should probably be an interface
writer.withBlock("public class #T private constructor(builder: Builder) {", "}", paramsSymbol) {
writer.withBlock(
"#L class #T private constructor(builder: Builder) {",
"}",
settings.build.visibility.structure,
paramsSymbol,
) {
renderFields()
renderCompanionObject()
write("")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import software.amazon.smithy.kotlin.codegen.model.buildSymbol
*/
class EndpointProviderGenerator(
private val writer: KotlinWriter,
private val settings: KotlinSettings,
private val providerSymbol: Symbol,
private val paramsSymbol: Symbol,
) {
Expand All @@ -33,7 +34,8 @@ class EndpointProviderGenerator(
fun render() {
renderDocumentation()
writer.write(
"public fun interface #T: #T<#T>",
"#L fun interface #T: #T<#T>",
settings.build.visibility.structure,
providerSymbol,
RuntimeTypes.SmithyClient.Endpoints.EndpointProvider,
paramsSymbol,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,12 @@ class EndpointDiscovererGenerator(private val ctx: CodegenContext, private val d
calls.
""".trimIndent(),
)
withBlock("public class #T {", "}", symbol) {
withBlock(
"#L class #T {",
"}",
ctx.settings.build.visibility.structure,
symbol,
) {
write(
"private val cache = #T<DiscoveryParams, #T>(10.#T, #T.System)",
RuntimeTypes.Core.Utils.ReadThroughCache,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,15 @@
*/
package software.amazon.smithy.kotlin.codegen.rendering.endpoints

import software.amazon.smithy.kotlin.codegen.KotlinSettings
import software.amazon.smithy.kotlin.codegen.core.KotlinWriter
import software.amazon.smithy.kotlin.codegen.model.buildSymbol
import software.amazon.smithy.kotlin.codegen.test.TestModelDefault
import software.amazon.smithy.kotlin.codegen.test.assertBalancedBracesAndParens
import software.amazon.smithy.kotlin.codegen.test.formatForTest
import software.amazon.smithy.kotlin.codegen.test.shouldContainOnlyOnceWithDiff
import software.amazon.smithy.model.node.Node
import software.amazon.smithy.model.shapes.ShapeId
import software.amazon.smithy.rulesengine.language.EndpointRuleSet
import kotlin.test.*

Expand Down Expand Up @@ -142,7 +144,13 @@ class DefaultEndpointProviderGeneratorTest {
name = "DefaultEndpointProvider"
namespace = TestModelDefault.NAMESPACE
}
DefaultEndpointProviderGenerator(writer, rules, defaultSymbol, interfaceSymbol, paramsSymbol).render()
val settings = KotlinSettings(
service = ShapeId.from("com.test#Test"),
pkg = KotlinSettings.PackageSettings("name", "version"),
sdkId = "testSdkId",
)

DefaultEndpointProviderGenerator(writer, rules, defaultSymbol, interfaceSymbol, paramsSymbol, settings).render()
generatedClass = writer.toString()
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,15 @@
*/
package software.amazon.smithy.kotlin.codegen.rendering.endpoints

import software.amazon.smithy.kotlin.codegen.KotlinSettings
import software.amazon.smithy.kotlin.codegen.core.KotlinWriter
import software.amazon.smithy.kotlin.codegen.model.buildSymbol
import software.amazon.smithy.kotlin.codegen.test.TestModelDefault
import software.amazon.smithy.kotlin.codegen.test.assertBalancedBracesAndParens
import software.amazon.smithy.kotlin.codegen.test.formatForTest
import software.amazon.smithy.kotlin.codegen.test.shouldContainOnlyOnceWithDiff
import software.amazon.smithy.model.node.Node
import software.amazon.smithy.model.shapes.ShapeId
import software.amazon.smithy.rulesengine.language.EndpointRuleSet
import kotlin.test.*

Expand Down Expand Up @@ -83,7 +85,12 @@ class EndpointParametersGeneratorTest {
name = "EndpointParameters"
namespace = TestModelDefault.NAMESPACE
}
EndpointParametersGenerator(writer, rules, paramsSymbol).render()
val settings = KotlinSettings(
service = ShapeId.from("com.test#Test"),
pkg = KotlinSettings.PackageSettings("name", "version"),
sdkId = "testSdkId",
)
EndpointParametersGenerator(writer, settings, rules, paramsSymbol).render()

generatedClass = writer.toString()
}
Expand Down
Loading