Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 1.0.2
    • Labels:
      None

      Description

      Spring provides an integration with tiles-2. We need one for tiles-3, i.e. integration with the Request API.

      Tentative implementation here: https://github.com/nlebas/tiles-request/tree/tiles-spring

        Issue Links

          Activity

          Nicolas Le Bas made changes -
          Status Open [ 1 ] Resolved [ 5 ]
          Fix Version/s 1.0.2 [ 12322970 ]
          Resolution Fixed [ 1 ]
          Hide
          Nicolas Le Bas added a comment -

          Released the necessary changes in tiles request into version 1.0.2.

          Show
          Nicolas Le Bas added a comment - Released the necessary changes in tiles request into version 1.0.2.
          Hide
          Nicolas Le Bas added a comment -

          OK, I believe I see your point: you want to use the existing spring ApplicationContext, that you may have customized to use a different ResourceLoader, instead of creating a new one like WildcardSAC does.

          This point to a larger issue IMHO: Tiles Request is not extensible enough when it comes to resource loading. We have some code to get resources from the ServletContext, from the Classpath, from Spring, but all of these are excluding each other because of the way it is implemented, and in some situations the code is duplicated. I think a strategy pattern would improve this: TREQ-17.

          PS: you do not need the ServletContextFactoryBean in a WebApplicationContext, it is implicitly available under the name "servletContext". If you want a single line declaration, perhaps this can work:

          <bean class="org.apache.tiles.request.servlet.wildcard.WildcardServletApplicationContext" autowire="constructor" />

          There's an error message in TilesConfigurer that needs to be updated, too.

          PPS: you can delegate to ServletApplicationContext instead of the Wildcard version. Same effect here.

          Show
          Nicolas Le Bas added a comment - OK, I believe I see your point: you want to use the existing spring ApplicationContext, that you may have customized to use a different ResourceLoader, instead of creating a new one like WildcardSAC does. This point to a larger issue IMHO: Tiles Request is not extensible enough when it comes to resource loading. We have some code to get resources from the ServletContext, from the Classpath, from Spring, but all of these are excluding each other because of the way it is implemented, and in some situations the code is duplicated. I think a strategy pattern would improve this: TREQ-17 . PS: you do not need the ServletContextFactoryBean in a WebApplicationContext, it is implicitly available under the name "servletContext". If you want a single line declaration, perhaps this can work: <bean class= "org.apache.tiles.request.servlet.wildcard.WildcardServletApplicationContext" autowire= "constructor" /> There's an error message in TilesConfigurer that needs to be updated, too. PPS: you can delegate to ServletApplicationContext instead of the Wildcard version. Same effect here.
          Hide
          Mck SembWever added a comment -

          I was a bit hasty in apply your patch.
          SpringApplicationContext is needed so resource loading happens through spring's context.
          I've re-added, modified so that a) delegation by default is to WildcardServletApplicationContext and b) delegation to other tile context types can happen by overriding createDelete(servletContext).

          Show
          Mck SembWever added a comment - I was a bit hasty in apply your patch. SpringApplicationContext is needed so resource loading happens through spring's context. I've re-added, modified so that a) delegation by default is to WildcardServletApplicationContext and b) delegation to other tile context types can happen by overriding createDelete(servletContext) .
          Hide
          Mck SembWever added a comment -

          These changes applied and included in the pull request.

          I'd like to add back SpringApplicationContext since it simplifies the spring xml from

              <bean id="servletContext" class="org.springframework.web.context.support.ServletContextFactoryBean"/>
              <bean class="org.apache.tiles.request.servlet.wildcard.WildcardServletApplicationContext">
                  <constructor-arg ref="servletContext"/>
              </bean>

          to

          <bean class="org.springframework.web.servlet.view.tiles3.SpringApplicationContext"/>
          Show
          Mck SembWever added a comment - These changes applied and included in the pull request. I'd like to add back SpringApplicationContext since it simplifies the spring xml from <bean id= "servletContext" class= "org.springframework.web.context.support.ServletContextFactoryBean" /> <bean class= "org.apache.tiles.request.servlet.wildcard.WildcardServletApplicationContext" > <constructor-arg ref= "servletContext" /> </bean> to <bean class= "org.springframework.web.servlet.view.tiles3.SpringApplicationContext" />
          Nicolas Le Bas made changes -
          Attachment 0002-Fixes-and-optimizations.patch [ 12539956 ]
          Hide
          Nicolas Le Bas added a comment -

          I think I had previously missed something, because you solution works very well.
          A few comments:

          • I'm having second thoughts about not having a pure-request implementation. Indeed very little change is needed to implement one: just a new class, TilesViewResolver, and moving 3 lines of actual code to it. Then we can make Tiles an optional dependency.
          • Since you had the good idea to simplify SpringApplicationContext, it is now identical to WildcardServletApplicationContext. Let's remove the new one and encourage people to use the existing one.
          • I've found a couple of bugs with TilesConfigurer, and the unit test was missing.

          I'm attaching a patch with these improvements and bug fixes, if you'd like to review it.

          Show
          Nicolas Le Bas added a comment - I think I had previously missed something, because you solution works very well. A few comments: I'm having second thoughts about not having a pure-request implementation. Indeed very little change is needed to implement one: just a new class, TilesViewResolver, and moving 3 lines of actual code to it. Then we can make Tiles an optional dependency. Since you had the good idea to simplify SpringApplicationContext, it is now identical to WildcardServletApplicationContext. Let's remove the new one and encourage people to use the existing one. I've found a couple of bugs with TilesConfigurer, and the unit test was missing. I'm attaching a patch with these improvements and bug fixes, if you'd like to review it.
          Hide
          Nicolas Le Bas added a comment -

          I remember trying something like that and failing for some reason.

          I'll test it properly soon and come back with a proper, specific feedback.

          Show
          Nicolas Le Bas added a comment - I remember trying something like that and failing for some reason. I'll test it properly soon and come back with a proper, specific feedback.
          Hide
          Mck SembWever added a comment -

          it should eventually extend AbtractTemplateViewResolver, not UrlBasedViewResolver.

          The following changeset does this.
          https://github.com/michaelsembwever/spring-framework-issues/commit/98f67c1e4e784f8aab289e1321ae54f4f391e902

          Agree to include this into the formal patch for SPR-8825?

          Show
          Mck SembWever added a comment - it should eventually extend AbtractTemplateViewResolver, not UrlBasedViewResolver. The following changeset does this. https://github.com/michaelsembwever/spring-framework-issues/commit/98f67c1e4e784f8aab289e1321ae54f4f391e902 Agree to include this into the formal patch for SPR-8825?
          Hide
          Nicolas Le Bas added a comment - - edited

          I'm not sure exactly why I implemented it this way. Possibly 2 reasons:

          • l10n support
          • calling renderer.isRenderable(path, request) and eventually pass control to the next view resolver (URLBasedViewResolver never passes control: it only checks viewNames before it even checks for redirects).

          It may be worth testing it again... it should eventually extend AbtractTemplateViewResolver, not UrlBasedViewResolver.

          You can always setup an UrlBasedViewResolver on top of it, but perhaps RendererViewResolver should implement Ordered.

          Edit: Now that I think of it, if we're implementing it as a part of Spring instead of Tiles, we are able to suggest improvements on UrlBasedViewResolver, so there must be a way to make it work.

          Show
          Nicolas Le Bas added a comment - - edited I'm not sure exactly why I implemented it this way. Possibly 2 reasons: l10n support calling renderer.isRenderable(path, request) and eventually pass control to the next view resolver (URLBasedViewResolver never passes control: it only checks viewNames before it even checks for redirects). It may be worth testing it again... it should eventually extend AbtractTemplateViewResolver, not UrlBasedViewResolver. You can always setup an UrlBasedViewResolver on top of it, but perhaps RendererViewResolver should implement Ordered. Edit: Now that I think of it, if we're implementing it as a part of Spring instead of Tiles, we are able to suggest improvements on UrlBasedViewResolver, so there must be a way to make it work.
          Hide
          Mck SembWever added a comment - - edited

          Regarding RendererViewResolver:

          • it now extends AbstractCachingViewResolver instead of UrlBasedViewResolver. This means that "redirect:.." and "forward:.." spring definition names no longer work. Is there any reason to remove these features?
          Show
          Mck SembWever added a comment - - edited Regarding RendererViewResolver : it now extends AbstractCachingViewResolver instead of UrlBasedViewResolver. This means that "redirect:.." and "forward:.." spring definition names no longer work. Is there any reason to remove these features?
          Hide
          Nicolas Le Bas added a comment -

          Yes, we can tackle it later. It shouldn't hold up SPR-8825 anyway, I stated as much over there.

          Show
          Nicolas Le Bas added a comment - Yes, we can tackle it later. It shouldn't hold up SPR-8825 anyway, I stated as much over there.
          Hide
          Mck SembWever added a comment -

          Do we agree that SpringRequest is (slightly) more useful if it extends DefaultRequestWrapper instead of DispatchRequestWrapper?

          Yes. But this can't happen until your DispatchRenderer comes into trunk and a subsequent release. So we can tackle it as a improvement afterwards?

          So is there anything holding up this issue and SPR-8825?

          Show
          Mck SembWever added a comment - Do we agree that SpringRequest is (slightly) more useful if it extends DefaultRequestWrapper instead of DispatchRequestWrapper? Yes. But this can't happen until your DispatchRenderer comes into trunk and a subsequent release. So we can tackle it as a improvement afterwards? So is there anything holding up this issue and SPR-8825?
          Hide
          Nicolas Le Bas added a comment -

          TilesConfigurer and DefaultRendererViewResolver

          Oh yes, I missed that one. I've found it easy enough to instanciate a new DefinitionRenderer directly in applicationContext.xml (with AOP for profiling).
          BTW, do you think we should make TilesContainer implement Renderer directly?

          One package is fine with me. From a spring point of view, this is just one component among many others.

          But how would DispatchRequest in turn become a hidden internal or removed?

          I failed to make my case in that discussion you refer to, and I don't want to reopen it here. DispatchRequest is needed for backwards-compatible behaviour, and an improvement over tiles-2.2. I still hope that sooner or later we will agree on what's needed for the future; in the mean time we have no clear plan for DispatchRequest, and I see it as touchy.

          Do we agree that SpringRequest is (slightly) more useful if it extends DefaultRequestWrapper instead of DispatchRequestWrapper? Right now we integrate only with spring MVC, but I hope things like spring-mail could work with tiles-request in the future.

          Unwrapping is not perfect either, since we're loosing some functionality (like the extra scopes added when wrapping), but that's basically what DispatchRenderer will be doing when relying on servlet/portlet anyway.

          Show
          Nicolas Le Bas added a comment - TilesConfigurer and DefaultRendererViewResolver Oh yes, I missed that one. I've found it easy enough to instanciate a new DefinitionRenderer directly in applicationContext.xml (with AOP for profiling). BTW, do you think we should make TilesContainer implement Renderer directly? One package is fine with me. From a spring point of view, this is just one component among many others. But how would DispatchRequest in turn become a hidden internal or removed? I failed to make my case in that discussion you refer to, and I don't want to reopen it here. DispatchRequest is needed for backwards-compatible behaviour, and an improvement over tiles-2.2. I still hope that sooner or later we will agree on what's needed for the future; in the mean time we have no clear plan for DispatchRequest, and I see it as touchy. Do we agree that SpringRequest is (slightly) more useful if it extends DefaultRequestWrapper instead of DispatchRequestWrapper? Right now we integrate only with spring MVC, but I hope things like spring-mail could work with tiles-request in the future. Unwrapping is not perfect either, since we're loosing some functionality (like the extra scopes added when wrapping), but that's basically what DispatchRenderer will be doing when relying on servlet/portlet anyway.
          Hide
          Mck SembWever added a comment -

          separate packages are fine, but if we move the code to .tiles3.request, what would be left in .tiles3? Only TilesConfigurer?

          TilesConfigurer and DefaultRendererViewResolver (the latter a subclass of RendererViewResolver that simply adds the default renderer to DefinitionRenderer, meaning the RendererViewResolver in .tiles.3.request would again be clean of tiles framework code).

          My question was more if you feel this is necessary? I'm otherwise happy to have everyone just in one package.

          ...uncomfortable with DispatchRequest and the forward/include algorithm beneath it.

          Yeah I'd forgotten about this: http://thread.gmane.org/gmane.comp.apache.tiles.devel/263/ and http://thread.gmane.org/gmane.comp.apache.tiles.devel/302/

          The idea is ultimately we can remove DispatchRequestWrapper since DispatchRenderer would handle either cases of RequestWrapper or DispatchRequest, so the combination isn't required. But how would DispatchRequest in turn become a hidden internal or removed? There was suggestion in moving dispatch(..) to DispatchRenderer (forward first request, include subsequent) and include/forward to ApplicationContext (a ViewApplicationContext wrapping context is required to override forward calls to be includes, if DispatchRenderer has no smart way of seeing a request that can't be forwarded).

          If your DispatchRenderer comes into the next release then SpringRequest can indeed extend the simpler DefaultRequestWrapper.
          But here spring-web+tiles integration will always be servlet based, these classes (DispatchRequestWrapper and DispatchRequest) are already public in the tiles-request-1.0 release, and work on refactoring dispatch/include/forward methods mean breaking compatibility, so is it important to hide these things right now?

          ... they should be called by the Renderer (perhaps DefinitionRenderer... the fewer implementation details we expose, the better.

          Agreed.

          Show
          Mck SembWever added a comment - separate packages are fine, but if we move the code to .tiles3.request, what would be left in .tiles3? Only TilesConfigurer? TilesConfigurer and DefaultRendererViewResolver (the latter a subclass of RendererViewResolver that simply adds the default renderer to DefinitionRenderer, meaning the RendererViewResolver in .tiles.3.request would again be clean of tiles framework code). My question was more if you feel this is necessary? I'm otherwise happy to have everyone just in one package. ...uncomfortable with DispatchRequest and the forward/include algorithm beneath it. Yeah I'd forgotten about this: http://thread.gmane.org/gmane.comp.apache.tiles.devel/263/ and http://thread.gmane.org/gmane.comp.apache.tiles.devel/302/ The idea is ultimately we can remove DispatchRequestWrapper since DispatchRenderer would handle either cases of RequestWrapper or DispatchRequest, so the combination isn't required. But how would DispatchRequest in turn become a hidden internal or removed? There was suggestion in moving dispatch(..) to DispatchRenderer (forward first request, include subsequent) and include/forward to ApplicationContext (a ViewApplicationContext wrapping context is required to override forward calls to be includes, if DispatchRenderer has no smart way of seeing a request that can't be forwarded). If your DispatchRenderer comes into the next release then SpringRequest can indeed extend the simpler DefaultRequestWrapper. But here spring-web+tiles integration will always be servlet based, these classes (DispatchRequestWrapper and DispatchRequest) are already public in the tiles-request-1.0 release, and work on refactoring dispatch/include/forward methods mean breaking compatibility, so is it important to hide these things right now? ... they should be called by the Renderer (perhaps DefinitionRenderer... the fewer implementation details we expose, the better. Agreed.
          Hide
          Nicolas Le Bas added a comment -

          Thanks for taking the time to review and test it. I missed a couple of things indeed since I'm not using JSP currently, and you did a great job of solving that!

          I'll need to test your changes more deeply, but a few ideas on the spot:

          • separate packages are fine, but if we move the code to .tiles3.request, what would be left in .tiles3? Only TilesConfigurer?
          • you know I'm uncomfortable with DispatchRequest and the forward/include algorithm beneath it. As much as possible, I'd like to keep it internal here at Apache so that we have a chance to improve on it eventually.
            As an alternative, I would suggest to have DispatchRenderer "unwrap" its request until it can find something dispatchable. I've been testing this for some time (I'm using it for TREQ-14 non-servlet freemarker support and possibly TREQ-13, too). It would fit the bill, I think.
          • startContext/endContext: they're needed for TILES-544, i.e. for including DispatchServlet-based stuff. But with a request-based integration, they should be called by the Renderer (perhaps DefinitionRenderer, or a new one?), not by RendererView. Testing would be required, but I think this integration would allow us to take charge of TILES-544 here at Apache. IMHO, the fewer implementation details we expose, the better.
          Show
          Nicolas Le Bas added a comment - Thanks for taking the time to review and test it. I missed a couple of things indeed since I'm not using JSP currently, and you did a great job of solving that! I'll need to test your changes more deeply, but a few ideas on the spot: separate packages are fine, but if we move the code to .tiles3.request, what would be left in .tiles3? Only TilesConfigurer? you know I'm uncomfortable with DispatchRequest and the forward/include algorithm beneath it. As much as possible, I'd like to keep it internal here at Apache so that we have a chance to improve on it eventually. As an alternative, I would suggest to have DispatchRenderer "unwrap" its request until it can find something dispatchable. I've been testing this for some time (I'm using it for TREQ-14 non-servlet freemarker support and possibly TREQ-13 , too). It would fit the bill, I think. startContext/endContext: they're needed for TILES-544 , i.e. for including DispatchServlet-based stuff. But with a request-based integration, they should be called by the Renderer (perhaps DefinitionRenderer, or a new one?), not by RendererView. Testing would be required, but I think this integration would allow us to take charge of TILES-544 here at Apache. IMHO, the fewer implementation details we expose, the better.
          Hide
          Mck SembWever added a comment -

          I've built upon your changeset Nicolas and committed it here:
          https://github.com/michaelsembwever/spring-framework-issues/commit/8c2d45bdcf698ae9072f4213fe72d0bf8ad6bb8f

          From there i've created a new pull request for SPR-8825
          https://github.com/SpringSource/spring-framework-issues/pull/30

          A quick review on the long commit msg would be nice. Of particular question is:

          • "...maybe seperate packages could be used ".tiles3" and ".tiles3.request" where the clean tiles-request code can be moved to?"
          • "...i'm uncertain if startContext and endContext is required..." (ref: TILES-544)
          Show
          Mck SembWever added a comment - I've built upon your changeset Nicolas and committed it here: https://github.com/michaelsembwever/spring-framework-issues/commit/8c2d45bdcf698ae9072f4213fe72d0bf8ad6bb8f From there i've created a new pull request for SPR-8825 https://github.com/SpringSource/spring-framework-issues/pull/30 A quick review on the long commit msg would be nice. Of particular question is: "...maybe seperate packages could be used ".tiles3" and ".tiles3.request" where the clean tiles-request code can be moved to?" "...i'm uncertain if startContext and endContext is required..." (ref: TILES-544 )
          Nicolas Le Bas made changes -
          Description Spring provides an integration with tiles-2. We need one for tiles-3, i.e. integration with the Request API.

          Tentative implementation here: https://github.com/nlebas/tiles/tree/tiles-spring
          Spring provides an integration with tiles-2. We need one for tiles-3, i.e. integration with the Request API.

          Tentative implementation here: https://github.com/nlebas/tiles-request/tree/tiles-spring
          Mck SembWever made changes -
          Field Original Value New Value
          Link This issue relates to TILES-545 [ TILES-545 ]
          Hide
          Nicolas Le Bas added a comment -

          Thanks. The other half (Tiles configuration) is over there: https://issues.apache.org/jira/browse/TILES-545.

          Contributing this to Spring instead of Apache is definitely an option for me, same license anyway.

          Come to think of it, we would get the most flexibility if this half (Spring->Request API) were maintained at Spring and the other (Tiles container creation) at Apache. The effort involved to maintain compatibility would be minimal and each project would have the freedom to evolve independently.

          Show
          Nicolas Le Bas added a comment - Thanks. The other half (Tiles configuration) is over there: https://issues.apache.org/jira/browse/TILES-545 . Contributing this to Spring instead of Apache is definitely an option for me, same license anyway. Come to think of it, we would get the most flexibility if this half (Spring->Request API) were maintained at Spring and the other (Tiles container creation) at Apache. The effort involved to maintain compatibility would be minimal and each project would have the freedom to evolve independently.
          Hide
          Mck SembWever added a comment -

          i like the way your code implements it behind the request api.
          i'll give this a go and test it properly soon.

          Show
          Mck SembWever added a comment - i like the way your code implements it behind the request api. i'll give this a go and test it properly soon.
          Show
          Mck SembWever added a comment - https://jira.springsource.org/browse/SPR-8825
          Nicolas Le Bas created issue -

            People

            • Assignee:
              Unassigned
              Reporter:
              Nicolas Le Bas
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development