"No valid constructors" if you only need Configuration

[kubukoz]

Play Version (2.5.x / etc)

2.6.13

API (Scala / Java / Neither / Both)

Scala API

Expected Behavior

1. Something like this should compile and run:

class Bindings(configuration: Configuration) extends AbstractModule {
  def configure(): Unit = ()
}

Actual Behavior

"No valid constructor" in the logs.

I was surprised to see that it didn't work, because I had a similar module already, only that one required both Environment and Configuration. After some time of debugging, I tried passing both, and it worked.

I traced the origin of this to the implementation of play.api.inject.Modules#constructModule:

      {
        tryConstruct(environment, configuration)
      } orElse {
        tryConstruct(new JavaEnvironment(environment), configuration.underlying)
      } orElse {
        tryConstruct(new JavaEnvironment(environment), new JavaConfiguration(configuration))
      } orElse {
        tryConstruct()
      } getOrElse {
        throw new PlayException("No valid constructors", "Module [" + className + "] cannot be instantiated.")
      }

If the reason for not supporting a single-argument constructor with just Configuration is that there would have to be support for all the possible combinations (which would probably bring 2-3x the amount of code that already is there), then I think there could be something done to change the way Play tries to construct a module - maybe allowing to use @Inject() on the constructor and bring anything that is already bound? (essentially making a module yet another component/bean)

[kubukoz]

One simple solution for the confusion caused by "No valid constructors" would be to make the error message say what constructors would be valid. It would certainly mean less work here :)

[marcospereira]

Hi Kubukoz,

One simple solution for the confusion caused by "No valid constructors" would be to make the error message say what constructors would be valid. It would certainly mean less work here :)

Yes, that would be a better error message. Do you want to submit a pull request to improve that?

Best.

[kubukoz]

Sure, should be a quick one.

[gmethvin]

Also since we are deprecating the Java play.Configuration we should only suggest the Environment, Config version for the Java API.

maybe allowing to use @Inject() on the constructor and bring anything that is already bound? (essentially making a module yet another component/bean)

Nothing is already bound at that point. The binding happens all at once when Play creates the injector with all the bindings aggregated from all your modules. The Configuration and Environment are special because Play creates them ahead of time.

The constructors that take Environment and Configuration are really an advanced feature that exists mainly because Play (and third-party Play modules) use them internally. This enables dynamically creating bindings from configuration (e.g. play.http.errorHandler = com.example.MyErrorHandler). Most of the time you really want to treat Configuration and Environment as a regular dependencies, and bind a component or a provider that declares them as regular dependencies in the constructor (or in a @Provides method).

[marcospereira]

Also since we are deprecating the Java play.Configuration we should only suggest the Environment, Config version for the Java API.

It is not present in the master branch anymore since Java deprecated APIs were removed. :-)

[gmethvin]

So the action item here is to just change the error message to list all the valid constructors. It may also be worth updating the scaladoc for Modules.locate to clarify the same.

I think if I were to design things from scratch I'd have a single type injected into the constructor, something like:

trait ModuleContext {
  def config: com.typesafe.config.Config
  def environment: play.api.Environment

  final def configuration: play.api.Configuration = play.api.Configuration(config)
  final def javaEnvironment: play.Environment = new play.Environment(environment)
}

Then your module would accept a ModuleContext in the constructor. This is a bit less confusing because the name suggests this is a special type associated with modules, rather than an arbitrary dependency. Hopefully this will help eliminate the confusion for DI newbies that they can "inject" things into modules.

I'm not sure it's worth the breaking changes for that, though. Just wanted to throw the idea out there.

[daric93]

Hi, can i work on it?

[marcospereira]

Hi @daric93,

Yes, thanks for that.

[fagossa]

Hello, @marcospereira, @kubukoz ,

What is the expected output for the exception message? Something along these lines would be acceptable?

No valid constructors, only these were found: play.api.inject.JavaGuiceConfigModule(play.Environment,com.typesafe.config.Config), play.api.inject.PlainGuiceModule()

[gmethvin]

We should list the valid constructors. Something like:

No valid public constructors for com.example.MyModule. Expected one of:
()
(play.api.Environment, play.api.Configuration)
(play.Environment, com.typesafe.config.Config)

[fagossa]

Hello, I have a simple implementation that fixes this problem

[kschwarz1116]

Is there a better solution here that doesn't flag environment as an unused parameter?