Home Original page

Return response headers for successful calls against the API by caitlinrussell · Pull Request #15 · microsoftgraph/msgraph-sdk-java

@caitlinrussell

This will be added under the "graphResponseHeaders" node in the additionalDataManager:

me.additionalDataManager().get("graphResponseHeaders");

Caitlin Bales (MSFT) added 5 commits

December 15, 2017 15:14

akrantz

Choose a reason for hiding this comment

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

Since this was my first code review, I felt compelled to add some feedback. The overall changes are fine. Let me know whether you want to make the suggested changes, but if you don't I would be fine with it.

}

@Override
public <T> T deserializeObject(final String inputString, final Class<T> clazz, Map<String, java.util.List<String>> responseHeaders) {

Choose a reason for hiding this comment

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

I believe that you could provide only one overload for the deserializeObject function by making the responseHeaders parameter accept the default value of null. I would favor using default parameters to minimize the overloads, and having separate overloads only when necessary. You would also need to update the interface definition to match, but I believe that would be an OK change to make.

Choose a reason for hiding this comment

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

Since Java doesn't support default parameters, the two simplest ideas I had were to use overloading or check for a null responseHeaders list. Since the latter requires changing the method signature across the library, I figured the former was a bit cleaner.

Choose a reason for hiding this comment

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

OK, I mistakenly thought Java had added default parameters.
JsonObject res = new GsonBuilder().create().fromJson(jsonString.toString(), JsonObject.class);
return res.get("access_token").toString();
return res.get("access_token").toString().replaceAll("\"", "");

Choose a reason for hiding this comment

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

It would be helpful to add a comment that quotes are being removed.

MIchaelMainer

try {
list.add(String.format("%d", connection.getResponseCode()));
} catch (IOException e) {
throw new IllegalArgumentException("Invalid connection response code: ", e);

Choose a reason for hiding this comment

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

Need to concatenate the error response code.

Choose a reason for hiding this comment

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

By definition of this error, it can't be done (connection.getResponseCode() itself needs to be wrapped in a catch). However, the JavaDoc explains that an IllegalArgumentException is thrown when a connection cannot be made, so I added this to the error.

Caitlin Bales (MSFT) added 2 commits

January 25, 2018 17:58

MIchaelMainer