Some possible defects in HttpServletResponse Javadoc?

22 04 2007

I’ve noticed what I think are some mistakes, inconsistencies and omissions in the Servlet API Javadoc for the javax.servlet.http.HttpServletResponse class’s sendError, sendRedirect and setStatus methods. Of course, this might just be me, but I think it’s worth raising.

So I’ve just raised the following issues as a bug report on the Glassfish issue tracker (issue number 2885), and thought I’d also document these issues here. Primarily just as a handy place to keep track of my own current assumptions and ideas about them, but also just in case this helps anyone else when trying to work out what these methods are supposed to do – with the caveat that everything here is just my own opinion at this point in time, whilst awaiting a response to the Glassfish bug report. If I do find I’ve got anything badly wrong, I’ll come back and add a comment afterwards.

So here are the problems, and what I personally think the javadoc is intended to mean (or ought to be saying):

1 setStatus(int) sets the location header.

The setStatus(int) javadoc includes the line “The container clears the buffer and sets the Location header, preserving cookies and other headers.”. This looks completely out of place – I can’t see why setting a non-error status code should need to clear the buffer, or what the Location header might have to do with this, or what the Location header is supposed to be set to.

My guess is that this is just in the wrong place, and is supposed to be part of the sendRedirect javadoc (which doesn’t currently say anything about clearing the buffer or setting the Location header).

2 sendError(int, String) and sendError(int) differences.

sendError(int, String) describes the nature of the default error response, the use of error-page declarations etc, but doesn’t say anything about clearing any existing buffered content. In contrast, sendError(int) says nothing about the nature of the error response or the use of error-page declarations, but does say “and clearing the buffer” (though it’s unclear whether this means before writing the error response, or with an empty response as the result).

My assumption is that despite the differences in their javadoc, these two sendError methods should be doing the same thing, just with or without an explicitly-given error message string. That is, the javadoc for both of these methods should actually say that they clear the buffer and then write an error page taking into account error-page declarations etc, with the default text/html page if there is no relevant error-page mapping.

3 sendRedirect doesn’t specify response content.

sendRedirect doesn’t say anything about what body content is sent. That is, whether existing buffer content is cleared or remains in the response, and whether or not the redirect writes any particular body content (e.g. a text/html page explaining the redirect and giving the link).

I tend to think that sendRedirect should be sending a page explaining the redirect and giving the link, as is the convention for HTTP redirects. But then it is a bit unclear what format should be used for such a page. Maybe it should be a text/html page, as is the case for the default error page. But then what if the client isn’t using HTML, and maybe isn’t accepting HTML? This isn’t just an “it’s all gone horribly wrong” error page, and might be part of the application’s normal flow, and any page sent is supposed to be a human-readable explanation of the redirect in case it isn’t followed automatically. So sending a specific type of page regardless of the client might be inappropriate. Also, maybe the application wants to produce its own customized redirect pages – in which case leaving any existing content in place would allow this, except that it’s risky because any caller that isn’t expecting this and doesn’t clear the buffer itself could end up sending a partially-built normal page by mistake.

So in the absence of anything in the javadoc saying what should be sent, for the time being I tend to think that clearing the buffer and sending an empty body content is the safest bet, but that the actual behaviour is implementation-dependent.

4 sendError implies Content-Length unchanged.

sendError(int, String) says “leaving cookies and other headers unmodified”, but presumably any Content-Length header should be cleared, as any existing value would presumably be wrong for the resulting error page, and could potentially cause it to be “auto committed” before being fully written. Or maybe the Content-Length header ought to get set to the actual length of the resulting error page? This also applies to sendRedirect (assuming it doesn’t just leave the existing buffered content unchanged), and anything else that clears existing content from the buffer.

My assumption is that if an error page is written or the buffer is otherwise cleared, the Content-Length header should then reflect the resulting page’s actual length regardless of any explicit content length that was set prior to the error, even if the javadoc says that headers should be unchanged.

5 setStatus(int) for error codes.

setStatus(int) says it is used when no error, but “if there is an error AND the caller wishes to invoke an error-page defined in the web application” then sendError “should” be used instead. This leaves it unclear whether the method can or cannot be used for error codes if the caller does NOT wish to invoke the error-page mechanism (i.e. the caller is setting the status code on its own, prior to explicitly writing the error page content for itself). So if called with an error code, what does this method actually do? Throw an exception? Ignore it? Just set the status code to it? Treat it as per sendError? Treat it as per sendError but with no default error page if there’s no error-page mapping? Implementation dependent?

My guess would be that this method is intended to accept error codes as well as non-error codes, and should just set the status code to whatever value is given.

But the javadoc doesn’t guarantee this, and it’s possible that what it’s trying to say is that error codes should/must only be set via sendError, and that setStatus should not be used for error codes and has undefined behaviour for them.

6 Valid status codes not specified.

More generally, none of the methods that take a status code specify what values are valid, or how any out-of-range values are treated (for example, whether integers outside the HTTP 1xx – 5xx range are allowed, what happens if sendError is called with non-error status codes, or what happens if a negative value is given).

In the absence of anything in the javadoc, I’d assume that it’s only safe to send 4xx-5xx values to sendError, 1xx-3xx and possibly also 4xx-5xx values to setStatus(int), and any 1xx-5xx value to the deprecated setStatus(int, message), with the handling of anything else being entirely implementation-dependent.

7 sendError(int, String) clarification.

As a minor clarification, where sendError(int, String) says that if an error-page is configured “it will be served back in preference to the suggested msg parameter”, maybe this should be “..in preference to the default error page” (especially as the resulting error page might actually make use of the given message).

8 sendRedirect status code?

It might also be useful for sendRedirect to specify the actual status code used (e.g. 302 as given by SC_MOVED_TEMPORARILY or SC_FOUND, or 307 as given by SC_TEMPORARY_REDIRECT, or whether this depends on circumstances).

For the time being I’m assuming that sendRedirect always sends a 302 as given by SC_FOUND.

9 setStatus(int, String) inadequately specified.

Whilst setStatus(int, String) is deprecated due to the ambiguous meaning of the “message” argument, it doesn’t adequately define what the method actually does (e.g. what does setting the “message” for this response actually mean? Is behaviour as per setStatus(int) if given a non-error code and as per sendError(int, String) if given an error code? Or something else?).

For the time being I’m assuming that this has been deliberately left as entirely unspecified and implementation dependent, and the behaviour of any calls to this deprecated method can’t be relied on (though a reasonable implementation might be to treat non-error codes as per setStatus(int) and error codes as per sendError(int, String) or sendError(int) depending on whether the message argument is present or null).

Conclusion

As with all such javadoc, there are probably lots of other things too, but the ones listed above are the ones that leave me most puzzled over what their javadoc really means and what behaviour can or can’t be relied on from these methods.

We’ll see what response this gets on the Glassfish bug list (which presumably needs to vet these issues and pass any suggested changes on to the Servlet API expert group, if I’ve understood how javax.servlet code is maintained at the moment – though I’m not really very sure about that at all).

In the meantime I’d be interested if anyone else has encountered any actual problems with the definition of these methods or their implementation by various servlet containers, or can set me straight or provide other interpretations of the Javadoc.

Advertisements

Actions

Information

One response

14 10 2009
closingbraces

Update: This has now been tackled by the Servlet “Expert Group”, and HttpServletResponse.java changes to address these issues committed in Glassfish v3_b68 (see https://glassfish.dev.java.net/issues/show_bug.cgi?id=2885 for status and history).

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s




%d bloggers like this: