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

Require configs to be explicitly bound and enable requireExplicitInject() #816

Merged
merged 11 commits into from Mar 6, 2019

Conversation

Projects
None yet
5 participants
@mightyguava
Copy link
Collaborator

mightyguava commented Mar 5, 2019

This addresses #794 and #784. See those issues for context on why.

Edit: We ended up using requireAtInjectionOnConstructors() instead of requireExplicitBindings(), which gets the benefit of making it difficult to accidentally inject an implicit binding, but doesn't add too much overhead with module configs.

The changes made here are:

  1. c30e7ec removes the code in ConfigModule that walks the app's Config object and binds its child Config classes automatically. This required a small refactoring of usages of Config.
  2. 891dfd6 calls binder().requireExplictBindings() in MiskApplication and MiskTestExtension, and fixes breakages in the misk main module.
  3. The remaining commits fix usages in other modules.

A particular note here is that in addition to enabling require explict injections, there's a breaking API change to action binding. Every action was registered as a WebActionEntry multibinding, that required an implicit binding for the action. With explicit bindings enabled, the multibinding of

multibind<WebActionEntry>().toInstance(WebActionEntry<ReturnADinosaurAction>())

would need an extra line

multibind<WebActionEntry>().toInstance(WebActionEntry<ReturnADinosaurAction>())
bind<ReturnADinosaurAction()>`

This makes for an unpleasant API, so instead I un-deprecated the WebActionModule to wrap the calls. This has the additional benefit that we no longer need the explicit binding verification for actions in Skim's ServiceBuilder.

This PR could probably have been split for #794 and #784, since I'll need to fix breakages in every app regardless and the config part is pretty small, I just put them together.

@mightyguava mightyguava requested review from swankjesse , mmihic and ryanhall07 Mar 5, 2019

@wesleyk

wesleyk approved these changes Mar 5, 2019

mightyguava added some commits Mar 5, 2019

Requires configs to be explicitly bound.
Fixes #794. This removes the code in `ConfigModule` that walks the
app's `Config` object and binds its child `Config` classes automatically.

@mightyguava mightyguava force-pushed the yunchi/explicit-inject branch from 51799e5 to d98bd2e Mar 5, 2019

@adrw
Copy link
Member

adrw left a comment

Couple questions regarding the API changes and how they'll effect downstream consumers of the multibound List<WebActionEntry> such as the WebActions tab and related endpoint /api/webactionmetadata.

@mightyguava mightyguava force-pushed the yunchi/explicit-inject branch from d98bd2e to e7dc8b6 Mar 5, 2019

@mightyguava mightyguava referenced this pull request Mar 5, 2019

Merged

Add HikariCP metrics #815

@mmihic

mmihic approved these changes Mar 5, 2019

@adrw

adrw approved these changes Mar 5, 2019

@mightyguava mightyguava force-pushed the yunchi/explicit-inject branch 2 times, most recently from 9ee83cb to 9db9126 Mar 6, 2019

@mightyguava mightyguava force-pushed the yunchi/explicit-inject branch from 9db9126 to 13d95f3 Mar 6, 2019

@mightyguava mightyguava force-pushed the yunchi/explicit-inject branch from b4bf588 to bed63c9 Mar 6, 2019

@swankjesse
Copy link
Member

swankjesse left a comment

Nice!

@@ -236,7 +236,7 @@ internal class SqsJobQueueTest {
multibind<Service>().to<DockerSqs.Service>()
install(MiskTestingServiceModule())
install(MockTracingBackendModule())
install(Modules.override(AwsSqsJobQueueModule()).with(SQSTestModule()))
install(Modules.override(AwsSqsJobQueueModule(AwsSqsJobQueueConfig())).with(SQSTestModule()))

This comment has been minimized.

@swankjesse

swankjesse Mar 6, 2019

Member

Make AwsSqsJobQueueConfig() a default value for that parameter?

This comment has been minimized.

@mightyguava

mightyguava Mar 6, 2019

Author Collaborator

I'd prefer to not use default values for configs. It'll make it more likely for developers to forget to pass them through in production when they just install a module. If they are sure they want the default, they should instantiate it explicitly.

@@ -80,6 +80,10 @@ class HibernateModule(

bind(configKey).toInstance(config)

// Explicitly bind non-annotated bindings
@@ -56,7 +56,6 @@ class RealEventRouterModule(val environment: Environment) : KAbstractModule() {
* not run multiple enqueued tasks concurrently! Instead it should have exactly 1 thread always.
*/
@Qualifier
@Retention(AnnotationRetention.RUNTIME)

This comment has been minimized.

@swankjesse

swankjesse Mar 6, 2019

Member

Is this the default? Neat

This comment has been minimized.

@mightyguava

mightyguava Mar 6, 2019

Author Collaborator

Yeah I haven't found a need for it at least.

multibind<WebActionEntry>().toInstance(WebActionEntry(webActionClass))
multibind<WebActionEntry>().toInstance(WebActionEntry(actionClass, url_path_prefix))
// Ensures that the action has an @Inject annotation and that its dependencies are satisfied
getProvider(actionClass.java)

This comment has been minimized.

@swankjesse

swankjesse Mar 6, 2019

Member

👍🏻

}

companion object {
@Deprecated("use multibind<WebActionEntry>().toInstance(WebActionEntry(...))")
/**
* Registers a web action.

This comment has been minimized.


/**
* Registers a web action with a path prefix.
* @param url_path_prefix: defaults to "/". If not empty, must match pattern requirements:

This comment has been minimized.

@swankjesse

swankjesse Mar 6, 2019

Member

rather than copy-pasting this 3x can you link to a single definition?

This comment has been minimized.

@mightyguava

mightyguava Mar 6, 2019

Author Collaborator

kotlindoc doesn't support linking to overloaded methods, but I'll just delete the redundant ones.

import javax.inject.Inject

@MiskTest
class MultiBinderTest {

This comment has been minimized.

@swankjesse

mightyguava added some commits Mar 6, 2019

skipSources() on WebActionModule provider check
Makes the error message for WebAction injector errors refer to the
module that installs WebActionModule instead of WebActionModule.
@mightyguava

This comment has been minimized.

Copy link
Collaborator Author

mightyguava commented Mar 6, 2019

Looks like we have overall consensus. Merging this for now since it's like a ticking merge conflict bomb. Feel free to continue commenting and I'll follow up with changes.

@mightyguava mightyguava merged commit e82b920 into master Mar 6, 2019

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@mightyguava mightyguava deleted the yunchi/explicit-inject branch Mar 6, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.