Lighthouse 460: Allow controllers to be instantiated using user provided instantiator

[benmccann]

No description provided.

[julienrf]

Does it work with @With annotations?

[benmccann]

Yes, it does with with @with !

[ijuma]

Interesting, this method requires a single static method which is a middle ground between "no statics" and "all statics". It seems like this approach is much less invasive than what would be required to achieve no statics.

[guillaumebort]

So, we reviewed this carefully. The problem is that this part:

classFactoryMethodName + "(classOf[" + packageName + "." + controller + "])." + route.call.

Can't work with Scala controller if they are written as object (as currently expected by Play). classOf[controllers.Application] doesn't work if controllers.Application is an object.

Also, it introduce runtime overhead (and associated complexity) for all calls.

We still think that we could introduce something like that, but it would require a special syntax for all dynamic routes

[benmccann]

Thanks for reviewing! The first part didn't seem like a concern to me because if you are using a singleton controller then there would be no reason to set the application.classFactoryMethod and so you would not hit that branch as you would use the current code. As for the runtime overhead, I didn't realize there was runtime overhead of calling classOf, but that seems easy enough to just call once at the beginning and cache the result of. I don't understand the last part about dynamic routes. Are you saying this pull request would break dynamic routes? Do you have any thoughts on what you would like to see as a solution instead of this?

[benmccann]

Guillaume, could you expand upon what kind of special syntax you were thinking about? I could take another stab at this pull request to be more inline with what you are thinking if I knew what that is. Also, it still seems to me like the entire point of setting this new option is to enable you to use classes instead of objects, so I don't see how it's a problem that it doesn't work with objects since you can leave the setting off in that case to have things work exactly as they do today.

[guillaumebort]

There is 2 solutions:

• Either we find a solution that work in all cases (for Java/Scala/Classes/Singletons) without introducing any hidden complexity (like if you enable this then this other thing will just magically fail)
• Or we introduce 2 different syntax, one for statically compiled routing, the other one for dynamically instantiated controllers.

[daggerrz]

I wonder if, for the Scala version, it would be doable to just have an object (or possibly a package object) bootstrap? E.g:

object controllers {
    implicit val configuration = ....
    lazy val MainController = new MainController(dependencies...)
 }

This conflicts with some of the Play generated routes code, but shouldn't be too hard fix.

[benmccann]

Hi Guillaume, thank you for the feedback. I've updated the pull request with to add a new syntax for instantiable controllers. I agree it is better this way as you can now have a mix of instantiable and non-instantiable controllers in the same project. Here's the example syntax I came up with:

# This calls a Scala object or Java static method
controllers.Application.index

# This instantiates the Scala class or Java class using a user-defined class factory
controllers.Application().index

[guillaumebort]

Thank you. I'm not sure about the syntax (I think we need more feedback from the community), but in the meantime there is another point I would like to improve.

Instead of using a magic instantiator className provided by configuration, it would be better to have this task handled by the Global object.

So I guess that we need two signatures added in the current GlobalSettings trait (for both API):

public <A> A getControllerInstance(Class<A> controllerClass)

and

def getControllerInstance[A](controllerClass: Class[A]): A

[benmccann]

I'm fine with that. I've updated the pull request accordingly.

[pk11]

Hi Ben, last time I checked this patch it did not work for reverse javascript routing but more importantly a proposed change like this should contain a test excising all the features this change may effect (as described here: https://github.com/playframework/Play20/wiki/Guidelines ). Thanks

[pk11]

features that might be effected by this proposed change:

• reverse routing
• reverse routing for javascript
• action composability (Java API)
• error reporting in case the instantiator fails

[benmccann]

Yes, I agree adding a few test cases makes sense. I'd first like to nail down the syntax though and make sure everyone is happy with the way this is intended to work. If you're happy with the core of this change, then I can address the finer points.

[pk11]

BTW I was referring to this thing:

a sample javascript reverse router (taken from target/../scr_managed/routes_reverseRouting.scala:

class ReverseProjects {




// @LINE:23
def addUser : JavascriptReverseRoute = JavascriptReverseRoute(
   "controllers.Projects.addUser",
   """
      function(project) {
      return _wA({method:"POST", url:"""" + Routes.prefix + { Routes.defaultPrefix} + """" + "projects/" + (""" + implicitly[PathBindable[Long]].javascriptUnbind + """)("project", project) + "/team"})
      }
   """
)

[benmccann]

Peter, I cannot figure out how to make the JS routing work in a project without any of these changes, so I have no idea how to test whether I'm breaking it. It would be very helpful if there were some documentation for that feature. Do I need to do a commonJs include in my project or do a script tag include? I have no idea.

The rest of the stuff you mentioned all looks to be working (reverse routing, action composability, error reporting in case the instantiator fails)

[pk11]

Hi Ben, I can not remember off the top of my head if it's documented or not but basically

if you render this page:
https://github.com/playframework/Play20/blob/master/samples/java/zentasks/app/views/main.scala.html
you will get a js file generated called http://localhost:9000/assets/javascripts/routes

this js file will contain stuff like

var jsRoutes = {}; (function(_root){
var _nS = function(c,f,b){var e=c.split(f||"."),g=b||_root,d,a;for(d=0,a=e.length;d<a;d++){g=g[e[d]]=g[e[d]]||{}}return g}
var _qS = function(items){var qs = ''; for(var i=0;i<items.length;i++) {if(items[i]) qs += (qs ? '&' : '') + items[i]}; return qs ? ('?' + qs) : ''}
var _s = function(p,s){return p+((s===true||(s&&s.secure))?'s':'')+'://'}
var _wA = function(r){return {ajax:function(c){c.url=r.url;c.type=r.method;return jQuery.ajax(c)}, method:r.method,url:r.url,absoluteURL: function(s){return _s('http',s)+'localhost:9000'+r.url},webSocketURL: function(s){return _s('ws',s)+'localhost:9000'+r.url}}}
_nS('controllers.Projects'); _root.controllers.Projects.add = 
      function() {
      return _wA({method:"POST", url:"/projects"})
      }
   ...

which can be used as a reverse router from javascript (kinda cool)

[benmccann]

Ok, finally got it figured out. I don't see that this pull request is causing any problem with it. I'm guessing the recent changes fixed any problems you saw earlier. I did some simple tests as shown below where I got the URLs for a few pages from the JS router and it worked fine.

<script type="text/javascript">
  document.writeln('<a href="' + jsRoutes.controllers.Application.index().url + '">Application.index</a><br/>');
  document.writeln('<a href="' + jsRoutes.controllers.Application.methodWithParams("hello world").url + '">Application.methodWithParams</a><br/>');
</script>

Let me know what you think about the routes file syntax and changes thus far. If it is close to being something that could be merged then I am happy to add some tests.

[benmccann]

FYI, all existing tests pass with this change. ./runtests reports "ALL TESTS PASSED"

[benmccann]

I added a test that demonstrates the new syntax. Please let me know your thoughts on this syntax.

[julienrf]

Why did you need to remove the private[this] qualifier? It allowed to reduce the number of transitive recompilations performed by sbt.

[benmccann]

Added back.

[benmccann]

Is there any additional feedback on this change? I would really love to get this in.

[benmccann]

How does this look? Is it pretty close to something that could go in? Would you like me to make further changes?

[guillaumebort]

No, no need further change for now. We will discuss this during our next team meeting.

[benmccann]

Great. Thanks for the update!

[benmccann]

Any further update on this?

[guillaumebort]

@pk11 Yes you are right. The whole configuration stuff shouldn't exist anymore. @benmccann I have commented directly in the patch.

There shouldn't exist any reference to the configuration anymore. The router should obtain a reference to the current application global and call the getControllerInstance method statically.

[benmccann]

Ok, I have updated this change per your suggestions. Thanks for the code review!

[guillaumebort]

Ok, it looks fine now. I'm ok to merge it.

[chadworth]

i'm very happy to see you agree to merge this. thank you! now hopefully you will press that button? :-)

[sadache]

one problem with the syntax is that we don't know which is the class we want to instantiate. Maybe we need a slightly different syntax.

[benmccann]

@sadache i don't understand what you're saying. for the line "new controllers.JavaControllerInstance.index()" we are instantiating the class controllers.JavaControllerInstance. Is there some special case you're talking about which I am not considering?

[benmccann]

Hey @sadache . I still can't figure out what you were trying to say. I think perhaps you don't understand what's happening in this example. I made a Java controller and named its class JavaControllerInstance. Then when a request hits /json_java_instance, the JavaControllerInstance class is instantiated and its index() method is called. I also created a separate controller with a separate class named ScalaControllerInstance just to prove that there would be no problem instantiating either a Java or Scala controller and calling a method on it.

Please get back to me so that this pull request can be either committed or updated as necessary.

[sadache]

@benmccann: I was just talking about not using the "new" keyword since it
will make the whole thing confusing. This was fixed bu Guillaume by
replacing it with the "@" character.

On Mon, Sep 10, 2012 at 6:21 PM, Ben McCann notifications@github.comwrote:

In framework/test/integrationtest/conf/routes:

@@ -14,6 +14,9 @@ GET /cookie controllers.JavaApi.setCookie()
GET /read/:name controllers.JavaApi.readCookie(name)
GET /clear/:name controllers.JavaApi.clearCookie(name)

+GET /json_java_instance new controllers.JavaControllerInstance.index()
+GET /json_scala_instance new controllers.ScalaControllerInstance.index()

Hey @sadache https://github.com/sadache . I still can't figure out what
you were trying to say. I think perhaps you don't understand what's
happening in this example. I made a Java controller and named its class
JavaControllerInstance. Then when a request hits /json_java_instance, the
JavaControllerInstance class is instantiated and its index() method is
called. I also created a separate controller with a separate class named
ScalaControllerInstance just to prove that there would be no problem
instantiating either a Java or Scala controller and calling a method on it.

Please get back to me so that this pull request can be either committed or
updated as necessary.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/365/files#r1567596.

www.sadekdrobi.com
ʎdoɹʇuǝ

[guillaumebort]

Ok, I'm working on merging this but it breaks the current test suite on master. @benmccann Have you tried on your side?

[guillaumebort]

It's now merged. I've fixed the specs.

Btw I removed the new keyword to avoid the confusion with any standard Scala/Java construct (I've discussed this with @sadache). For now I've replaced it by the @ symbol, inspired by the way instance members are annotated in ruby. It's probably not perfect but at least there is no confusion with existing Scala construct and no temptation to extend it with parameter. Also I like it visually:

GET   /         @controllers.Application.index()

But we can still change that.

The only remaining issue is that now that we have added back runtime and reflection at this point it can now fail (like instantiation exception). And the errors are not properly handled, ending up with crappy stack trace.

It would be great to handling it properly and, in DEV mode, to display the error in the route file (with the corresponding route highlighted).

[pk11]

I think @jroper could address that error handling problem you mentioned in his new error-reporting-improvement branch

[benmccann]

Great, thank you SOOOO much!!

[pk11]

one extra thing, should not we expose this method in GlobalSettings (Java API) as well? ie. https://github.com/playframework/Play20/blob/master/framework/src/play/src/main/java/play/GlobalSettings.java

[guillaumebort]

Yes that's true, I missed that... The current implementation is bad and I think that this feature will mostly target Java developers. So it should be possible to override the getControllerInstance method in the Java Global object.

@benmccann Can you please fix that?

[pk11]

@benmccann it should be the same technique as say https://github.com/playframework/Play20/blob/master/framework/src/play/src/main/scala/play/core/j/JavaGlobalSettingsAdapter.scala#L38 thanks.

[guillaumebort]

I will fix it.

[benmccann]

I was just about to look at it, but I see you beat me to it. Thanks! Really appreciated you getting this in. It's going to make our lives sooo much easier.

[benmccann]

I added some documentation on the Java wiki and linked to it with the text "Dependency injection of controllers". If this is a good place for the documentation then I can add a page to the Scala wiki as well in a similar fashion.

[jroper]

Hi Guillaume,

Which errors are you referring to not being handled properly? I can't see anything introduced by this change that is not handled appropriately. In the parser, errors with ugly stack traces are an existing feature of routes parsing, there is nothing in the parser currently that displays errors nicely, so for example, if I have:

GET / contr#oller.Application.index

this results in an ugly stack trace. This is the same for getting something wrong with the @ symbol. This is something that I am currently working on in another change, making this error reporting nice, with the line of code etc that it occurred on, but that is orthogonal to this change.

When it comes to the Scala compilation stage of the generated source, everything in this is strongly typed, there is a strongly typed reference to the class name, the getControllerInstance method uses generics so it returns an instance of that class, and so then the method that is called on that is also strongly typed. Errors at this stage are nicely displayed in the web interface without a stack trace, when compiled in SBT currently they are displayed but in the generated Scala source, but my work on implementing a mapper for errors in SBT.

The final source of errors, and this is the only place that reflection happens, is in the users implementation of getControllerInstance(). This would typically be implemented by calling Guice or Spring, and in this case the exception and stack trace are very important, and so should be displayed, an ugly stack trace in this case I think is the right behaviour.

[guillaumebort]

@jroper Every error should be reported properly (in dev mode it's better to show the related source code directly).

GET / contr#oller.Application.index should be handled as parsing error and properly reported. (It works for me).

The new potential error introduced by this fix, is a runtime instanciation error. I think it should be reported in the router with the corresponding route highlighted and a message like "Cannot instantiate controllers.XXXXX"

[jroper]

With the parse error, it's only reported properly in the browser, not on the console, which means not at all if you run play compile, #455 fixes this.

As far as reporting instantiation errors goes with the corresponding route, since this is a runtime error, not a compile time error like all the other errors we display, this is no trivial task. The first step would be to work out what line of code we are on in the Scala file, when there's a compile error, the compiler gives you this, but we would only have an exception, which would either mean walking through the exception stack trace to try and work out what line of code we are, or embedding line of code data into the router data structure. Then we could proceed with the usual mapping back to the routes file.

But then I don't know what highlighting the error in the routes file would do, since it is not a syntax/compile exception, the routes file is not really relavent, what's relavent is why the controller itself can't be instantiated - it would probably be more relavent to show the source code of the controller constructor. In any case, DI frameworks usually give detailed and helpful error messages, listing the sequence of dependencies they were trying to construct in order to construct the controller, with the reason why the last dependency couldn't be instantiated. This is the important information, and displaying code from the routes file here I think would just be distracting.

Providing an awesome user experience here I think would be providing DI plugins with a simple way to generate a meaningful error page themselves, for example one that listed the chain of dependencies that led to the error, and the error stack trace itself. I think this would best be done in combination with writing a spring/guice plugin, to best understand what such a plugin will need.

[kutchar]

So how does this line work with current Scala object type controllers?
https://github.com/playframework/Play20/blob/master/framework/src/play/src/main/scala/play/api/GlobalSettings.scala#L130

[jroper]

It doesn't, if you use the current routes syntax, then this line won't be invoked at all, if you put an @ symbol before your call in your route, then you are indicating that the controller needs to be instantiated, which means you have declared the controller as a class (or trait) and, and then this is the code that's used to instantiate it (actually, you should override it in your own Global class to have Guice or Spring instantiate it, because just doing a newInstance() gives you nothing, but this is the default implementation that Play provides).

[kutchar]

Cool, thanks @jroper

[guillaumebort]

Also I've created a demo application demonstrating using Spring with this new feature.

https://github.com/guillaumebort/play20-spring-demo

[kalmanb]

Thanks Guillaume,

It looks like exactly what we've been looking for - well done!

Thanks for sharing
Kal

On Wed, Sep 19, 2012 at 8:18 PM, Guillaume Bort notifications@github.comwrote:

Also I've created a demo application demonstrating using Spring with this
new feature.

https://github.com/guillaumebort/play20-spring-demo

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/365#issuecomment-8683921.

[benmccann]

Also, I created a page on the wiki for this:
https://github.com/playframework/Play20/wiki/JavaDependencyInjection

I only created the Java page for now. However, if the page is okay and is
linked to from the right spot on the main Java page then I can add a Scala
one as well.

On Wed, Sep 19, 2012 at 2:32 AM, Kalman notifications@github.com wrote:

Thanks Guillaume,

It looks like exactly what we've been looking for - well done!

Thanks for sharing
Kal

On Wed, Sep 19, 2012 at 8:18 PM, Guillaume Bort notifications@github.comwrote:

Also I've created a demo application demonstrating using Spring with
this
new feature.

https://github.com/guillaumebort/play20-spring-demo

—
Reply to this email directly or view it on GitHub<
https://github.com/playframework/Play20/pull/365#issuecomment-8683921>.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/365#issuecomment-8685491.

about.me/benmccann

[torgeir]

Is this still working in 2.1.1? I added @controllers.Application.index() for a route, but I'm seeing compilation errors

conf/routes:6: value index is not a member of controllers.Application

This is on the java play 2.1.1, java 7 and scala 2.10

[benmccann]

Yes, it works. Do you have a non-static index method in your Application
controller?

On Sat, Jun 29, 2013 at 1:34 PM, Torgeir Thoresen
notifications@github.comwrote:

Is this still working in 2.1.1? I added @controllers.Application.index()for a route, but I'm seeing compilation errors

conf/routes:6: value index is not a member of controllers.Application

This is on the java play 2.1.1, java 7 and scala 2.10

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/365#issuecomment-20236692
.

about.me/benmccann

[torgeir]

Indeed it does, my method was of course still static. Thanks 👍