Details

    • Type: New Feature
    • Status: Resolved
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 1.3
    • Fix Version/s: 1.3
    • Component/s: VelocityView
    • Labels:
      None
    • Environment:
      all

      Description

      This patch adds two new scopes:

      <scope>/some_path/*</scope> is a subset of the request scope restricting the instantiation of tools to servlet requests having an URI that begins with /some_path/.

      <scope>/some_pathname</scope> is a subset of the request scope restricting the instantiation of tools to servlet requests whose URI matches exactly /some_pathname

      1. docs.patch
        3 kB
        Claude Brisson
      2. path-scopes.patch
        6 kB
        Claude Brisson
      3. request-path.patch
        9 kB
        Claude Brisson

        Activity

        Hide
        nbubna Nathan Bubna added a comment -

        Cool. I've long liked this idea since it was brought up ages ago. I do have some minor quibbles about implementation, but i want to try and get this into 1.3. I'll gather my thoughts and get your feedback before i check it all in...

        Show
        nbubna Nathan Bubna added a comment - Cool. I've long liked this idea since it was brought up ages ago. I do have some minor quibbles about implementation, but i want to try and get this into 1.3. I'll gather my thoughts and get your feedback before i check it all in...
        Hide
        nbubna Nathan Bubna added a comment -

        Ok, here's some pushback...

        i'm not sure i like putting the path restrictions down as a scope.
        a) it causes the value into ServletToolInfo.setScope() to not match getScope()
        b) what if you want to restrict a session/application oriented tool to certain paths?

        what do you think about making the path a totally separate property of ServletToolInfo? i know that makes things more complicated, especially in ServletToolboxManager.getToolbox() if we want to path-restrict session/application tools. Maybe to start things off we could have the path restriction as a separate property and add a fourth "request-path" scope option. Then we don't get a set/getScope mismatch and still limit path restriction to request scope (simplifying getToolbox()) but leave open the possibility of allowing session or application tools to be path-restricted.

        thoughts anyone (esp Claude)?

        Show
        nbubna Nathan Bubna added a comment - Ok, here's some pushback... i'm not sure i like putting the path restrictions down as a scope. a) it causes the value into ServletToolInfo.setScope() to not match getScope() b) what if you want to restrict a session/application oriented tool to certain paths? what do you think about making the path a totally separate property of ServletToolInfo? i know that makes things more complicated, especially in ServletToolboxManager.getToolbox() if we want to path-restrict session/application tools. Maybe to start things off we could have the path restriction as a separate property and add a fourth "request-path" scope option. Then we don't get a set/getScope mismatch and still limit path restriction to request scope (simplifying getToolbox()) but leave open the possibility of allowing session or application tools to be path-restricted. thoughts anyone (esp Claude)?
        Hide
        nbubna Nathan Bubna added a comment -

        Another question...

        Why insist that wildcard paths end with '/' ? What if i wanted to enable a tool for all /view* (e.g. /viewFoo) but not /edit* (e.g. /editFoo)?

        Show
        nbubna Nathan Bubna added a comment - Another question... Why insist that wildcard paths end with '/' ? What if i wanted to enable a tool for all /view* (e.g. /viewFoo) but not /edit* (e.g. /editFoo)?
        Hide
        claude Claude Brisson added a comment -

        > what do you think about making the path a totally separate property of ServletToolInfo?

        +1 for a separate "request-path" parameter.
        That's obviously a cleaner implementation than mine.

        > Why insist that wildcard paths end with '/' ? What if i wanted to enable a tool for all /view* (e.g. /viewFoo) but not /edit* (e.g. /editFoo)?

        It was to somehow mimic the behaviour of the "/*" url-pattern wilcard behaviour in a webapp deployment descriptor, but you are right, we don't need to inforce it.

        Show
        claude Claude Brisson added a comment - > what do you think about making the path a totally separate property of ServletToolInfo? +1 for a separate "request-path" parameter. That's obviously a cleaner implementation than mine. > Why insist that wildcard paths end with '/' ? What if i wanted to enable a tool for all /view* (e.g. /viewFoo) but not /edit* (e.g. /editFoo)? It was to somehow mimic the behaviour of the "/*" url-pattern wilcard behaviour in a webapp deployment descriptor, but you are right, we don't need to inforce it.
        Hide
        claude Claude Brisson added a comment -

        A new candidate patch which implements the request-path element.

        I was thinking on the way to implement this feature for session and application tools, and I think that the best option is leave the tools in the toolbox and implement a filter in the ChainedContext getter.

        Show
        claude Claude Brisson added a comment - A new candidate patch which implements the request-path element. I was thinking on the way to implement this feature for session and application tools, and I think that the best option is leave the tools in the toolbox and implement a filter in the ChainedContext getter.
        Hide
        nbubna Nathan Bubna added a comment -

        Looks good. i have a few refinements, but i'll try and get this in on Monday.

        As for supporting this with session/application scoped tools, it wouldn't work well to do it in ChainedContext, since the ToolInfo isn't in really in view there. i'd rather keep the restriction in front of tool init rather than after. i think there may be ways to do this in the manager without much pain, but we may need to wait until 2.x.

        Show
        nbubna Nathan Bubna added a comment - Looks good. i have a few refinements, but i'll try and get this in on Monday. As for supporting this with session/application scoped tools, it wouldn't work well to do it in ChainedContext, since the ToolInfo isn't in really in view there. i'd rather keep the restriction in front of tool init rather than after. i think there may be ways to do this in the manager without much pain, but we may need to wait until 2.x.
        Hide
        nbubna Nathan Bubna added a comment -

        Implemented in revision 474952.

        Ok, it's in. I managed to make it easily support path-restrictions for session scope tools and to reject path-restrictions on application scoped tools for the time being. Let me know what you think and of course how it works for you!

        Show
        nbubna Nathan Bubna added a comment - Implemented in revision 474952. Ok, it's in. I managed to make it easily support path-restrictions for session scope tools and to reject path-restrictions on application scoped tools for the time being. Let me know what you think and of course how it works for you!
        Hide
        claude Claude Brisson added a comment -

        Clean implementation.

        Two things, though :

        1. Maybe it's me, but I don't understand at all where you manage to contextualize session scoped tools that are visible in a request. That is, I've got the feeling that visible tools are determinated only using the request that creates the session. I thought this could only be done by putting session scoped tools in a subclass of Map filtering the getter according to the request URI.

        2. My addition to the docs was lost with the first patch, so I resubmit it appart (I should have done that at first time).

        Show
        claude Claude Brisson added a comment - Clean implementation. Two things, though : 1. Maybe it's me, but I don't understand at all where you manage to contextualize session scoped tools that are visible in a request. That is, I've got the feeling that visible tools are determinated only using the request that creates the session. I thought this could only be done by putting session scoped tools in a subclass of Map filtering the getter according to the request URI. 2. My addition to the docs was lost with the first patch, so I resubmit it appart (I should have done that at first time).
        Hide
        nbubna Nathan Bubna added a comment -

        Doh. You're totally right on the session tool implementation. It doesn't make much sense to just do it for the session-creating request. I didn't think that through, just saw the opportunity to apply the path restriction there and took it. Maybe we should just support this for request scoped tools for now. Let me think about it...

        And thanks for the docs patch!!

        Show
        nbubna Nathan Bubna added a comment - Doh. You're totally right on the session tool implementation. It doesn't make much sense to just do it for the session-creating request. I didn't think that through, just saw the opportunity to apply the path restriction there and took it. Maybe we should just support this for request scoped tools for now. Let me think about it... And thanks for the docs patch!!
        Hide
        nbubna Nathan Bubna added a comment -

        http://svn.apache.org/viewvc?view=rev&rev=475015

        Ok, path restrictions are now limited to request-scoped tools. Thanks, Claude, for setting me straight!

        Show
        nbubna Nathan Bubna added a comment - http://svn.apache.org/viewvc?view=rev&rev=475015 Ok, path restrictions are now limited to request-scoped tools. Thanks, Claude, for setting me straight!
        Hide
        nbubna Nathan Bubna added a comment -

        http://svn.apache.org/viewvc?view=rev&rev=475017

        Documentation patch has been adapted to xdoc and applied. Thanks!

        Show
        nbubna Nathan Bubna added a comment - http://svn.apache.org/viewvc?view=rev&rev=475017 Documentation patch has been adapted to xdoc and applied. Thanks!

          People

          • Assignee:
            nbubna Nathan Bubna
            Reporter:
            claude Claude Brisson
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development