Thanks for the feedback, Jody!
First off, there are cases when the HTTPResponse is wrapped over a
FileInputStream. Maybe it's mostly for unit tests, but I think I've seen it in
other situations (like caching perhaps).
Therefore I intentionally avoided the getStatusCode method, but surely we could
introduce it, and simply return 200 or 404 in cases we have a FileInputStream.
Still I would like to keep the isOK as the main method to check if the server
has given a good response. I haven't settled on any list of status codes were
isOK should return true. I'm open for suggestions, but a minimum would be that
SimpleHttpResponse want throw an exception when calling getInputStream. I'm
also thinking that we should have that method open for overrides by subclasses,
and let them decide if the response is OK.
When it comes to the redirects, I think they should be handled by the
delegating http client. And I think they do.
I will have another look at the examples, and also your suggestion with
getResponseStream(boolean allowErrorResponse).
Best regards,
Roar Brænden
> 27. mai 2022 kl. 04:32 skrev Jody Garnett <jody.garn...@gmail.com>:
>
> Thanks for the proposal Roar:
>
> The GeoTools wiki is not restricted, we can consider setting that up as
> needed.
>
> If I understand correctly the proposal is:
> boolean isOK(): returns true for HTTP status 200
> not quite sure if you want to consider some of the other success codes such
> as 201 here also?
> Can/should this handle redirects?
> InputStream getErrorStream(): provides the error response content when isOK()
> false
> Unclear what this does if isOK() true? Do you wish to throw an exception to
> indicate error stream is not available?
> InputStream getResponseStream(): provides the response when isOK() is true,
> or throws an IOException (presumably with the contents of the error response?)
> Glad your proposal includes examples:
> - Where we would parse a ServiceException: Before and after show different
> logic (the first assumes failure)
> - Where we can't handle the error response.: Does not show use of
> getErrorStream()
>
> Let me try and write an example that uses both getErrorStream() and
> getResponseStream() to see if I understand the proposal:
>
> try {
> if (httpResponse.isOK()) {
> return ImageIOExt.readBufferedImage(httpResponse.getResponseStream());
> }
> else {
> if
> ("application/vnd.ogc.se_xml".equalsIgnoreCase(httpResponse.getContentType()))
> {
> throw parseXMLException(httpResponse.getErrorStream());
> }
> else if
> (httpResponse.getContentType().toLowerCase().startsWith("text/")){
> throw parseException(httpResponse.getErrorStream());
> }
> else {
> throw new IOException("Server returned error on request");
> }
> } finally {
> httpResponse.dispose();
> }
>
>
> Feedback:
> - This is an HTTP Request, can we just see the response code? If we had a
> status code isOK() becomes making a check against Arrays.asList(200,201,203);
> - Your ticket examples with HTTP 400 and 415 requests do not seem to be
> addressed here?
> - Tempting to document httpResponse.getContentType() as returning lowercase
> to avoid requiring equalsIgnoreCase() and toLowerCase() be used by all client
> code ever
> - Having two method names is difficult, and it maybe easier for users of the
> API if you added httpResponse.getResponseStream(boolean allowsErrorResponse)
> - Even better would be httpResponse.getResponseStream(int httpStatusCode )
> providing you make the status code available
>
> Putting these ideas together:
>
> try {
> if (httpResponse.getStatus() == 200) {
> return ImageIOExt.readBufferedImage(httpResponse.getResponseStream());
> }
> if (httpResponse.getStatus() == 400) {
> if
> ("application/vnd.ogc.se_xml".equals(httpResponse.getContentType())) {
> String message =
> parseXmlException(httpResponse.getErrorStream(400));
> throw new HttpException(400,message);
> }
> else if (httpResponse.getContentType().startsWith("text/")){
> String message =
> parseTextException(httpResponse.getErrorStream(400));
> throw new HttpException(400,message);
> }
> }
> throw new HttpException(httpResponse.getStatus(),"Couldn't get image of
> tile: " + tile.getId());
> } finally {
> httpResponse.dispose();
> }
>
> Food for thought.
> --
> Jody Garnett
>
>
> On Wed, 25 May 2022 at 00:34, Roar Brænden <roar.brenden...@gmail.com
> <mailto:roar.brenden...@gmail.com>> wrote:
> Hi,
>
> I have a Proposal for the gt-http library. More specific the HTTPResponse
> interface. I followed the instructions at the Wiki page, and created a New
> Page, but when I clicked Save Page I didn't get a Pull Request as the
> instructions indicated. It seems like the Page was created and freely visible.
>
> https://github.com/geotools/geotools/wiki/HTTPResponse-handling-error-responses
>
> <https://github.com/geotools/geotools/wiki/HTTPResponse-handling-error-responses>
>
> Could you please have a look, and maybe give some feedback?
>
> Best regards,
> Roar Brænden
>
>
> _______________________________________________
> GeoTools-Devel mailing list
> GeoTools-Devel@lists.sourceforge.net
> <mailto:GeoTools-Devel@lists.sourceforge.net>
> https://lists.sourceforge.net/lists/listinfo/geotools-devel
> <https://lists.sourceforge.net/lists/listinfo/geotools-devel>
_______________________________________________
GeoTools-Devel mailing list
GeoTools-Devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/geotools-devel
