Validate non-raw editor data against schema #3767

mproch · 2022-12-05T09:34:33Z

No description provided.

arkadius · 2022-12-06T08:12:52Z

...ils/src/main/scala/pl/touk/nussknacker/engine/schemedkafka/sink/AvroSinkValueParameter.scala

@@ -79,6 +81,6 @@ object AvroSinkSingleValueParameter {
      isLazyParameter = true,
      defaultValue = defaultValue.map(_.expression)
    )
-    SinkSingleValueParameter(parameter)
+    SinkSingleValueParameter(parameter, new AvroSchemaOutputValidator(schema, ValidationMode.lax))


why hardcoded lax here? Is it only used in some legacy code where we don't have this validation mode? If so, IMO we should move it upper to those legacy classes

arkadius · 2022-12-06T08:13:58Z

utils/json-utils/src/main/scala/pl/touk/nussknacker/engine/json/JsonSinkValueParameter.scala

@@ -41,8 +43,7 @@ object JsonSinkValueParameter {
      isLazyParameter = true,
      defaultValue = defaultValue.map(_.expression)
    )
-
-    SinkSingleValueParameter(parameter)
+    SinkSingleValueParameter(parameter, new JsonSchemaOutputValidator(schema, ValidationMode.lax))


The same as above

arkadius · 2022-12-06T08:17:42Z

...k/nussknacker/engine/lite/components/requestresponse/LiteRequestResponseFunctionalTest.scala

+    }
+
+    result should matchPattern {
+      case Invalid(NonEmptyList(CustomNodeError("response", _, Some("field")), Nil)) =>


Can you add checking of some part of message? Because without this I can't see what exactly this test doing. It is 'trala' and should be 'additionalField'?

arkadius · 2022-12-06T08:28:45Z

...omponents-utils/src/main/scala/pl/touk/nussknacker/engine/util/sinkvalue/SinkValueData.scala

+    override def validateParams(resultTypes: List[(String, BaseDefinedParameter)], prefix: List[String])(implicit nodeId: NodeId): ValidatedNel[ProcessCompilationError, Unit] = {
+      fields.map { case (fieldName, param) =>
+        val newPrefix = prefix :+ fieldName
+        val prefixed = resultTypes.filter(k => k._1.startsWith(newPrefix.mkString(".")))


Uhh, what happened here? Why this "new prefix" is here?

arkadius · 2022-12-06T08:30:46Z

...l/touk/nussknacker/engine/schemedkafka/schemaregistry/confluent/UniversalSchemaSupport.scala

@@ -31,7 +31,7 @@ trait UniversalSchemaSupport {
  def typeDefinition(schema: ParsedSchema): TypingResult
  def extractSinkValueParameter(schema: ParsedSchema)(implicit nodeId: NodeId): ValidatedNel[ProcessCompilationError, SinkValueParameter]
  def sinkValueEncoder(schema: ParsedSchema, mode: ValidationMode): Any => AnyRef
-  def validateRawOutput(schema: ParsedSchema, t: TypingResult, mode: ValidationMode): ValidatedNel[OutputValidatorError, Unit]
+  def parameterValidator(schema: ParsedSchema, mode: ValidationMode): SinkValueValidator


Why we need this method if we already have SinkValueParameter which takes responsibility of the validation of this nested fields. Why not to do it in extractSinkValueParameter. Let's keep this class simple because it will be hard to add new schema supports incrementally.

arkadius · 2022-12-06T08:32:54Z

...rc/main/scala/pl/touk/nussknacker/engine/schemedkafka/encode/AvroSchemaOutputValidator.scala

@@ -25,7 +26,7 @@ private[encode] case class AvroSchemaExpected(schema: Schema) extends OutputVali
  override def expected: String = AvroSchemaOutputValidatorPrinter.print(schema)
 }

-class AvroSchemaOutputValidator(validationMode: ValidationMode) extends LazyLogging {
+class AvroSchemaOutputValidator(schema: Schema, validationMode: ValidationMode) extends LazyLogging with SinkValueValidator {


Why this schema have to be on class level? It is tricky now, because we pass around nested schemas in private methods.

arkadius · 2022-12-06T08:41:36Z

.../src/main/scala/pl/touk/nussknacker/engine/schemedkafka/sink/UniversalKafkaSinkFactory.scala

+      (SchemaVersionParamName, DefinedEagerParameter(_: String, _)) ::
+      (SinkKeyParamName, _) ::
+      (SinkRawEditorParamName, DefinedEagerParameter(true, _)) :: Nil, _) =>
+        //TODO: use resultType here to show type hints on UI (breaks some tests...)


I don't understand this comment. resultType in which parameters? rawValueParm?

arkadius · 2022-12-06T08:50:57Z

...omponents-utils/src/main/scala/pl/touk/nussknacker/engine/util/sinkvalue/SinkValueData.scala

  }

-  case class SinkSingleValueParameter(value: Parameter) extends SinkValueParameter
+  case class SinkSingleValueParameter(value: Parameter, validator: SinkValueValidator) extends SinkValueParameter {
+    override def validateParams(resultTypes: List[(String, BaseDefinedParameter)], prefix: List[String])(implicit nodeId: NodeId): ValidatedNel[ProcessCompilationError, Unit] = resultTypes match {


I have one confusion about how this validateParams is related to Parameter.typ. Am I right that we do two validations:

against this Parameter.typ in our "engine"

using validateParams in component?

If so, IMO we should write some comment about this relation eg. that this Parameter.typ should be "wider" than validateParams to not limit this validations.

arkadius · 2022-12-06T09:03:02Z

...k/nussknacker/engine/lite/components/requestresponse/LiteRequestResponseFunctionalTest.scala

+      )
+
+    val result = runner.runWithRequests(scenario) { invoker =>
+      invoker(HttpRequest(HttpMethods.POST, entity = "{}")).rightValue


arkadius · 2023-02-20T17:17:51Z

Closing in favour of #3994

mproch changed the base branch from staging to release/1.7 December 5, 2022 09:34

github-actions bot added client client main fe docs ui and removed client client main fe ui docs labels Dec 5, 2022

mproch force-pushed the json_schema_fix branch from a398c00 to e1cd6db Compare December 5, 2022 11:16

github-actions bot added the docs label Dec 5, 2022

mproch marked this pull request as ready for review December 5, 2022 12:25

arkadius reviewed Dec 6, 2022

View reviewed changes

mproch marked this pull request as draft December 6, 2022 14:16

mproch force-pushed the json_schema_fix branch from 672cd86 to a9dde9f Compare December 21, 2022 08:54

github-actions bot added client client main fe submodules ui labels Dec 21, 2022

mproch changed the base branch from release/1.7 to staging December 21, 2022 08:55

mproch added 8 commits December 21, 2022 10:21

fixes

182624b

Validate non-raw editor

9490927

fix

20d64dd

Refactor

93d2f3e

test

8142290

refactor

260b75a

fix test cases...

9be259b

Fix test

aa10305

mproch force-pushed the json_schema_fix branch from a9dde9f to 9da5dfa Compare December 21, 2022 09:24

github-actions bot removed the client client main fe label Dec 21, 2022

github-actions bot removed submodules docs ui labels Dec 21, 2022

log deploy

c1a34a1

mproch force-pushed the json_schema_fix branch from 9da5dfa to c1a34a1 Compare December 21, 2022 10:35

arkadius closed this Feb 20, 2023

coutoPL deleted the json_schema_fix branch August 17, 2023 08:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Validate non-raw editor data against schema #3767

Validate non-raw editor data against schema #3767

mproch commented Dec 5, 2022

arkadius Dec 6, 2022

arkadius Dec 6, 2022

arkadius Dec 6, 2022

arkadius Dec 6, 2022

arkadius Dec 6, 2022

arkadius Dec 6, 2022

arkadius Dec 6, 2022

arkadius Dec 6, 2022 •

edited

Loading

arkadius Dec 6, 2022

arkadius commented Feb 20, 2023

Validate non-raw editor data against schema #3767

Validate non-raw editor data against schema #3767

Conversation

mproch commented Dec 5, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arkadius Dec 6, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arkadius commented Feb 20, 2023

arkadius Dec 6, 2022 •

edited

Loading