Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Template parts handling approach change: expression level instead of LazyParameter level #7174

Merged
3 changes: 1 addition & 2 deletions build.sbt
Original file line number Diff line number Diff line change
Expand Up @@ -1816,8 +1816,7 @@ lazy val flinkBaseComponentsTests = (project in flink("components/base-tests"))
)
.dependsOn(
flinkComponentsTestkit % Test,
flinkTableApiComponents % Test,
scenarioCompiler % "test->test"
flinkTableApiComponents % Test
)

lazy val flinkKafkaComponents = (project in flink("components/kafka"))
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
package pl.touk.nussknacker.engine.api

import pl.touk.nussknacker.engine.api.LazyParameter.TemplateLazyParameter.TemplateExpression
import pl.touk.nussknacker.engine.api.LazyParameter.{Evaluate, MappedLazyParameter, ProductLazyParameter}
import pl.touk.nussknacker.engine.api.typed.typing.{Typed, TypingResult}

Expand Down Expand Up @@ -69,25 +68,6 @@ object LazyParameter {

trait CustomLazyParameter[+T <: AnyRef] extends LazyParameter[T]

trait TemplateLazyParameter[T <: AnyRef] extends LazyParameter[T] {
def templateExpression: TemplateExpression
}

object TemplateLazyParameter {
case class TemplateExpression(parts: List[TemplateExpressionPart])
sealed trait TemplateExpressionPart

object TemplateExpressionPart {
case class Literal(value: String) extends TemplateExpressionPart

trait Placeholder extends TemplateExpressionPart {
val evaluate: Evaluate[String]
}

}

}

final class ProductLazyParameter[T <: AnyRef, Y <: AnyRef](
val arg1: LazyParameter[T],
val arg2: LazyParameter[Y]
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
package pl.touk.nussknacker.engine.api

case class TemplateEvaluationResult(renderedParts: List[TemplateRenderedPart]) {
def renderedTemplate: String = renderedParts.map(_.value).mkString("")
}
Comment on lines +3 to +5
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add validation and optimize string concatenation

The implementation is clean but could benefit from some improvements:

  1. Add validation for null/empty list
  2. 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.

Suggested change
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
}
}
}


sealed trait TemplateRenderedPart {
def value: String
}

object TemplateRenderedPart {
case class RenderedLiteral(value: String) extends TemplateRenderedPart

case class RenderedSubExpression(value: String) extends TemplateRenderedPart
}
Comment on lines +11 to +15
Copy link

@coderabbitai coderabbitai bot Nov 19, 2024

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add documentation and validation for rendered part types

The case classes need:

  1. Documentation explaining their distinct purposes
  2. Validation for non-null values
  3. 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.

Suggested change
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")
}
}

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link

Choose a reason for hiding this comment

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

@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.

3 changes: 3 additions & 0 deletions docs/MigrationGuide.md
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,9 @@ To see the biggest differences please consult the [changelog](Changelog.md).
* [#6988](https://github.com/TouK/nussknacker/pull/6988) Removed unused API classes: `MultiMap`, `TimestampedEvictableStateFunction`.
`MultiMap` was incorrectly handled by Flink's default Kryo serializer, so if you want to copy it to your code
you should write and register a proper serializer.
* [#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.
Comment on lines +55 to +57
Copy link

@coderabbitai coderabbitai bot Nov 19, 2024

Choose a reason for hiding this comment

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

💡 Codebase verification

⚠️ Potential issue

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:

  • LoggingService in engine/flink/management/dev-model
  • SpelTemplatePartsService in scenario-compiler/test
  • Custom components in engine/flink/components/base-tests

The migration guide correctly states that:

  1. The return type changes from String to TemplateEvaluationResult
  2. 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 SpelTemplateParameterEditor or SqlParameterEditor will now use TemplateEvaluationResult instead of String
  • Use TemplateEvaluationResult.renderedTemplate to access the previous String value
🏁 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

Copy link
Member Author

Choose a reason for hiding this comment

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

components in dev-model are not exposed publicly so it is not important for an end user, these same with classes inside tests modules

Copy link

Choose a reason for hiding this comment

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

@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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link

Choose a reason for hiding this comment

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

@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.


### REST API changes

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,118 @@
package pl.touk.nussknacker.engine.flink

import com.typesafe.config.ConfigFactory
import org.apache.flink.api.common.functions.FlatMapFunction
import org.apache.flink.api.connector.source.Boundedness
import org.apache.flink.streaming.api.datastream.DataStream
import org.apache.flink.util.Collector
import org.scalatest.funsuite.AnyFunSuite
import org.scalatest.matchers.should.Matchers
import pl.touk.nussknacker.engine.api.TemplateRenderedPart.{RenderedLiteral, RenderedSubExpression}
import pl.touk.nussknacker.engine.api._
import pl.touk.nussknacker.engine.api.component.{BoundedStreamComponent, ComponentDefinition}
import pl.touk.nussknacker.engine.api.context.ValidationContext
import pl.touk.nussknacker.engine.api.context.transformation.{DefinedLazyParameter, NodeDependencyValue, SingleInputDynamicComponent}
import pl.touk.nussknacker.engine.api.definition.{NodeDependency, OutputVariableNameDependency, Parameter, SpelTemplateParameterEditor}
import pl.touk.nussknacker.engine.api.parameter.ParameterName
import pl.touk.nussknacker.engine.api.typed.typing.Typed
import pl.touk.nussknacker.engine.build.ScenarioBuilder
import pl.touk.nussknacker.engine.flink.api.process.{AbstractOneParamLazyParameterFunction, FlinkCustomNodeContext, FlinkCustomStreamTransformation}
import pl.touk.nussknacker.engine.flink.test.FlinkSpec
import pl.touk.nussknacker.engine.flink.util.test.FlinkTestScenarioRunner._
import pl.touk.nussknacker.engine.graph.expression.Expression
import pl.touk.nussknacker.engine.process.FlinkJobConfig.ExecutionMode
import pl.touk.nussknacker.engine.spel.SpelExtension._
import pl.touk.nussknacker.engine.util.test.TestScenarioRunner
import pl.touk.nussknacker.test.ValidatedValuesDetailedMessage

class SpelTemplateLazyParameterTest extends AnyFunSuite with FlinkSpec with Matchers with ValidatedValuesDetailedMessage {

private lazy val runner = TestScenarioRunner
.flinkBased(ConfigFactory.empty(), flinkMiniCluster)
.withExecutionMode(ExecutionMode.Batch)
.withExtraComponents(
List(ComponentDefinition("spelTemplatePartsCustomTransformer", SpelTemplatePartsCustomTransformer))
)
.build()

test("flink custom transformer using spel template rendered parts") {
val scenario = ScenarioBuilder
.streaming("test")
.source("source", TestScenarioRunner.testDataSource)
.customNode(
"custom",
"output",
"spelTemplatePartsCustomTransformer",
"template" -> Expression.spelTemplate(s"Hello#{#input}")
)
.emptySink("sink", TestScenarioRunner.testResultSink, "value" -> "#output".spel)

val result = runner.runWithData(scenario, List(1, 2, 3), Boundedness.BOUNDED)
result.validValue.errors shouldBe empty
result.validValue.successes shouldBe List(
"[Hello]-literal[1]-subexpression",
"[Hello]-literal[2]-subexpression",
"[Hello]-literal[3]-subexpression"
)
}

}

object SpelTemplatePartsCustomTransformer
extends CustomStreamTransformer
with SingleInputDynamicComponent[FlinkCustomStreamTransformation]
with BoundedStreamComponent {

private val spelTemplateParameterName = ParameterName("template")

private val spelTemplateParameter = Parameter
.optional[String](spelTemplateParameterName)
.copy(
isLazyParameter = true,
editor = Some(SpelTemplateParameterEditor)
)

override type State = Unit

override def contextTransformation(context: ValidationContext, dependencies: List[NodeDependencyValue])(
implicit nodeId: NodeId
): SpelTemplatePartsCustomTransformer.ContextTransformationDefinition = {
case TransformationStep(Nil, _) => NextParameters(List(spelTemplateParameter))
case TransformationStep((`spelTemplateParameterName`, DefinedLazyParameter(_)) :: Nil, _) =>
val outName = OutputVariableNameDependency.extract(dependencies)
FinalResults(context.withVariableUnsafe(outName, Typed[String]), List.empty)
}

override def nodeDependencies: List[NodeDependency] = List(OutputVariableNameDependency)

override def implementation(
params: Params,
dependencies: List[NodeDependencyValue],
finalState: Option[Unit]
): FlinkCustomStreamTransformation = {
val templateLazyParam: LazyParameter[TemplateEvaluationResult] =
params.extractUnsafe[LazyParameter[TemplateEvaluationResult]](spelTemplateParameterName)
FlinkCustomStreamTransformation {
(dataStream: DataStream[Context], flinkCustomNodeContext: FlinkCustomNodeContext) =>
dataStream.flatMap(
new AbstractOneParamLazyParameterFunction[TemplateEvaluationResult](
templateLazyParam,
flinkCustomNodeContext.lazyParameterHelper
) with FlatMapFunction[Context, ValueWithContext[String]] {
override def flatMap(value: Context, out: Collector[ValueWithContext[String]]): Unit = {
collectHandlingErrors(value, out) {
val templateResult = evaluateParameter(value)
val result = templateResult.renderedParts.map {
case RenderedLiteral(value) => s"[$value]-literal"
case RenderedSubExpression(value) => s"[$value]-subexpression"
}.mkString
ValueWithContext(result, value)
}
}
},
flinkCustomNodeContext.valueWithContextInfo.forClass[String]
).asInstanceOf[DataStream[ValueWithContext[AnyRef]]]
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

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 = {
  ...
}

}
}

}

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,9 @@ object LoggingService extends EagerService {
def prepare(
@ParamName("logger") @Nullable loggerName: String,
@ParamName("level") @DefaultValue("T(org.slf4j.event.Level).DEBUG") level: Level,
@ParamName("message") @SimpleEditor(`type` = SimpleEditorType.SPEL_TEMPLATE_EDITOR) message: LazyParameter[String]
@ParamName("message") @SimpleEditor(`type` = SimpleEditorType.SPEL_TEMPLATE_EDITOR) message: LazyParameter[
TemplateEvaluationResult
]
)(implicit metaData: MetaData, nodeId: NodeId): ServiceInvoker =
new ServiceInvoker {

Expand All @@ -31,7 +33,7 @@ object LoggingService extends EagerService {
collector: ServiceInvocationCollector,
componentUseCase: ComponentUseCase
): Future[Any] = {
val msg = message.evaluate(context)
val msg = message.evaluate(context).renderedTemplate
level match {
case Level.TRACE => logger.trace(msg)
case Level.DEBUG => logger.debug(msg)
Expand Down
Loading
Loading