Return scenario validation error when incompatible changes were intro…

…duced for a component (#7504)
TouK · Jan 28, 2025 · e3375ca · e3375ca
1 parent 66546db
commit e3375ca
Show file tree

Hide file tree

Showing 8 changed files with 123 additions and 23 deletions.
diff --git a/common-api/src/main/scala/pl/touk/nussknacker/engine/graph/expression/Expression.scala b/common-api/src/main/scala/pl/touk/nussknacker/engine/graph/expression/Expression.scala
@@ -4,26 +4,36 @@ import io.circe.{Decoder, Encoder}
 import io.circe.generic.JsonCodec
 import io.circe.syntax.EncoderOps
 import pl.touk.nussknacker.engine.graph.expression.Expression.Language
+import pl.touk.nussknacker.engine.graph.expression.Expression.Language.{
+  DictKeyWithLabel,
+  Spel,
+  SpelTemplate,
+  TabularDataDefinition
+}
 
 // TODO in the future 'expression' should be a dedicated type rather than String, it would for example make DictKeyWithLabelExpression handling prettier
 @JsonCodec case class Expression(language: Language, expression: String)
 
 object Expression {
 
-  sealed trait Language extends Serializable
+  sealed trait Language extends Serializable {
+
+    override def toString: String = this match {
+      case Spel                  => "spel"
+      case SpelTemplate          => "spelTemplate"
+      case DictKeyWithLabel      => "dictKeyWithLabel"
+      case TabularDataDefinition => "tabularDataDefinition"
+    }
+
+  }
 
   object Language {
     object Spel                  extends Language
     object SpelTemplate          extends Language
     object DictKeyWithLabel      extends Language
     object TabularDataDefinition extends Language
 
-    implicit val encoder: Encoder[Language] = Encoder.encodeString.contramap {
-      case Spel                  => "spel"
-      case SpelTemplate          => "spelTemplate"
-      case DictKeyWithLabel      => "dictKeyWithLabel"
-      case TabularDataDefinition => "tabularDataDefinition"
-    }
+    implicit val encoder: Encoder[Language] = Encoder.encodeString.contramap(_.toString)
 
     implicit val decoder: Decoder[Language] = Decoder.decodeString.emap {
       case "spel"                  => Right(Spel)

diff --git a/...s-api/src/main/scala/pl/touk/nussknacker/engine/api/context/ProcessCompilationError.scala b/...s-api/src/main/scala/pl/touk/nussknacker/engine/api/context/ProcessCompilationError.scala
@@ -4,6 +4,7 @@ import cats.Applicative
 import cats.data.ValidatedNel
 import pl.touk.nussknacker.engine.api.NodeId
 import pl.touk.nussknacker.engine.api.context.ProcessCompilationError.InASingleNode
+import pl.touk.nussknacker.engine.api.definition.ParameterEditor
 import pl.touk.nussknacker.engine.api.parameter.ParameterName
 import pl.touk.nussknacker.engine.api.generics.ExpressionParseError.ErrorDetails
 import pl.touk.nussknacker.engine.api.process.ProcessName
@@ -328,6 +329,14 @@ object ProcessCompilationError {
   final case class UnsupportedDictParameterEditorType(paramName: ParameterName, typ: String, nodeIds: Set[String])
       extends PartSubGraphCompilationError
 
+  final case class IncompatibleParameterDefinitionModification(
+      paramName: ParameterName,
+      language: Language,
+      parameterEditor: Option[ParameterEditor],
+      nodeId: String
+  ) extends PartSubGraphCompilationError
+      with InASingleNode
+
   final case class UnknownFragmentOutput(id: String, nodeIds: Set[String]) extends ProcessCompilationError
 
   final case class DisablingManyOutputsFragment(fragmentNodeId: String)

diff --git a/...odel/src/main/scala/pl/touk/nussknacker/restmodel/validation/PrettyValidationErrors.scala b/...odel/src/main/scala/pl/touk/nussknacker/restmodel/validation/PrettyValidationErrors.scala
@@ -259,6 +259,14 @@ object PrettyValidationErrors {
           description = message,
           paramName = Some(paramName)
         )
+      case IncompatibleParameterDefinitionModification(paramName, language, parameterEditor, _) =>
+        node(
+          message =
+            "There was an incompatible change to the component's parameter definition. Please drag a new component from creator panel to use the current definition",
+          description =
+            s"Incompatible change to the parameter's definition detected. $parameterEditor editor doesn't support '$language' language",
+          paramName = Some(paramName)
+        )
     }
   }
 

diff --git a/designer/server/src/test/scala/pl/touk/nussknacker/test/utils/domain/ProcessTestData.scala b/designer/server/src/test/scala/pl/touk/nussknacker/test/utils/domain/ProcessTestData.scala
@@ -5,7 +5,7 @@ import pl.touk.nussknacker.engine.api.component.{ComponentGroupName, ProcessingM
 import pl.touk.nussknacker.engine.api.definition._
 import pl.touk.nussknacker.engine.api.dict.DictDefinition
 import pl.touk.nussknacker.engine.api.graph.{Edge, ProcessProperties, ScenarioGraph}
-import pl.touk.nussknacker.engine.api.parameter.ParameterName
+import pl.touk.nussknacker.engine.api.parameter.{ParameterName, ValueInputWithDictEditor}
 import pl.touk.nussknacker.engine.api.process.{ProcessName, ProcessingType, VersionId}
 import pl.touk.nussknacker.engine.api.typed.typing.{Typed, Unknown}
 import pl.touk.nussknacker.engine.api.{FragmentSpecificData, MetaData, ProcessAdditionalFields, StreamMetaData}
@@ -32,7 +32,6 @@ import pl.touk.nussknacker.test.mock.{
   StubModelDataWithModelDefinition,
   TestAdditionalUIConfigProvider
 }
-
 import pl.touk.nussknacker.ui.definition.ScenarioPropertiesConfigFinalizer
 import pl.touk.nussknacker.ui.definition.editor.JavaSampleEnum
 import pl.touk.nussknacker.ui.process.ProcessService.UpdateScenarioCommand
@@ -366,6 +365,26 @@ object ProcessTestData {
     List.empty
   )
 
+  val sampleFragmentWithPreset: CanonicalProcess =
+    CanonicalProcess(
+      MetaData(sampleFragmentName.value, FragmentSpecificData()),
+      List(
+        FlatNode(
+          FragmentInputDefinition(
+            "in",
+            List(
+              FragmentParameter(
+                ParameterName("param1"),
+                FragmentClazzRef[String]
+              ).copy(valueEditor = Some(ValueInputWithDictEditor("rgb", allowOtherValue = false)))
+            )
+          )
+        ),
+        canonicalnode.FlatNode(FragmentOutputDefinition("out1", "output", List.empty))
+      ),
+      List.empty
+    )
+
   val sampleFragment: CanonicalProcess = {
     CanonicalProcess(
       MetaData(sampleFragmentName.value, FragmentSpecificData()),

diff --git a/designer/server/src/test/scala/pl/touk/nussknacker/ui/api/ProcessesResourcesSpec.scala b/designer/server/src/test/scala/pl/touk/nussknacker/ui/api/ProcessesResourcesSpec.scala
@@ -19,8 +19,9 @@ import pl.touk.nussknacker.engine.api.deployment._
 import pl.touk.nussknacker.engine.api.deployment.simple.{SimpleProcessStateDefinitionManager, SimpleStateStatus}
 import pl.touk.nussknacker.engine.api.graph.{ProcessProperties, ScenarioGraph}
 import pl.touk.nussknacker.engine.api.process.{ProcessId, ProcessName, VersionId}
-import pl.touk.nussknacker.engine.build.ScenarioBuilder
+import pl.touk.nussknacker.engine.build.{GraphBuilder, ScenarioBuilder}
 import pl.touk.nussknacker.engine.canonicalgraph.CanonicalProcess
+import pl.touk.nussknacker.engine.graph.expression.Expression
 import pl.touk.nussknacker.engine.spel.SpelExtension._
 import pl.touk.nussknacker.restmodel.scenariodetails.ScenarioWithDetails
 import pl.touk.nussknacker.restmodel.validation.ValidationResults.ValidationResult
@@ -33,6 +34,12 @@ import pl.touk.nussknacker.test.config.WithAccessControlCheckingDesignerConfig.T
 }
 import pl.touk.nussknacker.test.config.WithAccessControlCheckingDesignerConfig.{TestCategory, TestProcessingType}
 import pl.touk.nussknacker.test.config.{WithAccessControlCheckingDesignerConfig, WithMockableDeploymentManager}
+import pl.touk.nussknacker.test.utils.domain.ProcessTestData.{
+  existingSinkFactory,
+  existingSourceFactory,
+  sampleFragmentOneOut,
+  sampleFragmentWithPreset
+}
 import pl.touk.nussknacker.test.utils.domain.{ProcessTestData, TestFactory}
 import pl.touk.nussknacker.test.utils.scalas.AkkaHttpExtensions.toRequestEntity
 import pl.touk.nussknacker.ui.api.description.scenarioActivity.Dtos.Legacy.ProcessActivity
@@ -233,6 +240,42 @@ class ProcessesResourcesSpec
     }
   }
 
+  test("should return validation error when fragment was modified in an incompatible way") {
+    // Create a fragment that has a preset parameter and a scenario that uses that fragment
+    val fragmentWithPreset = CanonicalProcessConverter.toScenarioGraph(sampleFragmentWithPreset)
+    val fragmentName       = sampleFragmentWithPreset.name.value
+    val scenarioWithFragment = ScenarioBuilder
+      .streaming(processName.value)
+      .source("source", existingSourceFactory)
+      .fragment(
+        fragmentName,
+        fragmentName,
+        List(("param1", Expression.dictKeyWithLabel("H000000", Some("Black")))),
+        Map("output" -> "fragmentResult"),
+        Map(
+          "output" -> GraphBuilder.emptySink("sink", existingSinkFactory)
+        )
+      )
+    saveFragment(ProcessName(fragmentName), fragmentWithPreset, category = Category1)(succeed)
+    saveCanonicalProcess(scenarioWithFragment, category = Category1)(succeed)
+
+    // Modify fragment so the parameter is no longer a preset
+    updateProcess(CanonicalProcessConverter.toScenarioGraph(sampleFragmentOneOut), ProcessName(fragmentName))(succeed)
+
+    // Verify that incompatible changes were introduced in the fragment and thus error is returned for scenario
+    Get(
+      s"/api/processes/${processName.value}"
+    ) ~> withReaderUser() ~> applicationRoute ~> check {
+      val scenarioDetails = responseAs[ScenarioWithDetails]
+      scenarioDetails.validationResult.value.errors.invalidNodes should not be empty
+      scenarioDetails.validationResult.value.errors.invalidNodes
+        .get(fragmentName)
+        .value
+        .find(_.typ == "IncompatibleParameterDefinitionModification")
+        .value
+    }
+  }
+
   test("not allow to archive still running process") {
     createDeployedExampleScenario(processName, category = Category1)
     MockableDeploymentManager.configureScenarioStatuses(

diff --git a/docs/Changelog.md b/docs/Changelog.md
@@ -66,6 +66,7 @@
 * [#7458](https://github.com/TouK/nussknacker/pull/7458) Flink scenario testing mechanism and scenario state verification mechanism: mini cluster created once and reused each time
 * [#7498](https://github.com/TouK/nussknacker/pull/7498) Support many migrations loaded using SPI. Loaded migration numbers
   cannot overlap, if they do, an exception is thrown.
+* [#7504](https://github.com/TouK/nussknacker/pull/7504) Return scenario validation error when an incompatible change was introduced in a fragment or component parameter definition.
 
 ## 1.18
 

diff --git a/...la/pl/touk/nussknacker/engine/management/scenariotesting/FlinkProcessTestRunnerSpec.scala b/...la/pl/touk/nussknacker/engine/management/scenariotesting/FlinkProcessTestRunnerSpec.scala
@@ -764,13 +764,15 @@ class FlinkProcessTestRunnerSpec
           )
           .emptySink("out", "valueMonitor", "Value" -> "#input.value1".spel)
 
-      val dictEditorException = intercept[IllegalStateException] {
+      val dictEditorException = intercept[IllegalArgumentException] {
         prepareTestRunner(useIOMonadInInterpreter).runTests(
           process,
           ScenarioTestData(List(createTestRecord(id = "2", value1 = 2)))
         )
       }
-      dictEditorException.getMessage shouldBe "DictKeyWithLabel expression can only be used with DictParameterEditor, got Some(DualParameterEditor(StringParameterEditor,RAW))"
+      dictEditorException.getMessage.startsWith(
+        "Compilation errors: IncompatibleParameterDefinitionModification(ParameterName(static),dictKeyWithLabel,Some(DualParameterEditor(StringParameterEditor,RAW))"
+      ) shouldBe true
     }
 
     "should run correctly when parameter was modified by AdditionalUiConfigProvider with dict editor and flink was provided with additional config" in {

diff --git a/scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/compile/ExpressionCompiler.scala b/scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/compile/ExpressionCompiler.scala
@@ -25,6 +25,7 @@ import pl.touk.nussknacker.engine.expression.parse.{
 import pl.touk.nussknacker.engine.graph.evaluatedparam.{BranchParameters, Parameter => NodeParameter}
 import pl.touk.nussknacker.engine.graph.expression.Expression
 import pl.touk.nussknacker.engine.graph.expression.Expression.Language
+import pl.touk.nussknacker.engine.graph.expression.Expression.Language.DictKeyWithLabel
 import pl.touk.nussknacker.engine.language.dictWithLabel.DictKeyWithLabelExpressionParser
 import pl.touk.nussknacker.engine.language.tabularDataDefinition.TabularDataDefinitionParser
 import pl.touk.nussknacker.engine.spel.SpelExpressionParser
@@ -240,7 +241,7 @@ class ExpressionCompiler(
       paramName: ParameterName
   )(
       implicit nodeId: NodeId
-  ) = {
+  ): ValidatedNel[PartSubGraphCompilationError, Expression] = {
     def substitute(dictId: String) = {
       DictKeyWithLabelExpressionParser
         .parseDictKeyWithLabelExpression(expression.expression)
@@ -262,18 +263,25 @@ class ExpressionCompiler(
         )
     }
 
-    if (expression.language == Language.DictKeyWithLabel && !expression.expression.isBlank)
+    def isDictKeyWithLabel(expression: Expression): Boolean =
+      expression.language == DictKeyWithLabel
+
+    val incompatibleChangeToParameterDefinitionDetected: ValidatedNel[PartSubGraphCompilationError, Expression] =
+      invalidNel(IncompatibleParameterDefinitionModification(paramName, expression.language, editor, nodeId.id))
+
+    def validateAndSubstitute(expression: Expression): ValidatedNel[PartSubGraphCompilationError, Expression] = {
       editor match {
-        case Some(DictParameterEditor(dictId)) => substitute(dictId)
-        case Some(DualParameterEditor(DictParameterEditor(dictId), _)) =>
-          substitute(dictId) // in `RAW` mode, expression.language is SpEL, and no substitution/validation is done
-        case editor =>
-          throw new IllegalStateException(
-            s"DictKeyWithLabel expression can only be used with DictParameterEditor, got $editor"
-          )
+        case Some(DictParameterEditor(dictId)) if isDictKeyWithLabel(expression) =>
+          if (expression.expression.isBlank) Valid(expression) else substitute(dictId)
+        case Some(DualParameterEditor(DictParameterEditor(dictId), _)) if isDictKeyWithLabel(expression) =>
+          if (expression.expression.isBlank) Valid(expression) else substitute(dictId)
+        case Some(DictParameterEditor(_)) if !isDictKeyWithLabel(expression) =>
+          incompatibleChangeToParameterDefinitionDetected
+        case _ if isDictKeyWithLabel(expression) => incompatibleChangeToParameterDefinitionDetected
+        case _                                   => Valid(expression)
       }
-    else
-      Valid(expression)
+    }
+    validateAndSubstitute(expression)
   }
 
   def compileValidator(