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

Merged
merged 7 commits into from Sep 10, 2012

Conversation

Projects
None yet
Contributor

benmccann commented Jun 17, 2012

No description provided.

Contributor

julienrf commented Jun 17, 2012

Does it work with @With annotations?

Contributor

benmccann commented Jun 17, 2012

Yes, it does with with @with !

ijuma commented Jun 17, 2012

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.

Collaborator

guillaumebort commented Jul 6, 2012

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

Contributor

benmccann commented Jul 6, 2012

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?

Contributor

benmccann commented Jul 10, 2012

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.

Collaborator

guillaumebort commented Jul 10, 2012

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.
Contributor

daggerrz commented Jul 11, 2012

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.

Contributor

benmccann commented Jul 11, 2012

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
Collaborator

guillaumebort commented Jul 11, 2012

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

Contributor

benmccann commented Jul 11, 2012

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

Collaborator

pk11 commented Jul 11, 2012

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

Collaborator

pk11 commented Jul 11, 2012

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
Contributor

benmccann commented Jul 11, 2012

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.

Collaborator

pk11 commented Jul 11, 2012

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"})
      }
   """
)
Contributor

benmccann commented Jul 11, 2012

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)

Collaborator

pk11 commented Jul 11, 2012

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)

Contributor

benmccann commented Jul 11, 2012

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.

Contributor

benmccann commented Jul 12, 2012

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

Contributor

benmccann commented Jul 16, 2012

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

@julienrf julienrf and 1 other commented on an outdated diff Jul 16, 2012

...src/play/src/main/scala/play/core/router/Router.scala
@@ -797,13 +817,13 @@ object Router {
case (r @ Route(_,_,_), i) =>
"""
|%s
- |private[this] lazy val %s%s = Route("%s", %s)
+ |lazy val %s%s = Route("%s", %s)
@julienrf

julienrf Jul 16, 2012

Contributor

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

@benmccann

benmccann Jul 16, 2012

Contributor

Added back.

@julienrf julienrf and 1 other commented on an outdated diff Jul 16, 2012

...src/play/src/main/scala/play/core/router/Router.scala
@@ -737,7 +757,7 @@ object Router {
case Seq(route, routes @ _*) => {
"""
|%s
- |def %s(%s): Call = {
+ |def %s(%s) = {
@julienrf

julienrf Jul 16, 2012

Contributor

Why did you need to remove explicit return types? It avoided to run the type inferencer (and thus made the compilation time faster).

@benmccann

benmccann Jul 16, 2012

Contributor

Thanks you for noticing. That was not intentional. I created this change before that change was committed and so I was missing recent changes. I have updated it to merge in those changes.

Contributor

benmccann commented Jul 19, 2012

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

Contributor

benmccann commented Jul 24, 2012

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

Collaborator

guillaumebort commented Jul 24, 2012

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

Contributor

benmccann commented Jul 24, 2012

Great. Thanks for the update!

Contributor

benmccann commented Aug 3, 2012

Any further update on this?

Contributor

manuelbernhardt commented Aug 24, 2012

I'd be very interested in this feature being available in the next release. Right now I have to resort to class gymnastics in order to use controllers with subcut.

Contributor

betehess commented Aug 24, 2012

I would also be interested to see this patch being integrated. The current solutions really are a PITA.

Contributor

benmccann commented Aug 31, 2012

@guillaumebort an interesting issue was raised recently by @jroper on another thread. he said that he'd like to be able to get an instance of Global from the injector. i think that might be difficult if the injection method lives in Global because it would cause a bit of a cycle. i hadn't thought about that before, but i think it's a good idea. what do you think about moving it back out to a separate class and if you wanted to do that do you care what i call it?

(also, this is the fifth most popular issue on lighthouse and the only one of those top five with a pull request pending. i really think that speaks to how much the community wants to see this and would hope it could be prioritized accordingly as being the next pull request to get checked in. this has been open for 3 months and there's been a lot of less important items given priority - http://lighthouse-stats.herokuapp.com/play/projects/82401/tickets/popular )

Collaborator

guillaumebort commented Aug 31, 2012

I still think that Global is the good place to manage this. We need a "global singleton instance" to manage everything that is global to the application, and Global was introduced for this.

Nothing prevent you to delegate the Global calls to other components managed by Spring if it's really what you want to do.

The only issue left is the new syntax to introduce in the route file. I'm not convinced by the current one. Why no simply prefixing the call by the keyword new? Like

GET    /     new controllers.Application.index()
Owner

jroper commented Aug 31, 2012

I like that syntax.

Also I didn't mean inject Global, I meant if Global has a constructor that
accepts a Play application, inject the application, otherwise just use the
noarg constructor like it currently does.

ijuma commented Aug 31, 2012

Both the syntax and the possibility of Global accepting a parameter sound good IMO.

Contributor

benmccann commented Aug 31, 2012

@jroper @ijuma to clarify when you say you like the syntax, do you mean you like Guillaume's proposed syntax or the existing syntax? I am happy to change if the consensus is for new over the parentheses. Thanks!

Contributor

benmccann commented Sep 1, 2012

I've updated this pull request per @guillaumebort 's proposed syntax and added another test.

Contributor

benmccann commented Sep 4, 2012

I've synced this change with the latest version of Play from head. Please take a look and let me know whether further changes are necessary before you commit it.

I also would like to see this make its way in. I agree with @manuelbernhardt and @betehess that it is a huge pain in the you know what currently. Pleeeaase do something here because @benmccann submitted this months ago and I do not understand why this is being ignored

Collaborator

guillaumebort commented Sep 6, 2012

It's not ignored, we are just working to make it right. This is an important change and we want to be sure that we come with the best solution.

Collaborator

guillaumebort commented Sep 6, 2012

But for me, it's now fine. @pk11 @sadache @jroper Is it ok for you guys?

Collaborator

pk11 commented Sep 6, 2012

I think generally speaking it's OK, however we may need to tweak a few parts (like the config loading piece), if that's OK, then I am fine with this.

@guillaumebort guillaumebort and 1 other commented on an outdated diff Sep 6, 2012

...src/play/src/main/scala/play/core/router/Router.scala
@@ -68,6 +68,17 @@ object Router {
*/
object RoutesCompiler {
+ import java.io.File
+ import scalax.file._
+ import com.typesafe.config._
+
+ // TODO: this doesn't seem like a great way of getting the config
@guillaumebort

guillaumebort Sep 6, 2012

Collaborator

Yes that's true. And anyway we don't need to read the config at all here since we are delegating the classes instantiation to the Global object. I am missing something?

@benmccann

benmccann Sep 6, 2012

Contributor

How do we know what the Global object is without reading the config? What if the user changes the application.global setting so that the global object is now com.example.MyGlobal?

@guillaumebort

guillaumebort Sep 6, 2012

Collaborator

We have to use the global instance provided by the application at Runtime.

val global = play.api.Play.maybeApplication.map(_.global).getOrElse(play.api.DefaultGlobal)
@benmccann

benmccann Sep 6, 2012

Contributor

Ah, I see. Thanks. I'll make the changes you suggested.

@benmccann

benmccann Sep 6, 2012

Contributor

Done

@guillaumebort guillaumebort and 1 other commented on an outdated diff Sep 6, 2012

...src/play/src/main/scala/play/core/router/Router.scala
@@ -68,6 +68,17 @@ object Router {
*/
object RoutesCompiler {
+ import java.io.File
+ import scalax.file._
+ import com.typesafe.config._
+
+ // TODO: this doesn't seem like a great way of getting the config
+ val config = ConfigFactory.load(ConfigFactory.parseFileAnySyntax(new File("conf/application.conf")));
+ val global = try {
+ config.getString("application.global");
+ } catch { case e: ConfigException.Missing => "Global" }
+ val classFactoryMethodName = global + ".getControllerInstance";
@guillaumebort

guillaumebort Sep 6, 2012

Collaborator

No need to use reflection here. We can have a static reference to the Global object.

@benmccann

benmccann Sep 6, 2012

Contributor

Done

@guillaumebort guillaumebort and 1 other commented on an outdated diff Sep 6, 2012

...src/play/src/main/scala/play/core/router/Router.scala
@@ -169,6 +177,14 @@ object Router {
Some(route.call.pos.column))
}
+ if (route.call.instantiate && classFactoryMethodName == null) {
@guillaumebort

guillaumebort Sep 6, 2012

Collaborator

I don't see the point of doing this. Since it's built-in, managed by the Global object, and using a special syntax, there is no need to check the configuration.

@benmccann

benmccann Sep 6, 2012

Contributor

Done

@guillaumebort guillaumebort and 1 other commented on an outdated diff Sep 6, 2012

...src/play/src/main/scala/play/core/router/Router.scala
@@ -590,6 +606,12 @@ object Router {
val reverseSignature = parameters.map(p => p.name + ":" + p.typeName).mkString(", ")
+ val controllerCall = if (route.call.instantiate) {
+ classFactoryMethodName + "(classOf[" + packageName + "." + controller + "])." + route.call.field.map(_ + ".").getOrElse("") + route.call.method + "(" + { parameters.map(_.name).mkString(", ") } + ")"
@guillaumebort

guillaumebort Sep 6, 2012

Collaborator

So, we should replace this by something like:

application.global.getControllerInstance(classOf[..])...
@benmccann

benmccann Sep 6, 2012

Contributor

Done

@guillaumebort guillaumebort and 1 other commented on an outdated diff Sep 6, 2012

...src/play/src/main/scala/play/core/router/Router.scala
@@ -874,7 +896,11 @@ object Router {
}.map("(" + _ + ") =>").getOrElse(""),
// call
- r.call.packageName + "." + r.call.controller + "." + r.call.field.map(_ + ".").getOrElse("") + r.call.method,
+ if (r.call.instantiate) {
+ classFactoryMethodName + "(classOf[" + r.call.packageName + "." + r.call.controller + "])." + r.call.field.map(_ + ".").getOrElse("") + r.call.method
@guillaumebort

guillaumebort Sep 6, 2012

Collaborator

Same as before.

@benmccann

benmccann Sep 6, 2012

Contributor

Done

@guillaumebort guillaumebort and 1 other commented on an outdated diff Sep 6, 2012

...src/play/src/main/scala/play/core/router/Router.scala
@@ -897,8 +923,9 @@ object Router {
case class HttpVerb(value: String) {
override def toString = value
}
- case class HandlerCall(packageName: String, controller: String, method: String, field: Option[String], parameters: Option[Seq[Parameter]]) extends Positional {
- override def toString = packageName + "." + controller + "." + field.map(_ + ".").getOrElse("") + method + parameters.map { params =>
+ case class HandlerCall(packageName: String, controller: String, instantiate: Boolean, method: String, field: Option[String], parameters: Option[Seq[Parameter]]) extends Positional {
+ val dynamic = if (instantiate) "()" else ""
@guillaumebort

guillaumebort Sep 6, 2012

Collaborator

Not coherent with the previous change. Shouldn't be replaced by new?

@benmccann

benmccann Sep 6, 2012

Contributor

Done. Good catch

Collaborator

guillaumebort commented Sep 6, 2012

@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.

Contributor

benmccann commented Sep 6, 2012

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

Collaborator

guillaumebort commented Sep 6, 2012

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

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

@sadache sadache commented on the diff Sep 8, 2012

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()
@sadache

sadache Sep 8, 2012

Collaborator

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

benmccann Sep 8, 2012

Contributor

@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

benmccann Sep 10, 2012

Contributor

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

sadache Sep 18, 2012

Collaborator

@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/playframework/Play20/pull/365/files#r1567596.

www.sadekdrobi.com
ʎdoɹʇuǝ

Collaborator

guillaumebort commented Sep 10, 2012

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

@guillaumebort guillaumebort merged commit eb26cf8 into playframework:master Sep 10, 2012

Collaborator

guillaumebort commented Sep 10, 2012

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).

Collaborator

pk11 commented Sep 10, 2012

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

Contributor

benmccann commented Sep 10, 2012

Great, thank you SOOOO much!!

Collaborator

pk11 commented Sep 10, 2012

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

Collaborator

guillaumebort commented Sep 10, 2012

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?

Collaborator

guillaumebort commented Sep 10, 2012

I will fix it.

Contributor

benmccann commented Sep 10, 2012

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.

Contributor

benmccann commented Sep 11, 2012

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.

Owner

jroper commented Sep 18, 2012

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.

Collaborator

guillaumebort commented Sep 18, 2012

@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"

Owner

jroper commented Sep 18, 2012

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, playframework#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.

Contributor

kutchar commented Sep 19, 2012

Owner

jroper commented Sep 19, 2012

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).

Contributor

kutchar commented Sep 19, 2012

Cool, thanks @jroper

Collaborator

guillaumebort commented Sep 19, 2012

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

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

kalmanb commented Sep 19, 2012

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/playframework/Play20/pull/365#issuecomment-8683921.

Contributor

benmccann commented Sep 19, 2012

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/playframework/Play20/pull/365#issuecomment-8685491.

about.me/benmccann

betehess referenced this pull request in w3c/validator-suite Sep 21, 2012

Closed

fix the configuration injection in the controller objects #43

torgeir commented Jun 29, 2013

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

Contributor

benmccann commented Jun 29, 2013

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/playframework/Play20/pull/365#issuecomment-20236692
.

about.me/benmccann

torgeir commented Jun 30, 2013

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment