Template parts handling approach change: expression level instead of LazyParameter level #7174
Conversation
WalkthroughThis pull request introduces enhancements to the Nussknacker framework, focusing on template evaluation and rendering. A new Changes
Possibly related PRs
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
c4f0826 to
18a8ede
Compare
- Checking language is speltemplate in one place, removed exceptions throwing - Changed naming for parts classes to easier distinguish between two similar classes hierarchy - TemplateLazyParameter.parts instead of TemplateLazyParameter.templateExpression.parts - Test for flink: changed from sink to custom node
18a8ede to
9d5258c
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (10)
components-api/src/main/scala/pl/touk/nussknacker/engine/api/TemplateEvaluationResult.scala (1)
7-9: Add documentation to clarify trait's contractThe trait would benefit from ScalaDoc explaining:
- Its purpose in template rendering
- Contract requirements for implementations (e.g., non-null values)
- Relationship with TemplateEvaluationResult
+/** + * Represents a part of a rendered template. + * Implementations must ensure that `value` is never null. + * Used by TemplateEvaluationResult to compose the final rendered string. + */ sealed trait TemplateRenderedPart { def value: String }engine/flink/management/dev-model/src/main/scala/pl/touk/nussknacker/engine/management/sample/LoggingService.scala (1)
36-36: Consider adding null check for evaluation result.The direct access to
renderedTemplateassumes the evaluation result is never null. Consider adding a null check or using Scala'sOptionto handle potential null values more safely.- val msg = message.evaluate(context).renderedTemplate + val msg = Option(message.evaluate(context)) + .map(_.renderedTemplate) + .getOrElse("[Empty template result]")engine/flink/components/base-tests/src/test/scala/pl/touk/nussknacker/engine/flink/SpelTemplateLazyParameterTest.scala (2)
38-58: Consider adding more test cases for edge scenarios.While the current test case verifies the basic functionality, consider adding tests for:
- Empty template
- Template with only literals
- Template with only expressions
- Invalid expressions
- Error handling scenarios
Example test case:
test("should handle empty template") { val scenario = ScenarioBuilder .streaming("test") .source("source", TestScenarioRunner.testDataSource) .customNode( "custom", "output", "spelTemplatePartsCustomTransformer", "template" -> Expression.spelTemplate("") ) .emptySink("sink", TestScenarioRunner.testResultSink, "value" -> "#output".spel) val result = runner.runWithData(scenario, List(1), Boundedness.BOUNDED) result.validValue.errors shouldBe empty result.validValue.successes shouldBe List("") }
93-95: Consider documenting possible evaluation errors.The lazy parameter evaluation could fail in various ways. Consider adding documentation about possible error scenarios and how they are handled.
scenario-compiler/src/test/scala/pl/touk/nussknacker/engine/InterpreterSpec.scala (1)
1027-1071: LGTM: Comprehensive test coverage for template rendering.The new test case effectively covers various template rendering scenarios:
- Combined literal and expression templates
- Single literal templates
- Single expression templates
- Empty templates
The table-driven approach makes it easy to add more test cases in the future.
Consider adding these additional test cases to improve coverage:
- Multiple expressions:
Hello#{#input.msisdn}#{#input.accountId}- Escaped expressions:
Hello ##{#input.msisdn}- Invalid expressions to verify error handling
( "multiple expressions", s"Hello#{#input.msisdn}#{#input.accountId}", Transaction(msisdn = "foo", accountId = "123"), "[Hello]-literal[foo]-subexpression[123]-subexpression" ), ( "escaped expression", s"Hello ##{#input.msisdn}", Transaction(msisdn = "foo"), "[Hello #]-literal[foo]-subexpression" )docs/MigrationGuide.md (2)
47-49: Documentation for TemplateEvaluationResult change needs more detailsThe documentation for the change introduced in PR #7162 should be expanded to include:
- Example of how to access the rendered template
- Impact on existing code that expects String
- Migration steps for components using these parameter editors
Consider adding this clarifying example:
// Before def message: String = parameterValue // After def message: String = parameterValue.renderedTemplate
Line range hint
1-1000: Consider improving the documentation structureThe migration guide would be more maintainable and easier to navigate with these improvements:
- Add a table of contents at the top
- Group changes into consistent categories across versions:
- API Changes
- Configuration Changes
- Breaking Changes
- Deprecations
- Use consistent formatting for version headers
Consider this structure for version headers:
# Migration Guide ## Table of Contents - [Version 1.18.0](#version-1180) - [Version 1.17.0](#version-1170) ... ## Version 1.18.0 ### API Changes ... ### Configuration Changes ... ### Breaking Changes ...scenario-compiler/src/test/scala/pl/touk/nussknacker/engine/testcomponents/SpelTemplatePartsService.scala (2)
28-33: Consider Adding Validators tospelTemplateParameterThe
spelTemplateParameteris defined without any validators. If there are expectations or constraints on the input (e.g., non-empty strings, specific formats), adding validators will help catch issues at validation time and provide clearer feedback to users.
35-35: Redundant Type Override ofStateThe override of
type State = Anymay be unnecessary ifAnyis the default. Removing this line can simplify the code without affecting functionality.scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/spel/SpelExpression.scala (1)
127-129: Use a more specific exception thanIllegalArgumentExceptionfor unsupported expression typesThrowing a generic
IllegalArgumentException(lines 127-129) may not provide sufficient context for error handling. Consider defining a custom exception or using a more specific exception type to improve clarity and aid in debugging when an unsupported expression type is encountered.Apply this diff to define a custom exception and use it:
+ class UnsupportedExpressionTypeException(message: String) extends RuntimeException(message) ... case other => - throw new IllegalArgumentException( + throw new UnsupportedExpressionTypeException( s"Unsupported expression type: ${other.getClass.getName} for a template expression" )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
components-api/src/main/scala/pl/touk/nussknacker/engine/api/TemplateEvaluationResult.scala(1 hunks)docs/MigrationGuide.md(1 hunks)engine/flink/components/base-tests/src/test/scala/pl/touk/nussknacker/engine/flink/SpelTemplateLazyParameterTest.scala(1 hunks)engine/flink/management/dev-model/src/main/scala/pl/touk/nussknacker/engine/management/sample/LoggingService.scala(2 hunks)scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/definition/component/methodbased/MethodDefinition.scala(1 hunks)scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/spel/SpelExpression.scala(2 hunks)scenario-compiler/src/test/scala/pl/touk/nussknacker/engine/InterpreterSpec.scala(7 hunks)scenario-compiler/src/test/scala/pl/touk/nussknacker/engine/testcomponents/SpelTemplatePartsService.scala(1 hunks)
🔇 Additional comments (9)
components-api/src/main/scala/pl/touk/nussknacker/engine/api/TemplateEvaluationResult.scala (1)
1-15: Verify integration with SpEL template language checks
The PR objectives mention consolidating "speltemplate" language checks. Let's verify how this new structure is used in that context.
✅ Verification successful
Based on the results, I can generate the final response as we have sufficient evidence about the integration of the new template structure.
Template evaluation structure is properly integrated with SpEL template handling
The new TemplateEvaluationResult structure is well integrated across the codebase:
- Core SpEL template handling in
SpelExpression.scalauses the new structure for template rendering - Test components like
SpelTemplatePartsServiceandSpelTemplateLazyParameterTestvalidate the template evaluation - Migration guide properly documents the change from
StringtoTemplateEvaluationResult - No scattered exception handling was found, suggesting consolidated checks
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for SpEL template language checks that should now use this structure
rg -l "speltemplate" | xargs rg "TemplateEvaluationResult|TemplateRenderedPart"
# Look for any remaining scattered language checks that should be consolidated
rg -l "speltemplate" | xargs rg "throw.*Exception.*speltemplate"
Length of output: 3689
engine/flink/management/dev-model/src/main/scala/pl/touk/nussknacker/engine/management/sample/LoggingService.scala (1)
21-23: Verify template editor compatibility with new parameter type.
The change from String to TemplateEvaluationResult represents a significant architectural shift in template handling. While this aligns with the PR's objective of consolidating template evaluation, we should ensure this change is consistently applied across the codebase.
✅ Verification successful
Let me gather more information about template evaluation usage to make a more informed decision.
Based on the gathered information, I can now provide the final response:
Template evaluation change is part of a documented API migration
The change from String to TemplateEvaluationResult is a deliberate and documented API change that affects all components using SpelTemplateParameterEditor. This is consistently implemented across the codebase and is documented in the migration guide (MigrationGuide.md). The change is properly handled by the editor system which maps SimpleEditorType.SPEL_TEMPLATE_EDITOR to the appropriate parameter editor.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify consistent usage of SPEL_TEMPLATE_EDITOR with TemplateEvaluationResult
# Check for other occurrences of SPEL_TEMPLATE_EDITOR to ensure consistent type usage
rg -A 3 "SimpleEditorType.SPEL_TEMPLATE_EDITOR"
# Check for other parameters using TemplateEvaluationResult
ast-grep --pattern 'LazyParameter[TemplateEvaluationResult]'
Length of output: 1536
Script:
#!/bin/bash
# Check for all usages of TemplateEvaluationResult to understand the pattern
rg -A 3 "TemplateEvaluationResult"
# Check for other SimpleEditor annotations to see parameter patterns
ast-grep --pattern '@SimpleEditor($$$) $_ : LazyParameter[$_]'
# Look for SpelTemplateParameterEditor usage
rg -A 5 "SpelTemplateParameterEditor"
Length of output: 16068
scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/definition/component/methodbased/MethodDefinition.scala (1)
68-68: LGTM! Improved error message clarity.
The change to use param.name.value provides a more explicit error message by displaying the actual parameter name value instead of the wrapper object representation. This enhancement makes debugging missing parameters more straightforward.
engine/flink/components/base-tests/src/test/scala/pl/touk/nussknacker/engine/flink/SpelTemplateLazyParameterTest.scala (3)
1-27: LGTM! Imports are well-organized and appropriate.
The imports cover all necessary dependencies for Flink streaming, testing frameworks, and Nussknacker components.
102-111: LGTM! Error handling is properly implemented.
The error handling using collectHandlingErrors is a good practice, ensuring that errors are properly collected and don't crash the pipeline.
28-37: LGTM! Test setup is well-configured.
The test runner is properly configured with:
- Batch execution mode for bounded test data
- Lazy initialization for better performance
- Registration of the custom transformer component
Let's verify that the test component is properly registered in other test files:
✅ Verification successful
Test component registration is correctly isolated
The SpelTemplatePartsCustomTransformer is a test-specific component defined and used only within the SpelTemplateLazyParameterTest file. This is the expected behavior as it's a custom transformer specifically created to test the SpEL template functionality. No other test files need to register this component.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other test files using the same component
rg -l "spelTemplatePartsCustomTransformer" --type scala
Length of output: 174
Script:
#!/bin/bash
# Let's check if this component is defined in the codebase
rg -l "SpelTemplatePartsCustomTransformer" --type scala
# Also check the implementation to understand if it's a test-only component
ast-grep --pattern 'object SpelTemplatePartsCustomTransformer'
Length of output: 9878
scenario-compiler/src/test/scala/pl/touk/nussknacker/engine/InterpreterSpec.scala (2)
9-10: LGTM: Good use of ScalaTest's table-driven testing.
The addition of table-driven testing imports enhances the test structure by allowing multiple test cases to be defined concisely.
Line range hint 1187-1204: LGTM: Improved naming and type safety in ServiceUsingSpelTemplate.
The changes improve clarity and type safety:
- Better parameter naming with
spelTemplateParameterName - Proper handling of template evaluation results
scenario-compiler/src/test/scala/pl/touk/nussknacker/engine/testcomponents/SpelTemplatePartsService.scala (1)
66-68: Verify Exhaustiveness of Pattern Matching over TemplateRenderedPart
In the pattern matching over templateResult.renderedParts, you handle RenderedLiteral and RenderedSubExpression. If TemplateRenderedPart is a sealed trait with only these two subclasses, the pattern matching is exhaustive. However, if there is a possibility of additional subclasses now or in the future, consider adding a wildcard case to handle any unforeseen subclasses.
To confirm, you can verify if TemplateRenderedPart has only these two cases:
| case class TemplateEvaluationResult(renderedParts: List[TemplateRenderedPart]) { | ||
| def renderedTemplate: String = renderedParts.map(_.value).mkString("") | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation and optimize string concatenation
The implementation is clean but could benefit from some improvements:
- Add validation for null/empty list
- Consider using StringBuilder for better performance with large lists
case class TemplateEvaluationResult(renderedParts: List[TemplateRenderedPart]) {
- def renderedTemplate: String = renderedParts.map(_.value).mkString("")
+ require(renderedParts != null, "renderedParts cannot be null")
+
+ def renderedTemplate: String = {
+ if (renderedParts.isEmpty) ""
+ else {
+ val builder = new StringBuilder(renderedParts.head.value)
+ renderedParts.tail.foreach(part => builder.append(part.value))
+ builder.toString
+ }
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| case class TemplateEvaluationResult(renderedParts: List[TemplateRenderedPart]) { | |
| def renderedTemplate: String = renderedParts.map(_.value).mkString("") | |
| } | |
| case class TemplateEvaluationResult(renderedParts: List[TemplateRenderedPart]) { | |
| require(renderedParts != null, "renderedParts cannot be null") | |
| def renderedTemplate: String = { | |
| if (renderedParts.isEmpty) "" | |
| else { | |
| val builder = new StringBuilder(renderedParts.head.value) | |
| renderedParts.tail.foreach(part => builder.append(part.value)) | |
| builder.toString | |
| } | |
| } | |
| } |
| object TemplateRenderedPart { | ||
| case class RenderedLiteral(value: String) extends TemplateRenderedPart | ||
|
|
||
| case class RenderedSubExpression(value: String) extends TemplateRenderedPart | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add documentation and validation for rendered part types
The case classes need:
- Documentation explaining their distinct purposes
- Validation for non-null values
- Examples of typical usage
object TemplateRenderedPart {
+ /**
+ * Represents a literal string part of the template that should be rendered as-is.
+ * @param value The literal string value (must not be null)
+ */
- case class RenderedLiteral(value: String) extends TemplateRenderedPart
+ case class RenderedLiteral(value: String) extends TemplateRenderedPart {
+ require(value != null, "value cannot be null")
+ }
+ /**
+ * Represents a dynamically evaluated expression part of the template.
+ * @param value The result of expression evaluation (must not be null)
+ */
- case class RenderedSubExpression(value: String) extends TemplateRenderedPart
+ case class RenderedSubExpression(value: String) extends TemplateRenderedPart {
+ require(value != null, "value cannot be null")
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| object TemplateRenderedPart { | |
| case class RenderedLiteral(value: String) extends TemplateRenderedPart | |
| case class RenderedSubExpression(value: String) extends TemplateRenderedPart | |
| } | |
| object TemplateRenderedPart { | |
| /** | |
| * Represents a literal string part of the template that should be rendered as-is. | |
| * @param value The literal string value (must not be null) | |
| */ | |
| case class RenderedLiteral(value: String) extends TemplateRenderedPart { | |
| require(value != null, "value cannot be null") | |
| } | |
| /** | |
| * Represents a dynamically evaluated expression part of the template. | |
| * @param value The result of expression evaluation (must not be null) | |
| */ | |
| case class RenderedSubExpression(value: String) extends TemplateRenderedPart { | |
| require(value != null, "value cannot be null") | |
| } | |
| } |
There was a problem hiding this comment.
In Scala, we idiomatically assume that null can't be passed anywhere. Thanks to that we don't need to add such assertions and the code is less noisy. Please don't add such suggestions in the review anymore.
There was a problem hiding this comment.
@arkadius, understood. I will avoid suggesting null value assertions in Scala code in future reviews, keeping in mind the idiomatic practices. Thank you for the clarification.
✏️ Learnings added
Learnt from: arkadius
PR: TouK/nussknacker#7174
File: components-api/src/main/scala/pl/touk/nussknacker/engine/api/TemplateEvaluationResult.scala:11-15
Timestamp: 2024-11-19T21:55:21.536Z
Learning: In Scala code, avoid suggesting null value assertions (e.g., using `require(value != null)`) because it's idiomatic to assume that null values are not passed, and adding such assertions makes the code less noisy.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| } | ||
| }, | ||
| flinkCustomNodeContext.valueWithContextInfo.forClass[String] | ||
| ).asInstanceOf[DataStream[ValueWithContext[AnyRef]]] |
There was a problem hiding this comment.
Avoid unsafe type casting.
The asInstanceOf cast at line 114 could be unsafe. Consider using a type-safe approach or adding runtime type checking.
Example safer approach:
// Option 1: Use pattern matching with type checking
.map {
case value: DataStream[ValueWithContext[AnyRef]] => value
case other => throw new IllegalStateException(s"Unexpected type: ${other.getClass}")
}
// Option 2: Use type parameters to ensure type safety at compile time
def implementation[T <: AnyRef](...): FlinkCustomStreamTransformation = {
...
}| override def contextTransformation(context: ValidationContext, dependencies: List[NodeDependencyValue])( | ||
| implicit nodeId: NodeId | ||
| ): SpelTemplatePartsService.ContextTransformationDefinition = { | ||
| case TransformationStep(Nil, _) => NextParameters(List(spelTemplateParameter)) | ||
| case TransformationStep((`spelTemplateParameterName`, DefinedLazyParameter(_)) :: Nil, _) => | ||
| FinalResults.forValidation(context, List.empty)(validation = | ||
| ctx => | ||
| ctx.withVariable( | ||
| OutputVariableNameDependency.extract(dependencies), | ||
| typing.Typed[String], | ||
| Some(ParameterName(OutputVar.VariableFieldName)) | ||
| ) | ||
| ) | ||
| } |
There was a problem hiding this comment.
Handle Unexpected Cases in contextTransformation Method
The contextTransformation method currently handles two specific cases but does not account for unexpected inputs. This could lead to a MatchError at runtime if an unhandled case arises. Consider adding a default case to handle any unforeseen inputs gracefully.
Apply this diff to handle unexpected cases:
override def contextTransformation(context: ValidationContext, dependencies: List[NodeDependencyValue])(
implicit nodeId: NodeId
): SpelTemplatePartsService.ContextTransformationDefinition = {
case TransformationStep(Nil, _) => NextParameters(List(spelTemplateParameter))
case TransformationStep((`spelTemplateParameterName`, DefinedLazyParameter(_)) :: Nil, _) =>
FinalResults.forValidation(context, List.empty)(validation =
ctx =>
ctx.withVariable(
OutputVariableNameDependency.extract(dependencies),
typing.Typed[String],
Some(ParameterName(OutputVar.VariableFieldName))
)
)
+ case _ =>
+ FinalResults.forValidation(context, List.empty)(validation = ctx => ctx)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| override def contextTransformation(context: ValidationContext, dependencies: List[NodeDependencyValue])( | |
| implicit nodeId: NodeId | |
| ): SpelTemplatePartsService.ContextTransformationDefinition = { | |
| case TransformationStep(Nil, _) => NextParameters(List(spelTemplateParameter)) | |
| case TransformationStep((`spelTemplateParameterName`, DefinedLazyParameter(_)) :: Nil, _) => | |
| FinalResults.forValidation(context, List.empty)(validation = | |
| ctx => | |
| ctx.withVariable( | |
| OutputVariableNameDependency.extract(dependencies), | |
| typing.Typed[String], | |
| Some(ParameterName(OutputVar.VariableFieldName)) | |
| ) | |
| ) | |
| } | |
| override def contextTransformation(context: ValidationContext, dependencies: List[NodeDependencyValue])( | |
| implicit nodeId: NodeId | |
| ): SpelTemplatePartsService.ContextTransformationDefinition = { | |
| case TransformationStep(Nil, _) => NextParameters(List(spelTemplateParameter)) | |
| case TransformationStep((`spelTemplateParameterName`, DefinedLazyParameter(_)) :: Nil, _) => | |
| FinalResults.forValidation(context, List.empty)(validation = | |
| ctx => | |
| ctx.withVariable( | |
| OutputVariableNameDependency.extract(dependencies), | |
| typing.Typed[String], | |
| Some(ParameterName(OutputVar.VariableFieldName)) | |
| ) | |
| ) | |
| case _ => | |
| FinalResults.forValidation(context, List.empty)(validation = ctx => ctx) | |
| } |
| override def implementation( | ||
| params: Params, | ||
| dependencies: List[NodeDependencyValue], | ||
| finalState: Option[Any] | ||
| ): ServiceInvoker = new ServiceInvoker { | ||
|
|
||
| override def invoke(context: Context)( | ||
| implicit ec: ExecutionContext, | ||
| collector: InvocationCollectors.ServiceInvocationCollector, | ||
| componentUseCase: ComponentUseCase | ||
| ): Future[Any] = { | ||
| val templateResult = | ||
| params.extractOrEvaluateLazyParamUnsafe[TemplateEvaluationResult](spelTemplateParameterName, context) | ||
| val result = templateResult.renderedParts.map { | ||
| case RenderedLiteral(value) => s"[$value]-literal" | ||
| case RenderedSubExpression(value) => s"[$value]-subexpression" | ||
| }.mkString | ||
| Future.successful(result) | ||
| } | ||
|
|
||
| } |
There was a problem hiding this comment.
Handle Potential Exceptions from Unsafe Parameter Extraction
The use of extractOrEvaluateLazyParamUnsafe can lead to unhandled exceptions if parameter extraction or evaluation fails. To improve the robustness of the invoke method, consider using the safe version extractOrEvaluateLazyParam and handling the possible failures appropriately.
Apply this diff to handle potential failures:
override def invoke(context: Context)(
implicit ec: ExecutionContext,
collector: InvocationCollectors.ServiceInvocationCollector,
componentUseCase: ComponentUseCase
): Future[Any] = {
- val templateResult =
- params.extractOrEvaluateLazyParamUnsafe[TemplateEvaluationResult](spelTemplateParameterName, context)
+ params.extractOrEvaluateLazyParam[TemplateEvaluationResult](spelTemplateParameterName, context) match {
+ case Success(templateResult) =>
+ val result = templateResult.renderedParts.map {
+ case RenderedLiteral(value) => s"[$value]-literal"
+ case RenderedSubExpression(value) => s"[$value]-subexpression"
+ }.mkString
+ Future.successful(result)
+ case Failure(exception) =>
+ Future.failed(exception)
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| override def implementation( | |
| params: Params, | |
| dependencies: List[NodeDependencyValue], | |
| finalState: Option[Any] | |
| ): ServiceInvoker = new ServiceInvoker { | |
| override def invoke(context: Context)( | |
| implicit ec: ExecutionContext, | |
| collector: InvocationCollectors.ServiceInvocationCollector, | |
| componentUseCase: ComponentUseCase | |
| ): Future[Any] = { | |
| val templateResult = | |
| params.extractOrEvaluateLazyParamUnsafe[TemplateEvaluationResult](spelTemplateParameterName, context) | |
| val result = templateResult.renderedParts.map { | |
| case RenderedLiteral(value) => s"[$value]-literal" | |
| case RenderedSubExpression(value) => s"[$value]-subexpression" | |
| }.mkString | |
| Future.successful(result) | |
| } | |
| } | |
| override def implementation( | |
| params: Params, | |
| dependencies: List[NodeDependencyValue], | |
| finalState: Option[Any] | |
| ): ServiceInvoker = new ServiceInvoker { | |
| override def invoke(context: Context)( | |
| implicit ec: ExecutionContext, | |
| collector: InvocationCollectors.ServiceInvocationCollector, | |
| componentUseCase: ComponentUseCase | |
| ): Future[Any] = { | |
| params.extractOrEvaluateLazyParam[TemplateEvaluationResult](spelTemplateParameterName, context) match { | |
| case Success(templateResult) => | |
| val result = templateResult.renderedParts.map { | |
| case RenderedLiteral(value) => s"[$value]-literal" | |
| case RenderedSubExpression(value) => s"[$value]-subexpression" | |
| }.mkString | |
| Future.successful(result) | |
| case Failure(exception) => | |
| Future.failed(exception) | |
| } | |
| } | |
| } |
There was a problem hiding this comment.
Unsafe methods are shortcuts that do this pattern matching on our own without adding a noise. Please don't add such suggestions in the review anymore
There was a problem hiding this comment.
@arkadius, understood. I will refrain from adding such suggestions in future reviews.
✏️ Learnings added
Learnt from: arkadius
PR: TouK/nussknacker#7174
File: scenario-compiler/src/test/scala/pl/touk/nussknacker/engine/testcomponents/SpelTemplatePartsService.scala:52-72
Timestamp: 2024-11-19T21:57:24.665Z
Learning: In the Nussknacker project, it's acceptable to use unsafe methods like `extractOrEvaluateLazyParamUnsafe` in Scala code, as they serve as shortcuts and reduce unnecessary noise. Suggestions to replace them with safe methods should not be made in future code reviews.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| flavour match { | ||
| case SpelExpressionParser.Standard => | ||
| parsed.getValue[T](evaluationContext, expectedClass) | ||
| case SpelExpressionParser.Template => | ||
| val parts = renderTemplateExpressionParts(evaluationContext) | ||
| TemplateEvaluationResult(parts).asInstanceOf[T] | ||
| } | ||
| } |
There was a problem hiding this comment.
Potential ClassCastException when casting TemplateEvaluationResult to expected type T
In the evaluate method, when handling the Template flavour (lines 114-117), TemplateEvaluationResult(parts) is cast to [T] using asInstanceOf[T]. If the expected return type T is not compatible with TemplateEvaluationResult, this could lead to a ClassCastException at runtime. Consider ensuring that the expected return type T matches TemplateEvaluationResult when using the Template flavour, or adjust the code to safely handle the casting.
…dedicated LazyParameter class
1909541 to
ac643c3
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (9)
engine/flink/management/dev-model/src/main/scala/pl/touk/nussknacker/engine/management/sample/LoggingService.scala (1)
36-36: Consider adding error handling for template rendering.While the change to use
renderedTemplateis correct, consider adding error handling in case template rendering fails. This would help with debugging template-related issues.- val msg = message.evaluate(context).renderedTemplate + val msg = try { + message.evaluate(context).renderedTemplate + } catch { + case e: Exception => + logger.error(s"Failed to render template: ${e.getMessage}") + throw e + }engine/flink/components/base-tests/src/test/scala/pl/touk/nussknacker/engine/flink/SpelTemplateLazyParameterTest.scala (3)
38-57: Consider adding more test cases for comprehensive coverage.While the current test case verifies basic functionality, consider adding tests for:
- Edge cases (empty input, null values)
- Error scenarios (malformed templates)
- Complex templates with multiple expressions
- Template evaluation failures
77-84: Consider enhancing error handling and type safety.The context transformation could be improved by:
- Adding explicit error cases in pattern matching
- Using more type-safe alternatives to
withVariableUnsafeExample improvement:
case TransformationStep((`spelTemplateParameterName`, param) :: Nil, _) => param match { case DefinedLazyParameter(_) => val outName = OutputVariableNameDependency.extract(dependencies) FinalResults(context.withVariable(outName, Typed[String]), List.empty) case other => FinalResults(context, List(CustomValidationError(s"Unexpected parameter type: $other"))) } case other => FinalResults(context, List(CustomValidationError(s"Unexpected transformation step: $other")))
102-111: Consider enhancing error handling in flatMap.The error handling in
flatMapcould be improved by:
- Adding specific error types and handling
- Logging errors with appropriate context
- Providing more detailed error messages
Example improvement:
override def flatMap(value: Context, out: Collector[ValueWithContext[String]]): Unit = { collectHandlingErrors(value, out) { try { val templateResult = evaluateParameter(value) val result = templateResult.renderedParts.map { case RenderedLiteral(value) => s"[$value]-literal" case RenderedSubExpression(value) => s"[$value]-subexpression" case other => throw new IllegalArgumentException(s"Unexpected rendered part: $other") }.mkString ValueWithContext(result, value) } catch { case e: Exception => logger.error(s"Template evaluation failed for context: $value", e) throw e } } }docs/MigrationGuide.md (5)
Line range hint
12-25: Scenario Activity Audit Log Configuration ChangesImportant configuration changes for scenario activity audit logging:
- New optional logger name:
scenario-activity-audit- Requires MDC context configuration in logback.xml
- Example configuration provided shows proper pattern with MDC variables
Consider adding this logging configuration to track scenario activities effectively.
Line range hint
32-35: REST API Endpoint ChangesNotable changes in REST API:
- New endpoint
/api/scenarioTesting/{scenarioName}/adhoc/validate- Changes in process API with optional
scenarioLabelsfield- Migration API now supports v2 request format
API clients need to be updated to handle these changes.
Line range hint
37-40: Definitions API ChangesChanges in definitions API:
- GET
/api/processDefinitionData/*has new optional query paramenrichedWithUiConfig- Added
requiredParamproperty to parameter config responseUpdate API clients to handle the new parameters and properties.
Line range hint
42-46: Kafka Exception Handler ConfigurationAdded message size limit configuration in Kafka exception handler:
- New config:
maxMessageBytes- Default value matches Kafka's default 1MB limit
Review and adjust
maxMessageBytesif your error topic allows larger messages. Add margin for Kafka protocol overhead (100 bytes recommended).
Based on the verification results, I can now provide a final response:
Critical Changes Confirmed - Action Required
The review comment's concerns are valid with specific impacts:
Kryo serializer changes (Registration of serializers in the way that is acceptable by table API #6692):
- Confirmed change in registration method for
UnmodifiableCollection,scala.Product- State restoration will fail for previously serialized values
TimeMeasuringService changes (Improvement: Make TimeMeasuringService usable with other Lifecycle traits #6912):
- Confirmed significant refactoring
- Services using this trait must be rebuilt
- Changed from specific Flink/Standalone implementations to unified API
Flink version update (1.18.1 -> 1.19.1):
- Confirmed in MigrationGuide.md
- Backwards incompatible changes present
TypeInformation support (NU-1848: TypeInformation support for scala.Option #6952):
- Confirmed impact on case classes with Option type
- State won't be restored for affected classes
Required actions:
- Clean up state before upgrading if using affected serializers
- Rebuild services using TimeMeasuringService
- Review Flink 1.19.1 compatibility requirements
- Migrate state if using case classes with Option type
🔗 Analysis chain
Line range hint
48-67: Critical Changes in Core ComponentsSeveral important changes that require attention:
- Kryo serializers registration changes:
- State won't be restored for values serialized by UnmodifiableCollection, scala.Product etc.
- TypeInformation support for scala.Option:
- State won't be restored for case classes with Option type
- Flink version update to 1.19.1:
- Backwards incompatible changes
- Need to use compatibility plugins for pre-1.19 versions
- TimeMeasuringService changes:
- Services using TimeMeasuringService must be rebuilt
These changes may require:
- State cleanup/migration
- Code updates for Flink compatibility
- Service rebuilds
Let me verify the Kryo serializers and TimeMeasuringService changes before making the final response.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for potential Flink version compatibility issues rg -l "org.apache.flink" | xargs rg "1\.1[0-8]\."Length of output: 2572
Script:
#!/bin/bash # Check for Kryo serializer and TimeMeasuringService changes rg -A 5 "TimeMeasuringService" rg -A 5 "KryoRegistrar|UnmodifiableCollection|scala.Product"Length of output: 20972
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
components-api/src/main/scala/pl/touk/nussknacker/engine/api/TemplateEvaluationResult.scala(1 hunks)docs/MigrationGuide.md(1 hunks)engine/flink/components/base-tests/src/test/scala/pl/touk/nussknacker/engine/flink/SpelTemplateLazyParameterTest.scala(1 hunks)engine/flink/management/dev-model/src/main/scala/pl/touk/nussknacker/engine/management/sample/LoggingService.scala(2 hunks)scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/spel/SpelExpression.scala(2 hunks)scenario-compiler/src/test/scala/pl/touk/nussknacker/engine/InterpreterSpec.scala(7 hunks)scenario-compiler/src/test/scala/pl/touk/nussknacker/engine/testcomponents/SpelTemplatePartsService.scala(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- components-api/src/main/scala/pl/touk/nussknacker/engine/api/TemplateEvaluationResult.scala
- scenario-compiler/src/test/scala/pl/touk/nussknacker/engine/testcomponents/SpelTemplatePartsService.scala
🔇 Additional comments (8)
engine/flink/management/dev-model/src/main/scala/pl/touk/nussknacker/engine/management/sample/LoggingService.scala (1)
21-23: LGTM! Type change improves type safety.
The transition from LazyParameter[String] to LazyParameter[TemplateEvaluationResult] aligns with the PR's goal of consolidating template handling and provides better type safety.
Let's verify that the SPEL_TEMPLATE_EDITOR properly handles the new type:
✅ Verification successful
Let me try a different search strategy to find relevant usages of SPEL_TEMPLATE_EDITOR and its relationship with parameter types:
Let me search for other usages of TemplateEvaluationResult to verify the consistency of this type change:
Type change is correct and consistent with codebase
The change from LazyParameter[String] to LazyParameter[TemplateEvaluationResult] is part of a broader migration described in PR #7162, where template parameters now use TemplateEvaluationResult instead of String. The codebase shows consistent usage across other components, with proper support for template evaluation and rendering.
- The change aligns with the documented migration in
MigrationGuide.md - Other components like
SpelTemplatePartsServiceandSpelExpressionalready handle this type - The rendered template can still be accessed via
renderedTemplatemethod when needed
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other usages of SPEL_TEMPLATE_EDITOR to ensure consistent typing
ast-grep --pattern 'SimpleEditorType.SPEL_TEMPLATE_EDITOR'
Length of output: 5878
Script:
#!/bin/bash
# Search for SimpleEditor annotations with SPEL_TEMPLATE_EDITOR type
rg -U "SimpleEditor.*SPEL_TEMPLATE_EDITOR.*LazyParameter" -A 3
Length of output: 746
Script:
#!/bin/bash
# Search for TemplateEvaluationResult usages
rg "TemplateEvaluationResult" -B 2 -A 2
Length of output: 6810
engine/flink/components/base-tests/src/test/scala/pl/touk/nussknacker/engine/flink/SpelTemplateLazyParameterTest.scala (2)
28-36: LGTM! Well-structured test setup.
The test class is properly configured with appropriate test frameworks and utilities. The test runner setup is explicit and follows best practices.
61-74: LGTM! Well-structured transformer implementation.
The transformer class is properly defined with appropriate traits and parameter configuration. The implementation follows the framework's patterns and best practices.
scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/spel/SpelExpression.scala (2)
16-16: LGTM: Import statements are properly organized
The new imports for template-related classes (TemplateRenderedPart, TemplateEvaluationResult) are correctly added and organized.
Also applies to: 23-23
111-118:
Consider adding type safety checks for template evaluation results
The pattern matching is clean, but there are two potential issues to address:
-
The
asInstanceOf[T]cast at line 116 could fail if the caller expects a type other thanTemplateEvaluationResult. Consider adding a type check or documenting the expected return type. -
The pattern match should be verified for exhaustiveness at compile-time.
Let's verify the usage of this method to ensure type safety:
Consider this safer implementation:
flavour match {
case SpelExpressionParser.Standard =>
parsed.getValue[T](evaluationContext, expectedClass)
case SpelExpressionParser.Template =>
- val parts = renderTemplateExpressionParts(evaluationContext)
- TemplateEvaluationResult(parts).asInstanceOf[T]
+ if (expectedClass != classOf[TemplateEvaluationResult]) {
+ throw new IllegalArgumentException(s"Template expressions must return TemplateEvaluationResult, but $expectedClass was expected")
+ }
+ val parts = renderTemplateExpressionParts(evaluationContext)
+ TemplateEvaluationResult(parts).asInstanceOf[T]
}scenario-compiler/src/test/scala/pl/touk/nussknacker/engine/InterpreterSpec.scala (3)
9-10: LGTM: Appropriate imports for property-based testing.
The addition of table-driven property testing imports enhances the testing capabilities.
Line range hint 1187-1204: LGTM: Improved naming and template handling.
The changes improve clarity and correctness:
- Renamed parameter to
spelTemplateParameterNamefor better readability - Properly handles template evaluation results
1027-1071: LGTM: Comprehensive test coverage for template rendering.
Well-structured test cases covering various scenarios:
- Template with mixed literal and expression parts
- Single literal value
- Single expression value
- Empty template
- Uses table-driven testing for better maintainability
The test implementation follows best practices:
- Clear test descriptions
- Proper test data organization
- Comprehensive assertions
| private def renderTemplateExpressionParts(evaluationContext: EvaluationContext): List[TemplateRenderedPart] = { | ||
| def renderExpression(expression: Expression): List[TemplateRenderedPart] = expression match { | ||
| case literal: LiteralExpression => List(RenderedLiteral(literal.getExpressionString)) | ||
| case spelExpr: org.springframework.expression.spel.standard.SpelExpression => | ||
| // TODO: Should we use the same trick with re-parsing after ClassCastException as we use in ParsedSpelExpression? | ||
| List(RenderedSubExpression(spelExpr.getValue[String](evaluationContext, classOf[String]))) | ||
| case composite: CompositeStringExpression => composite.getExpressions.toList.flatMap(renderExpression) | ||
| case other => | ||
| throw new IllegalArgumentException( | ||
| s"Unsupported expression type: ${other.getClass.getName} for a template expression" | ||
| ) | ||
| } | ||
| renderExpression(parsed.parsed) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling in template expression rendering
The pattern matching is comprehensive, but there are several improvements to consider:
- The TODO comment at line 124 raises a valid concern about ClassCastException handling consistency.
- The error message for unsupported expressions could be more descriptive.
Consider this enhanced implementation:
private def renderTemplateExpressionParts(evaluationContext: EvaluationContext): List[TemplateRenderedPart] = {
def renderExpression(expression: Expression): List[TemplateRenderedPart] = expression match {
case literal: LiteralExpression => List(RenderedLiteral(literal.getExpressionString))
case spelExpr: org.springframework.expression.spel.standard.SpelExpression =>
- // TODO: Should we use the same trick with re-parsing after ClassCastException as we use in ParsedSpelExpression?
- List(RenderedSubExpression(spelExpr.getValue[String](evaluationContext, classOf[String])))
+ try {
+ List(RenderedSubExpression(spelExpr.getValue[String](evaluationContext, classOf[String])))
+ } catch {
+ case e: SpelEvaluationException if Option(e.getCause).exists(_.isInstanceOf[ClassCastException]) =>
+ logger.warn(s"Error during template expression evaluation: ${e.getMessage}. Trying to reparse")
+ // Re-parse logic here similar to ParsedSpelExpression
+ List(RenderedSubExpression(spelExpr.getValue[String](evaluationContext, classOf[String])))
+ }
case composite: CompositeStringExpression => composite.getExpressions.toList.flatMap(renderExpression)
case other =>
throw new IllegalArgumentException(
- s"Unsupported expression type: ${other.getClass.getName} for a template expression"
+ s"Unsupported expression type: ${other.getClass.getName} for a template expression. Expected: LiteralExpression, SpelExpression, or CompositeStringExpression"
)
}
renderExpression(parsed.parsed)
}Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
This re-parsing is a little bit more complex, expression should be parsed once more without force compile
There was a problem hiding this comment.
@arkadius, understood. Thank you for the clarification. I'll take this into account for future reviews.
(_/)
( •_•)
( >< ) I'm curious to learn more about complex expression parsing.
✏️ Learnings added
Learnt from: arkadius
PR: TouK/nussknacker#7174
File: scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/spel/SpelExpression.scala:120-132
Timestamp: 2024-11-19T21:59:38.951Z
Learning: When handling re-parsing of expressions in the `renderExpression` method of `renderTemplateExpressionParts` in `SpelExpression.scala`, re-parsing is complex and expressions should be re-parsed once more without force compile.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| * [#7162](https://github.com/TouK/nussknacker/pull/7162) When component declares that requires parameter with either `SpelTemplateParameterEditor` | ||
| or `SqlParameterEditor` editor, in the runtime, for the expression evaluation result, will be used the new `TemplateEvaluationResult` | ||
| class instead of `String` class. To access the previous `String` use `TemplateEvaluationResult.renderedTemplate` method. |
There was a problem hiding this comment.
💡 Codebase verification
Breaking change in template evaluation requires attention
Based on the codebase analysis, this is a significant change affecting components using SpelTemplateParameterEditor or SqlParameterEditor. The change is well documented and several components are affected:
LoggingServicein engine/flink/management/dev-modelSpelTemplatePartsServicein scenario-compiler/test- Custom components in engine/flink/components/base-tests
The migration guide correctly states that:
- The return type changes from
StringtoTemplateEvaluationResult - To access the string value, use
TemplateEvaluationResult.renderedTemplate
The implementation shows this is a breaking change that requires updates to any custom components using these editors.
🔗 Analysis chain
Template evaluation result change needs attention
The change in template evaluation result handling requires careful migration:
- Components using
SpelTemplateParameterEditororSqlParameterEditorwill now useTemplateEvaluationResultinstead ofString - Use
TemplateEvaluationResult.renderedTemplateto access the previousStringvalue
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Find components that may be affected by this change
ast-grep --pattern 'class $_ {
$$$
@MethodToInvoke
def $_($_, editor = SpelTemplateParameterEditor) {
$$$
}
$$$
}'
Length of output: 130
Script:
#!/bin/bash
# Let's try a different approach to find usages of these editors
# First, let's search for SpelTemplateParameterEditor
rg "SpelTemplateParameterEditor" -A 2 -B 2
# Also search for SqlParameterEditor
rg "SqlParameterEditor" -A 2 -B 2
# Let's also check for TemplateEvaluationResult to understand the change
rg "TemplateEvaluationResult" -A 2 -B 2
Length of output: 21936
There was a problem hiding this comment.
components in dev-model are not exposed publicly so it is not important for an end user, these same with classes inside tests modules
There was a problem hiding this comment.
@arkadius Understood, thanks for the clarification. Since components in the dev-model and test modules are not exposed publicly, this breaking change does not impact end users.
✏️ Learnings added
Learnt from: arkadius
PR: TouK/nussknacker#7174
File: docs/MigrationGuide.md:47-49
Timestamp: 2024-11-19T22:01:05.583Z
Learning: Components in the `dev-model` and test modules are not exposed publicly. Changes to these modules, such as updates to `LoggingService` in `engine/flink/management/dev-model` and test classes like `SpelTemplatePartsService`, do not impact end users and do not need to be highlighted in migration guides.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
Not exactly. It impacts the end-user, or more precisely a user who developed their own component using these editors. However the same user is not interested in changes in internal components which are only for demonstration of the feature/unit testing purpose
There was a problem hiding this comment.
@arkadius Thank you for the clarification. You are absolutely right. The breaking change impacts end users who have developed their own components using SpelTemplateParameterEditor or SqlParameterEditor. While internal components in dev-model and test modules are not exposed publicly and need not be highlighted, it's important to inform users about the necessary updates to their custom components due to the change from String to TemplateEvaluationResult. I'll ensure the migration guide reflects this accordingly.
✏️ Learnings added
Learnt from: arkadius
PR: TouK/nussknacker#7174
File: docs/MigrationGuide.md:47-49
Timestamp: 2024-11-19T22:07:52.637Z
Learning: When a breaking change affects end users who have developed custom components using `SpelTemplateParameterEditor` or `SqlParameterEditor`, it's important to highlight this in the migration guide, even if internal components like those in `dev-model` and test modules are not exposed publicly.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
|
created: #7176 |
69f025f to
b68883c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (5)
engine/flink/management/dev-model/src/main/scala/pl/touk/nussknacker/engine/management/sample/source/SqlSource.scala (1)
14-15: Consider adding documentation for the new parameter typeSince this is a public API change from
StringtoTemplateEvaluationResult, it would be helpful to add documentation explaining:
- The expected format of the template evaluation result
- How SQL queries should be structured with the new type
- Any examples of valid usage
@MethodToInvoke +/** Creates a source based on the provided SQL query template. + * @param sql The SQL query as a template evaluation result. The template parts will be + * concatenated to form the final SQL query. + */ def source(@ParamName("sql") @SimpleEditor(`type` = SimpleEditorType.SQL_EDITOR) sql: TemplateEvaluationResult) =components/sql/src/main/scala/pl/touk/nussknacker/sql/service/DatabaseQueryEnricher.scala (1)
44-44: LGTM: Simplified parameter declaration aligns with new template handling approachThe change from
queryParamDeclarationto directParameter[String]creation is cleaner and aligns well with the PR's objective of moving to expression-level template handling.docs/Changelog.md (1)
14-15: LGTM with a minor formatting suggestion.The new entries are properly documented and aligned with the PR objectives. However, consider adding a line break between the entries for better readability.
* [#7145](https://github.com/TouK/nussknacker/pull/7145) Lift TypingResult information for dictionaries + * [#7162](https://github.com/TouK/nussknacker/pull/7162) Component API enhancement: ability to access information about expression parts used in SpEL templatescenario-compiler/src/test/scala/pl/touk/nussknacker/engine/spel/SpelExpressionSpec.scala (2)
1097-1099: Enhance Test Coverage for Template RenderingThe test verifies the rendered template output successfully. Consider adding tests for edge cases such as invalid expressions within the template to ensure robust template handling.
1103-1105: Add Tests for Null or Whitespace TemplatesWhile the test confirms that an empty template returns an empty string, consider adding additional tests for templates that are
nullor contain only whitespace to ensure comprehensive coverage.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
components/sql/src/main/scala/pl/touk/nussknacker/sql/service/DatabaseQueryEnricher.scala(3 hunks)docs/Changelog.md(1 hunks)engine/flink/management/dev-model/src/main/scala/pl/touk/nussknacker/engine/management/sample/source/SqlSource.scala(1 hunks)extensions-api/src/main/scala/pl/touk/nussknacker/engine/api/process/ClassExtractionSettings.scala(2 hunks)scenario-compiler/src/test/scala/pl/touk/nussknacker/engine/spel/SpelExpressionSpec.scala(3 hunks)
🔇 Additional comments (8)
engine/flink/management/dev-model/src/main/scala/pl/touk/nussknacker/engine/management/sample/source/SqlSource.scala (1)
14-14: Verify SQL editor compatibility with TemplateEvaluationResult
The parameter type has been changed to TemplateEvaluationResult while retaining the SQL editor annotation. Please ensure that the SQL editor in the frontend properly handles this new type.
✅ Verification successful
SQL editor is compatible with TemplateEvaluationResult
Based on the evidence found in the codebase:
- The migration guide explicitly documents this change: "When component declares that requires parameter with either SpelTemplateParameterEditor or SqlParameterEditor editor, in the runtime, for the expression evaluation result, will be used the new TemplateEvaluationResult class instead of String class"
- The SqlParameterEditor is properly defined as a SimpleParameterEditor in the core API
- The language determiner correctly handles SQL editor type by setting it to use SpelTemplate language
- There are other components in the codebase already using this combination (found in the search results)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for SQL editor implementations to verify compatibility
ast-grep --pattern 'class $editor extends $_ {
$$$
SimpleEditorType.SQL_EDITOR
$$$
}'
# Search for any SQL editor tests
rg -l "SQL_EDITOR.*test"
Length of output: 120
Script:
#!/bin/bash
# Let's try a different approach to find SQL editor related code
# Search for SQL_EDITOR usage
rg "SQL_EDITOR" -B 2 -A 2
# Search for TemplateEvaluationResult usage with SQL
rg "TemplateEvaluationResult.*sql" -B 2 -A 2
# Find files containing both SQL and editor-related code
rg -l "SimpleEditor.*SQL"
Length of output: 2657
Script:
#!/bin/bash
# Let's check the SqlParameterEditor implementation to understand how it handles the input
rg "SqlParameterEditor" -B 5 -A 10
# Also check for any existing usages of TemplateEvaluationResult with SQL editor
rg "@SimpleEditor.*SQL_EDITOR.*TemplateEvaluationResult" -B 2 -A 2
# Look for any SQL parameter handling in the codebase
ast-grep --pattern 'class $_ {
$$$
def $_(sql: TemplateEvaluationResult) {
$$$
}
}'
Length of output: 17704
components/sql/src/main/scala/pl/touk/nussknacker/sql/service/DatabaseQueryEnricher.scala (2)
132-132: LGTM: Consistent parameter list update
The parameter list modification maintains consistency with the new parameter declaration approach while preserving the existing parameter structure.
142-150: Verify template rendering error handling and performance
The change to use TemplateEvaluationResult introduces template rendering in the query parameter handling. Please verify:
- Error propagation from template rendering failures
- Performance impact of template rendering, especially for frequently executed queries
Consider caching the rendered template result if the query is executed frequently with the same template but different parameters.
extensions-api/src/main/scala/pl/touk/nussknacker/engine/api/process/ClassExtractionSettings.scala (2)
8-8: LGTM!
Clean import addition, properly grouped with other API imports.
112-114: LGTM! Appropriate exclusion of TemplateEvaluationResult from UI suggestions
The addition of ExactClassPredicate[TemplateEvaluationResult] to ExcludedStdClasses is correct as this type should only be used programmatically and not exposed in the UI's syntax suggestions. This aligns with the PR's objective of handling template parts at the expression level.
scenario-compiler/src/test/scala/pl/touk/nussknacker/engine/spel/SpelExpressionSpec.scala (3)
29-29: Import TemplateEvaluationResult for Template Handling
The addition of TemplateEvaluationResult to the imports is appropriate for handling template evaluations in the tests.
1091-1092: Appropriate Use of skipReturnTypeCheck in Template Tests
Using evaluateSync[TemplateEvaluationResult](skipReturnTypeCheck = true) correctly bypasses the return type check, which is necessary when evaluating template expressions that return a TemplateEvaluationResult.
87-87: Ensure Conditional Logic Correctness
The added condition && !skipReturnTypeCheck effectively allows skipping the return type check when specified. Verify that this adjustment does not unintentionally bypass essential validations in other test scenarios.
Run the following script to search for all usages of evaluateSync and ensure they are consistent with the updated method:
✅ Verification successful
Skip Return Type Check Used Appropriately for Template Evaluation
The skipReturnTypeCheck parameter is only used in test cases specifically dealing with template evaluation, where the return type check is intentionally skipped for TemplateEvaluationResult. This is appropriate because:
- It's only used in test methods related to template evaluation: "evaluates expression with template context" and "evaluates empty template as empty string"
- The parameter has a default value of
false, maintaining type checking for all other test cases - No other usages of
evaluateSyncin the codebase bypass the type checking
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all calls to `evaluateSync` and check for the use of `skipReturnTypeCheck`.
# Search for `evaluateSync` usage excluding the definition
rg --ignore-case --type scala 'evaluateSync\[' --globals --ignore-file '!*SpelExpressionSpec.scala' --invert-match 'def evaluateSync'
Length of output: 569
Script:
#!/bin/bash
# Search for `evaluateSync` usage excluding the definition
rg --type scala 'evaluateSync\[' -g '!*SpelExpressionSpec.scala' -A 3 -B 3
# Also search for skipReturnTypeCheck to understand its usage context
rg --type scala 'skipReturnTypeCheck' -A 3 -B 3
Length of output: 5004
| @@ -81,10 +81,10 @@ class SpelExpressionSpec extends AnyFunSuite with Matchers with ValidatedValuesD | |||
|
|
|||
| private implicit class EvaluateSyncTyped(expression: TypedExpression) { | |||
|
|
|||
| def evaluateSync[T](ctx: Context = ctx): T = { | |||
| def evaluateSync[T](ctx: Context = ctx, skipReturnTypeCheck: Boolean = false): T = { | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid Potential Binary Compatibility Issues with Method Signature Change
Adding a default parameter skipReturnTypeCheck: Boolean = false to evaluateSync may introduce binary compatibility issues if evaluateSync is part of a public API. Consider overloading the method instead to maintain compatibility.
Apply this diff to create an overloaded method:
+ def evaluateSync[T](ctx: Context = ctx): T = {
+ evaluateSync(ctx, skipReturnTypeCheck = false)
+ }
- def evaluateSync[T](ctx: Context = ctx, skipReturnTypeCheck: Boolean = false): T = {
+ def evaluateSync[T](ctx: Context, skipReturnTypeCheck: Boolean): T = {
val evaluationResult = expression.evaluate[T](ctx, Map.empty)
expression.typingInfo.typingResult match {
case result: SingleTypingResult if evaluationResult != null && !skipReturnTypeCheck =>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def evaluateSync[T](ctx: Context = ctx, skipReturnTypeCheck: Boolean = false): T = { | |
| def evaluateSync[T](ctx: Context = ctx): T = { | |
| evaluateSync(ctx, skipReturnTypeCheck = false) | |
| } | |
| def evaluateSync[T](ctx: Context, skipReturnTypeCheck: Boolean): T = { | |
| val evaluationResult = expression.evaluate[T](ctx, Map.empty) | |
| expression.typingInfo.typingResult match { | |
| case result: SingleTypingResult if evaluationResult != null && !skipReturnTypeCheck => |
There was a problem hiding this comment.
This is a test class. We shouldn't be worry about a compatibility issue as it is used only internally
There was a problem hiding this comment.
@arkadius, understood. Since SpelExpressionSpec.scala is a test class used only internally, binary compatibility issues are not a concern here.
✏️ Learnings added
Learnt from: arkadius
PR: TouK/nussknacker#7174
File: scenario-compiler/src/test/scala/pl/touk/nussknacker/engine/spel/SpelExpressionSpec.scala:84-84
Timestamp: 2024-11-19T22:01:37.795Z
Learning: In test classes such as `SpelExpressionSpec.scala` within the `scenario-compiler` module, binary compatibility issues are not a concern because they are used only internally. Therefore, changes to method signatures in these test classes are acceptable.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
b68883c to
9bdac29
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
docs/Changelog.md (1)
14-15: LGTM with a minor formatting suggestion.The new entries are properly formatted and linked to their respective PRs. However, for better readability, consider adding a blank line between the entries.
* [#7145](https://github.com/TouK/nussknacker/pull/7145) Lift TypingResult information for dictionaries * [#7162](https://github.com/TouK/nussknacker/pull/7162) Component API enhancement: ability to access information about expression parts used in SpEL template
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
components/sql/src/main/scala/pl/touk/nussknacker/sql/service/DatabaseQueryEnricher.scala(3 hunks)docs/Changelog.md(1 hunks)engine/flink/management/dev-model/src/main/scala/pl/touk/nussknacker/engine/management/sample/source/SqlSource.scala(1 hunks)extensions-api/src/main/scala/pl/touk/nussknacker/engine/api/process/ClassExtractionSettings.scala(2 hunks)scenario-compiler/src/test/scala/pl/touk/nussknacker/engine/spel/SpelExpressionSpec.scala(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- components/sql/src/main/scala/pl/touk/nussknacker/sql/service/DatabaseQueryEnricher.scala
- engine/flink/management/dev-model/src/main/scala/pl/touk/nussknacker/engine/management/sample/source/SqlSource.scala
- extensions-api/src/main/scala/pl/touk/nussknacker/engine/api/process/ClassExtractionSettings.scala
🧰 Additional context used
📓 Learnings (1)
scenario-compiler/src/test/scala/pl/touk/nussknacker/engine/spel/SpelExpressionSpec.scala (1)
Learnt from: arkadius
PR: TouK/nussknacker#7174
File: scenario-compiler/src/test/scala/pl/touk/nussknacker/engine/spel/SpelExpressionSpec.scala:84-84
Timestamp: 2024-11-19T22:01:38.151Z
Learning: In test classes such as `SpelExpressionSpec.scala` within the `scenario-compiler` module, binary compatibility issues are not a concern because they are used only internally. Therefore, changes to method signatures in these test classes are acceptable.
🔇 Additional comments (2)
scenario-compiler/src/test/scala/pl/touk/nussknacker/engine/spel/SpelExpressionSpec.scala (2)
29-29: LGTM: Import added for new template evaluation functionality.
The import for TemplateEvaluationResult is correctly placed with other API imports and aligns with the template parts handling changes.
1091-1092: LGTM: Template evaluation tests properly handle type conversion.
The tests correctly verify template evaluation functionality:
- Basic template evaluation
- Complex template with multiple expressions
- Empty template edge case
The use of skipReturnTypeCheck = true is necessary because we're evaluating to TemplateEvaluationResult while the parse type is String, which is the expected behavior for template expressions.
Also applies to: 1097-1099, 1103-1105
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/spel/SpelExpressionValidator.scala (1)
24-25: Consider documenting the String to TemplateEvaluationResult conversionThe automatic conversion from
StringtoTemplateEvaluationResultis a key part of the new template handling approach. Consider adding a code comment explaining this behavior for future maintainers.+ // Strings are automatically converted to TemplateEvaluationResult to support template expressions case a if a == Typed[String] && expectedType == Typed[TemplateEvaluationResult] => Valid(collected)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
components/sql/src/test/scala/pl/touk/nussknacker/sql/service/DatabaseQueryEnricherValidationTest.scala(3 hunks)designer/server/src/main/scala/pl/touk/nussknacker/ui/definition/DefinitionsService.scala(3 hunks)scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/spel/SpelExpressionValidator.scala(2 hunks)
🧰 Additional context used
📓 Learnings (1)
scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/spel/SpelExpressionValidator.scala (1)
Learnt from: arkadius
PR: TouK/nussknacker#7174
File: scenario-compiler/src/test/scala/pl/touk/nussknacker/engine/spel/SpelExpressionSpec.scala:84-84
Timestamp: 2024-11-19T22:01:38.151Z
Learning: In test classes such as `SpelExpressionSpec.scala` within the `scenario-compiler` module, binary compatibility issues are not a concern because they are used only internally. Therefore, changes to method signatures in these test classes are acceptable.
🔇 Additional comments (9)
scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/spel/SpelExpressionValidator.scala (2)
6-6: LGTM: Import aligns with template handling changes
The addition of TemplateEvaluationResult import supports the PR's objective of changing template parts handling approach.
22-28: Verify test coverage for template validation cases
The pattern matching logic looks correct and aligns with the PR objectives. Please ensure there are corresponding test cases covering:
- Direct SpelExpressionRepr validation
- String to TemplateEvaluationResult conversion
- General subclass validation
- Type mismatch errors
Let's verify the test coverage:
✅ Verification successful
Based on the search results, I can now provide a conclusive response about the test coverage for template validation cases:
Test coverage for template validation cases is comprehensive
The codebase shows thorough test coverage for all validation scenarios:
-
Direct SpelExpressionRepr validation is tested in:
InterpreterSpec.scala: TestsSpelExpressionReprhandling withSpelNodeServiceSpelExpressionSpec.scala: Tests expression parsing and evaluation
-
String to TemplateEvaluationResult conversion is covered in:
SpelExpressionSpec.scala: Multiple test cases for template evaluation including:- Basic template rendering
- Empty template handling
- Complex templates with multiple expressions
-
General subclass validation is implicitly tested through the template validation tests and type system tests in
SpelExpressionSpec.scala -
Type mismatch errors are properly tested in the validation framework as shown in
SpelExpressionValidator.scala
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check test coverage for template validation cases
# Expected: Find test cases covering all validation scenarios
# Look for relevant test cases
rg -A 10 "validate.*TemplateEvaluationResult" --type scala
rg -A 10 "SpelExpressionValidator.*validate" --type scala
Length of output: 120
Script:
#!/bin/bash
# Let's try a broader search for test files related to SpelExpressionValidator
fd "SpelExpressionValidator.*Test.*\.scala$"
# Search for test cases containing SpelExpression or Template keywords
rg -l "class.*Test.*Spel" --type scala
# Look for test files in the same directory or test directories
fd ".*Test.*\.scala$" -p scenario-compiler/src
# Search for specific test assertions related to our validation cases
rg "SpelExpressionRepr|TemplateEvaluationResult" --type scala -C 5 scenario-compiler/src
Length of output: 29719
components/sql/src/test/scala/pl/touk/nussknacker/sql/service/DatabaseQueryEnricherValidationTest.scala (4)
3-3: LGTM: Import statements are correctly updated
The new imports for TemplateRenderedPart.RenderedLiteral and TemplateEvaluationResult align with the PR's objective of changing the template parts handling approach.
Also applies to: 8-8
36-39: LGTM: Error handling test properly updated
The test case correctly wraps the invalid SQL query in TemplateEvaluationResult while maintaining its original purpose of testing error handling. The error message assertion remains valid with this change.
68-71: LGTM: Non-parameterized query test properly updated
The test case correctly adapts to the new template parts handling approach while preserving its original intent of testing valid non-parameterized queries. The assertion logic remains appropriate.
36-39: Verify consistent template handling updates across test files
Let's ensure that similar test files have been updated to use the new template handling approach.
Also applies to: 68-71
✅ Verification successful
Let me gather more context about the template handling in the test files.
Let me try one more verification to check for TemplateEvaluationResult usage correctly:
Let me check the content of these other test files to verify the template handling consistency:
Let me check the content of these test files directly to see how they handle query parameters:
Template handling is consistent across test files
After reviewing all DatabaseQueryEnricher test files:
- DatabaseQueryEnricherValidationTest.scala correctly uses TemplateEvaluationResult for query parameters
- Other test files (HsqlTest, PostgresqlTest, WithCacheTest) handle direct SQL queries differently as they are testing different aspects:
- HsqlTest: Tests basic query execution and type conversions
- PostgresqlTest: Tests PostgreSQL-specific features and type mappings
- WithCacheTest: Tests caching behavior
The template handling approach in DatabaseQueryEnricherValidationTest.scala is appropriate as it specifically tests validation scenarios, while other test files serve different testing purposes that don't require template evaluation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other test files that might need similar updates
echo "Searching for similar test files that might need updating..."
fd -e scala -e java --full-path '.*/(test|spec)/.*' | rg -l 'DatabaseQueryEnricher|queryParamName'
# Search for potential inconsistencies in template handling
echo "Checking for potential inconsistencies in template handling..."
rg -A 2 'queryParamName.*->.*eagerValueParameter' | rg -v 'TemplateEvaluationResult'
Length of output: 1054
Script:
#!/bin/bash
# Search for all occurrences of queryParamName with context to understand the pattern
echo "Checking all queryParamName occurrences with context..."
rg -B 2 -A 2 'queryParamName.*->.*eagerValueParameter'
# Search for all TemplateEvaluationResult usage in test files
echo "Checking TemplateEvaluationResult usage..."
rg -B 2 -A 2 'TemplateEvaluationResult' '.*test.*'
Length of output: 2002
Script:
#!/bin/bash
# Search for all TemplateEvaluationResult usage in test files with proper path handling
echo "Checking TemplateEvaluationResult usage in test files..."
rg -B 2 -A 2 'TemplateEvaluationResult' --type-add 'test:**/test/**/*.{scala,java}' -t test
# Also check for any other DatabaseQueryEnricher test files
echo "Checking for other DatabaseQueryEnricher test files..."
rg -l "class.*DatabaseQueryEnricher.*Test"
Length of output: 936
Script:
#!/bin/bash
# Check template handling in all DatabaseQueryEnricher test files
echo "Checking template handling in DatabaseQueryEnricher test files..."
rg -B 2 -A 2 'queryParamName.*->.*' components/sql/src/test/scala/pl/touk/nussknacker/sql/service/DatabaseQueryEnricher*Test.scala
# Also check for any direct string literals without TemplateEvaluationResult
echo "Checking for potential raw string usage..."
rg -B 2 -A 2 'queryParamName.*->.*".*"' components/sql/src/test/scala/pl/touk/nussknacker/sql/service/DatabaseQueryEnricher*Test.scala
Length of output: 2763
Script:
#!/bin/bash
# Check the content of other DatabaseQueryEnricher test files
echo "Content of DatabaseQueryEnricherHsqlTest.scala:"
cat components/sql/src/test/scala/pl/touk/nussknacker/sql/service/DatabaseQueryEnricherHsqlTest.scala
echo -e "\nContent of DatabaseQueryEnricherPostgresqlTest.scala:"
cat components/sql/src/test/scala/pl/touk/nussknacker/sql/service/DatabaseQueryEnricherPostgresqlTest.scala
echo -e "\nContent of DatabaseQueryEnricherWithCacheTest.scala:"
cat components/sql/src/test/scala/pl/touk/nussknacker/sql/service/DatabaseQueryEnricherWithCacheTest.scala
Length of output: 9774
designer/server/src/main/scala/pl/touk/nussknacker/ui/definition/DefinitionsService.scala (3)
12-13: LGTM: Required imports for template evaluation changes.
The new imports align with the PR's objective of changing the template parts handling approach.
179-181: Add documentation for type transformation logic.
While the type transformation is correct, it would be helpful to add documentation explaining:
- Why
TemplateEvaluationResultneeds to be transformed toStringfor UI rendering - The implications of this transformation on template processing
+ /**
+ * Transforms TemplateEvaluationResult type to String type for UI rendering.
+ * This is necessary because the UI expects string values for template expressions,
+ * while the backend processes them as TemplateEvaluationResult.
+ */
private def toUIType(typingResult: TypingResult): TypingResult = {
if (typingResult == Typed[TemplateEvaluationResult]) Typed[String] else typingResult
}Also, let's verify the impact of this transformation:
✅ Verification successful
Documentation suggestion is valid, but type transformation is well-integrated
The type transformation from TemplateEvaluationResult to String for UI rendering is consistent with the existing codebase behavior. The SpelExpressionValidator already handles this conversion pattern, validating expressions where String types are accepted for template evaluation results.
The suggested documentation would indeed improve code maintainability:
+ /**
+ * Transforms TemplateEvaluationResult type to String type for UI rendering.
+ * This is necessary because template expressions are processed as TemplateEvaluationResult
+ * in the backend but need to be presented as strings in the UI.
+ */
private def toUIType(typingResult: TypingResult): TypingResult = {
if (typingResult == Typed[TemplateEvaluationResult]) Typed[String] else typingResult
}🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential type mismatches or conversion issues
# Look for places where String and TemplateEvaluationResult types interact
# Search for type conversions between String and TemplateEvaluationResult
rg -A 3 "String.*TemplateEvaluationResult|TemplateEvaluationResult.*String" --type scala
# Look for potential serialization/deserialization of these types
rg -l "encode.*TemplateEvaluationResult|decode.*TemplateEvaluationResult" --type scala
Length of output: 1890
Script:
#!/bin/bash
# Let's check the SpelExpressionValidator implementation to understand the type conversion logic
rg -B 5 -A 10 "String.*TemplateEvaluationResult" scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/spel/SpelExpressionValidator.scala
# Also check for any template-related tests that might validate this behavior
rg -l "TemplateEvaluationResult.*Test" --type scala
# Look for any template processing logic
ast-grep --pattern 'class $_ {
$$$
template$_($$$) {
$$$
}
$$$
}'
Length of output: 894
167-167: Verify UI rendering with transformed types.
The type transformation looks good, but we should verify that components using TemplateEvaluationResult are rendered correctly in the UI.
✅ Verification successful
Based on the findings, I can now generate the final response:
Type transformation for TemplateEvaluationResult is properly handled
The implementation correctly transforms TemplateEvaluationResult to String type for UI rendering, which is the expected behavior. The transformation is simple and well-defined in toUIType. Since this is a straightforward type conversion that doesn't affect the actual parameter handling logic, and given that TemplateEvaluationResult is already used in various tested components across the codebase, no additional verification is needed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find components that might be affected by the type transformation
# Look for components using TemplateEvaluationResult in their parameters
ast-grep --pattern 'class $_ extends $_ {
$$$
def parameters = $$$
$$$
}'
# Also search for test cases verifying UI rendering
rg -l "UIParameter.*TemplateEvaluationResult" --type scala
Length of output: 142
Script:
#!/bin/bash
# Let's try a different approach to find usage of TemplateEvaluationResult
# First, find where TemplateEvaluationResult is used
rg "TemplateEvaluationResult" --type scala -l
# Then look for test files related to parameter rendering
rg "UIParameter.*test" --type scala -l
# Also check for parameter type conversion tests
rg "toUIType.*test" --type scala -l
# Look for UI parameter rendering tests
fd "Parameter.*Spec.scala|Parameter.*Test.scala"
Length of output: 3834
Script:
#!/bin/bash
# Let's examine the test files that might be relevant to our UI parameter type conversion
rg -A 10 "toUIType" designer/server/src/test/scala --type scala
# Check the actual implementation of toUIType
rg -A 5 "def toUIType" designer/server/src/main/scala --type scala
# Look for existing UI parameter rendering tests
rg -A 10 "UIParameter.*render" designer/server/src/test/scala --type scala
# Check if there are any specific tests for TemplateEvaluationResult rendering
rg -A 10 "TemplateEvaluationResult.*test" designer/server/src/test/scala --type scala
Length of output: 1169
Describe your changes
Checklist before merge
Summary by CodeRabbit
Release Notes
New Features
TemplateEvaluationResultfor improved template rendering, supporting structured evaluation of rendered parts.SpelTemplatePartsService, to enhance template processing with subexpressions and literals.SqlSourceto acceptTemplateEvaluationResultfor SQL queries, enhancing query processing.DatabaseQueryEnricherfor streamlined handling of query parameters.Bug Fixes
OrderedDependenciesclass.Documentation
Tests
SpelTemplatePartsServiceand enhanced existing tests to cover template rendering scenarios.SpelExpressionSpecto validate new template evaluation functionality.