Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: None
    • Component/s: None
    • Labels:
      None

      Description

      Hi,

      I found a critical problem with Click automapping:
      The missing of the possiblity to "exclude" some of the paths from mapping.

      I was trying to integrate TinyMCE in advanced mode but this is totally unsuable:

      • many of the TinyMCE pages/resources have the *.htm extension and of course, Click
        is trying to solve them and it's giving the "The page you requested was not found." message
        (e.g. for all the popup windows that TinyMCE is using, this problem can be seen).
        This situation (using *.htm files) is true for many other DHTML libraries that might be the
        target of integration or just use with Click framework.

      Please include such an "exclusion" functionality.
      The simplest and most intutitive way that I can think of is to do it like
      it's done with SiteMesh but in case of Click this would be in click.xml, so
      no extra file would be required.
      e.g.
      <pages package="examples.page" automapping="true">
      <page path="exception.htm" classname="ExceptionDemo"/>
      <excludes>
      <pattern>/exclude_me.htm"</pattern>
      <pattern>/admin/changelog.htm"</pattern>
      <pattern>/tiny_mce/*"</pattern>
      <pattern>/other_dhtml_lib/something/*"</pattern>
      </excludes>
      </pages>
      or a similar solution.

      Thanks in advance,

      Ahmed.

        Activity

        Ahmed Mohombe created issue -
        Hide
        Malcolm Edgar added a comment -

        Hi Ahmed,

        I would like to address this issue in the following release. 0.19

        regards malcolm

        Show
        Malcolm Edgar added a comment - Hi Ahmed, I would like to address this issue in the following release. 0.19 regards malcolm
        Hide
        Malcolm Edgar added a comment -

        Hi Ahmed,

        I had a think about this today.

        With J2EE spec (as you know) you dont have much granularity with mapping request to servlets, so to have ClickServlet handle *.htm files in the web app root directory and sub directories you need something like:

        <servlet-mapping>
        <servlet-name>click-servlet</servlet-name>
        <url-pattern>*.htm</url-pattern>
        </servlet-mapping>

        Now if the ClickServlet is going to handle these requests (which is pretty well unavoidable from what I understand) we need to do it in a performant manner. The easiest way to implement this is to use the existing Page/Velocity infrastructure and map certain path resources to the Page class. This way they will be served up using the existing infrastructure.

        Other alternatives I could think of are much more disruptive in terms of implementation, impacting both ClickServlet and ClickApp.

        Using this approach we are overriding the default automapping of Page classes to resources. So its possibly better to describe in the click.xml in these terms.

        <pages package="examples.page" automapping="true">
        <page path="exception.htm" classname="ExceptionDemo"/>
        <autooverrides path="/tiny_mce*" classname="net.sf.click.Page"/>
        </pages>

        The auto overrides element could also default to the Page class as well:

        <pages package="examples.page" automapping="true">
        <page path="exception.htm" classname="ExceptionDemo"/>
        <autooverrides path="/tiny_mce*"/>
        </pages>

        What do you thing?

        regards Malcolm

        Show
        Malcolm Edgar added a comment - Hi Ahmed, I had a think about this today. With J2EE spec (as you know) you dont have much granularity with mapping request to servlets, so to have ClickServlet handle *.htm files in the web app root directory and sub directories you need something like: <servlet-mapping> <servlet-name>click-servlet</servlet-name> <url-pattern>*.htm</url-pattern> </servlet-mapping> Now if the ClickServlet is going to handle these requests (which is pretty well unavoidable from what I understand) we need to do it in a performant manner. The easiest way to implement this is to use the existing Page/Velocity infrastructure and map certain path resources to the Page class. This way they will be served up using the existing infrastructure. Other alternatives I could think of are much more disruptive in terms of implementation, impacting both ClickServlet and ClickApp. Using this approach we are overriding the default automapping of Page classes to resources. So its possibly better to describe in the click.xml in these terms. <pages package="examples.page" automapping="true"> <page path="exception.htm" classname="ExceptionDemo"/> <autooverrides path="/tiny_mce*" classname="net.sf.click.Page"/> </pages> The auto overrides element could also default to the Page class as well: <pages package="examples.page" automapping="true"> <page path="exception.htm" classname="ExceptionDemo"/> <autooverrides path="/tiny_mce*"/> </pages> What do you thing? regards Malcolm
        Malcolm Edgar made changes -
        Field Original Value New Value
        Status Open [ 1 ] In Progress [ 3 ]
        Hide
        Ahmed Mohombe added a comment -

        Sorry but I think the approach you proposed it's a little bit complicated and it's not that straightforward from a user perspective.

        The SiteMesh approach is very intuitive and everybody who tried SiteMesh once is using since than in all the projects.

        The original idea would also have the advantage in case of JSPs not to "map"/"automap" those JSPs that fall in the "excluded" pattern, so to threat them as simple JSPs not as "views" to Click Pages like those with Velocity .

        Also "autooverrides" is just too unintuitive .
        "exclude"/""excludes" is more natural IMHO, since it needs no futher explanations, and require no internal knowledge "of something being overrriden" : there's a mapping and one is excluding things from it .

        Specifying "classname=net.sf.click.Page" is also not relevant IMHO, since when excluding one is thinking globaly to the entire webapplication, not "in pages".

        I also like the fine grained exclusion allowed by Sitemesh by using the combination of patterns. In many cases is not just an entire directory like in the case of TinyMCE : "/tiny_mce*"

        As I mentioned, I see this as an improvement (completeness) to the "automapping", so since the automapping is done by the ClickServlet, than indeed these exclusion should be handeled there too .

        Of course the synthax could also be more compressed:
        <pages package="examples.page" automapping="true">
        <excludes patterns="exclude_me.htm, /admin/changelog.htm,/tiny_mce/, /other_dhtml_lib/something/" />
        <page path="exception.htm" classname="ExceptionDemo"/>
        </pages>

        But even so, it's much simpler to read .

        Thanks in advance,

        Ahmed.

        Show
        Ahmed Mohombe added a comment - Sorry but I think the approach you proposed it's a little bit complicated and it's not that straightforward from a user perspective. The SiteMesh approach is very intuitive and everybody who tried SiteMesh once is using since than in all the projects. The original idea would also have the advantage in case of JSPs not to "map"/"automap" those JSPs that fall in the "excluded" pattern, so to threat them as simple JSPs not as "views" to Click Pages like those with Velocity . Also "autooverrides" is just too unintuitive . "exclude"/""excludes" is more natural IMHO, since it needs no futher explanations, and require no internal knowledge "of something being overrriden" : there's a mapping and one is excluding things from it . Specifying "classname=net.sf.click.Page" is also not relevant IMHO, since when excluding one is thinking globaly to the entire webapplication, not "in pages". I also like the fine grained exclusion allowed by Sitemesh by using the combination of patterns. In many cases is not just an entire directory like in the case of TinyMCE : "/tiny_mce*" As I mentioned, I see this as an improvement (completeness) to the "automapping", so since the automapping is done by the ClickServlet, than indeed these exclusion should be handeled there too . Of course the synthax could also be more compressed: <pages package="examples.page" automapping="true"> <excludes patterns="exclude_me.htm, /admin/changelog.htm,/tiny_mce/ , /other_dhtml_lib/something/ " /> <page path="exception.htm" classname="ExceptionDemo"/> </pages> But even so, it's much simpler to read . Thanks in advance, Ahmed.
        Hide
        Malcolm Edgar added a comment -

        Hi Ahmed,

        I completed a first pass at the implementation, before reading your comments. The config syntax I ended up using was:

        <pages package="examples.page" automapping="true">
        <page path="exception.htm" classname="ExceptionDemo"/>
        <override path="/tiny_mce/*" classname="net.sf.click.Page"/>
        </pages>

        The element <autooverrides/> is a bit unreadable.

        Now I take you point that excludes is more immediately understandable in terms of what you want done. However in terms of implementation. The page paths are assigned a Page class, so they can be handled by the ClickServlet. Other implementation involve a whole bunch more work do refactoring the ClickServlet to handle alternative execution paths. This is not something I want to do to this code base at this point.

        We can use an <excludes/> element, but these will be executed using the same base Click mechanism where by they are mapped to net.sf.click.Page class for both Velocity templates and JSP pages.

        <pages package="examples.page" automapping="true">
        <page path="exception.htm" classname="ExceptionDemo"/>
        <excludes patterns="/tiny_mce/*"/>
        </pages>

        We can set the header of these "excluded" pages to be cached by the browser, so its unlikely that once they have been retrived by the browser they will be reloaded. So you wont see much request traffic through the Click servlet.

        I am happy to do either approach for the config. Your call.

        regards Malcolm

        Show
        Malcolm Edgar added a comment - Hi Ahmed, I completed a first pass at the implementation, before reading your comments. The config syntax I ended up using was: <pages package="examples.page" automapping="true"> <page path="exception.htm" classname="ExceptionDemo"/> <override path="/tiny_mce/*" classname="net.sf.click.Page"/> </pages> The element <autooverrides/> is a bit unreadable. Now I take you point that excludes is more immediately understandable in terms of what you want done. However in terms of implementation. The page paths are assigned a Page class, so they can be handled by the ClickServlet. Other implementation involve a whole bunch more work do refactoring the ClickServlet to handle alternative execution paths. This is not something I want to do to this code base at this point. We can use an <excludes/> element, but these will be executed using the same base Click mechanism where by they are mapped to net.sf.click.Page class for both Velocity templates and JSP pages. <pages package="examples.page" automapping="true"> <page path="exception.htm" classname="ExceptionDemo"/> <excludes patterns="/tiny_mce/*"/> </pages> We can set the header of these "excluded" pages to be cached by the browser, so its unlikely that once they have been retrived by the browser they will be reloaded. So you wont see much request traffic through the Click servlet. I am happy to do either approach for the config. Your call. regards Malcolm
        Hide
        Malcolm Edgar added a comment -

        have checked in:

        <pages package="examples.page" automapping="true">
        <page path="exception.htm" classname="ExceptionDemo"/>
        <excludes pattern="/tiny_mce/*"/>
        </pages>

        config approach.

        regards malcolm

        Show
        Malcolm Edgar added a comment - have checked in: <pages package="examples.page" automapping="true"> <page path="exception.htm" classname="ExceptionDemo"/> <excludes pattern="/tiny_mce/*"/> </pages> config approach. regards malcolm
        Hide
        Ahmed Mohombe added a comment -

        >We can set the header of these "excluded" pages to be cached by the browser, so its unlikely that once they have
        > been retrived by the browser they will be reloaded. So you wont see much request traffic through the Click servlet.
        Please, please no .
        Excluded should mean what it's saying: "Do not touch. Let it how it is. It's not a Click page.".
        If the excluded resource is a *.htm than it will delivered like any *.html page.
        If the excluded resource is *.jsp (et co) than Tomcat will take care of it.
        I suppose for both cases, Click should not do a thing for them: just exclude them from it's processing.

        Regarding the traffic, I suppose there should be not Click traffic, since these resources are delivered by the Application Server (Tomcat, or whatever) with it's "default servlet" or what's there, not by ClickServlet since they are excluded.

        Thanks in advance,

        Ahmed.

        Show
        Ahmed Mohombe added a comment - >We can set the header of these "excluded" pages to be cached by the browser, so its unlikely that once they have > been retrived by the browser they will be reloaded. So you wont see much request traffic through the Click servlet. Please, please no . Excluded should mean what it's saying: "Do not touch. Let it how it is. It's not a Click page.". If the excluded resource is a *.htm than it will delivered like any *.html page. If the excluded resource is *.jsp (et co) than Tomcat will take care of it. I suppose for both cases, Click should not do a thing for them: just exclude them from it's processing. Regarding the traffic, I suppose there should be not Click traffic, since these resources are delivered by the Application Server (Tomcat, or whatever) with it's "default servlet" or what's there, not by ClickServlet since they are excluded. Thanks in advance, Ahmed.
        Hide
        Malcolm Edgar added a comment -

        Hi Ahmed,

        If you dont want the ClickServlet to handle the request. You need to configure the web.xml servlet mapping

        <servlet-mapping>
        <servlet-name>click-servlet</servlet-name>
        <url-pattern>pages/*.htm</url-pattern>
        </servlet-mapping>

        <servlet-mapping>
        <servlet-name>click-servlet</servlet-name>
        <url-pattern>click/*.htm</url-pattern>
        </servlet-mapping>

        Otherwise the request is going to be processed by ClickServlet. If the J2EE Servlet spec was better in terms of URL patterns you would not have this issue at all.

        regards Malcolm

        Show
        Malcolm Edgar added a comment - Hi Ahmed, If you dont want the ClickServlet to handle the request. You need to configure the web.xml servlet mapping <servlet-mapping> <servlet-name>click-servlet</servlet-name> <url-pattern>pages/*.htm</url-pattern> </servlet-mapping> <servlet-mapping> <servlet-name>click-servlet</servlet-name> <url-pattern>click/*.htm</url-pattern> </servlet-mapping> Otherwise the request is going to be processed by ClickServlet. If the J2EE Servlet spec was better in terms of URL patterns you would not have this issue at all. regards Malcolm
        Hide
        Christian Essl added a comment -

        Just out of my head:

        I also think that it would be better if excluded pages where not touched by ClickServlet at all.

        Don't know if this works: Maybe the click-servlet could be a filter, which processes all 'normal' click requests and let's excluded ones path trough.

        Christian

        Show
        Christian Essl added a comment - Just out of my head: I also think that it would be better if excluded pages where not touched by ClickServlet at all. Don't know if this works: Maybe the click-servlet could be a filter, which processes all 'normal' click requests and let's excluded ones path trough. Christian
        Hide
        Christian Essl added a comment -

        Again out of my head: If you implement click-servlet as a filter and it just sets the page as a request-param and gives further. I think than you could use any rendering technology and have other flexibilities - like a DaynamicImage control which first renders the <img> tag and on the second request in onProcess writes the bytes to the OutputStream.
        Christian

        Show
        Christian Essl added a comment - Again out of my head: If you implement click-servlet as a filter and it just sets the page as a request-param and gives further. I think than you could use any rendering technology and have other flexibilities - like a DaynamicImage control which first renders the <img> tag and on the second request in onProcess writes the bytes to the OutputStream. Christian
        Hide
        Malcolm Edgar added a comment -

        Yes it would be nice if J2EE spec supported exclusion filters, however it doesn't. If you are really concerned about performance you can use a servlet mapping of:

        <servlet-mapping>
        <servlet-name>click-servlet</servlet-name>
        <url-pattern>pages/*.htm</url-pattern>
        </servlet-mapping>

        <servlet-mapping>
        <servlet-name>click-servlet</servlet-name>
        <url-pattern>click/*.htm</url-pattern>
        </servlet-mapping>

        Using the servlet mapping as detailed above is probably the better solution to this issue than the <excludes/> configuration.

        I am beginning to wonder if I should remove this feature.

        I don't want to have to refactor ClickServlet just to by pass some JavaScript .htm files.

        regards Malcolm Edgar

        Show
        Malcolm Edgar added a comment - Yes it would be nice if J2EE spec supported exclusion filters, however it doesn't. If you are really concerned about performance you can use a servlet mapping of: <servlet-mapping> <servlet-name>click-servlet</servlet-name> <url-pattern>pages/*.htm</url-pattern> </servlet-mapping> <servlet-mapping> <servlet-name>click-servlet</servlet-name> <url-pattern>click/*.htm</url-pattern> </servlet-mapping> Using the servlet mapping as detailed above is probably the better solution to this issue than the <excludes/> configuration. I am beginning to wonder if I should remove this feature. I don't want to have to refactor ClickServlet just to by pass some JavaScript .htm files. regards Malcolm Edgar
        Hide
        Christian Essl added a comment -

        Hi Malcom,

        I know this is a solution (and IMO a better one than what is implemented currently). But this would mean that the very nice (and KISS) *.htm mapping is lost. I can't have click pages in root . I think you will also have to change the default app layout to the above, because you propably need to provide an exclusion path for all control-deployments which eventuelly might need it.

        The simplest alternative could be to have a (configurable?) different extension than htm. Which I think is no problem at all - Struts works like this and is used all over and nobody complains AFAIK. I'd prefer this solution works for everything - and is realy simple.

        The only other way to keep a .htm mapping with exclusion I see is as said a servlet.Filter. What I acutally meant is that the ClickServlet could implement javax.servlet.Filter and is configured as a Filter which mapps to *.htm (and gets all *.htm requests). 'Normal' requests are handled as they are handled now (parsed, rendered etc and *no call to doFilter()) for excluded resources the ClickFilter just calls doFilter() and let the container process the request (however mapped).

        I guess the changes to ClickServlet would be minimal. Basicly the Filter interface has to be added, the init() method is copied to init(FilterConfig) and the following method is added:

        doFilter(req,res,FilterChain chain){
        if(clickApp.isExcluded(ClickUtils.getResoucePath(req)

        { chain.doFilter(req,res); }

        else

        { this.service(req,res); }

        }
        (and the web-xml is changed to add the filter declaration and remove the servlet declaration)

        As said I have not tried this verson but I think it works

        However you prefer to do it please remove the current exclude impl. The clickservlet sould not handle resources the container should handle. And If you just keep ClickServlet unchanged to 0.18 I am fine as well.

        Just my 2c,
        Christian

        Show
        Christian Essl added a comment - Hi Malcom, I know this is a solution (and IMO a better one than what is implemented currently). But this would mean that the very nice (and KISS) *.htm mapping is lost. I can't have click pages in root . I think you will also have to change the default app layout to the above, because you propably need to provide an exclusion path for all control-deployments which eventuelly might need it. The simplest alternative could be to have a (configurable?) different extension than htm. Which I think is no problem at all - Struts works like this and is used all over and nobody complains AFAIK. I'd prefer this solution works for everything - and is realy simple. The only other way to keep a .htm mapping with exclusion I see is as said a servlet.Filter. What I acutally meant is that the ClickServlet could implement javax.servlet.Filter and is configured as a Filter which mapps to *.htm (and gets all *.htm requests). 'Normal' requests are handled as they are handled now (parsed, rendered etc and *no call to doFilter() ) for excluded resources the ClickFilter just calls doFilter() and let the container process the request (however mapped). I guess the changes to ClickServlet would be minimal. Basicly the Filter interface has to be added, the init() method is copied to init(FilterConfig) and the following method is added: doFilter(req,res,FilterChain chain){ if(clickApp.isExcluded(ClickUtils.getResoucePath(req) { chain.doFilter(req,res); } else { this.service(req,res); } } (and the web-xml is changed to add the filter declaration and remove the servlet declaration) As said I have not tried this verson but I think it works However you prefer to do it please remove the current exclude impl. The clickservlet sould not handle resources the container should handle. And If you just keep ClickServlet unchanged to 0.18 I am fine as well. Just my 2c, Christian
        Hide
        Christian Essl added a comment -

        Oh sorry I did not realize that the Filter chaining for RequestDispatchers is not well defined for servlet 2.3 so my filter proposal won't work - as you said.

        Christian

        Show
        Christian Essl added a comment - Oh sorry I did not realize that the Filter chaining for RequestDispatchers is not well defined for servlet 2.3 so my filter proposal won't work - as you said. Christian
        Hide
        Malcolm Edgar added a comment -

        Fix available in CVS, will be included in release 0.19

        Show
        Malcolm Edgar added a comment - Fix available in CVS, will be included in release 0.19
        Malcolm Edgar made changes -
        Resolution Fixed [ 1 ]
        Status In Progress [ 3 ] Resolved [ 5 ]
        Malcolm Edgar made changes -
        Status Resolved [ 5 ] Closed [ 6 ]
        Henri Yandell made changes -
        Project Import Fri Mar 20 14:11:32 PDT 2009 [ 1237583492744 ]

          People

          • Assignee:
            Malcolm Edgar
            Reporter:
            Ahmed Mohombe
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development