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

Feature request: includeFor integration #2318

Open
mullermp opened this issue Jun 12, 2024 · 5 comments
Open

Feature request: includeFor integration #2318

mullermp opened this issue Jun 12, 2024 · 5 comments
Labels
feature-request A feature should be added or improved. response-requested Waiting on additional info and feedback. Will move to 'closing-soon' in 7 days.

Comments

@mullermp
Copy link
Contributor

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
Copy link
Contributor

JordonPhillips commented Jun 19, 2024

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
Copy link
Contributor

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.

@JordonPhillips JordonPhillips added feature-request A feature should be added or improved. response-requested Waiting on additional info and feedback. Will move to 'closing-soon' in 7 days. labels Jun 19, 2024
@mullermp
Copy link
Contributor Author

mullermp commented Jun 19, 2024

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
Copy link
Contributor

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
Copy link
Contributor Author

mullermp commented Jul 1, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-request A feature should be added or improved. response-requested Waiting on additional info and feedback. Will move to 'closing-soon' in 7 days.
Projects
None yet
Development

No branches or pull requests

2 participants