Feature/get request by baywet · Pull Request #463 · microsoftgraph/msgraph-sdk-java

This was referenced

Sep 14, 2020

@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.

Choose a reason for hiding this comment

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

LGTM

baywet deleted the feature/get-request branch

September 14, 2020 20:41

Choose a reason for hiding this comment

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

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(),

Choose a reason for hiding this comment

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

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(),

Choose a reason for hiding this comment

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

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];

Choose a reason for hiding this comment

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

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;

Choose a reason for hiding this comment

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

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).

Choose a reason for hiding this comment

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

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;

Choose a reason for hiding this comment

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

IHttpProvider should not contain an implementation specific type and should be generic instead