5
\$\begingroup\$

My company has asked me to write a few Java based programs to deal with sending HTTP requests and parsing the response. After playing around with the Apache HTTP Commons library and making plenty of shortsighted programs using it, I've decided to make a little library for us to use and avoid major code duplication. We're a pretty small company working with a really old code base, and they don't seem to have time to do code reviews. So I present to you, my small API (leaving major comments outside to describe classes and break up the code segments).

  • As a small aside, you may see reference to static class ResponseBodyDisplay, which is a simple helper class (that looks like crap) to pull out the response body. It's not posted, because I didn't write it and it looks like poop.
/**    
 * This class facilitates an easy to use controller for creating HTTP     
 * requests without having a bunch of duplicate code.     
 *     
 * Typical use:    
 * HttpController.processBodyRequest(request, json)    
 *     
 * In retrospect, "getJSONField()" may be a little outside the scope    
 * of an HttpController. Meh!    
 */    

public class HttpController 
{ private static DefaultHttpClient httpClient =  new DefaultHttpClient();

  public static Response processBodyRequest(Request r, String json)
            throws IOException
  { r.applyHeaders();
    HttpEntityEnclosingRequestBase request = r.getHttpRequest();
    ByteArrayEntity dataEntity = new ByteArrayEntity(json.getBytes());
    request.setEntity(dataEntity);
    return executeRequest(request);
  }

  private static Response executeRequest(HttpEntityEnclosingRequestBase request) 
            throws IOException
  { HttpResponse httpResponse = httpClient.execute(request);
    Header[] headers = httpResponse.getAllHeaders();
    Response r = new Response(httpResponse);
    for (Header h: headers) 
    { r.addHeader(h.getName(), h.getValue());
    }
    r.setJSON(ResponseBodyDisplay._getResponseBody(httpResponse.getEntity()));
    return r;
  }

  public static String getJSONField(Response r, String fieldName) 
  { try
    { String responseJSON = r.getJSON();
      JSONTokener tokener = new JSONTokener(responseJSON);
      JSONObject responseJSONObject = new JSONObject(tokener);
      return responseJSONObject.getString(fieldName);
    }
    catch (Exception e)
    { e.printStackTrace();
      return null;
    }
  }
}
/**
 * Object container for header map and JSON String.  
 */
public class HttpObject 
{ protected String json;
  protected HashMap<String, String> headers;

  protected HttpObject()
  { headers = new HashMap<String, String>();
  }

  public void addHeader(String name, String value)
  { headers.put(name, value);
  }

  public void setJSON(String json) 
  { this.json = json;
  }

  public String getJSON()
  { return json;
  }
}
/**    
 *  Simulates Apache HttpRequest object with the necessary information     
 *  needed for us to process and manipulate an HttpRequest.     
 *      
 *  Typical use: Request.postRequest(uri) to generate request, then begin    
 *  attaching headers until the request is ready to be processed.     
 *      
 *  Looking back as I write this, the JSON string should be in here, and     
 *  the HttpController should pull it out. Maybe? Maybe not, since we could    
 *  be sending over multiple parameters.    
 */    
public class Request extends HttpObject
{ private HttpEntityEnclosingRequestBase httpRequest;

  private Request() 
  { super();
  }

  public static Request postRequest(String uri) 
  { return createRequest(new HttpPost(uri));
  }

  public static Request putRequest(String uri) 
  { return createRequest(new HttpPut(uri)); 
  }

  private static Request createRequest(HttpEntityEnclosingRequestBase httpRequest)
  { Request r = new Request();
  r.setHttpRequest(httpRequest);
  return r;
  }

  public HttpEntityEnclosingRequestBase getHttpRequest() 
  { return httpRequest;
  }

  public void applyHeaders()
  { Set<String> keys = headers.keySet();
    for (String key : keys) 
    { httpRequest.setHeader(key, headers.get(key));
    }
  }

  private void setHttpRequest(HttpEntityEnclosingRequestBase httpRequest)
  { this.httpRequest = httpRequest;
  }

  //HttpEntityEnclosingRequestBase can't be a get request.
//  public void getRequest(String uri) 
//  { httpRequest = new HttpGet(uri);
//  }
}

Same as Request really

public class Response extends HttpObject
{ HttpResponse httpResponse;

  public Response(HttpResponse r) 
  { httpResponse = r; 
  }

  public HttpResponse getHttpResonse()
  { return httpResponse;
  }
}

Finally, a sample run of the code.

public static String loginToLocation() 
{ Request r = Request.postRequest(HOST + "AuthenticateWebSafe");
    addCommonHeaders(r);
    try
    { Response resp = HttpController.processBodyRequest(r, generateLoginJSON());
      return HttpController.getJSONField(resp, "SessionKey");
    }
    catch (IOException e) //Not particularly graceful
    { e.printStackTrace();
      return null;
    }
}

private static void addCommonHeaders(Request r)
{ r.addHeader("accept", "application/json");
}

private static String generateLoginJSON() 
{ String loginString = "{\"email\":\"" + email + "\","
             + "\"pass\":\"" + pw + "\"}";
  return loginString;   
}
\$\endgroup\$
2
\$\begingroup\$

I think that your exception handling is problematic.

try {
    ...
} catch (IOException e) {
    e.printStackTrace();
    return null;
}

Besides logging, all that accomplishes is degrade an "informative" kind of exception into a null, which creates more problems than it solves:

  • The caller would have to explicitly check for a null result. The caller cannot use a try-catch block to handle the error condition, which is the preferred way to deal with such exceptional situations.
  • If the caller forgets to check for a null result, then you'll probably get a NullPointerException at some point. As a developer, which kind of stack trace would you rather try to diagnose: one with an IOException or one with a NullPointerException?
  • Suppose the caller does check for null. What kind of feedback could it present to the user? "Sorry, your request failed for some reason. We can't tell you why." An IOException, on the other hand, can contain more details about why the request failed. You can even choose to handle specific subclasses of IOException and ignore others.

If you catch an exception but aren't able to deal with it effectively, the best thing you could probably do is let it propagate — i.e., don't bother catching it at all in the first place, and declare it in the method signature if appropriate. Alternatively, if you need to avoid a leaky abstraction, you might want to wrap the exception before re-throwing it.

\$\endgroup\$
2
  • \$\begingroup\$ Thanks for the comment, and I completely agree. I should have given that section a bit more love, especially since I had an error there and it was a pain to deal with. Though ultimately, it was right at the tail end of the execution and I didn't expect much of that exception even if it was caught. I'll change this shortly and edit my original post. \$\endgroup\$ – Jarmo Jan 9 '14 at 20:09
  • \$\begingroup\$ If you edit the original post, include your enhancements as an addendum; please don't edit out the original code. \$\endgroup\$ – 200_success Jan 9 '14 at 21:04
2
\$\begingroup\$

This is a bug, bordering on a security risk:

private static String generateLoginJSON() 
{ String loginString = "{\"email\":\"" + email + "\","
             + "\"pass\":\"" + pw + "\"}";
  return loginString;   
}

Never generate JSON by string concatenation, because values need to be escaped. You already use JSONTokener, so use JSONStringer to do the inverse:

private static String generateLoginJSON() {
    return new JSONStringer()
           .object()
               .key("email").value(email)
               .key("pass").value(pw)
           .endObject()
           .toString();
}

Your test case might use values that happen to not require escaping. Nevertheless, careless concatenation is a filthy practice, best to be avoided altogether lest other programmers learn bad habits from your code.

This is, by the way, the same class of bugs as SQL injection, HTML injection, and HTTP header injection. They have the same root cause: building a string by concatenation (or interpolation), to be interpreted in some human-friendly language (text-based protocol). Every such language has special delimiter characters that need to be escaped, and failure to perform the escaping leads to disaster.

enter image description here

\$\endgroup\$
1
  • \$\begingroup\$ Thank you for the insight. This is not something I would have thought of, and I've corrected it here and in other areas of code. Thanks! \$\endgroup\$ – Jarmo Jan 13 '14 at 18:55

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.