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

[NU-983] Add missing validations to base components #4953

Merged
merged 37 commits into from Dec 4, 2023

Conversation

mslabek
Copy link
Member

@mslabek mslabek commented Oct 26, 2023

Describe your changes

The goal is to make the validation consistent between FE and BE for base components by adding BE validations that are done only on the FE.

Main goal - Bringing base components FE validations to BE

In base components these validations are:

  1. Switch - expression in edges - NotNullParameterValidator
  2. Filter - expression - NotNullParameterValidator
  3. Variable - expression - MandatoryParameterValidator
  4. MapVariable:
    • field key - unique value in relation to other field names in the node
    • field value - MandatoryParameterValidator

Also, this removes the default Mandatory value validation on the FE if no validator was set. This is going to be removed anyways after removing the FE validators.

Api changes

In MapVariable (and FragmentOuptut) and in Switch the validated parameters are not standard fields and they require their own solutions to be correctly displayed. BE can control under which field the error is displayed by the fieldName in the NodeValidationError object.

  1. In switch nodes fieldName = name of the node to which the choice points to
  2. In nodes using a user-controlled dynamic map of key-value pairs (MapVariable (and FragmentOuptut) the simplest solution was to rely on index of the field. This means that FE now has to display them in order of the index:
    • for keys fieldName = $fields-${index}-$key
    • for values fieldName = $fields-${index}-$value

Problems

  1. Drag'n'drop in MapVariable - this is problematic - after doing drag'n'drop the errors are displayed "as previously" until the next validation comes with the index after dropping. Solving this is problematic and imo out of scope of this PR.

Checklist before merge

  • Related issue ID is placed at the beginning of PR title in [brackets] (can be GH issue or Nu Jira issue)
  • Code is cleaned from temporary changes and commented out lines
  • Parts of the code that are not easy to understand are documented in the code
  • Changes are covered by automated tests
  • Showcase in dev-application.conf added to demonstrate the feature
  • Documentation added or updated
  • Added entry in Changelog.md describing the change from the perspective of a public distribution user
  • Added MigrationGuide.md entry in the appropriate subcategory if introducing a breaking change
  • Verify that PR will be squashed during merge

@mslabek mslabek force-pushed the NU-983/filter-required-expression-validation branch 2 times, most recently from cda1be6 to 9cbaab0 Compare October 29, 2023 15:40
@github-actions
Copy link
Contributor

github-actions bot commented Oct 29, 2023

created: #5129
⚠️ Be careful! Snapshot changes are not necessarily the cause of the error. Check the logs.

@mslabek mslabek changed the title [NU-983] Required expression validation in filter [NU-983] Add missing validations to base components Oct 30, 2023
@mslabek mslabek force-pushed the NU-983/filter-required-expression-validation branch 2 times, most recently from 564dc4f to 290ee13 Compare November 3, 2023 15:06
@github-actions github-actions bot added client client main fe ui labels Nov 5, 2023
@mslabek mslabek force-pushed the NU-983/filter-required-expression-validation branch from 81787c7 to 2b89ad3 Compare November 15, 2023 09:20
@mslabek mslabek force-pushed the NU-983/filter-required-expression-validation branch from 2ac22e7 to 40156b4 Compare November 20, 2023 15:18
@mslabek mslabek force-pushed the NU-983/filter-required-expression-validation branch 3 times, most recently from 17e3acc to 4ff0994 Compare November 23, 2023 13:19
@mslabek mslabek force-pushed the NU-983/filter-required-expression-validation branch from 4ff0994 to 832548a Compare November 27, 2023 22:02
@mslabek mslabek marked this pull request as ready for review November 28, 2023 09:02
@mslabek mslabek force-pushed the NU-983/filter-required-expression-validation branch from a840b44 to ff38bde Compare November 28, 2023 13:35
@github-actions github-actions bot added the docs label Nov 28, 2023
@mslabek mslabek force-pushed the NU-983/filter-required-expression-validation branch from fc9d263 to bc24811 Compare November 29, 2023 13:34
docs/Changelog.md Outdated Show resolved Hide resolved
# Conflicts:
#	docs/Changelog.md
#	interpreter/src/main/scala/pl/touk/nussknacker/engine/compile/nodecompilation/NodeCompiler.scala
#	interpreter/src/test/scala/pl/touk/nussknacker/engine/compile/NodeDataValidatorSpec.scala
@mslabek mslabek merged commit 4f94922 into staging Dec 4, 2023
16 of 17 checks passed
@mslabek mslabek deleted the NU-983/filter-required-expression-validation branch December 4, 2023 08:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
client client main fe docs ui
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants