playframework icon indicating copy to clipboard operation
playframework copied to clipboard

Adds support for Guice's Scopes.NO_SCOPE

Open RommelTJ opened this issue 1 year ago • 4 comments

Pull Request Checklist

  • [x] Have you read How to write the perfect pull request?
  • [x] Have you read through the contributor guidelines?
  • [x] Have you referenced any issues you're fixing using commit message keywords?
  • [x] Have you added copyright headers to new files?
  • [x] Have you checked that both Scala and Java APIs are updated?
  • [x] Have you updated the documentation for both Scala and Java sections?
  • [x] Have you added tests for any changed functionality?

Helpful things

Fixes

Resolves https://github.com/playframework/playframework/issues/10893

Purpose

This PR adds support for Guice's Scopes.NO_SCOPE.

We do this by creating a new @NoScopes annotation, and in GuiceInjectorBuilder, we detect this annotation and forward it along as such:

def guice(bindings: Seq[PlayBinding[?]], binderOptions: Set[BinderOption]): GuiceModule = {
  [...]
  (binding.scope, binding.eager) match {
            case (Some(scope), false) =>
              if (scope.getName.equals(classOf[NoScope].getName)) builder.in(Scopes.NO_SCOPE)
              else builder.in(scope)
  [...]
}

Background Context

Approach suggested by @mkurz in https://github.com/playframework/playframework/issues/10893.
Initial attempt was made in https://github.com/playframework/playframework/pull/11451, however, PR was abandoned.

References

https://github.com/playframework/playframework/issues/10893 https://github.com/orgs/playframework/discussions/12891

RommelTJ avatar Oct 03 '24 23:10 RommelTJ

Have you updated the documentation for both Scala and Java sections?

Since this is a new annotation, it's unclear to me if I need to add or edit any documentation. If I do, please point me in the right direction and I'd be happy to update it.

RommelTJ avatar Oct 03 '24 23:10 RommelTJ

@Mergifyio rebase

mkurz avatar Feb 18 '25 22:02 mkurz

rebase

❌ Base branch update has failed

Git reported the following error:

Rebasing (1/6)
Rebasing (2/6)
Rebasing (3/6)
Rebasing (4/6)
Rebasing (5/6)
Auto-merging core/play-guice/src/main/scala/play/api/inject/guice/GuiceInjectorBuilder.scala
CONFLICT (content): Merge conflict in core/play-guice/src/main/scala/play/api/inject/guice/GuiceInjectorBuilder.scala
error: could not apply 622423e3b... Updates GuiceInjectorBuilder to support NoScope
hint: Resolve all conflicts manually, mark them as resolved with
hint: "git add/rm <conflicted_files>", then run "git rebase --continue".
hint: You can instead skip this commit: run "git rebase --skip".
hint: To abort and get back to the state before "git rebase", run "git rebase --abort".
Could not apply 622423e3b... Updates GuiceInjectorBuilder to support NoScope

mergify[bot] avatar Feb 18 '25 22:02 mergify[bot]

Thanks!

However, the tests that you added were not testing what I described in #10893 and what we actually wanted to solve: You implemented Guice's AbstractModule, however the whole point of this feature is to make Play's play.[api.]inject.Module be able to bind to some kind of "no scope" scope (see Scala / Java doc). If using Guice directly, you should be able to do this already with bind(...)...in(Scopes.NO_SCOPE) anyway (however this is broken in Guice, see below...)

I pushed tests which I think a correct.

However... it turns out that Guice has a bug (at least I currently think it is a bug) and the whole mechanism I described does not even work in Guice itself.

That means we need to put this PR on hold until we know what's going on - I opened:

  • https://github.com/google/guice/issues/1869
    • Reproducer: https://github.com/mkurz/guice-noscope/

mkurz avatar Feb 20 '25 11:02 mkurz