google-code-export / activeweb

Automatically exported from code.google.com/p/activeweb
0 stars 0 forks source link

Controller is not reloaded when active_reload is false with custom routes so controller values has what was requested on previous request. #109

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago
What steps will reproduce the problem?
1. Set active_reload=false or do not set it.
2. Create a custom route. Ex: route("/{fiCode}/").to(HomeController.class);
3. Set a view value in the controller action. Ex: view("test","not empty");
4. Test the view parameter in another request before setting the parameter. 
values().get("test") == null 

What is the expected output? What do you see instead?
Expected -> true
I see -> false

What version of the product are you using? On what operating system?

Please provide any additional information below.

Original issue reported on code.google.com by ezio.fer...@gmail.com on 14 Jul 2012 at 1:38

GoogleCodeExporter commented 9 years ago
Not sure I fully understand this. The "active_reload" is for reloading 
controllers or not. Meaning, if set to false, controllers will not be reloaded. 
This is an expected bahavior, right?

Original comment by i...@polevoy.org on 16 Jul 2012 at 4:08

GoogleCodeExporter commented 9 years ago
The problem  is that when active_reload is false and you have custom routes the 
vivew values are not cleared on each request.

Original comment by ezio.fer...@gmail.com on 16 Jul 2012 at 5:10

GoogleCodeExporter commented 9 years ago
is there a chunk of code maybe in test that can reproduce this?

Original comment by i...@polevoy.org on 16 Jul 2012 at 6:31

GoogleCodeExporter commented 9 years ago
[deleted comment]
GoogleCodeExporter commented 9 years ago
I have to write one. But this is simple to understand looking at the code:

protected Route recognize(String uri, HttpMethod httpMethod) throws 
ClassLoadException {

        Route route = matchCustom(uri, httpMethod);
        if (route == null) { //proceed to built-in routes

            //DTO as map here
            Map<String, String> controllerPath = getControllerPath(uri);

            String controllerName = controllerPath.get(Router.CONTROLLER_NAME);
            String packageSuffix = controllerPath.get(Router.PACKAGE_SUFFIX);
            if (controllerName == null) {
                return null;
            }
            String controllerClassName = getControllerClassName(controllerName, packageSuffix);
            AppController controller = createControllerInstance(controllerClassName);

            if (uri.equals("/") && rootControllerName != null && httpMethod.equals(HttpMethod.GET)) {
                return new Route(controller, "index");
            }

            route = controller.restful() ? matchRestful(uri, controllerName, packageSuffix, httpMethod, controller) :
                    matchStandard(uri, controllerName, packageSuffix, controller);
        }

        return route;
    }

private Route matchCustom(String uri, HttpMethod httpMethod) throws 
ClassLoadException {

        for (Route route : routes) {
            if (route.matches(uri, httpMethod)) {
                return route;
            }
        }
        return null;
    }

On match custom you are reusing the same route object instance (and using the 
same controller object associated with this route instance) on every request 
instead of creating a new route (like on matchRestful and matchStandard). 

You should add route.getController().values().clear() before returning the 
route.

Original comment by ezio.fer...@gmail.com on 16 Jul 2012 at 6:43

GoogleCodeExporter commented 9 years ago

Original comment by ipolevoy@gmail.com on 16 Jul 2012 at 8:18

GoogleCodeExporter commented 9 years ago
well, this is embarrassing, thanks for catching this bug. It is fixed now, 
please get the latest snapshot:
https://oss.sonatype.org/content/repositories/snapshots/org/javalite/activeweb/1
.2-SNAPSHOT/

Original comment by ipolevoy@gmail.com on 16 Jul 2012 at 3:13

GoogleCodeExporter commented 9 years ago
I also made a new release 1.2 and pushed it to Maven Central. It fixes this 
defect

Original comment by ipolevoy@gmail.com on 17 Jul 2012 at 1:29

GoogleCodeExporter commented 9 years ago
Sorry, I missed the point here too. At current implementation/architecture you 
MUST create a new Route for each request. If you do not the route will be the 
same for all concurrent requests and this is not thread safe. If you just clear 
the values all concurrent threads for the same route you have the values 
cleared (some at the middle of the request).

Original comment by ezio.fer...@gmail.com on 17 Jul 2012 at 1:31

GoogleCodeExporter commented 9 years ago
oh, man, you are right, :) I'm being soft focused these days, will fix

Original comment by ipolevoy@gmail.com on 17 Jul 2012 at 1:38

GoogleCodeExporter commented 9 years ago
I made another fix for defect. Thanks for catching. Now custom routs, like any 
other routes are reloaded on each request.

Original comment by ipolevoy@gmail.com on 17 Jul 2012 at 4:19

GoogleCodeExporter commented 9 years ago
Please, check out the latest snapshot: 
https://oss.sonatype.org/content/repositories/snapshots/org/javalite/activeweb/1
.3-SNAPSHOT/
If you think this fixes the defect, I will cut a new  version tomorrow. 

Original comment by ipolevoy@gmail.com on 17 Jul 2012 at 4:20

GoogleCodeExporter commented 9 years ago
This should work. As future optimization route should be a controller  factory 
and create a new controller instance at each request. Route should not have the 
controller instance as internal attribute and only the controller class. Thank 
you for the bug fixing!!!

Original comment by ezio.fer...@gmail.com on 17 Jul 2012 at 4:47