[BUG] [cpp-ue4] response json parsing logging a error to unreal console when there is no actual parsing problem

[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

Related issues/PRs

Suggest a fix