Skip to content
This repository was archived by the owner on Jun 18, 2024. It is now read-only.

Fix SOAP fault parsing #3

Merged
vboctor merged 4 commits into from
Oct 2, 2014
Merged

Fix SOAP fault parsing #3

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
61127bc
Clean up ServiceRequestBase related classes a bit.
evpaassen Sep 1, 2014
6326bcc
Remove catch block from ServiceRequestBase. This exception is never t…
evpaassen Sep 1, 2014
1d2bdbf
Fix SOAP error handling: if an error code is returned, the SOAP fault…
evpaassen Sep 1, 2014
acfc829
ServiceRequestBase: combine 'request' declaration and initialization.
evpaassen Sep 29, 2014
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions src/main/java/microsoft/exchange/webservices/data/HangingServiceRequestBase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,9 +217,7 @@ protected HangingServiceRequestBase(ExchangeService service,
*/
protected void internalExecute() throws ServiceLocalException, Exception {
synchronized (this) {
OutParam<HttpWebRequest> outParam = new OutParam<HttpWebRequest>();
this.response = this.validateAndEmitRequest(outParam);
this.request = outParam.getParam();
this.response = this.validateAndEmitRequest();
this.internalOnConnect();
}
}
Expand Down
98 changes: 31 additions & 67 deletions src/main/java/microsoft/exchange/webservices/data/ServiceRequestBase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,20 +10,14 @@ Permission is hereby granted, free of charge, to any person obtaining a copy of

package microsoft.exchange.webservices.data;

import java.io.ByteArrayInputStream;
import java.io.ByteArrayOutputStream;
import java.io.IOException;
import java.io.InputStream;
import java.io.OutputStream;
import javax.xml.stream.XMLStreamException;
Copy link
Contributor

@vboctor vboctor Sep 2, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Order alphabetically.

Copy link
Contributor Author

@evpaassen evpaassen Sep 2, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IntelliJ IDEA orders javax.* imports before java.* imports by default. I'm not sure we want to change this as long as the coding standards for this project are undefined.

intellij-import-defaults

Copy link
Contributor

@vboctor vboctor Sep 3, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense.

import java.io.*;
import java.util.concurrent.Callable;
import java.util.concurrent.ExecutionException;
import java.util.concurrent.Future;
import java.util.zip.GZIPInputStream;
import java.util.zip.InflaterInputStream;

import javax.xml.stream.XMLStreamException;
import javax.xml.ws.http.HTTPException;

/**
* Represents an abstract service request.
*/
Expand Down Expand Up @@ -697,41 +691,39 @@ protected SoapFaultDetails readSoapFault(EwsServiceXmlReader reader) {

/**
* Validates request parameters, and emits the request to the server.
*
* @param request
* The request.
*
* @return The response returned by the server.
*/
protected HttpWebRequest validateAndEmitRequest(
OutParam<HttpWebRequest> request) throws ServiceLocalException,
Exception {
protected HttpWebRequest validateAndEmitRequest() throws ServiceLocalException, Exception {
this.validate();

request = this.buildEwsHttpWebRequest();
return this.getEwsHttpWebResponse(request);
HttpWebRequest request = this.buildEwsHttpWebRequest();
try {
return this.getEwsHttpWebResponse(request);
} catch (HttpErrorException e) {
processWebException(e, request);

// Wrap exception if the above code block didn't throw
throw new ServiceRequestException(String.format(Strings.ServiceRequestFailed, e.getMessage()), e);
Copy link
Contributor Author

@evpaassen evpaassen Sep 26, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure we should wrap the exception here either. The problem is the exception gets wrapped two or three times by this class and the problem with wrapping exceptions is that it makes handling the real exception much harder. Why are exceptions wrapped anyway?

}
}

/**
* <summary> Builds the HttpWebRequest object for current service request
* with exception handling.
*
* @return An IEwsHttpWebRequest instance
* @return An HttpWebRequest instance
*/
protected OutParam<HttpWebRequest> buildEwsHttpWebRequest()
throws Exception {
OutParam<HttpWebRequest> outparam = new OutParam<HttpWebRequest>();
protected HttpWebRequest buildEwsHttpWebRequest() throws Exception {
try {

outparam.setParam(this.getService().prepareHttpWebRequest());
HttpWebRequest request = this.getService().prepareHttpWebRequest();
AsyncExecutor ae = new AsyncExecutor();

// ExecutorService es = CallableSingleTon.getExecutor();
Callable getStream = new GetStream(outparam.getParam(),
"getOutputStream");
Callable getStream = new GetStream(request, "getOutputStream");
Future task = ae.submit(getStream, null);
ae.shutdown();
this.getService().traceHttpRequestHeaders(
TraceFlags.EwsRequestHttpHeaders, outparam.getParam());
this.getService().traceHttpRequestHeaders(TraceFlags.EwsRequestHttpHeaders, request);

boolean needSignature = this.getService().getCredentials() != null
&& this.getService().getCredentials().isNeedSignature();
Expand Down Expand Up @@ -762,72 +754,44 @@ protected OutParam<HttpWebRequest> buildEwsHttpWebRequest()
memoryStream);
}

ByteArrayOutputStream serviceRequestStream = (ByteArrayOutputStream) this
.getWebRequestStream(task);
{
EwsUtilities.copyStream(memoryStream, serviceRequestStream);
}

}

else {
ByteArrayOutputStream serviceRequestStream = this.getWebRequestStream(task);
EwsUtilities.copyStream(memoryStream, serviceRequestStream);
} else {
ByteArrayOutputStream requestStream = this
.getWebRequestStream(task);

EwsServiceXmlWriter writer1 = new EwsServiceXmlWriter(this
.getService(), requestStream);

this.writeToXml(writer1);

}

return outparam;
} catch (HTTPException e) {
if (e.getStatusCode() == WebExceptionStatus.ProtocolError.ordinal()
&& e.getCause() != null) {
this.processWebException(e, outparam.getParam());
}

// Wrap exception if the above code block didn't throw
throw new ServiceRequestException(String.format(
Strings.ServiceRequestFailed, e.getMessage()), e);
return request;
} catch (IOException e) {
// Wrap exception.
throw new ServiceRequestException(String.format(
Strings.ServiceRequestFailed, e.getMessage()), e);
throw new ServiceRequestException(String.format(Strings.ServiceRequestFailed, e.getMessage()), e);
}
}

/**
* Gets the IEwsHttpWebRequest object from the specifiedHttpWebRequest
* object with exception handling
*
* @param outparam The specified HttpWebRequest
* @param request The specified HttpWebRequest
* @return An HttpWebResponse instance
*/
protected HttpWebRequest getEwsHttpWebResponse(
OutParam<HttpWebRequest> outparam) throws Exception {
HttpWebRequest request = outparam.getParam();
int code;

protected HttpWebRequest getEwsHttpWebResponse(HttpWebRequest request) throws Exception {
try {
request.executeRequest();

code = request.executeRequest();

} catch (HttpErrorException ex) {
if (ex.getHttpErrorCode() == WebExceptionStatus.ProtocolError
.ordinal()
&& ex.getMessage() != null) {
this.processWebException(ex, request);
if (request.getResponseCode() >= 400) {
throw new HttpErrorException(
Copy link
Contributor

@vboctor vboctor Sep 2, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than wrapping this exception by the caller, should we wrap it here? This also makes this method always throws ServiceRequestException.

Copy link
Contributor Author

@evpaassen evpaassen Sep 2, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, the exception handling/wrapping in this library is kind of an issue on its own. Every method seems to just throw generic Exceptions all over the place, instead of throwing specific exceptions which can be handled adequately. If you think I should wrap it here already, I'll make the change, but I'm unsure here.

Copy link
Contributor

@vboctor vboctor Sep 3, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having a look at processWebException() that you have removed, it is used in 3 places in the file and it does handle several error cases. I wonder if the fix should make processWebException() handle the lack of headers instead of just not using it.

Copy link
Contributor Author

@evpaassen evpaassen Sep 3, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In fact, the fix is really that we do call processWebException() now. The processWebException() call was unreachable code before, because ex.getHttpErrorCode() == WebExceptionStatus.ProtocolError.ordinal() can never be true.

The only place getEwsHttpWebResponse() is called is in validateAndEmitRequest(), and I handled the HttpErrorException and call processWebException() there. But perhaps it makes more sense to do that in getEwsHttpWebResponse() already. If so, please let me know.

"The remote server returned an error: (" + request.getResponseCode() + ")" +
request.getResponseText(), request.getResponseCode());
}

// Wrap exception if the above code block didn't throw
throw new ServiceRequestException(String.format(
Strings.ServiceRequestFailed, ex.getMessage()), ex);
} catch (IOException e) {
// Wrap exception.
throw new ServiceRequestException(String.format(
Strings.ServiceRequestFailed, e.getMessage()), e);
throw new ServiceRequestException(String.format(Strings.ServiceRequestFailed, e.getMessage()), e);
}

return request;
Expand Down
81 changes: 36 additions & 45 deletions src/main/java/microsoft/exchange/webservices/data/SimpleServiceRequestBase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,39 +43,33 @@ protected SimpleServiceRequestBase(ExchangeService service)
* @throws Exception
* @throws microsoft.exchange.webservices.data.ServiceLocalException
*/
protected Object internalExecute()
throws ServiceLocalException, Exception {
OutParam<HttpWebRequest> outParam =
new OutParam<HttpWebRequest>();
HttpWebRequest response = this.validateAndEmitRequest(outParam);

try {
protected Object internalExecute() throws ServiceLocalException, Exception {
HttpWebRequest response = null;

try {
response = this.validateAndEmitRequest();
return this.readResponse(response);
}
catch (IOException ex) {
// Wrap exception.
throw new ServiceRequestException(String.
format(Strings.ServiceRequestFailed, ex.getMessage(), ex));
}
catch (Exception e) {
if (response != null) {
this.getService().processHttpResponseHeaders(TraceFlags.
EwsResponseHttpHeaders, response);
}

throw new ServiceRequestException(String.format(Strings.
ServiceRequestFailed, e.getMessage()), e);
}
finally
{
try {
response.close();
} catch (IOException ex) {
// Wrap exception.
throw new ServiceRequestException(String.
format(Strings.ServiceRequestFailed, ex.getMessage(), ex));
} catch (Exception e) {
if (response != null) {
this.getService().processHttpResponseHeaders(TraceFlags.
EwsResponseHttpHeaders, response);
}

throw new ServiceRequestException(String.format(Strings.ServiceRequestFailed, e.getMessage()), e);
} finally {
try {
if (response != null) {
response.close();
}
} catch (Exception e2) {
response = null;
}
}

}
}
}
}

/**
* Ends executing this async request.
Expand All @@ -88,19 +82,17 @@ protected Object endInternalExecute(IAsyncResult asyncResult) throws Exception {
return this.readResponse(response);
}

/**
* Begins executing this async request.
*
*@param callback The AsyncCallback delegate.
*@param state An object that contains state information for this request.
* @return An IAsyncResult that references the asynchronous request.
*/
protected AsyncRequestResult beginExecute(AsyncCallback callback, Object state) throws Exception
{
/**
* Begins executing this async request.
*
* @param callback The AsyncCallback delegate.
* @param state An object that contains state information for this request.
* @return An IAsyncResult that references the asynchronous request.
*/
protected AsyncRequestResult beginExecute(AsyncCallback callback, Object state) throws Exception {
this.validate();

HttpWebRequest request = (HttpWebRequest) this.buildEwsHttpWebRequest()
.getParam();
HttpWebRequest request = this.buildEwsHttpWebRequest();

WebAsyncCallStateAnchor wrappedState = new WebAsyncCallStateAnchor(
this, request, callback /* user callback */, state /*user state*/);
Expand All @@ -109,8 +101,7 @@ protected AsyncRequestResult beginExecute(AsyncCallback callback, Object state)
Callable cl = new CallableMethod(request);
Future task = es.submit(cl, callback);
es.shutdown();
AsyncRequestResult ft = new AsyncRequestResult(this, request, task,
null);
AsyncRequestResult ft = new AsyncRequestResult(this, request, task, null);

// ct.setAsyncRequest();
// webAsyncResult =
Expand All @@ -119,9 +110,9 @@ protected AsyncRequestResult beginExecute(AsyncCallback callback, Object state)
return ft;
// return new AsyncRequestResult(this, request, webAsyncResult, state /*
// user state */);
}
}

/**
/**
* Reads the response.
* @return serviceResponse
* @throws Exception
Expand Down