[BUG] [cpp-ue4] response json parsing logging a error to unreal console when there is no actual parsing problem
Created by: leith-bartrich
Bug Report Checklist
-
Have you provided a full/minimal spec to reproduce the issue? -
Have you validated the input using an OpenAPI validator (example)? -
Have you tested with the latest master to confirm the issue still exists? -
Have you searched for related issues/PRs? -
What's the actual output vs expected output? -
[Optional] Sponsorship to speed up the bug fix or feature request (example)
Description
The cpp-ue4 template creates code that in turn, creates and logs errors about parsing errors, that are not actually parsing errors.
The actual error log statement is generated in the API source like so:
void OpenAPIOrgbuilderApi::HandleResponse(FHttpResponsePtr HttpResponse, bool bSucceeded, Response& InOutResponse) const
{
InOutResponse.SetHttpResponse(HttpResponse);
InOutResponse.SetSuccessful(bSucceeded);
if (bSucceeded && HttpResponse.IsValid())
{
InOutResponse.SetHttpResponseCode((EHttpResponseCodes::Type)HttpResponse->GetResponseCode());
FString ContentType = HttpResponse->GetContentType();
FString Content;
if (ContentType.IsEmpty())
{
return; // Nothing to parse
}
else if (ContentType.StartsWith(TEXT("application/json")) || ContentType.StartsWith("text/json"))
{
Content = HttpResponse->GetContentAsString();
TSharedPtr<FJsonValue> JsonValue;
auto Reader = TJsonReaderFactory<>::Create(Content);
if (FJsonSerializer::Deserialize(Reader, JsonValue) && JsonValue.IsValid())
{
if (InOutResponse.FromJson(JsonValue))
return; // Successfully parsed
}
}
else if(ContentType.StartsWith(TEXT("text/plain")))
{
Content = HttpResponse->GetContentAsString();
InOutResponse.SetResponseString(Content);
return; // Successfully parsed
}
// Report the parse error but do not mark the request as unsuccessful. Data could be partial or malformed, but the request succeeded.
UE_LOG(Logfpipelite_client, Error, TEXT("Failed to deserialize Http response content (type:%s):\n%s"), *ContentType , *Content);
return;
}
Essentially, if the response operation returns True upon trying to deserialize, it returns. If however the deserialize returns False, it falls through to the UE_LOG statement at the end but, it still returns and continues.
This suggests the design is to be lenient and allow execution continue but call out what it sees as an error.
And in fact, yes, an object that is properly deserialized but indicates a failure anyway, does continue on its merry way.
If this error only popped up on actual parsing errors, this might be okay. However, for Django's default api-token-auth schema, it erroneously claims to have a parsing error because the username and password parameters are not part of the response object. Those parameters are required, but they're also writeOnly. They should not be part of the response. The template has enough information from the generator to avoid the mistake, but fails to do so. Hence, I'm classifying this a bug.
Here is the relevant code generated by the development HEAD for a AutthToken model:
bool OpenAPIAuthToken::FromJson(const TSharedPtr<FJsonValue>& JsonValue)
{
const TSharedPtr<FJsonObject>* Object;
if (!JsonValue->TryGetObject(Object))
return false;
bool ParseSuccess = true;
ParseSuccess &= TryGetJsonValue(*Object, TEXT("username"), Username);
ParseSuccess &= TryGetJsonValue(*Object, TEXT("password"), Password);
ParseSuccess &= TryGetJsonValue(*Object, TEXT("token"), Token);
return ParseSuccess;
}
The username and password params here are actually marked required and writeOnly in the schema. However, the generated code treats them as required for the response. If they are absent, it indicates to the caller that the parse was not successful. This code's only purpose is to parse a response from server to client. Therefore, their absence is not only acceptable, but probably is the correct behavior in a strict reading of the schema. It's also worth noting that token is actually optional and its absence also should not generate a big red parsing error.
My proposed solution on the other hand, currently generates this code for the AuthTokenModel:
bool OpenAPIAuthToken::FromJson(const TSharedPtr<FJsonValue>& JsonValue)
{
const TSharedPtr<FJsonObject>* Object;
if (!JsonValue->TryGetObject(Object))
return false;
bool ParseSuccess = true;
//Required but write-only. Its absence is reasonable when parsing JSON response.
TryGetJsonValue(*Object, TEXT("username"), Username);
//Required but write-only. Its absence is reasonable when parsing JSON response.
TryGetJsonValue(*Object, TEXT("password"), Password);
//Optional. Its absence would be okay.
TryGetJsonValue(*Object, TEXT("token"), Token);
return ParseSuccess;
}
The comments in the code itself are explanatory. In this case, all three parameters can actually all fail to parse because either, they're optional, or they're required but write-only. This results in no error being logged because there is no parsing error here.
The generated output for the LegalEntity model shows slightly more useful code here:
bool OpenAPILegalEntity::FromJson(const TSharedPtr<FJsonValue>& JsonValue)
{
const TSharedPtr<FJsonObject>* Object;
if (!JsonValue->TryGetObject(Object))
return false;
bool ParseSuccess = true;
//Required and not write-only. Its absence is likely a problem.
ParseSuccess &= TryGetJsonValue(*Object, TEXT("short_name"), ShortName);
//Required and not write-only. Its absence is likely a problem.
ParseSuccess &= TryGetJsonValue(*Object, TEXT("long_name"), LongName);
//Required and not write-only. Its absence is likely a problem.
ParseSuccess &= TryGetJsonValue(*Object, TEXT("description"), Description);
//Optional. Its absence would be okay.
TryGetJsonValue(*Object, TEXT("id"), Id);
//Optional. Its absence would be okay.
TryGetJsonValue(*Object, TEXT("created_at"), CreatedAt);
//Optional. Its absence would be okay.
TryGetJsonValue(*Object, TEXT("modified_at"), ModifiedAt);
return ParseSuccess;
}
As explained by the comments, my proposed solution does preserve the behavior of indicating a parsing error if a required parameter fails to parse. short_name, long_name and description all can flip the parseSuccess flag.
To be clear, I think there probably should be a design change here. Where instead of indicating a monolithic ParseSuccess or failure for the whole process, we should log more detailed information using Unreal's Verbose and VeryVerbose log types. But that is not a bug and is more a feature expansion. It's something I'd fix separately.
Also, there's another issue that this illuminates a bit. Required parameters are generated as fields without unreal's TOptional<> regardless of their readOnly or writeOnly status. And really, any required parameter that is also readOnly or writeOnly probably should be implemented with TOptional<>. But again, that's a different issue and a different discussion.
I'll likely create a pull request shortly.
openapi-generator version
development HEAD: openapi-generator-project 5.3.0-SNAPSHOT
OpenAPI declaration file content or url
openapi: 3.0.2
info:
title: ''
version: ''
paths:
/orgbuilder/legal_entity/:
get:
operationId: listLegalEntitys
description: ''
parameters: []
responses:
'200':
content:
application/json:
schema:
type: array
items:
$ref: '#/components/schemas/LegalEntity'
description: ''
tags:
- orgbuilder
post:
operationId: createLegalEntity
description: ''
parameters: []
requestBody:
content:
application/json:
schema:
$ref: '#/components/schemas/LegalEntity'
application/x-www-form-urlencoded:
schema:
$ref: '#/components/schemas/LegalEntity'
multipart/form-data:
schema:
$ref: '#/components/schemas/LegalEntity'
responses:
'201':
content:
application/json:
schema:
$ref: '#/components/schemas/LegalEntity'
description: ''
tags:
- orgbuilder
/orgbuilder/legal_entity/{id}/:
get:
operationId: retrieveLegalEntity
description: ''
parameters:
- name: id
in: path
required: true
description: A UUID string identifying this legal entity.
schema:
type: string
responses:
'200':
content:
application/json:
schema:
$ref: '#/components/schemas/LegalEntity'
description: ''
tags:
- orgbuilder
put:
operationId: updateLegalEntity
description: ''
parameters:
- name: id
in: path
required: true
description: A UUID string identifying this legal entity.
schema:
type: string
requestBody:
content:
application/json:
schema:
$ref: '#/components/schemas/LegalEntity'
application/x-www-form-urlencoded:
schema:
$ref: '#/components/schemas/LegalEntity'
multipart/form-data:
schema:
$ref: '#/components/schemas/LegalEntity'
responses:
'200':
content:
application/json:
schema:
$ref: '#/components/schemas/LegalEntity'
description: ''
tags:
- orgbuilder
patch:
operationId: partialUpdateLegalEntity
description: ''
parameters:
- name: id
in: path
required: true
description: A UUID string identifying this legal entity.
schema:
type: string
requestBody:
content:
application/json:
schema:
$ref: '#/components/schemas/LegalEntity'
application/x-www-form-urlencoded:
schema:
$ref: '#/components/schemas/LegalEntity'
multipart/form-data:
schema:
$ref: '#/components/schemas/LegalEntity'
responses:
'200':
content:
application/json:
schema:
$ref: '#/components/schemas/LegalEntity'
description: ''
tags:
- orgbuilder
delete:
operationId: destroyLegalEntity
description: ''
parameters:
- name: id
in: path
required: true
description: A UUID string identifying this legal entity.
schema:
type: string
responses:
'204':
description: ''
tags:
- orgbuilder
/orgbuilder/api-token-auth/:
post:
operationId: createAuthToken
description: ''
parameters: []
requestBody:
content:
application/x-www-form-urlencoded:
schema:
$ref: '#/components/schemas/AuthToken'
multipart/form-data:
schema:
$ref: '#/components/schemas/AuthToken'
application/json:
schema:
$ref: '#/components/schemas/AuthToken'
responses:
'201':
content:
application/json:
schema:
$ref: '#/components/schemas/AuthToken'
description: ''
tags:
- orgbuilder
components:
schemas:
LegalEntity:
type: object
properties:
id:
type: string
format: uuid
readOnly: true
created_at:
type: string
format: date-time
readOnly: true
modified_at:
type: string
format: date-time
readOnly: true
short_name:
type: string
description: The short name of the item (used for filenames.)
maxLength: 255
long_name:
type: string
description: A longer, formal name for the item, that would show up in text.
maxLength: 255
description:
type: string
description: A short description of the item for informational purposes.
required:
- short_name
- long_name
- description
AuthToken:
type: object
properties:
username:
type: string
writeOnly: true
password:
type: string
writeOnly: true
token:
type: string
readOnly: true
required:
- username
- password
Generation Details
my command-line: openapi-generator generate -g cpp-ue4 -i ./openapi-schema.yml --additional-properties cppNamespace=fpipelite::client --additional-properties unrealModuleName=fpipelite_client
yours will likely vary a bit.
Steps to reproduce
- create a basic Django REST API including the standard api-auth-token view
- export a OpenAPI schema
- generate cpp-ue4 from the schema
- examine generated code