Correctly handle "_root_." prefix in routes file (needed for namespaceReverseRouter) #10153

mkurz · 2020-03-30T20:43:58Z

First of all:
I have a reproducer: https://github.com/mkurz/play-namespace-reverse-router-bug/tree/reproducer

Now step by step:
If you set up a very very simple Play project (with latest version 2.8.1) with just this simple conf/routes file that only contains the route to the build-in assets controller:

GET     /assets/*file               controllers.Assets.versioned(file)

and with the config namespaceReverseRouter set to true in build.sbt:

lazy val root: Project = (project in file("."))
  .enablePlugins(PlayWeb)
  .settings(
    name := "namespace-reverse-router-bug",
    scalaVersion := "2.13.1",
    namespaceReverseRouter := true, // <-- that's the important setting
  )

the project does not compile:

[error] ./play-namespace-reverse-router-bug/conf/routes:1:1: type Assets is not a member of package router.controllers
[error] GET     /assets/*file               controllers.Assets.versioned(file)
[error] ./play-namespace-reverse-router-bug/conf/routes:1:1: type Assets is not a member of package router.controllers
[error] GET     /assets/*file               controllers.Assets.versioned(file)
[error] two errors found

You can test this yourself by checking out the reproducer branch of my reproducer project.

To makes things easy I also uploaded the generated files from the target folder, see this branch.

The line causing the problem is:

  Assets_0: controllers.Assets,

which can be found twice, here and here in the generated Routes.scala.

Now look at the file/package structure that gets generated:

target/scala-2.13/routes/
└── main
    └── router
        ├── controllers                  <-- Shadows the root controllers package
        │   ├── javascript
        │   │   └── JavaScriptReverseRoutes.scala
        │   ├── ReverseRoutes.scala
        │   └── routes.java
        ├── RoutesPrefix.scala
        └── Routes.scala                 <-- Can not resolve 'controllers.Assets'

Seen from Routes.scala the root controllers package is shadowed by the router.controllers package, which of course does not contain controllers.Assets.

The most obvious solution for problems like this is to add Scala's _root_. prefix to the package:

GET     /assets/*file               _routes_.controllers.Assets.versioned(file)

(See the master branch, where I applied this change).

This does work, project compiles, however it is not really nice.
First of all there are some annoying and ugly warnings:

[warn] ./play-namespace-reverse-router-bug/conf/routes: _root_ in root position of qualifier refers to the root package, not package _root_ in package router, which is in scope
[warn] ./play-namespace-reverse-router-bug/conf/routes:1:1: _root_ in root position of qualifier refers to the root package, not package _root_ in package router, which is in scope
[warn] GET     /assets/*file               _root_.controllers.Assets.versioned(file)
[warn] ./play-namespace-reverse-router-bug/conf/routes:1:1: _root_ in root position of qualifier refers to the root package, not package _root_ in package router, which is in scope
[warn] GET     /assets/*file               _root_.controllers.Assets.versioned(file)
[warn] three warnings found

Second, look at the file/package structure:

target/scala-2.13/routes/
└── main
    └── router
        ├── _root_
        │   └── controllers
        │       ├── javascript
        │       │   └── JavaScriptReverseRoutes.scala
        │       ├── ReverseRoutes.scala
        │       └── routes.java
        ├── RoutesPrefix.scala
        └── Routes.scala

As you can see to generate reverse routes you would have to write router._root_.controllers.MyController.myAction() which is not what we wanted to achieve here. We wanted to use the root Assets class in Routes.scala nothing more:

  Assets_0: _root_.controllers.Assets,

I uploaded this generated files to the generated-with-_root_ branch. Also see the diff (be aware files moved).

So basically what we want here is to only change Routes.scala to prepend _root_. to the variables where necessary, but generate all other files like before.
And this is what this pull request does: It ignore the _root_. prefix for the reverse routers.
With this fix applied, this is what changes now in the generated files: diff.

Besides adding comprehensive scripted tests, I also applied this fix to a fairly large project of mine which also has javascript reverse routes (also for assets) and it works like a charm.

One last thing: Usually what you want to do when adding namespaceReverseRouter := true is to move controllers from app/controllers/ to app/router/controller/. However for the build-in Assets class this is not possible because obviously you can not move the file, because it's in the play jar...

mkurz · 2020-03-30T20:48:19Z

core/play/src/main/scala/play/core/routing/HandlerInvoker.scala

@@ -68,15 +68,15 @@ object HandlerInvokerFactory {

  private def loadJavaControllerClass(handlerDef: HandlerDef): Class[_] = {
    try {
-      handlerDef.classLoader.loadClass(handlerDef.controller)
+      handlerDef.classLoader.loadClass(handlerDef.controller.stripPrefix("_root_."))


This has nothing to do with the fix this pull request is about, however we should also strip _root_. here, because _root_ is a Scala feature and the Java classloader can not load classes with _root_. prefixed via the loadClass(...) method.
You can test this in jshell:

jshell> Thread.currentThread().getContextClassLoader().loadClass("java.lang.Runtime") $1 ==> class java.lang.Runtime

works, however prefixing fails;

jshell> Thread.currentThread().getContextClassLoader().loadClass("_root_.java.lang.Runtime") | Exception java.lang.ClassNotFoundException: _root_.java.lang.Runtime | at URLClassLoader.findClass (URLClassLoader.java:471) | at DefaultLoaderDelegate$RemoteClassLoader.findClass (DefaultLoaderDelegate.java:154) | at ClassLoader.loadClass (ClassLoader.java:588) | at ClassLoader.loadClass (ClassLoader.java:521) | at (#2:1)

mkurz · 2020-03-30T20:49:23Z

dev-mode/routes-compiler/src/main/scala/play/routes/compiler/RoutesGenerator.scala

@@ -181,7 +181,7 @@ object InjectedRoutesGenerator extends RoutesGenerator {
      routes: List[Route],
      namespaceReverseRouter: Boolean
  ) = {
-    routes.groupBy(_.call.packageName).map {
+    routes.groupBy(_.call.packageName.map(_.stripPrefix("_root_."))).map {


For ReverseRoutes.scala.

mkurz · 2020-03-30T20:49:38Z

dev-mode/routes-compiler/src/main/scala/play/routes/compiler/RoutesGenerator.scala

@@ -209,7 +209,7 @@ object InjectedRoutesGenerator extends RoutesGenerator {
      routes: List[Route],
      namespaceReverseRouter: Boolean
  ) = {
-    routes.groupBy(_.call.packageName).map {
+    routes.groupBy(_.call.packageName.map(_.stripPrefix("_root_."))).map {


For JavaScriptReverseRoutes.scala.

mkurz · 2020-03-30T20:49:58Z

dev-mode/routes-compiler/src/main/scala/play/routes/compiler/RoutesGenerator.scala

@@ -236,7 +236,7 @@ object InjectedRoutesGenerator extends RoutesGenerator {
      rules: List[Rule],
      namespaceReverseRouter: Boolean
  ) = {
-    rules.collect { case r: Route => r }.groupBy(_.call.packageName).map {
+    rules.collect { case r: Route => r }.groupBy(_.call.packageName.map(_.stripPrefix("_root_."))).map {


For routes.java.

mkurz · 2020-03-30T20:52:20Z

...test/play-sbt-plugin/routes-compiler-routes-compilation/tests/assets/JavaScriptRouterSpec.js

+    it("should provide the GET method for assets", function() {
+        var data = jsRoutes.controllers.Assets.versioned();
+        assert.equal("GET", data.method);
+    });


Just make sure this pull request did not break the any existing (js) reverse router.

mkurz · 2020-03-30T20:53:00Z

...-plugin/src/sbt-test/play-sbt-plugin/routes-compiler-injected-routes-compilation/conf/routes

@@ -72,7 +72,8 @@ GET         /routestest             controllers.Application.routetest(yield)
 # Test for default values for scala keywords
 GET         /routesdefault          controllers.Application.routedefault(type ?= "x")

-GET         /public/*file           controllers.Assets.versioned(path="/public", file: controllers.Assets.Asset)
+# Let's prefix with "_root_." (should not make any difference however because namespaceReverseRouter is not set in build.sbt)
+GET         /public/*file           _root_.controllers.Assets.versioned(path="/public", file: controllers.Assets.Asset)


Adding _root_ here does not make any difference, however I want to add it so it's tested once as well.

gmethvin · 2020-04-06T21:29:53Z

One last thing: Usually what you want to do when adding namespaceReverseRouter := true is to move controllers from app/controllers/ to app/router/controller/. However for the build-in Assets class this is not possible because obviously you can not move the file, because it's in the play jar...

This seems like a good argument to move controllers.Assets to a different package, ideally under play. I remember we talked about this before but there may have been some challenges I don't remember specifically.

mkurz · 2020-04-06T21:40:26Z

This seems like a good argument to move controllers.Assets to a different package, ideally under play.

That would help as well. However adding support for the _root_ prefix still makes sense, there may be other use cases where this could be needed. Plus we can backport it to 2.8.x 😉

Did you have a look at my PR? Should be good to go I think.

mkurz · 2020-04-09T06:53:36Z

@Mergifyio backport 2.8.x

mergify · 2020-04-09T06:54:33Z

Command backport 2.8.x: success

Backports have been created

#10171 Correctly handle "root." prefix in routes file (needed for namespaceReverseRouter) (bp #10153) has been created for branch 2.8.x

mkurz · 2020-04-09T06:54:33Z

Thanks @gmethvin!

Correctly handle "_root_." prefix in routes file (needed for namespaceReverseRouter) (bp #10153)

mkurz commented Mar 30, 2020

View reviewed changes

mkurz force-pushed the fix-namespace-reverse-router-bug branch 2 times, most recently from 8e730c7 to 6561563 Compare March 31, 2020 11:53

Correctly handle "_root_." prefix in routes file

28f1989

mkurz force-pushed the fix-namespace-reverse-router-bug branch from 6561563 to 28f1989 Compare March 31, 2020 13:21

mkurz added this to the Play 2.8.2 milestone Apr 2, 2020

gmethvin approved these changes Apr 9, 2020

View reviewed changes

mergify bot merged commit 6f7a016 into playframework:master Apr 9, 2020

mkurz deleted the fix-namespace-reverse-router-bug branch April 9, 2020 06:54

mergify bot mentioned this pull request Apr 9, 2020

Correctly handle "_root_." prefix in routes file (needed for namespaceReverseRouter) (bp #10153) #10171

Merged

mergify bot added a commit that referenced this pull request Apr 9, 2020

Merge pull request #10171 from playframework/mergify/bp/2.8.x/pr-10153

4b8fa52

Correctly handle "_root_." prefix in routes file (needed for namespaceReverseRouter) (bp #10153)

ignasi35 mentioned this pull request May 25, 2020

Update filters-helpers, play-ahc-ws, ... to 2.8.2 playframework/playframework.com#383

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Correctly handle "_root_." prefix in routes file (needed for namespaceReverseRouter) #10153

Correctly handle "_root_." prefix in routes file (needed for namespaceReverseRouter) #10153

mkurz commented Mar 30, 2020

mkurz Mar 30, 2020

mkurz Mar 30, 2020

mkurz Mar 30, 2020

mkurz Mar 30, 2020

mkurz Mar 30, 2020

mkurz Mar 30, 2020

gmethvin commented Apr 6, 2020

mkurz commented Apr 6, 2020

mkurz commented Apr 9, 2020

mergify bot commented Apr 9, 2020

mkurz commented Apr 9, 2020

Correctly handle "_root_." prefix in routes file (needed for namespaceReverseRouter) #10153

Correctly handle "_root_." prefix in routes file (needed for namespaceReverseRouter) #10153

Conversation

mkurz commented Mar 30, 2020

mkurz Mar 30, 2020

Choose a reason for hiding this comment

mkurz Mar 30, 2020

Choose a reason for hiding this comment

mkurz Mar 30, 2020

Choose a reason for hiding this comment

mkurz Mar 30, 2020

Choose a reason for hiding this comment

mkurz Mar 30, 2020

Choose a reason for hiding this comment

mkurz Mar 30, 2020

Choose a reason for hiding this comment

gmethvin commented Apr 6, 2020

mkurz commented Apr 6, 2020

mkurz commented Apr 9, 2020

mergify bot commented Apr 9, 2020

mkurz commented Apr 9, 2020