Created by: leith-bartrich
fixes #10315
As described in the original bug report, there were issues with response handling. Specifically surrounding connection failures and what was eventually termed an "opinionated part of the handling."
This pull request restructures the building of the response and disambiguates parts of it. It also documents heavily the purpose of the published and processed items in the response. From the commits themselves:
- fixed likely mistake in assigning RetryManager field.
- reworked response handling to better incorporate client connection errors and more granular information.
- differentiated various strings and returns in the response between http and rest. e.g. RestRequestSuccesful and HttpRequestSuccesful are two different things.
- removed an instance of the client setting an arbitrary server response.
- renamed HandleRespons to TryBuildResponse. The response may not have actually happened.
- Added HttpRequest to the Response.
- Renamed internal operationResponse to operationProcessRequestComplete. Mirror's Unreal's naming for what it's handling.
- converted some white-space spaces to tabs.
- changed rest response content and description to TOptional.
- reordering and documenting Response data.
This is a breaking changeset for downstream developers. The disambiguation of parts of the response will very likely break application code that uses it. And unfortunately, I suspect that's necessary.
Also, I do not have the resources to test it extensively. I have tested it on my local code-base with plenty of artificially induced client and server failure situations, to check robustness. But I don't have a breadth of APIs and servers to test against. I think a change as extensive as this, requires more eyes and testing than I can provide.
Looking at the diff for TryBuildResponse is probably not as useful as looking at the full version of it. The changes are substantial. Similarly I'd suggest looking at the whole response object with the comments in place.
The overall approach to the TryBuildResponse method is now one of publishing and processing, in a less than "opinionated" manner. In hopes that it publishes and processes everything that one would need, without requiring diving deeper into the Unreal HTTP layer. But ultimately it provides the access to that layer if needed.
For reference, I'm providing some example code here of how I use this data in my own project.
First, a template method for general REST request and response handling structure (using my BP wrapper classes, but close enough to the underlying objects to be understood):
template<typename U, typename V, typename W>
UFUNCTION(BlueprintCallable, Category = "URESTView")
void DoHandleRESTRequest(U * request, W * api, TFunction<void (V*)> on_success, TFunction<void(V*)> on_connection_fail, TFunction<void(V*)> on_unknown_fail )
{
request->OnResponseEvent.AddLambda([this,on_success, on_connection_fail, on_unknown_fail](U * request, V * response)
{
if (!response->IsHttpRequestSuccessful())
{
if (response->IsHttpClientConnectionError())
{
this->LogError("The client couldn't connect.");
on_connection_fail(response);
} else
{
this->LogError("HTTP request failed.");
on_unknown_fail(response);
}
return;
}
if (!response->IsRestRequestSuccessful())
{
this->LogError("The REST request failed. HTTP Response Code: " + FString::FromInt(response->GetHttpResponseCode()) );
if (response->RestResponseDescriptionIsSet())
{
this->LogInformation("REST Response Description: " + response->GetRestResponseDescription());
}
if (response->RestResponseContentIsSet())
{
this->LogInformation("REST Response Content: " + response->GetRestResponseContent());
}
on_unknown_fail(response);
return;
}
if (!response->IsRestContentFullyParsed())
{
this->LogWarning("The REST response didn't parse fully as per the Schema.");
}
on_success(response);
return;
});
request->ExecuteRequest(api);
}and further, a use of this templated method in a ClientView object, from my codebase:
void ULegalEntitiesManagerView::RequestUpdateLegalEntity(UFPLLegalEntity* entry)
{
auto request = NewObject<UFPLOrgbuilderApiUpdateLegalEntityRequest>();
request->SetId(entry->GetId().ToString());
request->SetLegalEntity(entry);
this->LogInformation("Requesting commit of LegalEntity: " + entry->GetId().ToString());
DoHandleRESTRequest<UFPLOrgbuilderApiUpdateLegalEntityRequest,UFPLOrgbuilderApiUpdateLegalEntityResponse,UFPLOrgbuilderApi>(request,this->api,
[this](auto response)
{
this->LogInformation("Successful commit of LegalEntity.");
auto returned = response->GetContent();
this->OnReplaceLegalEntity.Broadcast(returned);
this->OnShowManager.Broadcast();
},
[this](auto response)
{
this->LogInformation("The server never received the commit. It's safe to retry.");
this->OnShowManager.Broadcast();
},
[this](auto response)
{
this->LogInformation("The server may have received and processed the update. But it's safe to retry the commit.");
this->OnShowManager.Broadcast();
}
);
this->DoCancelableRequest("Updating LegalEntitity: " + entry->GetId().ToString(),[request](){request->CancelRequest();});
}Of note in these client usage examples is that most information is being published back to the user. But a client application layer could also make use the same information to execute more complex behavior.
One thing this adjustment has made fairly clear is that this part of the generated code will likely required substantial rework at a later time.
A schema can define different "content" or return types for an endpoint depending on the server response code. And as it sits, this (and the prior) implementation will make the mistake of trying to parse the first specified response in the schema, every time, no matter the response code. Which works well enough to get things working for simple Schemas.
Further, it also won't handle common rest styled errors either. There are two points in the comments were I've noted that we could expand to check for and parse some well known error types. But those kinds of expansion are beyond the scope of this fix.
It's debatable as to if the TryBuildResponse method should try to parse Content from a server response that doesn't match the response code in the schema. I have left the behavior in place where it'll try anyway and gracefully fail to do so. Which doesn't match a pedantic reading of the Schema, but is instead very lenient towards the server and Schema.
Finally, I apologize for some ugliness to the diff. The codebase is clearly a tab indented set, and I took the opportunity to include some fixes where some lines in the files I was working in were erroneously space intended. So, if you can't figure out why some lines seem to be changing as they don't relate to the changes above and don't seem substantive, consider that it may be a whitespace fix. If this is a serious issue, I can go back and squash out those fixes.
For now, this is a draft pull request. Largely because I think it needs more testing before it could be merged. Also, because I have not addressed the issue of how to (or if to) signal these kinds of breaking changes.
tagging @Kahncode
PR checklist
-
Read the contribution guidelines. -
Pull Request title clearly describes the work in the pull request and Pull Request description provides details about how to validate the work. Missing information here may result in delayed response from the community. -
Run the following to build the project and update samples: Commit all changed files. This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master. These must match the expectations made by your contribution. You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example./mvnw clean package ./bin/generate-samples.sh ./bin/utils/export_docs_generators.sh./bin/generate-samples.sh bin/configs/java*. For Windows users, please run the script in Git BASH. -
File the PR against the correct branch: master(5.3.0),6.0.x -
If your PR is targeting a particular programming language, @mention the technical committee members, so they are more likely to review the pull request.