Skip to content

Add TransformFunctionProvider to DynamicConfigTransformer#6881

Open
kkondaka wants to merge 5 commits into
opensearch-project:mainfrom
kkondaka:dyn-config-xfmr
Open

Add TransformFunctionProvider to DynamicConfigTransformer#6881
kkondaka wants to merge 5 commits into
opensearch-project:mainfrom
kkondaka:dyn-config-xfmr

Conversation

@kkondaka
Copy link
Copy Markdown
Collaborator

Description

This PR introduces a type-safe mechanism for dynamically invoking transformation functions during
pipeline template resolution. Previously, utility methods (e.g., calculateDepth, getAccountIdFromRole)
lived directly in DynamicConfigTransformer and were invoked reflectively on this. This was fragile,
untestable in isolation, and violated separation of concerns.

Changes
New API contracts (data-prepper-api):
• PipelineTransformFunctionProvider — marker interface that classes must implement to be valid function providers
• @TransformationFunction — method-level annotation marking methods as callable from template YAML FUNCTION_NAME placeholders

Externalized functions (data-prepper-plugins/aws-plugin):
• Created PipelineTransformFunctions with all 5 static utility methods (calculateDepth, calculateDepthForRdsSource, getSourceCoordinationIdentifierEnvVariable, getIncludePrefixForRdsSource, getAccountIdFromRole)
• Each method is public static, annotated with @TransformationFunction, and the class implements PipelineTransformFunctionProvider

Rule model changes (data-prepper-pipeline-parser):
• Added function_providers field to RuleTransformerModel (deserialized from rule YAML)
• Threaded functionProviders through RuleFileEvaluation → RuleEvaluatorResult → DynamicConfigTransformer
• invokeMethod() now validates:

  1. The class implements PipelineTransformFunctionProvider
  2. The method is annotated with @TransformationFunction
  3. Invokes statically via method.invoke(null, arg)

Rule YAML updates:
• Added function_providers to all production rule files (rds-rule.yaml, rds-joins-rule.yaml, mongodb-rule.yaml, documentdb-rule.yaml)

Issues Resolved

Resolves #[Issue number to be closed when this PR is merged]

Check List

  • [X ] New functionality includes testing.
  • New functionality has a documentation issue. Please link to it in this PR.
    • New functionality has javadoc added
  • [ X] Commits are signed with a real name per the DCO

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: Kondaka <krishkdk@amazon.com>
@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 22, 2026

✅ License Header Check Passed

All newly added files have proper license headers. Great work! 🎉

kkondaka added 3 commits May 22, 2026 08:16
Signed-off-by: Kondaka <krishkdk@amazon.com>
Signed-off-by: Kondaka <krishkdk@amazon.com>
Signed-off-by: Kondaka <krishkdk@amazon.com>
dependsOn jacocoTestReport
afterEvaluate {
classDirectories.setFrom(files(classDirectories.files.collect {
fileTree(dir: it, exclude: ['**/PipelineTransformFunctions.class'])
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why are we excluding this class? It should have code coverage.


private Class<?> resolveClassForMethod(final List<String> functionProviders, String methodName, Class<?> parameterType) throws ReflectiveOperationException {
if (functionProviders.size() == 1) {
return Class.forName(functionProviders.get(0));
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We should use this instead:

Class.forName(className, false, this.getClass().getClassLoader());

It will not run any static initializers. This let's us check that this is supposed to be a function.

for (final String functionProvider : functionProviders) {
try {
Class<?> candidate = Class.forName(functionProvider);
candidate.getMethod(methodName, parameterType);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

In this case, we get the method before checking on the interface. We should do the isAssignableFrom() check before getting the method.

Also, there is some code duplication since this code gets the method, then returns the Class and then the caller has to get the method again. I think just find the method here.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

isAssignableFrom() check is being done before getting method after doing resolveClassForMethod The class is needed to do isAssignableFrom() check and after that clazz.getMethod is done.


import java.util.Arrays;
import java.util.List;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Here are some test cases we want to have:

  1. Happy path — unit test that calls invokeMethod with a valid provider, annotated method, and verifies the correct return value. The only happy-path coverage is indirectly through the full transformation integration tests.
  2. Non-existent class name — test for Class.forName throwing ClassNotFoundException. What happens with function_providers: ["com.example.does.not.Exist"]?
  3. Multiple providers, correct resolution — test where the first provider lacks the method but the second has it, verifying it picks the right one.
  4. Non-static annotated method — the contract says methods must be public static, but there's no test that an instance method annotated with @TransformationFunction fails gracefully when method.invoke(null, arg) is called. That would throw an NPE or IllegalArgumentException from the reflection API, not a clear error message.
  5. Static initializer safety — test proving that a malicious static initializer doesn't run if the class fails the interface check (this would fail until my other comment on using a different forName is applied).

private String pluginName;

@JsonProperty("function_providers")
private List<String> functionProviders;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I wonder if we should have a special requirement on the package name as additional protection. For example, the package name must contain dataprepper_transformer. This would place a requirement for packages like:

  • org.opensearch.dataprepper.documentdb.dataprepper_transformer.MyTransformer
  • com.mycompany.myplugin.dataprepper_transformer.MyTransformer

Signed-off-by: Kondaka <krishkdk@amazon.com>
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

Successfully merging this pull request may close these issues.

2 participants