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 index 388084c62a2..029a3d3f20c 100644 --- 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,13 +4,28 @@ 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 @@ -18,12 +33,7 @@ object Expression { 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/components-api/src/main/scala/pl/touk/nussknacker/engine/api/context/ProcessCompilationError.scala b/components-api/src/main/scala/pl/touk/nussknacker/engine/api/context/ProcessCompilationError.scala index d4d544bd48b..9beaa392c3a 100644 --- a/components-api/src/main/scala/pl/touk/nussknacker/engine/api/context/ProcessCompilationError.scala +++ b/components-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/designer/restmodel/src/main/scala/pl/touk/nussknacker/restmodel/validation/PrettyValidationErrors.scala b/designer/restmodel/src/main/scala/pl/touk/nussknacker/restmodel/validation/PrettyValidationErrors.scala index 0d89489ce9b..f3f1effdbf8 100644 --- a/designer/restmodel/src/main/scala/pl/touk/nussknacker/restmodel/validation/PrettyValidationErrors.scala +++ b/designer/restmodel/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 index 0fbc61496cf..209b2593591 100644 --- 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 index 1cda12db9a8..a20089deebf 100644 --- 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 index c5d9df1d8ad..ccdc31f17d4 100644 --- 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/engine/flink/management/src/test/scala/pl/touk/nussknacker/engine/management/scenariotesting/FlinkProcessTestRunnerSpec.scala b/engine/flink/management/src/test/scala/pl/touk/nussknacker/engine/management/scenariotesting/FlinkProcessTestRunnerSpec.scala index bfb1153446d..2250a0ed934 100644 --- a/engine/flink/management/src/test/scala/pl/touk/nussknacker/engine/management/scenariotesting/FlinkProcessTestRunnerSpec.scala +++ b/engine/flink/management/src/test/scala/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 index 0100f2b248d..d53f505d3ac 100644 --- 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(