Hadoop Common
  1. Hadoop Common
  2. HADOOP-7440

HttpServer.getParameterValues throws NPE for missing parameters

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 0.23.0
    • Fix Version/s: 0.23.0
    • Component/s: None
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      If the requested parameter was not specified in the request, the raw request's getParameterValues function returns null. Thus, trying to access unquoteValue.length throws NPE.

        Activity

        Hide
        Hudson added a comment -

        Integrated in Hadoop-Common-trunk #739 (See https://builds.apache.org/job/Hadoop-Common-trunk/739/)
        HADOOP-7440. HttpServer.getParameterValues throws NPE for missing parameters. Contributed by Uma Maheswara Rao G and Todd Lipcon.

        todd : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1143212
        Files :

        • /hadoop/common/trunk/common/src/java/org/apache/hadoop/http/HttpServer.java
        • /hadoop/common/trunk/common/CHANGES.txt
        • /hadoop/common/trunk/common/src/test/core/org/apache/hadoop/http/TestHtmlQuoting.java
        • /hadoop/common/trunk/common/src/test/core/org/apache/hadoop/http/TestHttpServer.java
        Show
        Hudson added a comment - Integrated in Hadoop-Common-trunk #739 (See https://builds.apache.org/job/Hadoop-Common-trunk/739/ ) HADOOP-7440 . HttpServer.getParameterValues throws NPE for missing parameters. Contributed by Uma Maheswara Rao G and Todd Lipcon. todd : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1143212 Files : /hadoop/common/trunk/common/src/java/org/apache/hadoop/http/HttpServer.java /hadoop/common/trunk/common/CHANGES.txt /hadoop/common/trunk/common/src/test/core/org/apache/hadoop/http/TestHtmlQuoting.java /hadoop/common/trunk/common/src/test/core/org/apache/hadoop/http/TestHttpServer.java
        Hide
        Todd Lipcon added a comment -

        I see that now. Sorry, Devaraj. I misread. Unfortunately there's no way to edit a commit message.

        Show
        Todd Lipcon added a comment - I see that now. Sorry, Devaraj. I misread. Unfortunately there's no way to edit a commit message.
        Hide
        Ravi Teja Ch N V added a comment -

        Hi Todd, I think you have entered the wrong user for the credit, the patch for the other issue was contributed by Devaraj.

        Show
        Ravi Teja Ch N V added a comment - Hi Todd, I think you have entered the wrong user for the credit, the patch for the other issue was contributed by Devaraj.
        Hide
        Hudson added a comment -

        Integrated in Hadoop-Common-trunk-Commit #673 (See https://builds.apache.org/job/Hadoop-Common-trunk-Commit/673/)
        HADOOP-7440. HttpServer.getParameterValues throws NPE for missing parameters. Contributed by Uma Maheswara Rao G and Todd Lipcon.

        todd : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1143212
        Files :

        • /hadoop/common/trunk/common/src/java/org/apache/hadoop/http/HttpServer.java
        • /hadoop/common/trunk/common/CHANGES.txt
        • /hadoop/common/trunk/common/src/test/core/org/apache/hadoop/http/TestHtmlQuoting.java
        • /hadoop/common/trunk/common/src/test/core/org/apache/hadoop/http/TestHttpServer.java
        Show
        Hudson added a comment - Integrated in Hadoop-Common-trunk-Commit #673 (See https://builds.apache.org/job/Hadoop-Common-trunk-Commit/673/ ) HADOOP-7440 . HttpServer.getParameterValues throws NPE for missing parameters. Contributed by Uma Maheswara Rao G and Todd Lipcon. todd : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1143212 Files : /hadoop/common/trunk/common/src/java/org/apache/hadoop/http/HttpServer.java /hadoop/common/trunk/common/CHANGES.txt /hadoop/common/trunk/common/src/test/core/org/apache/hadoop/http/TestHtmlQuoting.java /hadoop/common/trunk/common/src/test/core/org/apache/hadoop/http/TestHttpServer.java
        Hide
        Todd Lipcon added a comment -

        I committed this to trunk and credited both Uma and myself. I borrowed Uma's tests from the other JIRA in addition to my own.

        Show
        Todd Lipcon added a comment - I committed this to trunk and credited both Uma and myself. I borrowed Uma's tests from the other JIRA in addition to my own.
        Hide
        Todd Lipcon added a comment -

        Since this code is in a filter, which is supposed to be transparent, I don't think changing the spec would be a good idea.

        Show
        Todd Lipcon added a comment - Since this code is in a filter, which is supposed to be transparent, I don't think changing the spec would be a good idea.
        Hide
        Bharath Mundlapudi added a comment -

        To be specific, I am proposing the following.

                if (unquoteValue == null) {
                  return new String[0];
                }
        

        If you think, this will confuse the users of this API since users might refer servlet spec then I am fine with Todd's change.

        Show
        Bharath Mundlapudi added a comment - To be specific, I am proposing the following. if (unquoteValue == null ) { return new String [0]; } If you think, this will confuse the users of this API since users might refer servlet spec then I am fine with Todd's change.
        Hide
        Bharath Mundlapudi added a comment -

        Hi Todd & Devaraj,

        I didn't see it was a servlet api. One of the problems we saw with propagating nullness to clients is that, clients forget to add these null checks. But we fixed case by case basis since some where fundamental JDK APIs.

        If you just search for how people are using this API, you will note that the following example will throw NPE.

              String[] paramValues = request.getParameterValues(paramName);
              if (paramValues.length == 1) {
                String paramValue = paramValues[0];
                if (paramValue.length() == 0)
                  out.print("<I>No Value</I>");
                else
                  out.print(paramValue);
              } else {
                out.println("<UL>");
                for(int i=0; i<paramValues.length; i++) {
                  out.println("<LI>" + paramValues[i]);
                }
                out.println("</UL>");
              }
        

        I am proposing, if we are the API users, we should not return null rather return empty array. This will be much cleaner API. Is there any down side in returning empty array?

        Show
        Bharath Mundlapudi added a comment - Hi Todd & Devaraj, I didn't see it was a servlet api. One of the problems we saw with propagating nullness to clients is that, clients forget to add these null checks. But we fixed case by case basis since some where fundamental JDK APIs. If you just search for how people are using this API, you will note that the following example will throw NPE. String [] paramValues = request.getParameterValues(paramName); if (paramValues.length == 1) { String paramValue = paramValues[0]; if (paramValue.length() == 0) out.print( "<I>No Value</I>" ); else out.print(paramValue); } else { out.println( "<UL>" ); for ( int i=0; i<paramValues.length; i++) { out.println( "<LI>" + paramValues[i]); } out.println( "</UL>" ); } I am proposing, if we are the API users, we should not return null rather return empty array. This will be much cleaner API. Is there any down side in returning empty array?
        Hide
        Steve Loughran added a comment -

        +1 for this patch. It propagates the nullness, and leaves it up to the caller to cope with it, as per the servlet specification.

        Show
        Steve Loughran added a comment - +1 for this patch. It propagates the nullness, and leaves it up to the caller to cope with it, as per the servlet specification.
        Hide
        Uma Maheswara Rao G added a comment -

        Yes Devaraj,
        Application layer should takecare of handling the null return values. Let us don't violate the specification.

        Show
        Uma Maheswara Rao G added a comment - Yes Devaraj, Application layer should takecare of handling the null return values. Let us don't violate the specification.
        Hide
        Devaraj K added a comment -

        Hi Bharath,

        As per the specification, it should return null if the parameter does not exist.

        http://download.oracle.com/javaee/5/api/javax/servlet/ServletRequest.html#getParameterValues(java.lang.String)

        getParameterValues

        String[] getParameterValues(String name)Returns an array of String objects containing all of the values the given request parameter has, or null if the parameter does not exist.
        If the parameter has a single value, the array has a length of 1.

        Parameters:

        name - a String containing the name of the parameter whose value is requested

        Returns:

        an array of String objects containing the parameter's values

        Show
        Devaraj K added a comment - Hi Bharath, As per the specification, it should return null if the parameter does not exist. http://download.oracle.com/javaee/5/api/javax/servlet/ServletRequest.html#getParameterValues(java.lang.String ) getParameterValues String[] getParameterValues(String name)Returns an array of String objects containing all of the values the given request parameter has, or null if the parameter does not exist. If the parameter has a single value, the array has a length of 1. Parameters: name - a String containing the name of the parameter whose value is requested Returns: an array of String objects containing the parameter's values
        Hide
        Devaraj K added a comment -

        Duplicate of HADOOP-7354.

        Show
        Devaraj K added a comment - Duplicate of HADOOP-7354 .
        Hide
        Bharath Mundlapudi added a comment -

        Hi Todd,

        One minor comment: instead of returning null for this function, can we return empty string array? This was the cleanup we have done in the past with couple of JDK APIs.
        The answer was never return null for the array return types. One reason is all the client code needs to check for null condition everywhere. So we should avoid returning null for array types. Do you agree?

        Otherwise, patch looks good.

        Show
        Bharath Mundlapudi added a comment - Hi Todd, One minor comment: instead of returning null for this function, can we return empty string array? This was the cleanup we have done in the past with couple of JDK APIs. The answer was never return null for the array return types. One reason is all the client code needs to check for null condition everywhere. So we should avoid returning null for array types. Do you agree? Otherwise, patch looks good.
        Hide
        Aaron T. Myers added a comment -

        +1, looks good to me.

        Show
        Aaron T. Myers added a comment - +1, looks good to me.
        Hide
        Hadoop QA added a comment -

        +1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12485182/hadoop-7440.txt
        against trunk revision 1142556.

        +1 @author. The patch does not contain any @author tags.

        +1 tests included. The patch appears to include 3 new or modified tests.

        +1 javadoc. The javadoc tool did not generate any warning messages.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        +1 core tests. The patch passed core unit tests.

        +1 system test framework. The patch passed system test framework compile.

        Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/691//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HADOOP-Build/691//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/691//console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - +1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12485182/hadoop-7440.txt against trunk revision 1142556. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. +1 system test framework. The patch passed system test framework compile. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/691//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HADOOP-Build/691//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/691//console This message is automatically generated.

          People

          • Assignee:
            Todd Lipcon
            Reporter:
            Todd Lipcon
          • Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development