With OpenAPI generated clients, unable to deploy multiple resources in the same deployment

ThorbenLindhauer commented 3 years ago

This issue was imported from JIRA:

Field	Value
JIRA Link	CAM-13105
Reporter	@yanavasileva
Has restricted visibility comments	true

Environment (Required on creation):

Camunda Platform Rest OpenAPI json and running engine >= 7.13.0

Description (Required on creation; please attach any relevant screenshots, stacktraces, log files, etc. to the ticket):

Incorrect OpenAPI schema definitions prevents usage of the documented create deployment endpoint.

Steps to reproduce (Required on creation):

Use the OpenAPI json to create a client
Use the the client to create deployment with two or more deployment resources.

Observed Behavior (Required on creation):

Only the last one resource gets deployed

Expected behavior (Required on creation):

All of the deployment resources are deploy as part of the same deployment

Root Cause (Required on prioritization):

Incorrect definition of MultiFormDeploymentDto OpenAPI schema definition and data property, it's a single string property at the moment with predefined name.

Solution Ideas (Optional):

Hints (Optional):

Is it possible to document the endpoint with the current specification?
- Ideally we need to define an array binary properties with undefined names
- Other alternatives?
Adjust the REST API implementation to avoid unique keys for the resource array. (check comments)

Workaround:

Modify the generated client code to correctly resolve the deployment resources.

Links:

is duplicated by https://jira.camunda.com/browse/CAM-13510

ThorbenLindhauer commented 3 years ago

This comment was imported from JIRA and written by user LaY5wB0

What is this name?

This pseudonym name was generated based on the user name in JIRA to protect the personal data of our JIRA users. You can use this identifier to search for issues by the same reporter.

Is there a timeline on this? Or a fix we can provide as a PR?

This is not a big issue to fix:

You just need to update the data property to:

"data": {
  "type": "array",
  "items": {
    "type": "string",
    "format": "binary",
    "nullable":  true
  },
  "description": "The binary data to create the deployment resource.\nIt is possible to have more than one form part with different form part names for the binary data to create a deployment.",
  "nullable":  true
}

(Can have other opinions about nullable=true being placed everywhere, but that is a another issue ;) )

ThorbenLindhauer commented 3 years ago

This comment was imported from JIRA and written by user @yanavasileva

LaY5wB0, the ticket is not scheduled yet. So far I am not aware of fix that will work for this endpoint. (More details: I created an issue in OpenAPI spec repo to ask details about our use case: https://github.com/OAI/OpenAPI-Specification/issues/2479. An alternative might be to rewrite the REST API request but we have not decided on doing this either.)

Regarding your suggestion adjusting the data type to an array, the issue will persist as only the last object will be deployed with the field name data, the rest will be overwritten. Here is an example of swagger ui when the suggestion is applied:

curl -X 'POST' \
  'http://localhost:8080/engine-rest/deployment/create' \
  -H 'accept: application/json' \
  -H 'Content-Type: multipart/form-data' \
  -F 'tenant-id=' \
  -F 'deployment-source=test' \
  -F 'deploy-changed-only=false' \
  -F 'enable-duplicate-filtering=false' \
  -F 'deployment-name=' \
  -F 'data=@diagram_1.bpmn' \
  -F 'data=@diagram_2.bpmn'

The issue is that the resources must be past in multi-form request, by uploading files with different field names. Please let us know if you are interested to work on this or find something else regarding the issue.

ThorbenLindhauer commented 3 years ago

This comment was imported from JIRA and written by user LaY5wB0

What is this name?

This pseudonym name was generated based on the user name in JIRA to protect the personal data of our JIRA users. You can use this identifier to search for issues by the same reporter.

That appears to be a implementation issue either in your generator or Camunda. Are you saying that the example: https://swagger.io/docs/specification/describing-request-body/file-upload/ under "Multiple File Upload" section is incorrect?

ThorbenLindhauer commented 3 years ago

This comment was imported from JIRA and written by user LaY5wB0

What is this name?

This pseudonym name was generated based on the user name in JIRA to protect the personal data of our JIRA users. You can use this identifier to search for issues by the same reporter.

@Yana This looks to be a Camunda implementation bug where it assumes that all keys will be unique:

Apply a breakpoint on this line and run your deployment curl from above.

https://github.com/camunda/camunda-bpm-platform/blob/744ca587552b13c1ceb20f88d8f5895fa60bbc76/engine-rest/engine-rest/src/main/java/org/camunda/bpm/engine/rest/mapper/MultipartFormData.java#L44

You can observe that the both files are passed into the MultipartFormData

There is no detection if the field is a file or text. Given camunda uses the file name as the unique identifier (it is what is shown in the API, fields, etc), If the field is a payload part is type file, then it should most likely be using the getFileName rather than getFieldName.

ThorbenLindhauer commented 3 years ago

This comment was imported from JIRA and written by user LaY5wB0

What is this name?

This pseudonym name was generated based on the user name in JIRA to protect the personal data of our JIRA users. You can use this identifier to search for issues by the same reporter.

Uploaded screenshot of debug showing both files are passed. It is the MultipartFormData class that is overwriting the data because it is not detecting that it is type file.

ThorbenLindhauer commented 3 years ago

This comment was imported from JIRA and written by user @yanavasileva

Hi LaY5wB0,

Thank you for analysis on this.

I agree that the issue could be handled with two approaches:

figure out how to document the OpenAPI by the spec with the current REST API implementation, or
make adjustments in the REST API implementation itself that will allow quick resolution on the OpenAPI documentation issue

The suggested change from getFieldName to getFileName for the deployment endpoint sounds good at first glance. However, the MultipartFormData DTO is used for other endpoints as well (e.g. variables, where the change will break functionality). With other words, to keep the impact of the change to a minimum the DTO for deployment endpoint will have to be extracted and separated. Leading to not so trivial change and in addition, we need to consider backwards compatibility.

As the create deployment endpoint is important part of the REST API and as OpenAPI documentation is mostly finished in 7.16, I will send the ticket for a decision. Further we will discuss the issue internally, how to go forward with a fix.

Please let us know if you would be interested in providing a fix, in case you have ideas for the OpenAPI part or you will be willing to work on adjustment of the REST API implementation.

Best regards, Yana

ThorbenLindhauer commented 3 years ago

This comment was imported from JIRA and written by user LaY5wB0

What is this name?

This pseudonym name was generated based on the user name in JIRA to protect the personal data of our JIRA users. You can use this identifier to search for issues by the same reporter.

Can you explain the breaking change you expect to occur? Can you link to a code line that would change in a breaking fashion?

ThorbenLindhauer commented 3 years ago

This comment was imported from JIRA and written by user @yanavasileva

LaY5wB0 Are you referring to my comment about variables?

ThorbenLindhauer commented 3 years ago

This comment was imported from JIRA and written by user LaY5wB0

What is this name?

This pseudonym name was generated based on the user name in JIRA to protect the personal data of our JIRA users. You can use this identifier to search for issues by the same reporter.

Yes. (Or any other endpoint that uses the dto, that you see as causing a breaking change)

ThorbenLindhauer commented 3 years ago

This comment was imported from JIRA and written by user LaY5wB0

What is this name?

This pseudonym name was generated based on the user name in JIRA to protect the personal data of our JIRA users. You can use this identifier to search for issues by the same reporter.

Hi.

Reviewing the various attachment endpoints, I am unclear where you expect a breaking change to occur. Providing a specific code line would be very helpful.

Example:

Task Attachment: https://docs.camunda.org/manual/7.15/reference/rest/task/attachment/post-task-attachment/

https://github.com/camunda/camunda-bpm-platform/blob/master/engine-rest/engine-rest/src/main/java/org/camunda/bpm/engine/rest/sub/task/impl/TaskAttachmentResourceImpl.java#L117

Task attachment uses different structure for file submissions and have the file names applied as part of the form params.

Additionally if you follow: https://github.com/camunda/camunda-bpm-platform/blob/master/engine-rest/engine-rest/src/main/java/org/camunda/bpm/engine/rest/sub/task/impl/TaskAttachmentResourceImpl.java#L144 into the cmd, it converts the content into a byte array, and the filename is provided as part of the form param "attachment-name" not the binary header.

https://github.com/camunda/camunda-bpm-platform/blob/master/engine/src/main/java/org/camunda/bpm/engine/impl/cmd/CreateAttachmentCmd.java#L107-L108

and

https://github.com/camunda/camunda-bpm-platform/blob/master/engine/src/main/java/org/camunda/bpm/engine/impl/cmd/CreateAttachmentCmd.java#L82

ThorbenLindhauer commented 3 years ago

This comment was imported from JIRA and written by user @yanavasileva

LaY5wB0 I will have a look again and double check, maybe I misread something the first time, and come back to you next week.

ThorbenLindhauer commented 3 years ago

This comment was imported from JIRA and written by user @yanavasileva

Hi LaY5wB0,

Here is one of the endpoints where the implementation will need to be adjusted further in case we decide to switch to formPart.getFileName() in MultipartFormData:

Setting a binary task variable with valueType=File, https://docs.camunda.org/manual/develop/reference/rest/task/variables/post-task-variable-binary/ MultipartFormData is used for resolving the valueType which is not a file input but a string. So the fileName will be null for such fields which will lead to errors with the current implementation in VariableValueDto#fromFormPart(). Test example: TaskVariableRestResourceInteractionTest#testPostSingleFileVariableWithMimeType

With above I am not applying that the code cannot be improved. I just want to make transparent that the suggested change will not be trivial and it will affect other endpoints if we do not consider all aspects.

I hope that brings more clarity on my previous comment.

Best regards, Yana

ThorbenLindhauer commented 3 years ago

This comment was imported from JIRA and written by user @tmetzke

Hi LaY5wB0,

I wanted to provide a current status report. We will consider this ticket for the next prioritization round. This does not yet mean we will work on this in the near future, it does however mean we see a certain priority for it. We will update here accordingly when we made a decision.

Best, Tobias

ThorbenLindhauer commented 2 years ago

This comment was imported from JIRA and written by user @toco-cam

rP62zaw Quick update: We will consider this in the planning for the 7.18 release

toco-cam commented 1 year ago

@ThorbenLindhauer as this is a bug - can you recommend how to schedule best?

camunda / camunda-bpm-platform