Feature request: includeFor integration

[mullermp]

It is useful to have an integration only apply for a specific service.

We have this in Ruby like so: https://github.com/smithy-lang/smithy-ruby/blob/779dd349560371b525758a45e83e21673e67753a/codegen/smithy-ruby-codegen/src/main/java/software/amazon/smithy/ruby/codegen/RubyIntegration.java#L45

Kotlin also has this: https://github.com/smithy-lang/smithy-kotlin/blob/main/codegen/smithy-kotlin-codegen/src/main/kotlin/software/amazon/smithy/kotlin/codegen/integration/KotlinIntegration.kt#L60

I'm unsure of the recommended method arguments for this, but the need is for integrations to only apply in some cases, such as s3, or if a service has a particular trait. Such a method (I propose includeFor) should run before preprocessModel and other integration hooks - it should be the very first thing checked IMO.

Currently we run into issues using preprocessModel, which runs before our hook to includeFor, so each preprocessModel method has to check and return the model if certain shapes aren't present or if the service model isn't what the integration is targeting.

[JordonPhillips]

The problem is that there's not really a good generic place to run this sort of thing. If you were to run it before preprocessModel as you suggest, for example, what happens if another integration changes the model such that it would change whether you run the rest of the integration or not?

You could use the configure hook to bail early on settings, such as a configured service.

[JordonPhillips]

There's not really a super clean and robust way of offering this generically. The most flexible way would be to add something like this to the interface:

default boolean enabled() {
    return true;
}

Then we could skip over any that return false, and you'd be able to change that whenever you like based on whatever reason. Alternatively (or additionally) we could provide some base classes that have methods for you to implement on configuration or when the model's ready. They'd be some really odd interfaces though. A wrapping class might be better.

[mullermp]

what happens if another integration changes the model such that it would change whether you run the rest of the integration or not?

I was suggesting that all integrations run includeFor, and then whatever ones were selected, then run preprocessModel on all of those? (not includeFor then preprocess for each integration). Would that work?

There's not really a super clean and robust way of offering this generically.

It would be nice to inspect the model and/or the service shape, so we can check "Does this apply to S3?" or some other service. Our interface is a boolean.

[JordonPhillips]

then whatever ones were selected, then run preprocessModel on all of those

There is still the problem of preprocess changing the model such that a given integration is now valid or is now no longer valid. For example, imagine you have an integration that generates a protocol implementation for restXml. What happens when another integration removes that protocol from the service during preprocessModel? In the best case it would check before doing anything else and raise a meaningful error or otherwise disable itself. In this case there's no meaningful benefit to having includeFor. More likely it would unexpectedly fail when trying to access some property on the trait, raising an error that is likely hard for a user to understand. But it's also possible it just generates an implementation anyway, which puts the generated code in an invalid state.

So you can't provide the model before preprocess is called. You can provide the settings, which don't change and which do include the service id, but there's plenty of reason to need the final model (like requiring the presence of a protocol trait).

We could provide hooks at both points in time, one with just settings and one with full context. Ideally in some way where the post-preprocess method forbids use of preprocess. The question is how much foot-gunning do we want to accept.

[mullermp]

I understand your concern. I don't really have a preference, but there is a valid need for integrations to only apply to a single service, as Ruby and Kotlin (and others?) have done. I still think it's reasonable to have includeFor run for all integrations and where true, only apply those to the service before any pre processing, even if pre processing would otherwise make it valid in that case.