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.

Sign up for GitHub

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

fix: codegen visitor filtering bug #1049

Merged
merged 2 commits into from
Mar 8, 2024
Merged

fix: codegen visitor filtering bug #1049

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fab1199
Fixed CodegenVisitor filtering bug
0marperez Mar 7, 2024
090e7b7
Api changes sneaked in from another branch
0marperez Mar 7, 2024
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
18 changes: 13 additions & 5 deletions ...hy-kotlin-codegen/src/main/kotlin/software/amazon/smithy/kotlin/codegen/CodegenVisitor.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ class CodegenVisitor(context: PluginContext) : ShapeVisitor.Default<Unit>() {
private val fileManifest: FileManifest = context.fileManifest
private val symbolProvider: SymbolProvider
private val writers: KotlinDelegator
private val integrations: List<KotlinIntegration>
private val integrations: MutableList<KotlinIntegration>
private val protocolGenerator: ProtocolGenerator?
private val applicationProtocol: ApplicationProtocol
private val baseGenerationContext: GenerationContext
Expand All @@ -50,20 +50,28 @@ class CodegenVisitor(context: PluginContext) : ShapeVisitor.Default<Unit>() {
LOGGER.info("Discovering KotlinIntegration providers...")
integrations = ServiceLoader.load(KotlinIntegration::class.java, classLoader)
.onEach { integration -> LOGGER.info("Loaded KotlinIntegration: ${integration.javaClass.name}") }
.filter { integration -> integration.enabledForService(context.model, settings) } // TODO: Change so we don't filter until previous integrations model modifications are complete
.onEach { integration -> LOGGER.info("Enabled KotlinIntegration: ${integration.javaClass.name}") }
.sortedBy(KotlinIntegration::order)
.toMutableList()
Copy link
Contributor

Choose a reason for hiding this comment

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

design suggestion: instead of making this a MutableList, it might be better to have configuredIntegrations and enabledIntegrations (configuredIntegrations - disabledIntegrations)

Copy link
Contributor

Choose a reason for hiding this comment

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

example:

        configuredIntegrations = ServiceLoader.load(KotlinIntegration::class.java, classLoader)
...
        val enabledIntegrations = configuredIntegrations.mapNotNull { integration ->
            if (integration.enabledForService(resolvedModel, settings)) {
                println("Executing KotlinIntegration: ${integration.javaClass.name}")
                resolvedModel = integration.preprocessModel(resolvedModel, settings)
                integration
            } else {
                null
            }
        }


var resolvedModel = context.model
val disabledIntegrations = mutableListOf<KotlinIntegration>()

LOGGER.info("Preprocessing model")

// Model pre-processing:
// 1. Start with the model from the plugin context
// 2. Apply integrations
// 3. Flatten error shapes (see: https://github.com/awslabs/smithy/pull/919)
// 4. Normalize the operations
var resolvedModel = context.model
for (integration in integrations) {
resolvedModel = integration.preprocessModel(resolvedModel, settings)
if (integration.enabledForService(resolvedModel, settings)) {
LOGGER.info("Enabled KotlinIntegration: ${integration.javaClass.name}")
resolvedModel = integration.preprocessModel(resolvedModel, settings)
} else {
disabledIntegrations.add(integration)
}
}
integrations.removeAll(disabledIntegrations)

resolvedModel = ModelTransformer.create()
.copyServiceErrorsToOperations(resolvedModel, settings.getService(resolvedModel))
Expand Down
21 changes: 7 additions & 14 deletions ...tware/amazon/smithy/kotlin/codegen/rendering/auth/SigV4AsymmetricAuthSchemeIntegration.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,15 +25,15 @@ import software.amazon.smithy.model.shapes.ShapeId
* Register support for the `aws.auth#sigv4a` auth scheme.
*/
class SigV4AsymmetricAuthSchemeIntegration : KotlinIntegration {
// Needs to happen after the `SigV4AsymmetricTraitCustomization` (-60).
override val order: Byte = -50

// Needs to be true due to the way integrations are filtered out before application and sigV4a customization.
// See 'CodegenVisitor' & 'SigV4AsymmetricTraitCustomization'
override fun enabledForService(model: Model, settings: KotlinSettings): Boolean = true
Copy link
Contributor

Choose a reason for hiding this comment

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

note: There is one more of these hardcoded true in UnsupportedSigningAlgorithmIntegration, do you plan to change that one separately?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that one is in aws-sdk-kotlin. The change is in a pull request here: awslabs/aws-sdk-kotlin#1245

override fun enabledForService(model: Model, settings: KotlinSettings): Boolean =
ServiceIndex
.of(model)
.getAuthSchemes(settings.service)
.values
.any { it.javaClass == SigV4ATrait::class.java }

override fun authSchemes(ctx: ProtocolGenerator.GenerationContext): List<AuthSchemeHandler> =
if (modelHasSigV4aTrait(ctx)) listOf(SigV4AsymmetricAuthSchemeHandler()) else emptyList()
listOf(SigV4AsymmetricAuthSchemeHandler())
}

private class SigV4AsymmetricAuthSchemeHandler : AuthSchemeHandler {
Expand Down Expand Up @@ -69,10 +69,3 @@ private class SigV4AsymmetricAuthSchemeHandler : AuthSchemeHandler {
writer.write("#T(#T, #S)", RuntimeTypes.Auth.HttpAuthAws.SigV4AsymmetricAuthScheme, RuntimeTypes.Auth.Signing.AwsSigningStandard.DefaultAwsSigner, signingService)
}
}

internal fun modelHasSigV4aTrait(ctx: ProtocolGenerator.GenerationContext): Boolean =
ServiceIndex
.of(ctx.model)
.getAuthSchemes(ctx.service)
.values
.any { it.javaClass == SigV4ATrait::class.java }
135 changes: 0 additions & 135 deletions ...e/amazon/smithy/kotlin/codegen/rendering/auth/SigV4AsymmetricAuthSchemeIntegrationTest.kt
View file
Open in desktop

This file was deleted.

Loading