[NU-1800] Add TemplateEvaluationResult to evaluate SpEL expression parts in LazyParameter #7162
Conversation
WalkthroughThe changes primarily enhance the handling of template expressions within the Nussknacker framework. Key modifications include the introduction of the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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 (
|
...c/main/scala/pl/touk/nussknacker/engine/compile/nodecompilation/EvaluableLazyParameter.scala
Outdated
Show resolved
Hide resolved
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
...c/main/scala/pl/touk/nussknacker/engine/compile/nodecompilation/EvaluableLazyParameter.scala
Outdated
Show resolved
Hide resolved
scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/spel/SpelExpression.scala
Outdated
Show resolved
Hide resolved
scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/spel/SpelExpression.scala
Outdated
Show resolved
Hide resolved
scenario-compiler/src/test/scala/pl/touk/nussknacker/engine/InterpreterSpec.scala
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (9)
scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/compile/nodecompilation/EvaluableLazyParameter.scala (2)
38-57: Enhance error handling and simplify pattern matchingThe template expression handling could be improved in several ways:
- Error messages should be more descriptive and include actual expression types
- The nested pattern matching could be simplified
Consider this refactoring:
override def templateExpression: TemplateExpression = compiledParameter.expression match { case expression: SpelExpression => - expression.templateSubexpressions match { - case Some(subexpressions) => - val templateParts = subexpressions.map { - case Placeholder(expression) => { - new TemplateExpressionPart.Placeholder { - override val evaluate: Evaluate[String] = context => { - expressionEvaluator.evaluate[String](expression, "expressionId", nodeId.id, context)(jobData).value - } - } - } - case Literal(value) => TemplateExpressionPart.Literal(value) - } - TemplateExpression(templateParts) - case None => - throw new IllegalStateException("Non SpEL-template expression received in SpelTemplateLazyParameter") - } - case _ => throw new IllegalStateException("Non SpEL expression received in SpelTemplateLazyParameter") + expression.templateSubexpressions.map { subexpressions => + val templateParts = subexpressions.map { + case Placeholder(expr) => new TemplateExpressionPart.Placeholder { + override val evaluate: Evaluate[String] = context => + expressionEvaluator.evaluate[String](expr, "expressionId", nodeId.id, context)(jobData).value + } + case Literal(value) => TemplateExpressionPart.Literal(value) + } + TemplateExpression(templateParts) + }.getOrElse(throw new IllegalStateException( + s"Expression ${expression} is not a SpEL template expression" + )) + case other => throw new IllegalStateException( + s"Expected SpEL expression but got: ${other.getClass.getName}" + )
35-36: Consider extracting common evaluation logicThe evaluate implementation is duplicated from EvaluableLazyParameter. Consider extracting this to a trait or using composition to avoid duplication.
components-api/src/main/scala/pl/touk/nussknacker/engine/api/LazyParameter.scala (2)
72-74: Add documentation for the new traitConsider adding scaladoc to explain:
- The purpose and use cases of
TemplateLazyParameter- The relationship between template expressions and lazy parameter evaluation
- Example usage demonstrating template interpolation
Example documentation:
/** * Represents a lazy parameter that supports template expressions with interpolated values. * Template expressions consist of literal parts and placeholders that are evaluated lazily. * * Example usage: * {{{ * val template = new TemplateLazyParameter[String] { * def templateExpression = TemplateExpression(List( * Literal("Hello "), * Placeholder(ctx => ctx.name) * )) * // ... other implementations * } * }}} * * @tparam T the type of the evaluated expression */
83-85: Consider making Placeholder's evaluate type more flexibleThe
Placeholdertrait'sevaluatemethod is fixed to returnString. Consider parameterizing it to support different types of interpolated values:- trait Placeholder extends TemplateExpressionPart { - val evaluate: Evaluate[String] - } + trait Placeholder[T] extends TemplateExpressionPart { + val evaluate: Evaluate[T] + def asString: Evaluate[String] // For template rendering + }This would allow for type-safe interpolation of non-string values while maintaining the ability to render them as strings in the final template.
scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/spel/SpelExpression.scala (4)
103-103: Add documentation for the templateSubexpressions methodConsider adding ScalaDoc to explain the purpose of this method, its return type semantics, and when it should be used.
104-113: Consider extracting placeholder creation logicThe
createEvaluablePlaceholderhelper function uses hardcoded values for typing and flavor. Consider:
- Making these parameters configurable
- Moving this logic to a separate factory method
- Adding error handling for parser failures
- def createEvaluablePlaceholder(expression: org.springframework.expression.spel.standard.SpelExpression) = { + private def createPlaceholder( + expression: org.springframework.expression.spel.standard.SpelExpression, + expectedType: TypingResult = typing.Typed[String], + exprFlavor: Flavour = Standard + ): Placeholder = { val parsedTemplateExpr = ParsedSpelExpression(expression.getExpressionString, parsed.parser, expression) val compiledExpr = new SpelExpression( parsedTemplateExpr, - typing.Typed[String], - Standard, + expectedType, + exprFlavor, evaluationContextPreparer ) Placeholder(compiledExpr) }
114-131: Simplify nested pattern matching logicThe current implementation has deep nesting which could be simplified using early returns or pattern matching decomposition.
Consider restructuring like this:
def templateSubexpressions: Option[List[SpelTemplateExpressionPart]] = { if (flavour.languageId != Language.SpelTemplate) return None Some(parsed.parsed match { case compositeExpr: CompositeStringExpression => compositeExpr.getExpressions.toList.map(expressionToPart) case expr => List(expressionToPart(expr)) }) } private def expressionToPart(expr: Expression): SpelTemplateExpressionPart = expr match { case lit: LiteralExpression => Literal(lit.getExpressionString) case spelExpr: org.springframework.expression.spel.standard.SpelExpression => createPlaceholder(spelExpr) case other => throw new IllegalArgumentException( s"Unsupported expression type: [${other.getClass.getName}]. Expected: LiteralExpression or SpelExpression" ) }
126-127: Enhance error message for unsupported expression typesThe current error message could be more helpful by including:
- The expected types
- The actual expression content
- Guidance on how to fix the issue
- throw new IllegalArgumentException(s"Unsupported expression type: [${other.getClass.getName}]") + throw new IllegalArgumentException( + s"""Unsupported expression type: [${other.getClass.getName}] for expression: '${other.getExpressionString}' + |Expected: LiteralExpression or SpelExpression for template parsing. + |Please ensure the expression follows the template syntax.""".stripMargin)scenario-compiler/src/test/scala/pl/touk/nussknacker/engine/InterpreterSpec.scala (1)
1028-1053: Consider adding more test cases for comprehensive coverage.The current test cases cover basic scenarios well. Consider adding these additional cases for more thorough testing:
- Multiple templated expressions (e.g.,
Hello#{#input.msisdn}#{#input.accountId})- Nested function calls (e.g.,
#{#input.msisdn.toLowerCase().trim()})- Special characters in templates (e.g.,
Hello @#{#input.msisdn}!)val testCases = Seq( // ... existing test cases ... + ( + "multiple templated expressions", + s"Hello#{#input.msisdn}#{#input.accountId}", + Transaction(msisdn = "foo", accountId = "123"), + "[Hello]-literal[foo]-templated[123]-templated" + ), + ( + "nested function calls in template", + "#{#input.msisdn.toLowerCase().trim()}", + Transaction(msisdn = "FOO "), + "[foo]-templated" + ), + ( + "special characters in template", + "Hello @#{#input.msisdn}!", + Transaction(msisdn = "foo"), + "[Hello @]-literal[foo]-templated[!]-literal" + ), )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
components-api/src/main/scala/pl/touk/nussknacker/engine/api/LazyParameter.scala(2 hunks)scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/compile/nodecompilation/EvaluableLazyParameter.scala(2 hunks)scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/spel/SpelExpression.scala(3 hunks)scenario-compiler/src/test/scala/pl/touk/nussknacker/engine/InterpreterSpec.scala(5 hunks)scenario-compiler/src/test/scala/pl/touk/nussknacker/engine/testcomponents/SpelTemplateAstOperationService.scala(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- scenario-compiler/src/test/scala/pl/touk/nussknacker/engine/testcomponents/SpelTemplateAstOperationService.scala
🔇 Additional comments (7)
scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/compile/nodecompilation/EvaluableLazyParameter.scala (3)
Line range hint 15-26: LGTM: Clean implementation with good separation of concerns
The class implementation is clean and follows good practices with proper type parameterization and delegation of evaluation logic.
64-73: Reusing past review comment about type casting safety
The unsafe type casting issue mentioned in the previous review is still present and should be addressed.
80-105: Consider decoupling parameter type determination from editor implementation
While the current implementation works, it tightly couples parameter type determination to editor implementation. As mentioned in previous reviews, consider:
- Adding an explicit
isTemplateParameterflag toParameter - Using expression type-based determination instead of editor-based determination
This would make the system more flexible and easier to extend in the future.
Let's verify the current usage of editor types in the codebase:
components-api/src/main/scala/pl/touk/nussknacker/engine/api/LazyParameter.scala (1)
3-3: LGTM: Import statement is correctly placed
The self-reference import for TemplateExpression follows Scala's idiomatic practices.
scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/spel/SpelExpression.scala (1)
84-89: Consider revising the naming scheme for template expression parts
As mentioned in the previous review, the current naming scheme (SpelTemplateExpressionPart, Literal, Placeholder) could be improved to be more intuitive in spoken language and documentation. Consider alternatives that better describe the role of each part in template expressions.
Some alternative naming suggestions:
SpelTemplateSegmentwithStaticSegment/DynamicSegmentTemplateElementwithTextElement/ExpressionElement
scenario-compiler/src/test/scala/pl/touk/nussknacker/engine/InterpreterSpec.scala (2)
9-10: LGTM: Import for table-driven tests.
The import for table-driven property tests is appropriate for the new parameterized test case.
1027-1071: LGTM: Well-structured table-driven test for template AST operations.
The test implementation is clean and covers essential scenarios using a table-driven approach with clear test case descriptions.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (4)
scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/compile/nodecompilation/EvaluableLazyParameter.scala (2)
52-54: Improve error messages for better debuggingThe error messages in the exception cases could be more descriptive to help with debugging. Include more context about the actual expression type received.
- throw new IllegalStateException("Non SpEL-template expression received in SpelTemplateLazyParameter") + throw new IllegalStateException(s"Expected SpEL-template expression but got: ${expression.templateSubexpressions}") - case _ => throw new IllegalStateException("Non SpEL expression received in SpelTemplateLazyParameter") + case other => throw new IllegalStateException(s"Expected SpelExpression but got: ${other.getClass.getSimpleName}")
41-49: Consider simplifying the pattern matching logicThe nested pattern matching with a single-expression block can be simplified for better readability.
- case Placeholder(expression) => { - new TemplateExpressionPart.Placeholder { - override val evaluate: Evaluate[String] = context => { - expressionEvaluator.evaluate[String](expression, "expressionId", nodeId.id, context)(jobData).value - } - } - } + case Placeholder(expression) => new TemplateExpressionPart.Placeholder { + override val evaluate: Evaluate[String] = context => + expressionEvaluator.evaluate[String](expression, "expressionId", nodeId.id, context)(jobData).value + }scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/spel/SpelExpression.scala (2)
84-89: Consider revising the naming based on previous feedbackThe structure using a sealed trait with final case classes is well-designed. However, as mentioned in the previous review, the current naming might be confusing in spoken language. Consider alternatives that better describe the parts of a template:
-sealed trait SpelTemplateExpressionPart -object SpelTemplateExpressionPart { - final case class Literal(value: String) extends SpelTemplateExpressionPart - final case class Placeholder(expression: SpelExpression) extends SpelTemplateExpressionPart +sealed trait TemplateSegment +object TemplateSegment { + final case class StaticText(value: String) extends TemplateSegment + final case class EvaluableExpression(expression: SpelExpression) extends TemplateSegmentThis makes it clearer that a template consists of static text segments and evaluable expression segments.
103-122: Consider optimizing template parsing performanceThe implementation is functionally correct but could benefit from performance optimizations:
The creation of new SpelExpression instances inside
parseTemplatecould be expensive, especially for complex templates. Consider caching or reusing instances where possible.The recursive parsing of composite expressions could be optimized using a more efficient collection transformation.
Here's a suggested optimization:
def templateSubexpressions: Option[List[SpelTemplateExpressionPart]] = { def parseTemplate(expression: Expression): List[SpelTemplateExpressionPart] = expression match { case lit: LiteralExpression => List(Literal(lit.getExpressionString)) case spelExpr: org.springframework.expression.spel.standard.SpelExpression => - val parsedTemplateExpr = ParsedSpelExpression(spelExpr.getExpressionString, parsed.parser, spelExpr) - val compiledExpr = new SpelExpression( - parsedTemplateExpr, - typing.Typed[String], - Standard, - evaluationContextPreparer - ) - List(Placeholder(compiledExpr)) + // Cache the parsed expression using memoization or a cache map + List(Placeholder(getOrCreateSpelExpression(spelExpr))) case compositeExpr: CompositeStringExpression => - compositeExpr.getExpressions.toList.flatMap(parseTemplate) + // Use more efficient collection transformation + compositeExpr.getExpressions.iterator.map(parseTemplate).flatten.toList case other => throw new IllegalArgumentException(s"Unsupported expression type: [${other.getClass.getName}]") } flavour.languageId match { case Language.SpelTemplate => Some(parseTemplate(parsed.parsed)) case _ => None } } +private def getOrCreateSpelExpression(spelExpr: org.springframework.expression.spel.standard.SpelExpression): SpelExpression = { + // Implement caching logic here + val parsedTemplateExpr = ParsedSpelExpression(spelExpr.getExpressionString, parsed.parser, spelExpr) + new SpelExpression( + parsedTemplateExpr, + typing.Typed[String], + Standard, + evaluationContextPreparer + ) +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
engine/flink/components/base-tests/src/test/scala/pl/touk/nussknacker/engine/flink/TemplateLazyParameterTest.scala(1 hunks)scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/compile/nodecompilation/EvaluableLazyParameter.scala(2 hunks)scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/compile/nodecompilation/EvaluableLazyParameterCreator.scala(3 hunks)scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/compile/nodecompilation/ParameterEvaluator.scala(1 hunks)scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/spel/SpelExpression.scala(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- engine/flink/components/base-tests/src/test/scala/pl/touk/nussknacker/engine/flink/TemplateLazyParameterTest.scala
- scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/compile/nodecompilation/ParameterEvaluator.scala
🔇 Additional comments (5)
scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/compile/nodecompilation/EvaluableLazyParameter.scala (1)
85-101: Consider alternative approaches to parameter type determination
The current implementation uses language-based pattern matching to determine the parameter type. As discussed in previous reviews, there might be more robust alternatives:
- Add an
isTemplateParameterflag inParameterdefinition - Use pattern matching on the expression type itself
The current approach couples the parameter type to the language, which might be too restrictive.
Let's verify the expression types in use:
scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/compile/nodecompilation/EvaluableLazyParameterCreator.scala (2)
45-52: LGTM! Good use of the factory pattern.
The refactoring to use EvaluableLazyParameterFactory.build improves encapsulation and maintainability by centralizing the creation logic in a dedicated factory.
23-23: Verify the impact of the API change.
The return type change from EvaluableLazyParameter[T] to LazyParameter[T] makes the API more flexible but could affect existing code that expects specifically EvaluableLazyParameter[T].
✅ Verification successful
Let me gather more context about the usage patterns to better understand the impact.
Let me search for one more pattern to understand how the return value is used.
Based on the evidence gathered from the shell scripts, I can now provide a final response:
Return type change from EvaluableLazyParameter[T] to LazyParameter[T] is safe
The change is well-contained and safe because:
- The implementation still handles
EvaluableLazyParameterthrough pattern matching as seen in the original file - No direct dependencies on
EvaluableLazyParameterreturn type were found in the codebase - The default strategy in
ParameterEvaluatoralready works with the genericLazyParametertype - This change provides better flexibility while maintaining backward compatibility
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for direct usage of EvaluableLazyParameter type to identify potential breaking changes
rg -t scala "EvaluableLazyParameter\[.*?\]" --no-heading
Length of output: 1197
Script:
#!/bin/bash
# Search for pattern matching and type casting involving LazyParameter and EvaluableLazyParameter
rg -t scala "LazyParameter.*?[=:].*?Evaluable" -A 2 --no-heading
# Search for create method invocations
ast-grep --pattern 'create($$$)'
Length of output: 3752
Script:
#!/bin/bash
# Look for usages of the create method in EvaluableLazyParameterCreator and how its return value is handled
rg -t scala "EvaluableLazyParameterCreator.*?create" -B 2 -A 4 --no-heading
# Also check for any pattern matching on LazyParameter
ast-grep --pattern 'case $_: LazyParameter[$_] => $$$'
Length of output: 134
scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/spel/SpelExpression.scala (2)
31-32: LGTM: Import statements are well-organized
The new imports are correctly scoped and necessary for the template expression functionality.
103-122: Verify template parsing edge cases
The implementation should be tested with various template patterns to ensure robustness.
✅ Verification successful
Template parsing implementation appears sufficiently tested
The implementation has adequate test coverage and error handling:
- Template expressions are tested in
TemplateLazyParameterTestwith real-world usage patterns (e.g.,Hello#{#input}) - Error handling for unsupported expressions is implemented
- The implementation handles all standard SpEL template cases: literals, SpEL expressions, and composite expressions
- The code is used in production scenarios as evidenced by the test cases
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for template expression test cases to verify coverage
rg -l "SpelTemplate|templateSubexpressions" --type scala --glob "*Test*.scala"
# Look for actual template usage in the codebase
ast-grep --pattern 'Language.SpelTemplate'
Length of output: 6742
Script:
#!/bin/bash
# Let's examine the test files to see how template expressions are tested
rg -A 10 "SpelTemplate|templateSubexpressions" scenario-api/src/test/scala/pl/touk/nussknacker/engine/ScenarioApiShowcasesTest.scala
# Check template lazy parameter tests
rg -A 10 "SpelTemplate|templateSubexpressions" engine/flink/components/base-tests/src/test/scala/pl/touk/nussknacker/engine/flink/TemplateLazyParameterTest.scala
# Look for actual template expression implementations
ast-grep --pattern 'class $_ {
$$$
templateSubexpressions$$$
$$$
}'
Length of output: 1490
Script:
#!/bin/bash
# Let's look for test implementations of SpelTemplateAstOperationService
rg -A 15 "class SpelTemplateAstOperationService"
# Search for any test cases involving template expressions
rg -A 10 "test.*template" --type scala --glob "*Test*.scala"
# Look for template-related error handling tests
rg "throw.*IllegalArgumentException.*Unsupported expression" --type scala
Length of output: 2094
...c/main/scala/pl/touk/nussknacker/engine/compile/nodecompilation/EvaluableLazyParameter.scala
Outdated
Show resolved
Hide resolved
...scala/pl/touk/nussknacker/engine/compile/nodecompilation/EvaluableLazyParameterCreator.scala
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
engine/flink/components/base-tests/src/test/scala/pl/touk/nussknacker/engine/flink/TemplateLazyParameterTest.scala (3)
62-62: Remove debug println statementDebug print statements should be removed before merging.
- println(result)
51-68: Consider adding more test casesWhile the current test case validates basic functionality, consider adding tests for:
- Empty input values
- Special characters in template
- Multiple placeholders
- Error cases (invalid templates)
72-76: Add documentation for the sink implementationPlease add ScalaDoc comments explaining:
- The purpose of this sink
- Expected input format
- Template processing behavior
- Any limitations or requirements
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
engine/flink/components/base-tests/src/test/scala/pl/touk/nussknacker/engine/flink/TemplateLazyParameterTest.scala(1 hunks)
🔇 Additional comments (3)
engine/flink/components/base-tests/src/test/scala/pl/touk/nussknacker/engine/flink/TemplateLazyParameterTest.scala (3)
1-42: LGTM! Well-structured test class setup
The test class follows best practices by extending appropriate testing traits and importing necessary dependencies.
43-50: LGTM! Appropriate test runner configuration
The runner is well-configured with lazy initialization and batch mode execution, properly registering the template operation sink component.
95-95: Verify empty nodeDependencies list
The nodeDependencies method returns an empty list. Please verify if this is intentional and add a comment explaining why no dependencies are needed.
...s/base-tests/src/test/scala/pl/touk/nussknacker/engine/flink/TemplateLazyParameterTest.scala
Outdated
Show resolved
Hide resolved
...s/base-tests/src/test/scala/pl/touk/nussknacker/engine/flink/TemplateLazyParameterTest.scala
Outdated
Show resolved
Hide resolved
...c/main/scala/pl/touk/nussknacker/engine/compile/nodecompilation/EvaluableLazyParameter.scala
Outdated
Show resolved
Hide resolved
scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/spel/SpelExpression.scala
Outdated
Show resolved
Hide resolved
scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/spel/SpelExpression.scala
Outdated
Show resolved
Hide resolved
...c/main/scala/pl/touk/nussknacker/engine/compile/nodecompilation/EvaluableLazyParameter.scala
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/compile/nodecompilation/EvaluableLazyParameter.scala (2)
51-51: Consider making returnType configurableThe returnType is hardcoded to String, which might be too restrictive for some use cases. Consider making it configurable through the constructor.
- override def returnType: TypingResult = Typed[String] + override def returnType: TypingResult = compiledParameter.returnType
64-66: Improve error messages for better debuggingThe error messages could be more descriptive to help with debugging. Include more context about the actual expression received.
- throw new IllegalStateException("Non SpEL-template expression received in SpelTemplateLazyParameter") + throw new IllegalStateException(s"Expression '${expression.original}' is a SpEL expression but lacks template subexpressions") - case _ => throw new IllegalStateException("Non SpEL expression received in SpelTemplateLazyParameter") + case other => throw new IllegalStateException(s"Expected SpEL expression but got ${other.getClass.getSimpleName} with value '${other.original}'")Also applies to: 75-75
scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/spel/SpelExpression.scala (1)
106-116: Consider extracting SpelExpression creation logicThe creation of a new SpelExpression for placeholders could be extracted to a helper method to improve maintainability and reduce code duplication if similar logic is needed elsewhere.
Consider applying this refactor:
+ private def createPlaceholderExpression( + spelExpr: org.springframework.expression.spel.standard.SpelExpression + ): SpelExpression = { + val parsedPlaceholderExpr = ParsedSpelExpression( + spelExpr.getExpressionString, + parsed.parser, + spelExpr + ) + new SpelExpression( + parsedPlaceholderExpr, + typing.Typed[String], + Standard, + evaluationContextPreparer + ) + } lazy val templateSubexpressions: Option[List[SpelTemplateExpressionPart]] = { def parseParts(expression: Expression): List[SpelTemplateExpressionPart] = expression match { case lit: LiteralExpression => List(Literal(lit.getExpressionString)) case spelExpr: org.springframework.expression.spel.standard.SpelExpression => - val parsedPlaceholderExpr = ParsedSpelExpression(spelExpr.getExpressionString, parsed.parser, spelExpr) - val compiledExpr = new SpelExpression( - parsedPlaceholderExpr, - typing.Typed[String], - Standard, - evaluationContextPreparer - ) - List(Placeholder(compiledExpr)) + List(Placeholder(createPlaceholderExpression(spelExpr))) case compositeExpr: CompositeStringExpression => compositeExpr.getExpressions.toList.flatMap(parseParts) case other => throw new IllegalArgumentException(s"Unsupported expression type: [${other.getClass.getName}]") }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/compile/nodecompilation/EvaluableLazyParameter.scala(2 hunks)scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/spel/SpelExpression.scala(3 hunks)
🔇 Additional comments (3)
scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/spel/SpelExpression.scala (3)
31-32: LGTM: Imports are correctly organized
The new imports are properly placed and necessary for the template expression functionality.
84-89: LGTM: Well-structured template expression parts
The sealed trait hierarchy with Literal and Placeholder case classes is well-designed and follows Scala best practices. The naming reflects the previous discussion about making the distinction clear in human-spoken sentences.
103-122: LGTM: Well-implemented template expression parsing
The implementation correctly handles different expression types and properly flattens composite expressions. The lazy evaluation and error handling are implemented appropriately.
...c/main/scala/pl/touk/nussknacker/engine/compile/nodecompilation/EvaluableLazyParameter.scala
Outdated
Show resolved
Hide resolved
...c/main/scala/pl/touk/nussknacker/engine/compile/nodecompilation/EvaluableLazyParameter.scala
Outdated
Show resolved
Hide resolved
- 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
…dedicated LazyParameter class
Template parts handling approach change: expression level instead of LazyParameter level
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (14)
components-api/src/main/scala/pl/touk/nussknacker/engine/api/TemplateEvaluationResult.scala (4)
3-5: Add scaladoc to document the class purpose and behavior.The implementation is clean and handles edge cases well. Consider adding documentation to explain:
- The purpose of this class in template evaluation
- The relationship between rendered parts and the final template
- Example usage
+/** + * Represents the result of evaluating a template, containing a list of rendered parts + * that can be either literals or evaluated expressions. + * + * @param renderedParts List of template parts after evaluation + */ case class TemplateEvaluationResult(renderedParts: List[TemplateRenderedPart]) {
7-9: Add scaladoc to document the trait's role.Good use of sealed trait for the ADT. Consider adding documentation to explain:
- The purpose of this trait
- The contract that implementing classes must fulfill
+/** + * Represents a part of a rendered template. + * Implementations can represent different types of template parts + * (e.g., literal text or evaluated expressions). + */ sealed trait TemplateRenderedPart {
11-15: Add documentation and consider input validation.The implementation is clean. Consider these improvements:
- Add documentation to explain the purpose of each case class
- Consider adding validation for empty values if that's an invalid state
object TemplateRenderedPart { + /** + * Represents a literal (constant) part of the template. + * @param value The literal text + */ case class RenderedLiteral(value: String) extends TemplateRenderedPart + /** + * Represents a part of the template that was derived from evaluating an expression. + * @param value The result of expression evaluation + */ case class RenderedSubExpression(value: String) extends TemplateRenderedPart }
1-15: Well-designed type hierarchy for template evaluation.The implementation provides a clean and type-safe foundation for template evaluation:
- Good use of ADT pattern with sealed trait
- Clear separation between literal and expression parts
- Immutable design following functional programming principles
- Flexible structure that can be extended if new types of template parts are needed in the future
This design aligns well with the changes in other files mentioned in the summary, particularly with
SpelExpressionandSpelTemplatePartsService.engine/flink/management/dev-model/src/main/scala/pl/touk/nussknacker/engine/management/sample/source/SqlSource.scala (1)
Line range hint
1-17: Consider adding documentation for template evaluationSince this change is part of a larger template evaluation feature, consider adding documentation:
- Add scaladoc to explain how template evaluation works in this context
- Update the "only for test FE sql editor" comment to clarify if/how template evaluation affects testing
- Consider adding an entry in the developer documentation about template evaluation in SQL sources
scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/spel/SpelExpressionValidator.scala (1)
22-28: Pattern matching logic looks good, consider adding documentationThe pattern matching hierarchy is well-structured, with specific cases handled before general ones. The new case for String to TemplateEvaluationResult conversion is properly positioned.
Consider adding a comment explaining the String to TemplateEvaluationResult conversion case, as it's a non-obvious type conversion that future maintainers might need to understand:
+ // Special case: Allow String values to be automatically converted to TemplateEvaluationResult case a if a == Typed[String] && expectedType == Typed[TemplateEvaluationResult] => Valid(collected)components/sql/src/test/scala/pl/touk/nussknacker/sql/service/DatabaseQueryEnricherValidationTest.scala (1)
68-70: Consider enhancing test assertionsWhile the test case has been correctly updated to use TemplateEvaluationResult, consider strengthening the assertions to verify:
- The actual content/structure of the output variable
- The type of the result set
- The number of records returned
Example enhancement:
result match { case service.FinalResults(ctx, _, _) => ctx.contains("out") shouldBe true val outVar = ctx.getVariable("out") outVar.typingInfo should be(/* expected type */) // Add more specific assertions case _ => fail("Enricher does not return final results") }engine/flink/components/base-tests/src/test/scala/pl/touk/nussknacker/engine/flink/SpelTemplateLazyParameterTest.scala (2)
38-57: Consider adding more test cases for comprehensive coverage.While the current test case verifies basic template functionality, consider adding tests for:
- Empty input values
- Special characters in templates
- Multiple interpolation points
- Error cases (malformed templates)
103-111: Consider enhancing error handling with specific error types.The
collectHandlingErrorsblock could benefit from more specific error handling to provide better error messages for different failure scenarios (e.g., template parsing errors vs evaluation errors).designer/server/src/main/scala/pl/touk/nussknacker/ui/definition/DefinitionsService.scala (1)
179-181: Add documentation for type conversion rationale.The implementation is correct, but would benefit from documentation explaining why
TemplateEvaluationResultneeds to be converted toStringfor UI display.Add a comment like:
+ // Convert TemplateEvaluationResult to String for UI display since template evaluation + // results need to be rendered as editable string values in the interface private def toUIType(typingResult: TypingResult): TypingResult = { if (typingResult == Typed[TemplateEvaluationResult]) Typed[String] else typingResult }components/sql/src/main/scala/pl/touk/nussknacker/sql/service/DatabaseQueryEnricher.scala (1)
142-150: LGTM: Template evaluation handling added correctly.The change properly integrates template evaluation support:
- Type-safe handling of
TemplateEvaluationResult- Appropriate rendering before validation
- Consistent error handling with rendered template
Consider adding template validation at compile-time to catch template syntax errors early. This could prevent runtime issues with malformed templates.
scenario-compiler/src/test/scala/pl/touk/nussknacker/engine/testcomponents/SpelTemplatePartsService.scala (2)
28-33: Consider making 'template' parameter required if it's always expected.The
spelTemplateParameteris currently defined as an optional parameter. However, in both the context transformation and implementation, it is always expected to be provided. To avoid potential issues when the parameter is not supplied, consider making it a required parameter.
35-35: Specify 'State' type more precisely or remove if unused.The
Statetype is overridden asAny. IfStateis not used in this component, you can omit this override for brevity. If it is used with a specific type, consider specifying it explicitly for better type safety and code clarity.scenario-compiler/src/test/scala/pl/touk/nussknacker/engine/spel/SpelExpressionSpec.scala (1)
84-87: Add ScalaDoc comments for the new parameter inevaluateSyncThe addition of
skipReturnTypeCheckenhances flexibility. While changes to method signatures in test classes are acceptable, adding a ScalaDoc comment explaining the purpose ofskipReturnTypeCheckwould improve code maintainability and readability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (17)
components-api/src/main/scala/pl/touk/nussknacker/engine/api/TemplateEvaluationResult.scala(1 hunks)components/sql/src/main/scala/pl/touk/nussknacker/sql/service/DatabaseQueryEnricher.scala(3 hunks)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)docs/Changelog.md(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)engine/flink/management/dev-model/src/main/scala/pl/touk/nussknacker/engine/management/sample/source/SqlSource.scala(1 hunks)engine/flink/schemed-kafka-components-utils/src/main/scala/pl/touk/nussknacker/engine/schemedkafka/FlinkUniversalSchemaBasedSerdeProvider.scala(1 hunks)extensions-api/src/main/scala/pl/touk/nussknacker/engine/api/process/ClassExtractionSettings.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/main/scala/pl/touk/nussknacker/engine/spel/SpelExpressionValidator.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/spel/SpelExpressionSpec.scala(3 hunks)scenario-compiler/src/test/scala/pl/touk/nussknacker/engine/testcomponents/SpelTemplatePartsService.scala(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- engine/flink/schemed-kafka-components-utils/src/main/scala/pl/touk/nussknacker/engine/schemedkafka/FlinkUniversalSchemaBasedSerdeProvider.scala
🚧 Files skipped from review as they are similar to previous changes (1)
- scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/spel/SpelExpression.scala
🧰 Additional context used
📓 Learnings (5)
components-api/src/main/scala/pl/touk/nussknacker/engine/api/TemplateEvaluationResult.scala (1)
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:22.695Z
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.
engine/flink/components/base-tests/src/test/scala/pl/touk/nussknacker/engine/flink/SpelTemplateLazyParameterTest.scala (1)
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.895Z
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.
scenario-compiler/src/test/scala/pl/touk/nussknacker/engine/InterpreterSpec.scala (2)
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.
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.895Z
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.
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.
scenario-compiler/src/test/scala/pl/touk/nussknacker/engine/testcomponents/SpelTemplatePartsService.scala (1)
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.895Z
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.
🔇 Additional comments (28)
components-api/src/main/scala/pl/touk/nussknacker/engine/api/TemplateEvaluationResult.scala (1)
1-2: LGTM!
Clean package declaration with no unnecessary imports.
engine/flink/management/dev-model/src/main/scala/pl/touk/nussknacker/engine/management/sample/source/SqlSource.scala (2)
7-7: LGTM!
Clean import addition for the new TemplateEvaluationResult type.
14-14: Verify template evaluation behavior
The parameter type change from String to TemplateEvaluationResult aligns with the broader template evaluation enhancement. Please ensure that all callers of this method have been updated to handle the new type.
scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/spel/SpelExpressionValidator.scala (1)
6-6: LGTM: Import statement properly added
The import for TemplateEvaluationResult is correctly placed and necessary for the new template evaluation functionality.
engine/flink/management/dev-model/src/main/scala/pl/touk/nussknacker/engine/management/sample/LoggingService.scala (2)
36-36: LGTM: Proper usage of TemplateEvaluationResult
The change correctly accesses the rendered template string using the new TemplateEvaluationResult type.
21-23:
Breaking change: Update message parameter type to use TemplateEvaluationResult
The change from LazyParameter[String] to LazyParameter[TemplateEvaluationResult] is a breaking change that will affect existing scenarios using the LoggingService.
Please ensure that:
- An entry is added to
_MigrationGuide.mdexplaining the migration path for existing scenarios - The changelog reflects this breaking change
scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/definition/component/methodbased/MethodDefinition.scala (1)
68-68: LGTM! Consider adding test coverage for error messages.
The change improves error message clarity by using the value property of ParameterName. This will make debugging easier for users.
Consider adding a test case to verify the format of error messages when parameters are missing.
components/sql/src/test/scala/pl/touk/nussknacker/sql/service/DatabaseQueryEnricherValidationTest.scala (2)
3-3: LGTM: Import changes align with new template functionality
The new imports for template-related classes are properly organized and necessary for the updated test implementation.
Also applies to: 8-8
36-38: LGTM: Test case properly adapted for template evaluation
The test case has been correctly updated to use TemplateEvaluationResult while maintaining the original test semantics for SQL error handling.
Please verify that the error message "unexpected token: FROM in statement [select from]" is still the expected error format with the new template evaluation system.
engine/flink/components/base-tests/src/test/scala/pl/touk/nussknacker/engine/flink/SpelTemplateLazyParameterTest.scala (4)
1-27: LGTM! Imports are well-organized and appropriate.
The imports cover all necessary dependencies for Flink testing, ScalaTest framework, and Nussknacker components.
28-37: LGTM! Test setup follows best practices.
The test class is properly configured with:
- Appropriate test traits for Flink testing
- Batch execution mode for deterministic testing
- Required custom components registration
61-74: LGTM! Transformer configuration is well-structured.
The transformer object is properly configured with:
- Appropriate trait extensions for Flink integration
- Well-defined template parameter with correct editor
114-114: Verify the safety of type casting.
The asInstanceOf cast to DataStream[ValueWithContext[AnyRef]] might be risky. Consider:
- Using a more specific type instead of AnyRef
- Adding a comment explaining why this cast is safe
designer/server/src/main/scala/pl/touk/nussknacker/ui/definition/DefinitionsService.scala (2)
12-13: LGTM! Required imports for template evaluation support.
The new imports support the template evaluation functionality being added.
167-167: LGTM! Verify template parameter handling in UI.
The type transformation ensures template evaluation results are properly converted to string types for UI display. Please verify that template parameters are correctly rendered in the UI across different component types.
components/sql/src/main/scala/pl/touk/nussknacker/sql/service/DatabaseQueryEnricher.scala (2)
44-44: LGTM: Parameter declaration simplified.
The change from queryParamDeclaration to direct Parameter[String] simplifies the code while maintaining the same functionality and SQL editor support.
Please ensure all usages of the old queryParamDeclaration have been updated throughout the codebase.
132-132: LGTM: Parameter list updated consistently.
The parameter list update aligns with the simplified parameter declaration approach.
extensions-api/src/main/scala/pl/touk/nussknacker/engine/api/process/ClassExtractionSettings.scala (2)
8-8: LGTM!
The import is properly placed and follows the codebase's import organization pattern.
114-114: LGTM! Appropriate exclusion of TemplateEvaluationResult
The addition of TemplateEvaluationResult to excluded classes is correct as it's a programmatic type that shouldn't be exposed in UI suggestions, consistent with how other similar types are handled.
scenario-compiler/src/test/scala/pl/touk/nussknacker/engine/InterpreterSpec.scala (2)
1027-1071: Well-structured test implementation with comprehensive coverage.
The new test case effectively validates the template evaluation functionality using table-driven testing. It covers important scenarios:
- Combined subexpression with literal value
- Single literal value
- Single function call expression
- Empty value handling
The test structure is clean and maintainable with:
- Clear scenario descriptions
- Good use of
withCluefor better error reporting - Consistent test data organization
Line range hint 1187-1204: LGTM: Clean implementation of template parameter handling.
The changes to ServiceUsingSpelTemplate properly integrate with the new template evaluation functionality:
- Descriptive parameter naming
- Correct handling of
TemplateEvaluationResultin the return type - Proper parameter configuration with SpelTemplateParameterEditor
docs/Changelog.md (2)
16-17: LGTM! Clear and concise feature description.
The changelog entry properly describes the new feature addition for accessing expression parts in SpEL templates, with appropriate PR reference.
Line range hint 1-1000: Verify changelog format consistency.
The changelog follows a good structure with:
- Version numbers as main headers
- Clear categorization of changes (End-user, Administrator)
- PR references
- Chronological ordering (newest first)
scenario-compiler/src/test/scala/pl/touk/nussknacker/engine/testcomponents/SpelTemplatePartsService.scala (2)
64-64: Confirmed appropriate use of extractOrEvaluateLazyParamUnsafe.
Based on previous learnings, using extractOrEvaluateLazyParamUnsafe is acceptable in this context to reduce unnecessary code complexity.
This comment references learnings retrieved from long-term memory regarding the acceptable use of unsafe methods in the Nussknacker project.
65-68: Ensure comprehensive pattern matching over renderedParts.
If TemplateRenderedPart is a sealed trait with only RenderedLiteral and RenderedSubExpression as subclasses, this pattern matching is sufficient. However, if more subclasses might be added in the future, consider adding a default case to handle unexpected types and prevent a potential MatchError at runtime.
scenario-compiler/src/test/scala/pl/touk/nussknacker/engine/spel/SpelExpressionSpec.scala (3)
1091-1092: Verify necessity of skipReturnTypeCheck = true in evaluateSync
Is it necessary to bypass the return type check in this test case? Adjusting the expected return type might allow the test to proceed without setting skipReturnTypeCheck to true, enhancing test robustness.
1097-1099: Ensure skipReturnTypeCheck = true is required in these tests
Consider whether it's possible to avoid setting skipReturnTypeCheck = true by aligning the expected return types in the test assertions. This could improve the reliability and clarity of the tests.
1103-1105: Confirm the use of skipReturnTypeCheck = true in evaluateSync
Please verify if skipping the return type check is essential for this test. If feasible, adjusting the test to use the appropriate return type may enhance test consistency.
…rts in LazyParameter (#7162) Add TemplateEvaluationResult to evaluate SpEL expression parts in LazyParameter --------- Co-authored-by: Arek Burdach <arek.burdach@gmail.com>
…rts in LazyParameter (#7162) Add TemplateEvaluationResult to evaluate SpEL expression parts in LazyParameter --------- Co-authored-by: Arek Burdach <arek.burdach@gmail.com>
Describe your changes
Checklist before merge
Summary by CodeRabbit
Release Notes
New Features
SpelTemplatePartsServicefor enhanced template rendering capabilities.Bug Fixes
Documentation
Tests
SpelTemplatePartsService.