Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Response body not parsed for 202 -- doesn't match HTTP spec or Graph API behavior #684

Open
david-burke-securian opened this issue Nov 18, 2022 · 12 comments

Comments

@david-burke-securian
Copy link

david-burke-securian commented Nov 18, 2022

When the response code of an API call is 202 Accepted, then CoreHttpProvider calls handleEmptyResponse(), which ignores the response body.

This causes the copyNotebook operation in the microsoft-graph jar to return an OnenoteOperation object with all null properties, and so the id can't be used to query the operation's status.

The HTTP spec for 202 says "The representation sent with this response ought to describe the request's current status and point to (or embed) a status monitor that can provide the user with an estimate of when the request will be fulfilled." so I believe the API itself is behaving correctly/within the spec and that it is this project that is incorrectly assuming that a 202 response will not have any body content.

Expected behavior

graphClient.sites(site.id).onenote().notebooks(notebook.id).copyNotebook(params).buildRequest().post(); returns a OnenoteOperation with fields populated from the response body.

Actual behavior

graphClient.sites(site.id).onenote().notebooks(notebook.id).copyNotebook(params).buildRequest().post(); returns a OnenoteOperation with all fields set to null.

Steps to reproduce the behavior

SiteRequestBuilder siteRequestBuilder = graphClient.sites(site.id);
OnenoteOperation operation = siteRequestBuilder.onenote().notebooks(notebook.id).copyNotebook(params).buildRequest().post();
// operation.id is null here, so the following line fails:
operation = siteRequestBuilder .onenote().operations(operation.id).buildRequest().get();
@ghost ghost added the ToTriage label Nov 18, 2022
@ghost ghost added this to Issues to triage in Graph SDK - Triage Nov 18, 2022
@ghost ghost removed the ToTriage label Nov 18, 2022
@levdavid
Copy link

we are still experiencing the issue. Is there an ETA on addressing it?

@andrueastman
Copy link
Member

According to the API documentation at the link below, the API does not return a response body for this call hence the null values in the response.

https://learn.microsoft.com/en-us/graph/api/notebook-copynotebook?view=graph-rest-1.0&tabs=http#response

Any chance you can check the response headers for the same and confirm if the Operation-Location header is present to use for the checking the status of the operation?

@levdavid
Copy link

levdavid commented Jan 24, 2023 via email

@levdavid
Copy link

levdavid commented Jan 24, 2023 via email

@baywet
Copy link
Member

baywet commented Jan 24, 2023

This is a new API pattern that all the SDKs would need to implement a task for (like batching, large file upload...)
We currently don't have a design for it, but a task to create the design has been created here microsoftgraph/msgraph-sdk-design#83

@david-burke-securian
Copy link
Author

This workaround is ugly, but it works:

NotebookCopyNotebookRequestBuilder notebookRequestBuilder = graphClient.onenote().notebooks(notebookId).copyNotebook(params);
IHttpRequest iHttpRequest = notebookRequestBuilder.buildRequest().withHttpMethod(HttpMethod.POST);

// Can't use a lambda because generateResult has type parameters
// https://docs.oracle.com/javase/specs/jls/se8/html/jls-15.html#jls-15.27.3
IStatefulResponseHandler<OnenoteOperation, OnenoteOperation> handler = new IStatefulResponseHandler<OnenoteOperation, OnenoteOperation>() {
	@Override
	public <T> OnenoteOperation generateResult(IHttpRequest request, T response, ISerializer serializer, ILogger logger) throws Exception {
		if (!(response instanceof Response)) {
			throw new ClientException("unsupported response type", null);
		}
		final Response nativeResponse = (Response) response;

		if (nativeResponse.code() >= HttpResponseCode.HTTP_OK
			&& nativeResponse.code() < HttpResponseCode.HTTP_MULTIPLE_CHOICES) {
			try (final ResponseBody body = nativeResponse.body()) {
				final byte[] responseBytes = body.bytes();
				return serializer.deserializeObject(new ByteArrayInputStream(responseBytes), OnenoteOperation.class);
			}
		} else {
			throw new ClientException("Non-2xx response " + nativeResponse.code() + ": " + nativeResponse.body().string(), null);
		}
	}
};
OnenoteOperation operation = notebookRequestBuilder.getClient().getHttpProvider().send(iHttpRequest, OnenoteOperation.class, params, handler);

From there you can update the status in a loop via:

operation = graphClient.onenote().operations(operation.id).buildRequest().get();

However, I've noticed that as soon as the operation is complete this call begins to return an error (20112 "Invalid Entity ID specified"). It's as if the server immediately forgets about the job so that you can never actually see the "complete" status. I'm handling this by catching the exception and treating it like success:

try {
	operation = graphClient.onenote().operations(operation.id).buildRequest().get();
} catch (GraphServiceException e) {
	GraphError error = e.getError().error;
	if (operation.id != null && StringUtils.equals(error.code, "20112")) {
		// 20112 = "Invalid Entity ID specified".
		// This happens if the server is no longer tracking this operation, which happens pretty quickly after the operation completes.
		break;
	} else {
		throw e;
	}
}

Again: ugly but it works.

@levdavid
Copy link

@david-burke-securian Thank you for pointing out the workaround! While we already have created our custom HTTP client to make requests to the API directly (and not via SDK since it's broken), I feel like having this ugly workaround mentioned anywhere in documentation would have saved us 4 days of debugging and development work. Another issue worth mentioning is that if you put a json string into a post request's body an empty 502 bad gateway error is returned.

@baywet thank you for pointing out the changes in the API. Just to confirm, does it mean that the response body for the configure post request will go away soon? please advise.

@levdavid
Copy link

levdavid commented Jan 25, 2023

sample response @andrueastman :

{
    "$schema": "https://schema.mp.microsoft.com/schema/configure-status/2022-07-01",
    "jobId": UUID.randomUuid(),
    "jobStatus": "running",
    "jobResult": "pending",
    "jobStart": "2023-01-25T05:47:48.2531456Z",
    "errors": []
}

@andrueastman
Copy link
Member

sample response @andrueastman :

{
    "$schema": "https://schema.mp.microsoft.com/schema/configure-status/2022-07-01",
    "jobId": UUID.randomUuid(),
    "jobStatus": "running",
    "jobResult": "pending",
    "jobStart": "2023-01-25T05:47:48.2531456Z",
    "errors": []
}

Raised https://github.com/microsoftgraph/microsoft-graph-docs/issues/19997 for the documentation to be followed up.

@levdavid
Copy link

Thank you! Also keep in mind that since currently the successful POST request returns HTTP 204, the response body is being omitted by the SDK

@baywet
Copy link
Member

baywet commented Jan 25, 2023

To answer your question from earlier, no it wouldn't disappear, it'd just mean we'd make available what we call a "task" in our design language that'd do the polling for you and give you the result. Kind of like batching support today. But again this is probably a long way out, after the next major release of this SDK.

@levdavid
Copy link

Oh that would be cool, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Graph SDK - Triage
  
Issues to triage
Development

No branches or pull requests

6 participants