Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.2.11-core
    • Fix Version/s: 1.2.11-core
    • Component/s: Portlet
    • Labels:
      None

      Description

      The method ExternalContextUtils.isAction(final ExternalContext externalContext) does not reliably detect an action request.
      If the object returned by ExternalContext.getRequest() implements the ServletRequest interface the method above returns
      true. This can also apply for a RenderRequest which is not an ActionRequest.

      The following fixes this:

      /**

      • Returns <code>true</code> if this externalContext represents an "action". An action request
      • is any ServletRequest or a portlet ActionRequest. It is assumed that the ExternalContext
        *
      • @return a boolean of <code>true</code> if this is a Portlet ActionRequest or an non-portlet
      • request.
        */
        public static boolean isAction(final ExternalContext externalContext)
        {
        final Object request = externalContext.getRequest();

      if (_PORTLET_ACTION_REQUEST_CLASS == null)

      { _LOG .fine("Portlet API's are not on the classpath so isAction will only check for servlet request."); return request instanceof ServletRequest; }

      return (request instanceof ServletRequest && !_PORTLET_RENDER_REQUEST_CLASS.isInstance(request) ||
      _PORTLET_ACTION_REQUEST_CLASS.isInstance(request)) ;
      }

      _PORTLET_RENDER_REQUEST_CLASS has to be properly initialized. Not sure why this method checks for ServletRequest.

      1. TRINIDAD-1377-patch.txt
        4 kB
        Felix Röthenbacher

        Issue Links

          Activity

          Hide
          Scott O'Bryan added a comment -

          Please read the documentation. This method detects Action "type" requests, meaning that you have access to headers and the RequestInputStream. The Javadocs say that ServletRequests will return true.

          In the standalone ExternalContextUtils, this has turned into getClientRequest, but this is an older API.

          Show
          Scott O'Bryan added a comment - Please read the documentation. This method detects Action "type" requests, meaning that you have access to headers and the RequestInputStream. The Javadocs say that ServletRequests will return true. In the standalone ExternalContextUtils, this has turned into getClientRequest, but this is an older API.
          Hide
          Felix Röthenbacher added a comment -

          What do you mean by Action 'type' requests? Is a javax.portlet.RenderRequest an Action 'type' request? This is true if run in the Pluto RI as it uses an HttpServletRequest (which implements ServletRequest) as request object for both javax.portlet.ActionRequest and javax.portlet.RenderRequest. I guess you can't assume that a RenderRequest does not also implement ServletRequest.

          Feel free to close this bug if you still think this is not a bug.

          Show
          Felix Röthenbacher added a comment - What do you mean by Action 'type' requests? Is a javax.portlet.RenderRequest an Action 'type' request? This is true if run in the Pluto RI as it uses an HttpServletRequest (which implements ServletRequest) as request object for both javax.portlet.ActionRequest and javax.portlet.RenderRequest. I guess you can't assume that a RenderRequest does not also implement ServletRequest. Feel free to close this bug if you still think this is not a bug.
          Hide
          Scott O'Bryan added a comment -

          Action "type" request are, Portlet 1.0 Action Request, servlet request, and eventually Resource Requests (Portlet 2.0). In the more updated vernacular in the command project, this essentially tests for a "client" request which is a request that comes directly from the client.

          For Action and Servlet Requests this will return true. for all render request, INCLUDING THOSE THAT WRAP SERVLET REQUEST, it returns false. Look at the last line..

          return (request instanceof ServletRequest && !_PORTLET_RENDER_REQUEST_CLASS.isInstance(request) ||
          _PORTLET_ACTION_REQUEST_CLASS.isInstance(request)) ;

          This says if it's a servlet request that us NOT an render request, or if it's an action request, return true. If this is not happening, it's a bug.

          I'll again re-iterate, Trinidad treats this API as if it's a request that comes from the client. It tells us, for instance, whether we can receive an input stream, whether we are in the inital phases of a request. As I said, in the commons version of this class, it is essentially the isClientRequest flag. The definition of this and the isClientRequest flag are the same.

          On the flipside is the isResponseWritable. These include Servlet Requests as well, but would also include a render request. Here is a list of, basically, all the objects we do (will soon) handle by the configurators and what they return..

          isClientRequest (isAction in Trinidad) isResponseWritable (no equiv in Trinidad)
          ServletRequest true true
          ActionRequest true false
          RenderRequest false true
          ResourceRequest true true
          EventRequest false false

          How is this used? Most of the time within Trinidad, we need to check for whether something has access to headers or is in response to say a real request (like PPR Request or whatnot). Other times we need to know whether we are in a portlet environment. By far the fewest number of times do we have to check if we are in a portlet ACTION REQUEST specifically. In this case, the results of isPortlet and isAction need to be used together, OR the ExternalContextUtils in the commons should be used (it supports an enum for request type).

          Scott

          Show
          Scott O'Bryan added a comment - Action "type" request are, Portlet 1.0 Action Request, servlet request, and eventually Resource Requests (Portlet 2.0). In the more updated vernacular in the command project, this essentially tests for a "client" request which is a request that comes directly from the client. For Action and Servlet Requests this will return true. for all render request, INCLUDING THOSE THAT WRAP SERVLET REQUEST, it returns false. Look at the last line.. return (request instanceof ServletRequest && !_PORTLET_RENDER_REQUEST_CLASS.isInstance(request) || _PORTLET_ACTION_REQUEST_CLASS.isInstance(request)) ; This says if it's a servlet request that us NOT an render request, or if it's an action request, return true. If this is not happening, it's a bug. I'll again re-iterate, Trinidad treats this API as if it's a request that comes from the client. It tells us, for instance, whether we can receive an input stream, whether we are in the inital phases of a request. As I said, in the commons version of this class, it is essentially the isClientRequest flag. The definition of this and the isClientRequest flag are the same. On the flipside is the isResponseWritable. These include Servlet Requests as well, but would also include a render request. Here is a list of, basically, all the objects we do (will soon) handle by the configurators and what they return.. isClientRequest (isAction in Trinidad) isResponseWritable (no equiv in Trinidad) ServletRequest true true ActionRequest true false RenderRequest false true ResourceRequest true true EventRequest false false How is this used? Most of the time within Trinidad, we need to check for whether something has access to headers or is in response to say a real request (like PPR Request or whatnot). Other times we need to know whether we are in a portlet environment. By far the fewest number of times do we have to check if we are in a portlet ACTION REQUEST specifically. In this case, the results of isPortlet and isAction need to be used together, OR the ExternalContextUtils in the commons should be used (it supports an enum for request type). Scott
          Hide
          Felix Röthenbacher added a comment -

          You are refering to my proposed fix in your comment before. This is how it is currently in the codebase:

          return request instanceof ServletRequest || _PORTLET_ACTION_REQUEST_CLASS.isInstance(request);

          This also returns true for RenderRequest if the request objects implements ServletRequest (as it does in the Pluto RI).

          I would even suggest to use the portlet bridge functionality to detect which phase we are in (if in any at all):

          public static boolean isAction(final ExternalContext externalContext)

          { PortletPhase portletPhase = (PortletPhase) externalContext.getRequestMap().get(PORTLET_PHASE_REQ_ATTR); return !PortletPhase.RENDER_PHASE.equals(portletPhase); }

          whereas PORTLET_PHASE_REQ_ATTR is defined as

          public final static String PORTLET_PHASE_REQ_ATTR =
          "javax.portlet.faces.phase";

          This allows us to get rid of all the 'instanceof' class detection. The same applies for isPortlet():

          public static boolean isPortlet(final ExternalContext externalContext)

          { PortletPhase portletPhase = (PortletPhase) externalContext.getRequestMap().get(PORTLET_PHASE_REQ_ATTR); return portletPhase != null; }

          This requires that the 'provided' scope in the pom.xml for the portlet-bridge-api dependency is removed.

          Show
          Felix Röthenbacher added a comment - You are refering to my proposed fix in your comment before. This is how it is currently in the codebase: return request instanceof ServletRequest || _PORTLET_ACTION_REQUEST_CLASS.isInstance(request); This also returns true for RenderRequest if the request objects implements ServletRequest (as it does in the Pluto RI). I would even suggest to use the portlet bridge functionality to detect which phase we are in (if in any at all): public static boolean isAction(final ExternalContext externalContext) { PortletPhase portletPhase = (PortletPhase) externalContext.getRequestMap().get(PORTLET_PHASE_REQ_ATTR); return !PortletPhase.RENDER_PHASE.equals(portletPhase); } whereas PORTLET_PHASE_REQ_ATTR is defined as public final static String PORTLET_PHASE_REQ_ATTR = "javax.portlet.faces.phase"; This allows us to get rid of all the 'instanceof' class detection. The same applies for isPortlet(): public static boolean isPortlet(final ExternalContext externalContext) { PortletPhase portletPhase = (PortletPhase) externalContext.getRequestMap().get(PORTLET_PHASE_REQ_ATTR); return portletPhase != null; } This requires that the 'provided' scope in the pom.xml for the portlet-bridge-api dependency is removed.
          Hide
          Scott O'Bryan added a comment -

          Yeah, I don't the bridge dependency, but doing instanceof is, strangely, faster. I agree with your fix however. I'll make sure this is reopened. I'm surprised we never found this before. We use it everywhere.

          Show
          Scott O'Bryan added a comment - Yeah, I don't the bridge dependency, but doing instanceof is, strangely, faster. I agree with your fix however. I'll make sure this is reopened. I'm surprised we never found this before. We use it everywhere.
          Hide
          Scott O'Bryan added a comment -

          I see. It works for me because I've been using a portlet environment where the RenderRequest does NOT extend the servlet request. Duh! You're totally right Felix. Can you submit a patch and I'll apply it. It would be easier for me to apply it and you'd get some cred (mainly in the form of my undying thanks). Thank you for arguing the point.

          Show
          Scott O'Bryan added a comment - I see. It works for me because I've been using a portlet environment where the RenderRequest does NOT extend the servlet request. Duh! You're totally right Felix. Can you submit a patch and I'll apply it. It would be easier for me to apply it and you'd get some cred (mainly in the form of my undying thanks). Thank you for arguing the point.
          Hide
          Scott O'Bryan added a comment -

          Sorry, I don't MIND the bridge dependency. I think we already have it.

          Show
          Scott O'Bryan added a comment - Sorry, I don't MIND the bridge dependency. I think we already have it.
          Hide
          Felix Röthenbacher added a comment -

          Use Bridge.PortletPhase request attribute to determine if it's a portlet request or an action request. Adding dependency of portlet-bridge-api and updating to portlet-bridge version 1.0.0-beta.

          Show
          Felix Röthenbacher added a comment - Use Bridge.PortletPhase request attribute to determine if it's a portlet request or an action request. Adding dependency of portlet-bridge-api and updating to portlet-bridge version 1.0.0-beta.
          Hide
          Scott O'Bryan added a comment -

          I'm going to need to change this patch actually. We don't want to force all Trinidad to always have the PortletBridge jars in the classpath. Since the ExternalContextUtils is used in both environments, this would cause us to HAVE to include the bridge jars even in a portlet only implementation.

          Furthermore, testing for instanceof in post jre 1.4 is on orders magnitude faster then accessing the request attribute. My timings of this scenario using a 1.5 jre was that for 1 million requests, instanceof ran in 2 ms whereas the request attribute too around 300ms.

          Show
          Scott O'Bryan added a comment - I'm going to need to change this patch actually. We don't want to force all Trinidad to always have the PortletBridge jars in the classpath. Since the ExternalContextUtils is used in both environments, this would cause us to HAVE to include the bridge jars even in a portlet only implementation. Furthermore, testing for instanceof in post jre 1.4 is on orders magnitude faster then accessing the request attribute. My timings of this scenario using a 1.5 jre was that for 1 million requests, instanceof ran in 2 ms whereas the request attribute too around 300ms.
          Hide
          Felix Röthenbacher added a comment -

          The newly dependency is on the portlet bridge API only, not on the implementation. As the portlet bridge API is sort of a specification (JSR 301) I think this is viable. We already have a similar dependency on the portlet API.

          I have no doubt that the 'instanceof' operator is faster than accessing a key in the request hash map. For the sake of readable and simple code I'd nethertheless tend to use the tools provided by the portlet bridge specification. I.e. I'd even simplify my patch by using the BridgeUtil.isPortlet() method. Again, this method is provided in the portlet bridge API and doesn't need a dependency on a specific implementation.

          Show
          Felix Röthenbacher added a comment - The newly dependency is on the portlet bridge API only, not on the implementation. As the portlet bridge API is sort of a specification (JSR 301) I think this is viable. We already have a similar dependency on the portlet API. I have no doubt that the 'instanceof' operator is faster than accessing a key in the request hash map. For the sake of readable and simple code I'd nethertheless tend to use the tools provided by the portlet bridge specification. I.e. I'd even simplify my patch by using the BridgeUtil.isPortlet() method. Again, this method is provided in the portlet bridge API and doesn't need a dependency on a specific implementation.
          Hide
          Scott O'Bryan added a comment -

          I've tested it. And to my surprise it is. Also, Trindiad DOES NOT have a runtime dependency on Portlet 1.0 API OR the bridge API. You need it for compile time but not for runtime. This patch would make these jars have to be available for runtime.

          Show
          Scott O'Bryan added a comment - I've tested it. And to my surprise it is. Also, Trindiad DOES NOT have a runtime dependency on Portlet 1.0 API OR the bridge API. You need it for compile time but not for runtime. This patch would make these jars have to be available for runtime.
          Hide
          Scott O'Bryan added a comment -

          BTW- I understand the readability which is why I added the ExternalContextUtils. It moves the ugliness to the utility method. Your code just needs ExternalContextUtils.isPortlet() .. Anyway, I'm submitting a patch to TRINIDAD-1376. Can you try it out to see if it fixes your issues?

          Show
          Scott O'Bryan added a comment - BTW- I understand the readability which is why I added the ExternalContextUtils. It moves the ugliness to the utility method. Your code just needs ExternalContextUtils.isPortlet() .. Anyway, I'm submitting a patch to TRINIDAD-1376 . Can you try it out to see if it fixes your issues?
          Hide
          Scott O'Bryan added a comment -

          Patch applied and should be in the 1.2.11-core release of Trinidad

          Show
          Scott O'Bryan added a comment - Patch applied and should be in the 1.2.11-core release of Trinidad

            People

            • Assignee:
              Scott O'Bryan
              Reporter:
              Felix Röthenbacher
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development