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

Using TempFiles breaks duplicateFiltering #34

Closed
ChrisSchoe opened this issue Aug 29, 2022 · 8 comments
Closed

Using TempFiles breaks duplicateFiltering #34

ChrisSchoe opened this issue Aug 29, 2022 · 8 comments

Comments

@ChrisSchoe
Copy link

Hey guys,
I love the idea of making it easier to use Camunda's rest API, especially for diagram deployment! However, this does not seem to work as intended at the moment:

TempFiles -> Random File Names -> Duplicate Check Broken

Using TempFiles to temporarily store the (e.g.) .bpmn-files right before uploading them leads to the flags dulicateFiltering/deployChangedOnly not working properly.
Explanation: Using TempFiles create files whose filename consists of the given prefix and suffix while also adding a (somewhat) unique random number at runtime. This number is then part of the filename and is different with every new startup of the application.

Unfortunately, this filename is then part of the resource being deployed via the rest api and then used to check whether or not the deployed diagram differs from the latest version. As these names will (virtually) always be different from the last deployment call (even if nothing has changed about the diagram itself), the API call will always create a new deployment despite deployChangedOnly being set (cf. ResourceManager::findLatestResourcesByDeploymentName within the Camunda RestApi code.)

A workaround

On possible way around this (while still using openapi generated clients) is to specify <library>resttemplate</library> as part of the ConfigOptions of the client generation, as these clients (also) accept ByteArrayInputStreams instead of files for multi part form parameters. Thus, files can be uploaded from within .jars without creating temp files.

A problem with the workaround and/or Camunda's open API spec

Using "resttemplate" as the client library carries its own problems, as the default serialization of the multipart form here leads to the boolean parameters deployChangedOnly and duplicateFiltering not being parsed correctly by the Rest API, thus necessitating additional steps to fix the serialization. (The OpenApi-Spec gives their type as 'boolean', which the opanepi generator interprets as Boolean. As this is a proper class, the values get serialized as application/json instead of text fields within the multipartform when using Spring Boot's Resttemplate with the default configuration. This is then not interpreted as intended by Camunda's Rest Api (cf. DeploymentRestServiceImpl::extractDuplicateFilteringForDeployment ), where only test values are read.)
Currently, Camuna's Openapi spec does not specify that this value should be encoded as a string (cf. https://github.com/camunda/camunda-bpm-platform/blob/master/engine-rest/engine-rest-openapi/src/main/templates/models/org/camunda/bpm/engine/rest/dto/MultiFormDeploymentDto.ftl ). The API reference itself is quite clear about all values having to be encoded as plain text. Perhaps it might be a good idea to add the encoding to the Camunda's Openapi spec ( cf. https://swagger.io/docs/specification/describing-request-body/multipart-requests/ )? :) I am not sure, though, whether the generated client would pick up on that.

@berndruecker
Copy link
Collaborator

Howdy @ChrisSchoe - great issue (I mean in terms of details :-)).

We could double check if we can somehow access the proper name of the file (which I am not sure about, that's why there are UUID#s -d3ddc3d) - or probably we could do some kind of naming schema, that makes sure the names are the same every time it gets deployed? Like simply increasing a number (1.bpmn, 2.bpmn)? This is still brittle - but should normally work if there are no changes...

Otherwise, we might want to create an issue to improve the OpenAPI spec?

WDYT?

@turyanitsa
Copy link
Contributor

can we use the checksum of a file to determine its name? In this case, if the file changes, there will be a new name, but if new bpmn are added, then this will not affect the name (unlike the serial number).
For example

val md = MessageDigest.getInstance("MD5")
val r = DigestInputStream(resource.inputStream, md)
Base64.getEncoder().encodeToString(md.digest()) + ".bpmn"
//1B2M2Y8AsgTpgAmY7PhCfg==.bpmn

This is an alternative solution with its pros and cons.

@berndruecker
Copy link
Collaborator

I like the idea of a checksum of the file content - this should give us a stable name while not relying on any information from the resource. @turyanitsa - would you be motivated to adjust your PR for this change by any chance?

@turyanitsa
Copy link
Contributor

@berndruecker , of course, I will do it with pleasure

@berndruecker
Copy link
Collaborator

@ChrisSchoe Do you agree that #39 closes this issue as resolved ?

@ChrisSchoe
Copy link
Author

Yeah,
That looks great. I like the checksum idea to replace unavailable names, it looks much more resilient than random strings, and only changes if actual changes occurred.
Good job implementing a fix this quickly, thanks!:)

Ps: once I'm back in the office next week, I might still create a corresponding issue on the open API spec as explained in my original post, but I'd consider this issue right here closed :)

@ChrisSchoe
Copy link
Author

Fixed by #39

@berndruecker
Copy link
Collaborator

Great - thx @ChrisSchoe and @turyanitsa

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants