Feature/get request #463
Feature/get request #463
Conversation
|
@baywet, can you please summarize which parts of the code are generated, which parts are hand-crafted? |
|
@zengin this commit is the only code-gen in the PR aa0b2c3 the rest is handcrafted. basically I'm adding 2 methods + 2 overloads for one method (withHttpMethod and getHttpRequest) in IHttpRequest and implementing that in the different BaseRequest classes. Then I'm relying on IHttpRequest inheritance (the code-gen part) to expose the methods in the object model without having to cast. |
MIchaelMainer
left a comment
MIchaelMainer
left a comment
There was a problem hiding this comment.
You get my review post merge. No required actions
| } | ||
|
|
||
| // Request level middleware options | ||
| RedirectOptions redirectOptions = new RedirectOptions(request.getMaxRedirects() > 0? request.getMaxRedirects() : this.connectionConfig.getMaxRedirects(), |
There was a problem hiding this comment.
nit: separate the ? one space from the last condition character, I started to incorrectly read the ternary operator.
| // Request level middleware options | ||
| RedirectOptions redirectOptions = new RedirectOptions(request.getMaxRedirects() > 0? request.getMaxRedirects() : this.connectionConfig.getMaxRedirects(), | ||
| request.getShouldRedirect() != null? request.getShouldRedirect() : this.connectionConfig.getShouldRedirect()); | ||
| RetryOptions retryOptions = new RetryOptions(request.getShouldRetry() != null? request.getShouldRetry() : this.connectionConfig.getShouldRetry(), |
There was a problem hiding this comment.
General, not actionable per this PR, comment: we need a generic way to check for and add middleware options instead of this hardcoded approach. Please resolve this comment.
| // Send an empty body through with a POST request | ||
| // This ensures that the Content-Length header is properly set | ||
| if (request.getHttpMethod() == HttpMethod.POST) { | ||
| bytesToWrite = new byte[0]; |
There was a problem hiding this comment.
To be consistent, also so that the log shows that the POST was sent intentionally without a request body. logger.logDebug("Sending empty request body");
| RequestBody requestBody = null; | ||
| // Handle cases where we've got a body to process. | ||
| if (bytesToWrite != null) { | ||
| final String mediaContentType = contenttype; |
There was a problem hiding this comment.
nit: Lines 302 - 332 - these lines have a discrete concern around getting a request body if bytesToWrite is not null so we have the option to make this a testable unit. Something like: GetRequestBody(byte[] bytesToWrite).
There was a problem hiding this comment.
I agree on the fact that sendRequestInternal was waaay too long to be testable. The fact that this change already breaks it into to pieces is a step in the right direction for better testability. And yes, moving this section to a dedicated method would be a good next step. Same thing for the part that deals with the response in sendRequestInternal
| * @param <responseType> the type of the response object | ||
| * @return the Request object to be executed | ||
| */ | ||
| <requestBodyType, responseType> Request getHttpRequest(final requestBodyType serializedObject, final IProgressCallback<responseType> progress) throws ClientException; |
There was a problem hiding this comment.
IHttpProvider should not contain an implementation specific type and should be generic instead
Fixes #301
Depends on core 1.0.3
Depends on microsoftgraph/msgraph-sdk-java-core#70
Depends on microsoftgraph/msgraph-sdk-java-core#71
Changes proposed in this pull request
Other links
TODO: once this PR gets merged, ping the graph explorer API repo to update the sample https://docs.microsoft.com/en-us/graph/sdks/batch-requests?context=graph%2Fapi%2F1.0&view=graph-rest-1.0&tabs=java