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

feat(document): intrinsic operations #4128

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

Conversation

chillleader
Copy link
Member

@chillleader chillleader commented Feb 25, 2025

Description

For an introduction into intrinsic operations concept, please see #3843

This PR adds intrinsic operation support. It consists of two parts:

  • New operation structure (new IntrinsicOperation, IntrinsicOperationExecutor and IntrinsicOperationProvider interfaces with stricter input/output typing and hopefully a better developer experience)
  • Refactored the Jackson module to have a better structure and to support operation deserialization

Use-cases considered in the Jackson module:

  • Operation can output strings
  • Operation can output documents which can be consumed in any way the document module allows (e.g. inputstream/bytearray)
  • Operation can output a generic object
  • Nested operations are also supported without any extra effort as we are reusing the same object mapper for parameter binding

Related issues

closes #3843
related https://github.com/camunda/team-connectors/issues/1004

Checklist

  • PR has a milestone or the no milestone label.

@chillleader chillleader added this to the 8.7.0 milestone Feb 25, 2025
@chillleader chillleader self-assigned this Feb 25, 2025
@chillleader chillleader requested a review from a team as a code owner February 25, 2025 10:37
@chillleader chillleader force-pushed the intrinsic-operations branch from 603b7ca to a3f95cf Compare March 5, 2025 22:42
@chillleader chillleader requested review from Copilot, a team and sbuettner March 6, 2025 09:30
@chillleader chillleader changed the title feat(document): intrinsic operations choredocument): intrinsic operations Mar 6, 2025
@chillleader chillleader changed the title choredocument): intrinsic operations chore(document): intrinsic operations Mar 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Overview

This pull request introduces intrinsic operation support by adding new operation interfaces and refactoring the Jackson module to better support operation deserialization and improve the developer experience.

  • Added new intrinsic operation interfaces (IntrinsicOperation, IntrinsicOperationExecutor, and IntrinsicOperationProvider) with stricter input/output typing.
  • Implemented new operations, including Base64 and CreateLink, and updated supporting code such as parameter binding and document link generation.
  • Updated test and factory files along with minor refactorings in document store implementations.

Reviewed Changes

File Description
connector-sdk/document/src/main/java/io/camunda/operation/impl/Base64Operation.java Introduces a base64 conversion operation with proper handling of Document and String inputs.
connector-sdk/document/src/main/java/io/camunda/operation/IntrinsicOperation.java Adds the new annotation interface to mark intrinsic operations.
connector-sdk/document/src/main/java/io/camunda/operation/IntrinsicOperationParameterBinder.java Implements parameter binding for intrinsic operations with improved type conversion and error reporting.
connector-sdk/document/src/main/java/io/camunda/operation/IntrinsicOperationProvider.java Defines the marker interface for intrinsic operation providers.
connector-sdk/document/src/main/java/io/camunda/operation/DefaultIntrinsicOperationExecutor.java Provides an executor that discovers and executes intrinsic operations, including duplicate operation detection.
connector-sdk/document/src/main/java/io/camunda/operation/impl/CreateLinkOperation.java Adds support for the link creation intrinsic operation with optional TTL.
connector-sdk/document/src/main/java/io/camunda/operation/IntrinsicOperationParams.java Updates the parameter structure to use a sealed interface supporting positional parameters.
connector-sdk/document/src/main/java/io/camunda/document/CamundaDocument.java Enhances the Document implementation with new generateLink methods.
connector-sdk/document/src/main/java/io/camunda/document/store/* Refactors document store implementations to streamline link generation and content retrieval.
connector-sdk/core/src/test/java/io/camunda/connector/api/json/* Adjusts tests to reflect changes in JSON deserialization and updated model packages.
connector-runtime/spring-boot-starter-camunda-connectors/src/main/java/io/camunda/connector/runtime/OutboundConnectorsAutoConfiguration.java Updates the Jackson module imports to the new package structure.

Copilot reviewed 60 out of 60 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (3)

connector-sdk/document/src/main/java/io/camunda/operation/DefaultIntrinsicOperationExecutor.java:63

  • [nitpick] Consider including method names and additional context in the error message for duplicated operations to improve debugging clarity.
throw new IllegalArgumentException("Operation with name: " + entry.getKey() + " duplicated in providers: " + provider.getClass().getName() + " and " + operationSources.get(entry.getKey()).provider.getClass().getName());

connector-sdk/document/src/main/java/io/camunda/operation/IntrinsicOperationParameterBinder.java:60

  • [nitpick] Consider including the expected parameter type in the error message to aid in debugging parameter binding issues.
throw new IllegalArgumentException("Parameter at index " + index + " is required but not provided");

connector-sdk/document/src/main/java/io/camunda/operation/impl/CreateLinkOperation.java:28

  • [nitpick] Consider renaming the parameter 'ttl' to 'timeToLive' for consistency with other parts of the codebase.
public String execute(Document document, @Nullable Duration ttl) {
Comment on lines 27 to 36
public String execute(Object input) {
if (input instanceof Document) {
return ((Document) input).asBase64();
}
if (input instanceof String) {
return Base64.getEncoder().encodeToString(((String) input).getBytes());
}
throw new IllegalArgumentException(
"Unsupported input type: " + input.getClass() + ". Expected Document or String.");
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Super nice and concise. Could be even nicer with a switch statement :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Connector SDK is still on Java 17 😢

Copy link
Contributor

@ztefanie ztefanie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good work, nice feature 🚀

Why is the prefixed "chore" and not "feature"?

I tried to test this, but couldn't get it working. I used camunda platform connectors-8.7.0-alpha2. I did a maven clean install in connectors. I used desktop modeler with this BPMN:
image
but the issue that is created has not description:

@chillleader
Copy link
Member Author

Why is the prefixed "chore" and not "feature"?

Right, let's change it back to feat, I initially decided to make it a chore because I was considering to split the implementation in several PRs and only get a QA approval for the last one. However, we added more functions in this PR so it makes sense to call it a feature and require QA approval.

I tried to test this, but couldn't get it working. I used camunda platform connectors-8.7.0-alpha2. I did a maven clean
install in connectors. I used desktop modeler with this BPMN:

This would not work as we decided not to implement the FEEL part yet. For now, if you want to invoke an operation, you need to provide the whole payload in the input, i.e.:

{
  
  "camunda.operation.type": "base64",
  "params": ["test123"]
}

@chillleader chillleader changed the title chore(document): intrinsic operations feat(document): intrinsic operations Mar 10, 2025
@chillleader chillleader added the qa:required Will trigger the QA workflow label Mar 10, 2025
@johnBgood
Copy link
Collaborator

QA information

Test Environment

  • Test required in (you can check both):
    • SaaS
    • SM
  • Create a new test for this feature in our Pre Release Tests for @Szik to be able to test

Test Scope

Please describe the test scope, happy path, edge cases that come to your mind, and whatever you think might relevant to test

Test Data

Please provide the test data, if needed (files, URLs, code snippets, FEEL expressions)

@chillleader chillleader force-pushed the intrinsic-operations branch from 56d943c to 33e1eeb Compare March 10, 2025 09:45
@chillleader
Copy link
Member Author

@ztefanie it should work now, just a heads up that I also just changed the discriminator field name from operation to function so the payload should look like this now:

{
  
  "camunda.function.type": "base64",
  "params": ["test123"]
}

Copy link
Contributor

🚀 Deployment Results 🚀

Please find below the results of the latest deployments.

connectors-intrinsic-opera-c8sm

Typical Errors
--------------
- ErrImagePull / ImagePullBackOff - The CI hasn't finished building the image yet or the image build has failed
  - Check whether the build is still in progress
    - If it is then increase "argocd_wait_for_sync_timeout"
    - If the build has failed then check the CI for the possible cause of the error
- Readiness / Liveness probe failed - The container is not fully functional yet
  - There could be many causes for this error, so there's no a single good solution for the problem
    - The best generic approach is to check the ArgoCD App for the underlying problems using the link above
      - Click on the problematic component and investigate "EVENTS" and "LOGS" tabs for clues
    - It could also be simply happening because, although everything seems to be correct, the app is not fully functional yet
      - In this case simply increase "argocd_wait_for_sync_timeout" value
  • Details for Troubleshooting: 🛟
Kubernetes events
-----------------

🔧 Troubleshooting 🔧

The 🔗 ArgoCD link can be used to check the state and configuration of all the services deployed as part of the preview environments and get logs.

In case of error, please check 📋 Deployment Jobs and 🔗 ArgoCD to debug, and check our (yet to come) troubleshooting page

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport release/8.7 qa:required Will trigger the QA workflow
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Intrinsic operations in the Connector SDK
5 participants