Add TransformFunctionProvider to DynamicConfigTransformer#6881
Add TransformFunctionProvider to DynamicConfigTransformer#6881kkondaka wants to merge 5 commits into
Conversation
Signed-off-by: Kondaka <krishkdk@amazon.com>
✅ License Header Check PassedAll newly added files have proper license headers. Great work! 🎉 |
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']) |
There was a problem hiding this comment.
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)); |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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; | ||
|
|
There was a problem hiding this comment.
Here are some test cases we want to have:
- Happy path — unit test that calls
invokeMethodwith a valid provider, annotated method, and verifies the correct return value. The only happy-path coverage is indirectly through the full transformation integration tests. - Non-existent class name — test for
Class.forNamethrowing ClassNotFoundException. What happens with function_providers: ["com.example.does.not.Exist"]? - Multiple providers, correct resolution — test where the first provider lacks the method but the second has it, verifying it picks the right one.
- 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. - 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
forNameis applied).
| private String pluginName; | ||
|
|
||
| @JsonProperty("function_providers") | ||
| private List<String> functionProviders; |
There was a problem hiding this comment.
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.MyTransformercom.mycompany.myplugin.dataprepper_transformer.MyTransformer
Signed-off-by: Kondaka <krishkdk@amazon.com>
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:
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
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.