[NU-7164] Add basic parser for fragment input definitions #7167
Conversation
📝 WalkthroughWalkthroughThe changes in this pull request introduce modifications to various components, primarily enhancing the handling of class definitions and validation processes. Key updates include the renaming of the Changes
Possibly related PRs
Suggested labels
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
DeamonDev
force-pushed
the
nu-7164-validate-fragment-input-params-on-backend
branch
3 times, most recently
from
204fd64
to
360f0ab
Compare
|
created: #7197 |
DeamonDev
force-pushed
the
nu-7164-validate-fragment-input-params-on-backend
branch
2 times, most recently
from
a2ea8eb
to
455f826
Compare
DeamonDev
force-pushed
the
nu-7164-validate-fragment-input-params-on-backend
branch
from
455f826
to
9034882
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (11)
scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/definition/fragment/FragmentComponentDefinitionExtractor.scala (1)
Line range hint
18-25: Consider adding ScalaDoc for the classGiven that this class now handles class definitions as part of fragment component extraction, it would be helpful to add ScalaDoc documentation explaining:
- The purpose of the
classDefinitionsparameter- How class definitions are used in fragment parameter validation
- Any requirements or constraints for the provided class definitions
designer/server/src/test/scala/pl/touk/nussknacker/ui/definition/EdgeTypesPreparerTest.scala (1)
25-25: Consider adding test coverage for non-empty classDefinitionsSince FragmentComponentDefinitionExtractor now supports class definitions, it would be valuable to add test cases that verify the behavior with non-empty class definitions to ensure complete coverage of the new functionality.
designer/client/src/components/graph/node-modal/fragment-input-definition/FragmentInputDefinition.tsx (1)
38-64: Document the type option format changeGiven this change modifies how type values are represented, please ensure:
- The change is documented in the changelog
- If this is a breaking change, update the migration guide
- Add code comments explaining why
useTypeOptionsV2usesdisplayinstead ofrefClazzNamescenario-compiler/src/test/scala/pl/touk/nussknacker/engine/compile/nodecompilation/FragmentParameterValidatorTest.scala (1)
Line range hint
1-89: Consider adding tests for ClassDefinition parameter.Since the AI summary mentions that
FragmentParameterValidatornow accepts a set ofClassDefinitionas a parameter, consider adding test cases that verify the validation behavior with differentClassDefinitionscenarios.engine/flink/executor/src/main/scala/pl/touk/nussknacker/engine/process/compiler/FlinkProcessCompilerDataFactory.scala (1)
140-151: Consider documenting the definition adjustment processGiven that this is core compilation infrastructure, consider:
- Adding code comments explaining the role of class definitions in the adjustment process
- Documenting any performance implications of passing the full class definition set
- Adding examples of typical adjustments in the documentation
scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/compile/nodecompilation/NodeCompiler.scala (1)
Line range hint
169-210: LGTM! Clean refactoring of validation calls.The validation logic has been cleanly refactored to use the new
fragmentParameterValidatorinstance while maintaining the same functionality. The error handling and validation context management remain robust.Consider extracting the validation chain into a private method for improved readability:
+ private def validateFragmentInput( + fragmentInputDefinition: FragmentInputDefinition, + parameterDefinitions: Writer[List[PartSubGraphCompilationError], List[Parameter]], + validationContext: ValidationContext + )(implicit nodeId: NodeId) = { + val parameterNameValidation = fragmentParameterValidator.validateParameterNames(parameterDefinitions.value) + val fixedValuesErrors = fragmentInputDefinition.parameters + .map(fragmentParameterValidator.validateFixedExpressionValues(_, validationContext, expressionCompiler)) + .sequence + .map(_ => ()) + val dictValueEditorErrors = fragmentInputDefinition.parameters + .map(fragmentParameterValidator.validateValueInputWithDictEditor(_, expressionConfig.dictionaries, classLoader)) + .sequence + .map(_ => ()) + parameterNameValidation |+| fixedValuesErrors |+| dictValueEditorErrors + }scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/definition/fragment/FragmentParameterTypingParser.scala (1)
12-15: Offer assistance in implementing an AST-based parser for arbitrary depth expressionsThe TODO comment indicates that the current parser using regexes is insufficient for handling arbitrary depth expressions in type parsing. I can assist in developing a recursive parser that constructs an Abstract Syntax Tree (AST) to accurately parse complex and nested type expressions.
Would you like me to help implement this parser or open a new GitHub issue to track this enhancement?
scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/definition/fragment/FragmentParametersDefinitionExtractor.scala (1)
39-44: Enhanced flexibility withclassDefinitionsparameterIntroducing
classDefinitionsas a constructor parameter with a default value allows the extractor to utilize provided class definitions, increasing flexibility and robustness. The initialization offragmentParameterTypingParserwithclassLoaderandclassDefinitionsensures proper parsing capabilities.Consider adding unit tests for new functionality
To ensure the robustness of the newly introduced
classDefinitionsparameter andfragmentParameterTypingParser, consider adding unit tests to cover these changes, especially to verify parsing logic and error handling.Would you like assistance in creating the unit tests for these additions?
scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/compile/nodecompilation/NodeDataValidator.scala (2)
Line range hint
23-23: Address the TODO comment to removeValidationNotPerformedThe comment
// TODO: Remove ValidationNotPerformedindicates an intention to refactor or eliminate theValidationNotPerformedcase object. Consider addressing this TODO by removingValidationNotPerformedand updating the code to handle all validation scenarios appropriately without it.Would you like assistance in refactoring the code to remove
ValidationNotPerformed?
Line range hint
75-107: Consider refactoring thevalidateFragmentmethod for improved readabilityThe
validateFragmentmethod contains nested validations and complex error handling, which can make it harder to read and maintain. Refactoring parts of this method into smaller, well-named helper functions could enhance readability and modularity.scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/compile/nodecompilation/FragmentParameterValidator.scala (1)
160-164: Enhance error handling for type parsing failuresIn the
validateValueInputWithDictEditormethod, when parsing the class name to a typing result,.getOrElse(Unknown)is used. While this prevents exceptions, it may conceal underlying issues if the class name is invalid or cannot be parsed. Consider handling parsing failures explicitly by logging an error or returning a validation error to make debugging easier and maintain strict type safety.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (19)
designer/client/src/components/graph/node-modal/fragment-input-definition/FragmentInputDefinition.tsx(1 hunks)designer/server/src/main/scala/pl/touk/nussknacker/ui/definition/AlignedComponentsDefinitionProvider.scala(1 hunks)designer/server/src/test/scala/pl/touk/nussknacker/ui/definition/DefinitionsServiceSpec.scala(1 hunks)designer/server/src/test/scala/pl/touk/nussknacker/ui/definition/EdgeTypesPreparerTest.scala(1 hunks)designer/server/src/test/scala/pl/touk/nussknacker/ui/definition/component/ComponentGroupsPreparerSpec.scala(1 hunks)designer/server/src/test/scala/pl/touk/nussknacker/ui/definition/component/ComponentsUsageHelperTest.scala(1 hunks)engine/flink/executor/src/main/scala/pl/touk/nussknacker/engine/process/compiler/FlinkProcessCompilerDataFactory.scala(2 hunks)engine/flink/executor/src/main/scala/pl/touk/nussknacker/engine/process/compiler/StubbedFlinkProcessCompilerDataFactory.scala(3 hunks)scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/compile/ProcessCompiler.scala(1 hunks)scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/compile/ProcessCompilerData.scala(1 hunks)scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/compile/nodecompilation/FragmentParameterValidator.scala(5 hunks)scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/compile/nodecompilation/NodeCompiler.scala(4 hunks)scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/compile/nodecompilation/NodeDataValidator.scala(1 hunks)scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/definition/fragment/FragmentComponentDefinitionExtractor.scala(2 hunks)scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/definition/fragment/FragmentParameterTypingParser.scala(1 hunks)scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/definition/fragment/FragmentParametersDefinitionExtractor.scala(6 hunks)scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/definition/test/ModelDataTestInfoProvider.scala(1 hunks)scenario-compiler/src/test/scala/pl/touk/nussknacker/engine/compile/nodecompilation/FragmentParameterValidatorTest.scala(4 hunks)utils/flink-components-testkit/src/main/scala/pl/touk/nussknacker/engine/flink/util/test/FlinkProcessCompilerDataFactoryWithTestComponents.scala(2 hunks)
🔇 Additional comments (34)
scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/definition/fragment/FragmentComponentDefinitionExtractor.scala (3)
13-13: LGTM: Import addition is appropriate
The addition of ClassDefinition import is necessary to support the new class definitions parameter.
22-22: LGTM: Constructor parameter addition is well-designed
The new classDefinitions parameter is appropriately added with a specific type Set[ClassDefinition]. This aligns with the PR's objective of enhancing fragment input definition parsing.
27-27: LGTM: Parameters extractor initialization updated correctly
The parametersExtractor initialization properly passes both the classLoader and classDefinitions to the new constructor signature.
Please ensure that all callers of FragmentComponentDefinitionExtractor are updated to provide the required classDefinitions parameter.
designer/server/src/test/scala/pl/touk/nussknacker/ui/definition/EdgeTypesPreparerTest.scala (1)
25-25: Verify if Set.empty is the appropriate default for classDefinitions
The change adapts to the new FragmentComponentDefinitionExtractor API by passing Set.empty. While this works for the current test case, please verify that not providing any class definitions is the intended behavior here.
designer/server/src/main/scala/pl/touk/nussknacker/ui/definition/AlignedComponentsDefinitionProvider.scala (2)
62-62: LGTM: Class definitions parameter addition
The addition of class definitions to FragmentComponentDefinitionExtractor is a logical enhancement that will enable proper type handling for fragment input definitions.
Line range hint 44-45: Verify if nested fragments support is in scope
There's a TODO comment about fragment nesting support. Given that this PR introduces fragment input parsing capabilities, should nested fragment support be addressed as part of this work or tracked separately?
Would you like me to create a separate issue to track the nested fragments support?
designer/client/src/components/graph/node-modal/fragment-input-definition/FragmentInputDefinition.tsx (2)
44-44: Add validation for type.display property
The code assumes type.display always exists. Consider adding validation or a fallback value.
- value: type.display,
+ value: type.display ?? type.refClazzName,64-64: Verify compatibility with existing consumers
The switch from useTypeOptions to useTypeOptionsV2 changes the value format from refClazzName to display. Please ensure all consumers of these values (e.g., getDefaultFields, field processors) can handle the new format correctly.
scenario-compiler/src/test/scala/pl/touk/nussknacker/engine/compile/nodecompilation/FragmentParameterValidatorTest.scala (4)
Line range hint 26-33: LGTM! Verify test coverage remains unchanged.
The change from static to instance method call is implemented correctly. The test logic remains intact, maintaining the same validation behavior.
Line range hint 42-49: LGTM! Consistent with the pattern.
The instantiation follows the same pattern as other test cases, maintaining consistency throughout the test suite.
Line range hint 58-65: LGTM! Error case handling preserved.
The change maintains the same error validation logic while adopting the new instantiation pattern.
Line range hint 74-84: LGTM! Fixed values validation preserved.
The modification correctly implements the instance-based approach while preserving the fixed values validation logic.
utils/flink-components-testkit/src/main/scala/pl/touk/nussknacker/engine/flink/util/test/FlinkProcessCompilerDataFactoryWithTestComponents.scala (2)
11-11: LGTM: Import addition aligns with new functionality
The addition of ClassDefinition import is consistent with the PR's objective of implementing fragment input definition parsing.
65-66: Verify integration with test scenarios
The addition of classDefinitions parameter to adjustDefinitions looks correct. However, since this is in a test utility class, please ensure that:
- All test scenarios are updated to provide appropriate ClassDefinition sets
- Edge cases (empty set, null) are properly handled
scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/compile/ProcessCompilerData.scala (1)
56-56: LGTM! Verify integration with fragment validation.
The addition of definitionWithTypes.classDefinitions.all to FragmentParametersDefinitionExtractor aligns with the PR's objective of enhancing fragment input definitions parsing.
Please ensure that:
- The fragment validation logic correctly utilizes these class definitions
- The changes are covered by integration tests that verify the complete fragment parameter validation flow
engine/flink/executor/src/main/scala/pl/touk/nussknacker/engine/process/compiler/FlinkProcessCompilerDataFactory.scala (2)
12-12: LGTM: Import addition is consistent with usage
The addition of ClassDefinition import alongside ClassDefinitionSet aligns with the new parameter requirements.
140-144: Verify impact on definition adjustment logic
The addition of class definitions to adjustDefinitions expands its context. While the change is structurally sound, please ensure:
- All subclasses properly handle the new parameter
- The default implementation in this class (returning originalModelDefinition) remains valid with the additional parameter
scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/definition/test/ModelDataTestInfoProvider.scala (1)
27-30: LGTM! Verify test data generation with class definitions.
The addition of class definitions to FragmentParametersDefinitionExtractor aligns well with the PR's objective of enhancing fragment input definition parsing. The implementation maintains type safety and follows Scala best practices.
Please ensure that test data generation works correctly with various class definitions, particularly for complex fragment input scenarios.
designer/server/src/test/scala/pl/touk/nussknacker/ui/definition/component/ComponentGroupsPreparerSpec.scala (1)
193-193: Consider adding test cases for non-empty class definitions.
While passing Set.empty works, it might not thoroughly test the new class definitions functionality for fragment components. Consider:
- Adding test cases with non-empty class definitions to validate the complete feature.
- Adding a comment explaining why an empty set is acceptable here if this is intentional.
designer/server/src/test/scala/pl/touk/nussknacker/ui/definition/DefinitionsServiceSpec.scala (1)
299-299: LGTM! Consider adding edge cases for class definitions.
The addition of model.modelDefinitionWithClasses.classDefinitions.all aligns with the broader changes to enhance fragment handling with class definitions. Consider adding test cases that verify behavior with:
- Empty class definitions
- Complex nested class hierarchies
- Invalid/malformed class definitions
scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/compile/ProcessCompiler.scala (1)
355-357: Verify fragment parameter handling with invalid class definitions.
The addition of classDefinitions to FragmentParametersDefinitionExtractor enhances type handling for fragment parameters. However, since this is in the critical validation path, please ensure:
- The extractor properly handles cases where class definitions are invalid or incomplete
- Error messages are clear and helpful when class resolution fails
- Existing fragment parameters continue to work as expected with this change
Consider adding validation tests that cover edge cases such as:
- Missing class definitions
- Invalid class definitions
- Complex nested class hierarchies
designer/server/src/test/scala/pl/touk/nussknacker/ui/definition/component/ComponentsUsageHelperTest.scala (1)
179-179: LGTM! Consider adding test cases for non-empty classDefinitions.
The change correctly adapts to the new FragmentComponentDefinitionExtractor constructor signature. However, the current test only covers the empty set case for classDefinitions. Consider adding test cases with non-empty class definitions to ensure complete coverage of the new functionality.
scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/compile/nodecompilation/NodeCompiler.scala (1)
102-103: LGTM! Clean initialization of the validator.
The initialization of fragmentParameterValidator with class definitions from the extractor is well-structured and follows good practices.
engine/flink/executor/src/main/scala/pl/touk/nussknacker/engine/process/compiler/StubbedFlinkProcessCompilerDataFactory.scala (2)
13-13: Import statement added correctly
The import of ClassDefinition is necessary for the added classDefinitions parameter and is appropriately included.
66-67: Verify constructor argument order for FragmentParametersDefinitionExtractor
Confirm that the parameters definitionContext.userCodeClassLoader and classDefinitions are passed in the correct order as per the constructor's definition to ensure proper initialization.
scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/definition/fragment/FragmentParametersDefinitionExtractor.scala (5)
8-8: Added necessary import for ClassUtils
The addition of org.apache.commons.lang3.ClassUtils import is appropriate for class manipulation utilities.
17-17: Imported ClassDefinition and ClassDefinitionSet
Including ClassDefinition and ClassDefinitionSet is necessary for handling class definitions within the extractor.
33-34: Included Try import
Adding scala.util.Try prepares the codebase for potential exception handling where parsing or class loading might fail.
Line range hint 85-92: Improved validation using classDefinitions in FragmentParameterValidator
Passing classDefinitions to FragmentParameterValidator enhances parameter validation by allowing checks against provided class definitions, ensuring that parameters conform to expected class structures.
Line range hint 145-156: Verify the handling of unknown class names
When parseClassNameToTypingResult fails, returning an Unknown typing result and logging an error may impact downstream type checking. Please verify that this behavior appropriately handles cases where class names cannot be resolved, and consider if more specific error handling or user feedback is warranted to prevent silent failures.
scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/compile/nodecompilation/NodeDataValidator.scala (2)
48-51: Verify the impact of the new parameter in FragmentParametersDefinitionExtractor
The constructor of FragmentParametersDefinitionExtractor now includes classDefinitions.all as an additional parameter. Please ensure that:
- All instances where
FragmentParametersDefinitionExtractoris instantiated are updated with this new parameter. - Dependent components or classes correctly handle the updated constructor.
- There are no unintended side effects due to this change, especially in serialization or reflective operations.
Line range hint 75-107: Ensure unit tests cover the updated validation logic in validateFragment
With the changes to the validateFragment method, please verify that unit tests are updated or added to cover the new validation logic, especially concerning fragment outputs and parameter handling. This will help ensure that the method behaves as expected under various scenarios.
scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/compile/nodecompilation/FragmentParameterValidator.scala (2)
48-49: Verify the default initialization of classDefinitions
The FragmentParameterValidator is now a case class with classDefinitions defaulting to an empty set. Ensure that when an instance is created, especially outside of tests, classDefinitions is populated appropriately. An empty set might lead to incomplete type parsing in methods like validateValueInputWithDictEditor, potentially causing validation issues.
Line range hint 6-23: Approved: Necessary imports added
The additional import statements for ClassUtils, TypingResult, ClassDefinition, and FragmentParameterTypingParser are appropriate and required for the new functionality introduced. They integrate well with the existing codebase.
...client/src/components/graph/node-modal/fragment-input-definition/FragmentInputDefinition.tsx
Outdated
Show resolved
Hide resolved
...main/scala/pl/touk/nussknacker/engine/process/compiler/FlinkProcessCompilerDataFactory.scala
Show resolved
Hide resolved
...ain/scala/pl/touk/nussknacker/engine/definition/fragment/FragmentParameterTypingParser.scala
Show resolved
Hide resolved
...ala/pl/touk/nussknacker/engine/process/compiler/StubbedFlinkProcessCompilerDataFactory.scala
Show resolved
Hide resolved
DeamonDev
force-pushed
the
nu-7164-validate-fragment-input-params-on-backend
branch
from
9034882
to
fe7d67c
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/compile/nodecompilation/FragmentParameterValidator.scala (1)
48-49: Consider adding ScalaDoc for better documentation.The case class design is good, encapsulating validation logic with backward compatibility. Consider adding ScalaDoc to document the purpose and usage of the class.
designer/server/src/test/scala/pl/touk/nussknacker/ui/definition/component/ComponentGroupsPreparerSpec.scala (1)
193-193: Document the purpose of the empty set parameter.The addition of
Set.emptyas a parameter toFragmentComponentDefinitionExtractoraligns with the PR objectives for fragment input definitions handling. However, consider adding a brief comment explaining what this parameter represents and why an empty set is appropriate in the test context.designer/server/src/test/scala/pl/touk/nussknacker/ui/definition/DefinitionsServiceSpec.scala (1)
Line range hint
1-350: Add test coverage for fragment input definition parsingGiven that this PR introduces a basic parser for fragment input definitions, consider adding the following test cases:
- Basic fragment input definition parsing
- Complex input definition scenarios
- Error cases for malformed input definitions
- Edge cases (empty inputs, special characters, etc.)
This would ensure the new parser functionality is thoroughly tested.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (19)
designer/client/src/components/graph/node-modal/fragment-input-definition/FragmentInputDefinition.tsx(2 hunks)designer/server/src/main/scala/pl/touk/nussknacker/ui/definition/AlignedComponentsDefinitionProvider.scala(1 hunks)designer/server/src/test/scala/pl/touk/nussknacker/ui/definition/DefinitionsServiceSpec.scala(1 hunks)designer/server/src/test/scala/pl/touk/nussknacker/ui/definition/EdgeTypesPreparerTest.scala(1 hunks)designer/server/src/test/scala/pl/touk/nussknacker/ui/definition/component/ComponentGroupsPreparerSpec.scala(1 hunks)designer/server/src/test/scala/pl/touk/nussknacker/ui/definition/component/ComponentsUsageHelperTest.scala(1 hunks)engine/flink/executor/src/main/scala/pl/touk/nussknacker/engine/process/compiler/FlinkProcessCompilerDataFactory.scala(2 hunks)engine/flink/executor/src/main/scala/pl/touk/nussknacker/engine/process/compiler/StubbedFlinkProcessCompilerDataFactory.scala(3 hunks)scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/compile/ProcessCompiler.scala(1 hunks)scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/compile/ProcessCompilerData.scala(1 hunks)scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/compile/nodecompilation/FragmentParameterValidator.scala(5 hunks)scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/compile/nodecompilation/NodeCompiler.scala(4 hunks)scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/compile/nodecompilation/NodeDataValidator.scala(1 hunks)scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/definition/fragment/FragmentComponentDefinitionExtractor.scala(2 hunks)scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/definition/fragment/FragmentParameterTypingParser.scala(1 hunks)scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/definition/fragment/FragmentParametersDefinitionExtractor.scala(6 hunks)scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/definition/test/ModelDataTestInfoProvider.scala(1 hunks)scenario-compiler/src/test/scala/pl/touk/nussknacker/engine/compile/nodecompilation/FragmentParameterValidatorTest.scala(4 hunks)utils/flink-components-testkit/src/main/scala/pl/touk/nussknacker/engine/flink/util/test/FlinkProcessCompilerDataFactoryWithTestComponents.scala(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (15)
- designer/client/src/components/graph/node-modal/fragment-input-definition/FragmentInputDefinition.tsx
- designer/server/src/main/scala/pl/touk/nussknacker/ui/definition/AlignedComponentsDefinitionProvider.scala
- designer/server/src/test/scala/pl/touk/nussknacker/ui/definition/EdgeTypesPreparerTest.scala
- designer/server/src/test/scala/pl/touk/nussknacker/ui/definition/component/ComponentsUsageHelperTest.scala
- engine/flink/executor/src/main/scala/pl/touk/nussknacker/engine/process/compiler/StubbedFlinkProcessCompilerDataFactory.scala
- scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/compile/ProcessCompiler.scala
- scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/compile/ProcessCompilerData.scala
- scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/compile/nodecompilation/NodeCompiler.scala
- scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/compile/nodecompilation/NodeDataValidator.scala
- scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/definition/fragment/FragmentComponentDefinitionExtractor.scala
- scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/definition/fragment/FragmentParameterTypingParser.scala
- scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/definition/fragment/FragmentParametersDefinitionExtractor.scala
- scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/definition/test/ModelDataTestInfoProvider.scala
- scenario-compiler/src/test/scala/pl/touk/nussknacker/engine/compile/nodecompilation/FragmentParameterValidatorTest.scala
- utils/flink-components-testkit/src/main/scala/pl/touk/nussknacker/engine/flink/util/test/FlinkProcessCompilerDataFactoryWithTestComponents.scala
🧰 Additional context used
📓 Learnings (1)
engine/flink/executor/src/main/scala/pl/touk/nussknacker/engine/process/compiler/FlinkProcessCompilerDataFactory.scala (1)
Learnt from: DeamonDev
PR: TouK/nussknacker#7167
File: engine/flink/executor/src/main/scala/pl/touk/nussknacker/engine/process/compiler/FlinkProcessCompilerDataFactory.scala:150-151
Timestamp: 2024-11-20T12:19:55.759Z
Learning: In the Nussknacker project, the method `adjustDefinitions` in `FlinkProcessCompilerDataFactory.scala` has only one usage, allowing its signature to be changed without affecting backward compatibility.
🔇 Additional comments (8)
engine/flink/executor/src/main/scala/pl/touk/nussknacker/engine/process/compiler/FlinkProcessCompilerDataFactory.scala (3)
12-12: LGTM: Import addition is appropriate
The added import for ClassDefinition types is necessary for the new parameter types and follows Scala conventions.
140-144: LGTM: Proper passing of class definitions
The addition of classDefinitions.all as an argument to adjustDefinitions is consistent with the signature change and provides the complete set of class definitions needed for adjustment.
150-151: LGTM: Method signature enhancement
The addition of the classDefinitions parameter enhances the method's capability to work with class definitions during the adjustment process.
scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/compile/nodecompilation/FragmentParameterValidator.scala (4)
6-6: LGTM! Well-organized imports and clear type constraints.
The new imports are logically grouped and the permittedTypesForEditors constant provides a clear contract for supported editor types.
Also applies to: 18-18, 21-23
Line range hint 71-84: LGTM! Clear validation logic with descriptive error messages.
The type validation is well-implemented with clear error messages and effective pattern matching.
160-164: Consider enhancing error handling for typing parser.
While the typing parser implementation is good, consider handling parsing failures more explicitly instead of falling back to Unknown. This could help catch configuration issues earlier in the development cycle.
- val fragmentParameterTypingResult = fragmentParameterTypingParser
- .parseClassNameToTypingResult(
- fragmentParameter.typ.refClazzName
- )
- .getOrElse(Unknown)
+ val fragmentParameterTypingResult = fragmentParameterTypingParser
+ .parseClassNameToTypingResult(fragmentParameter.typ.refClazzName) match {
+ case Some(result) => result
+ case None =>
+ return invalidNel(InvalidFragmentParameterType(
+ fragmentParameter.typ.refClazzName,
+ nodeId.id,
+ fragmentParameter.name
+ ))
+ }Line range hint 48-164: Verify test coverage for new validation logic.
The changes introduce significant enhancements to type validation. Please ensure comprehensive test coverage for:
- Different class definition scenarios
- Edge cases in type parsing
- Dictionary value type validation
designer/server/src/test/scala/pl/touk/nussknacker/ui/definition/DefinitionsServiceSpec.scala (1)
299-299: Consider adding tests for class definition validation scenarios
The addition of class definitions to FragmentComponentDefinitionExtractor suggests that fragment validation now depends on these definitions. Consider adding test cases to verify:
- Fragment validation with empty class definitions
- Fragment validation with invalid class definitions
- Fragment validation with complex class hierarchies
DeamonDev
force-pushed
the
nu-7164-validate-fragment-input-params-on-backend
branch
from
fe7d67c
to
d70db87
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (5)
scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/compile/nodecompilation/FragmentParameterValidator.scala (3)
48-49: Consider making classDefinitions parameter required.While providing a default empty Set maintains backward compatibility, it might mask potential issues where class definitions should be provided. Consider making this parameter required to ensure proper validation context is always provided.
Line range hint
166-176: Consider enhancing error messages for better debugging.While the type compatibility check is correct, consider enhancing the error message in
DictIsOfInvalidTypeto include:
- Expected vs actual type format examples
- Suggestion for how to fix the type mismatch
160-164: Consider caching parsed typing results.The
FragmentParameterTypingParseris created and used for each validation. Consider caching the parsed results for frequently used class names to improve performance, especially when dealing with multiple validations.designer/server/src/test/scala/pl/touk/nussknacker/ui/definition/component/ComponentGroupsPreparerSpec.scala (1)
193-193: Consider adding test cases for non-empty class definitions.The addition of
Set.emptyas theclassDefinitionsparameter aligns with the broader changes for fragment handling. However, since this test file focuses on component group preparation, it would be valuable to add test cases that verify behavior with non-empty class definitions to ensure comprehensive coverage.designer/client/src/components/graph/node-modal/editors/expression/Table/TableEditor.tsx (1)
Line range hint
516-516: Implement the TODO for isSwitchableTo methodThe
isSwitchableTomethod currently returnstruewithout any implementation. This could lead to incorrect switching behavior in the editor.Would you like me to help implement proper switching logic based on the expression schema validation?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (20)
designer/client/src/components/graph/node-modal/editors/expression/Table/TableEditor.tsx(1 hunks)designer/client/src/components/graph/node-modal/fragment-input-definition/FragmentInputDefinition.tsx(2 hunks)designer/server/src/main/scala/pl/touk/nussknacker/ui/definition/AlignedComponentsDefinitionProvider.scala(1 hunks)designer/server/src/test/scala/pl/touk/nussknacker/ui/definition/DefinitionsServiceSpec.scala(1 hunks)designer/server/src/test/scala/pl/touk/nussknacker/ui/definition/EdgeTypesPreparerTest.scala(1 hunks)designer/server/src/test/scala/pl/touk/nussknacker/ui/definition/component/ComponentGroupsPreparerSpec.scala(1 hunks)designer/server/src/test/scala/pl/touk/nussknacker/ui/definition/component/ComponentsUsageHelperTest.scala(1 hunks)engine/flink/executor/src/main/scala/pl/touk/nussknacker/engine/process/compiler/FlinkProcessCompilerDataFactory.scala(2 hunks)engine/flink/executor/src/main/scala/pl/touk/nussknacker/engine/process/compiler/StubbedFlinkProcessCompilerDataFactory.scala(3 hunks)scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/compile/ProcessCompiler.scala(1 hunks)scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/compile/ProcessCompilerData.scala(1 hunks)scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/compile/nodecompilation/FragmentParameterValidator.scala(5 hunks)scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/compile/nodecompilation/NodeCompiler.scala(4 hunks)scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/compile/nodecompilation/NodeDataValidator.scala(1 hunks)scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/definition/fragment/FragmentComponentDefinitionExtractor.scala(2 hunks)scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/definition/fragment/FragmentParameterTypingParser.scala(1 hunks)scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/definition/fragment/FragmentParametersDefinitionExtractor.scala(6 hunks)scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/definition/test/ModelDataTestInfoProvider.scala(1 hunks)scenario-compiler/src/test/scala/pl/touk/nussknacker/engine/compile/nodecompilation/FragmentParameterValidatorTest.scala(4 hunks)utils/flink-components-testkit/src/main/scala/pl/touk/nussknacker/engine/flink/util/test/FlinkProcessCompilerDataFactoryWithTestComponents.scala(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (16)
- designer/client/src/components/graph/node-modal/fragment-input-definition/FragmentInputDefinition.tsx
- designer/server/src/main/scala/pl/touk/nussknacker/ui/definition/AlignedComponentsDefinitionProvider.scala
- designer/server/src/test/scala/pl/touk/nussknacker/ui/definition/DefinitionsServiceSpec.scala
- designer/server/src/test/scala/pl/touk/nussknacker/ui/definition/EdgeTypesPreparerTest.scala
- designer/server/src/test/scala/pl/touk/nussknacker/ui/definition/component/ComponentsUsageHelperTest.scala
- engine/flink/executor/src/main/scala/pl/touk/nussknacker/engine/process/compiler/FlinkProcessCompilerDataFactory.scala
- engine/flink/executor/src/main/scala/pl/touk/nussknacker/engine/process/compiler/StubbedFlinkProcessCompilerDataFactory.scala
- scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/compile/ProcessCompilerData.scala
- scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/compile/nodecompilation/NodeCompiler.scala
- scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/compile/nodecompilation/NodeDataValidator.scala
- scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/definition/fragment/FragmentComponentDefinitionExtractor.scala
- scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/definition/fragment/FragmentParameterTypingParser.scala
- scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/definition/fragment/FragmentParametersDefinitionExtractor.scala
- scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/definition/test/ModelDataTestInfoProvider.scala
- scenario-compiler/src/test/scala/pl/touk/nussknacker/engine/compile/nodecompilation/FragmentParameterValidatorTest.scala
- utils/flink-components-testkit/src/main/scala/pl/touk/nussknacker/engine/flink/util/test/FlinkProcessCompilerDataFactoryWithTestComponents.scala
🔇 Additional comments (5)
scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/compile/nodecompilation/FragmentParameterValidator.scala (2)
6-6: LGTM! Well-structured imports and constants.
The new imports and the permittedTypesForEditors constant are well-organized and provide a clear contract for supported types.
Also applies to: 18-18, 21-23
160-164: LGTM! Robust type parsing implementation.
The introduction of FragmentParameterTypingParser improves type safety and validation. The parsing logic is well-structured and handles the type resolution appropriately.
scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/compile/ProcessCompiler.scala (1)
355-355: Breaking change: FragmentParametersDefinitionExtractor constructor modification
The addition of classDefinitions.all parameter to FragmentParametersDefinitionExtractor looks good and aligns with the PR objective of enhancing fragment input definitions parsing. However, this is a breaking change that modifies a constructor signature.
Please ensure that:
- All existing instantiations of
FragmentParametersDefinitionExtractorare updated - The breaking change is documented in
_MigrationGuide.mdas mentioned in the PR checklist - Test coverage exists for the new parameter validation functionality
designer/client/src/components/graph/node-modal/editors/expression/Table/TableEditor.tsx (2)
100-101: Verify type options behavior with new parameter
The addition of false parameter to useTypeOptions hook affects how type options are filtered and displayed in the table editor. This change is part of a broader standardization of type options handling across components.
Please ensure that:
- The table editor correctly displays and filters type options with the new parameter
- The behavior aligns with the fragment input definition component where
useTypeOptions(true)is used
The type filtering logic in the following line looks correct, properly integrating with the modified hook:
const supportedTypes = useMemo(() => orderedTypeOptions.filter(({ value }) => SUPPORTED_TYPES.includes(value)), [orderedTypeOptions]);Line range hint 513-515: LGTM: Robust error handling implementation
The error handling and validation implementation is well-structured:
- Proper integration of validation labels
- Conditional rendering based on
showValidationprop - Clear error messages using i18next for internationalization
Also applies to: 468-469
designer/client/src/components/graph/node-modal/editors/expression/Table/TableEditor.tsx
Outdated
Show resolved
Hide resolved
...client/src/components/graph/node-modal/fragment-input-definition/FragmentInputDefinition.tsx
Outdated
Show resolved
Hide resolved
DeamonDev
force-pushed
the
nu-7164-validate-fragment-input-params-on-backend
branch
from
d70db87
to
50c1207
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/compile/nodecompilation/FragmentParameterValidator.scala (2)
Line range hint
39-46: Consider expanding permitted types and adding documentation.While the current set of permitted types covers basic scenarios, consider:
- Adding documentation explaining why these specific types are permitted
- Including other common types like Integer, Float, Double if they should be supported
- Adding scaladoc to explain the purpose and usage of this constant
object FragmentParameterValidator { + /** List of permitted types that can be used with editors in fragment parameters. + * These types are validated during fragment parameter compilation. + */ val permittedTypesForEditors: List[FragmentClazzRef] = List( FragmentClazzRef[java.lang.Boolean], FragmentClazzRef[String], - FragmentClazzRef[java.lang.Long] + FragmentClazzRef[java.lang.Long], + FragmentClazzRef[java.lang.Integer], + FragmentClazzRef[java.lang.Float], + FragmentClazzRef[java.lang.Double] )
48-49: Consider making classDefinitions required or documenting the implications.The empty Set default for
classDefinitionsmight silently ignore validation in some cases. Consider:
- Making it a required parameter to ensure explicit handling
- Adding documentation about the implications of an empty Set
- Adding runtime validation when the Set is empty but required for validation
-case class FragmentParameterValidator(classDefinitions: Set[ClassDefinition] = Set.empty) { +/** Validates fragment parameters against their type definitions and constraints. + * @param classDefinitions Set of class definitions used for type validation. + * An empty set will limit type validation capabilities. + */ +case class FragmentParameterValidator(classDefinitions: Set[ClassDefinition]) { + require(classDefinitions.nonEmpty, "ClassDefinitions are required for proper type validation")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (20)
designer/client/src/components/graph/node-modal/editors/expression/Table/TableEditor.tsx(3 hunks)designer/client/src/components/graph/node-modal/fragment-input-definition/FragmentInputDefinition.tsx(2 hunks)designer/server/src/main/scala/pl/touk/nussknacker/ui/definition/AlignedComponentsDefinitionProvider.scala(1 hunks)designer/server/src/test/scala/pl/touk/nussknacker/ui/definition/DefinitionsServiceSpec.scala(1 hunks)designer/server/src/test/scala/pl/touk/nussknacker/ui/definition/EdgeTypesPreparerTest.scala(1 hunks)designer/server/src/test/scala/pl/touk/nussknacker/ui/definition/component/ComponentGroupsPreparerSpec.scala(1 hunks)designer/server/src/test/scala/pl/touk/nussknacker/ui/definition/component/ComponentsUsageHelperTest.scala(1 hunks)engine/flink/executor/src/main/scala/pl/touk/nussknacker/engine/process/compiler/FlinkProcessCompilerDataFactory.scala(2 hunks)engine/flink/executor/src/main/scala/pl/touk/nussknacker/engine/process/compiler/StubbedFlinkProcessCompilerDataFactory.scala(3 hunks)scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/compile/ProcessCompiler.scala(1 hunks)scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/compile/ProcessCompilerData.scala(1 hunks)scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/compile/nodecompilation/FragmentParameterValidator.scala(5 hunks)scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/compile/nodecompilation/NodeCompiler.scala(4 hunks)scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/compile/nodecompilation/NodeDataValidator.scala(1 hunks)scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/definition/fragment/FragmentComponentDefinitionExtractor.scala(2 hunks)scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/definition/fragment/FragmentParameterTypingParser.scala(1 hunks)scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/definition/fragment/FragmentParametersDefinitionExtractor.scala(6 hunks)scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/definition/test/ModelDataTestInfoProvider.scala(1 hunks)scenario-compiler/src/test/scala/pl/touk/nussknacker/engine/compile/nodecompilation/FragmentParameterValidatorTest.scala(4 hunks)utils/flink-components-testkit/src/main/scala/pl/touk/nussknacker/engine/flink/util/test/FlinkProcessCompilerDataFactoryWithTestComponents.scala(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (17)
- designer/client/src/components/graph/node-modal/fragment-input-definition/FragmentInputDefinition.tsx
- designer/server/src/main/scala/pl/touk/nussknacker/ui/definition/AlignedComponentsDefinitionProvider.scala
- designer/server/src/test/scala/pl/touk/nussknacker/ui/definition/DefinitionsServiceSpec.scala
- designer/server/src/test/scala/pl/touk/nussknacker/ui/definition/EdgeTypesPreparerTest.scala
- designer/server/src/test/scala/pl/touk/nussknacker/ui/definition/component/ComponentsUsageHelperTest.scala
- engine/flink/executor/src/main/scala/pl/touk/nussknacker/engine/process/compiler/FlinkProcessCompilerDataFactory.scala
- engine/flink/executor/src/main/scala/pl/touk/nussknacker/engine/process/compiler/StubbedFlinkProcessCompilerDataFactory.scala
- scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/compile/ProcessCompiler.scala
- scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/compile/ProcessCompilerData.scala
- scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/compile/nodecompilation/NodeCompiler.scala
- scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/compile/nodecompilation/NodeDataValidator.scala
- scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/definition/fragment/FragmentComponentDefinitionExtractor.scala
- scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/definition/fragment/FragmentParameterTypingParser.scala
- scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/definition/fragment/FragmentParametersDefinitionExtractor.scala
- scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/definition/test/ModelDataTestInfoProvider.scala
- scenario-compiler/src/test/scala/pl/touk/nussknacker/engine/compile/nodecompilation/FragmentParameterValidatorTest.scala
- utils/flink-components-testkit/src/main/scala/pl/touk/nussknacker/engine/flink/util/test/FlinkProcessCompilerDataFactoryWithTestComponents.scala
🔇 Additional comments (5)
scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/compile/nodecompilation/FragmentParameterValidator.scala (2)
6-6: LGTM: New imports are properly organized.
The added imports support the new functionality for class definition handling and typing.
Also applies to: 18-18, 21-23
Line range hint 1-214: Verify usage patterns of FragmentParameterValidator.
Let's verify how this validator is used across the codebase to ensure proper initialization with class definitions.
✅ Verification successful
Let me gather more context about the actual usage of the validator in production code.
Based on the gathered information, I can now generate the final response:
FragmentParameterValidator is properly initialized with class definitions.
The validator is correctly used across the codebase:
- In NodeCompiler, it's initialized with class definitions from FragmentDefinitionExtractor:
fragmentParameterValidator = FragmentParameterValidator(fragmentDefinitionExtractor.classDefinitions) - The validator is used for validating fragment parameters, dictionary editors, and fixed expression values in a consistent way
- All validation methods are invoked with proper context and required dependencies
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for FragmentParameterValidator instantiations
rg "new FragmentParameterValidator" -A 2
# Search for potential validation calls
rg "validateValueInputWithDictEditor|validateAgainstClazzRefAndGetEditor" -A 2
Length of output: 4898
Script:
#!/bin/bash
# Search for FragmentParameterValidator instantiations in non-test code
rg "new FragmentParameterValidator" --type scala --glob '!**/test/**'
# Search for where classDefinitions are used/passed
rg "classDefinitions.*FragmentParameterValidator" --type scala
# Look for NodeCompiler initialization to see how fragmentParameterValidator is created
ast-grep --pattern 'class NodeCompiler($$$) {
$$$
}'
Length of output: 95900
designer/server/src/test/scala/pl/touk/nussknacker/ui/definition/component/ComponentGroupsPreparerSpec.scala (1)
193-193: Consider enhancing test coverage for fragment input definitions with class definitions.
While using Set.empty works for the current test cases that verify basic fragment component behavior, it might not provide sufficient coverage for the new fragment input definition parsing feature. Consider:
- Adding test cases that verify fragment input definitions with various class definitions
- Adding a comment explaining why an empty set is sufficient for the existing test cases
Let's verify if there are any existing tests for fragment input definitions with class definitions:
✅ Verification successful
Empty class definitions set is appropriate for these tests
The empty set for classDefinitions in ComponentGroupsPreparerSpec.scala is appropriate because:
- The test file focuses on verifying component group preparation and alignment, not fragment parameter validation
- The actual fragment parameter validation and class definition handling is tested in dedicated test files:
FragmentParameterValidatorTest.scalaFragmentParametersDefinitionExtractor.scala
No changes are needed as the current test coverage for fragment class definitions is properly handled in the appropriate test files, and this specific test file's scope is limited to component group behavior.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for tests related to fragment input definitions and class definitions
rg -l "FragmentInputDefinition.*ClassDefinition" --type scala
Length of output: 63
Script:
#!/bin/bash
# Let's first find files related to fragment input definitions
rg -l "FragmentInputDefinition" --type scala
# Then search for test files containing "fragment" and "class"
rg -l "class.*fragment|fragment.*class" --type scala --glob "*Test*.scala" --glob "*Spec*.scala"
# Also search for the FragmentComponentDefinitionExtractor usage
rg "FragmentComponentDefinitionExtractor" --type scala -C 3
Length of output: 18712
Script:
#!/bin/bash
# Let's check the FragmentParameterValidatorTest for insights on class definitions
rg -A 10 "class FragmentParameterValidatorTest" scenario-compiler/src/test/scala/pl/touk/nussknacker/engine/compile/nodecompilation/FragmentParameterValidatorTest.scala
# Also check the actual implementation of FragmentComponentDefinitionExtractor
rg -A 10 "class FragmentComponentDefinitionExtractor" scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/definition/fragment/FragmentComponentDefinitionExtractor.scala
# And look for any tests using classDefinitions with fragments
rg "classDefinitions.*fragment|fragment.*classDefinitions" --type scala
Length of output: 2147
designer/client/src/components/graph/node-modal/editors/expression/Table/TableEditor.tsx (2)
34-37: LGTM: Imports are well-organized and necessary.
The new imports are properly structured and essential for the new hook implementation.
125-126: 🛠️ Refactor suggestion
Add defensive programming for defaultTypeOption.
The code should handle the case when defaultTypeOption is null, which could happen if the process definition data is not available.
Let's verify the usage of defaultTypeOption in the codebase:
Consider adding null checks:
- const { defaultTypeOption, orderedTypeOptions } = useTableEditorTypeOptions();
- const supportedTypes = useMemo(() => orderedTypeOptions.filter(({ value }) => SUPPORTED_TYPES.includes(value)), [orderedTypeOptions]);
+ const { defaultTypeOption, orderedTypeOptions } = useTableEditorTypeOptions();
+ if (!defaultTypeOption) {
+ // Handle the case when no type options are available
+ return <div>Loading type options...</div>;
+ }
+ const supportedTypes = useMemo(() => orderedTypeOptions.filter(({ value }) => SUPPORTED_TYPES.includes(value)), [orderedTypeOptions]);
...in/scala/pl/touk/nussknacker/engine/compile/nodecompilation/FragmentParameterValidator.scala
Show resolved
Hide resolved
designer/client/src/components/graph/node-modal/editors/expression/Table/TableEditor.tsx
Show resolved
Hide resolved
DeamonDev
force-pushed
the
nu-7164-validate-fragment-input-params-on-backend
branch
from
50c1207
to
3c31755
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (5)
scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/definition/fragment/FragmentParametersDefinitionExtractor.scala (2)
84-86: Consider enhancing error logging for validation failures.While the validation logic is solid, consider adding debug-level logging when validation fails to help with troubleshooting in production environments.
FragmentParameterValidator(classDefinitions).validateAgainstClazzRefAndGetEditor( valueEditor = editor, initialValue = fragmentParameter.initialValue, refClazz = fragmentParameter.typ, paramName = fragmentParameter.name, nodeIds = Set(nodeId.id) ) match { case Valid(editor) => (Some(editor), List.empty) - case Invalid(e) => (None, e.toList) + case Invalid(e) => + logger.debug(s"Validation failed for parameter ${fragmentParameter.name}: ${e.toList.mkString(", ")}") + (None, e.toList) }
41-41: Consider making classDefinitions a required parameter.While providing a default empty Set for
classDefinitionsmaintains backward compatibility, it might mask configuration errors where class definitions should have been provided but weren't. Consider making this a required parameter in a future major version update.scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/compile/nodecompilation/FragmentParameterValidator.scala (2)
Line range hint
40-44: Document the rationale for permitted types.Consider adding documentation explaining why only these specific types are permitted for editors. This will help future maintainers understand the constraints and make informed decisions when modifying the list.
object FragmentParameterValidator { + /** + * List of types that are supported by parameter editors. + * These types are chosen because: + * - They represent common primitive types + * - They have well-defined string representations + * - They are commonly used in fragment parameters + */ val permittedTypesForEditors: List[FragmentClazzRef] = List( FragmentClazzRef[java.lang.Boolean], FragmentClazzRef[String], FragmentClazzRef[java.lang.Long] )
48-49: Add class documentation.The case class would benefit from documentation explaining its purpose and the role of classDefinitions. This helps users understand when and how to use this validator.
+/** + * Validates fragment parameters against their type definitions and editor constraints. + * + * @param classDefinitions Set of class definitions used for type resolution and validation. + * These definitions are used to parse and validate parameter types + * when working with dictionary editors and custom types. + */ case class FragmentParameterValidator(classDefinitions: Set[ClassDefinition] = Set.empty) {designer/server/src/test/scala/pl/touk/nussknacker/ui/definition/component/ComponentGroupsPreparerSpec.scala (1)
Line range hint
141-175: Enhance fragment-related test coverage.The existing fragment-related tests only verify basic functionality. Consider adding test cases that:
- Verify fragment input definition parsing with various class definitions
- Test error cases (invalid class definitions, malformed inputs)
- Test edge cases (empty class definitions, complex nested types)
Here's a suggested test structure:
test("validate fragment input definition with class definitions") { val classDefinitions = Set( // Add sample class definitions here ) val model = getAlignedComponentsWithStaticDefinition( ModelDefinitionBuilder.empty.build, Map.empty, forFragment = true, classDefinitions = classDefinitions ) val groups = ComponentGroupsPreparer.prepareComponentGroups(model) // Add assertions to verify: // 1. Fragment input definition components have correct parameters based on class definitions // 2. Type validation works correctly // 3. Error cases are handled properly }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (20)
designer/client/src/components/graph/node-modal/editors/expression/Table/TableEditor.tsx(3 hunks)designer/client/src/components/graph/node-modal/fragment-input-definition/FragmentInputDefinition.tsx(2 hunks)designer/server/src/main/scala/pl/touk/nussknacker/ui/definition/AlignedComponentsDefinitionProvider.scala(1 hunks)designer/server/src/test/scala/pl/touk/nussknacker/ui/definition/DefinitionsServiceSpec.scala(1 hunks)designer/server/src/test/scala/pl/touk/nussknacker/ui/definition/EdgeTypesPreparerTest.scala(1 hunks)designer/server/src/test/scala/pl/touk/nussknacker/ui/definition/component/ComponentGroupsPreparerSpec.scala(1 hunks)designer/server/src/test/scala/pl/touk/nussknacker/ui/definition/component/ComponentsUsageHelperTest.scala(1 hunks)engine/flink/executor/src/main/scala/pl/touk/nussknacker/engine/process/compiler/FlinkProcessCompilerDataFactory.scala(2 hunks)engine/flink/executor/src/main/scala/pl/touk/nussknacker/engine/process/compiler/StubbedFlinkProcessCompilerDataFactory.scala(3 hunks)scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/compile/ProcessCompiler.scala(1 hunks)scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/compile/ProcessCompilerData.scala(1 hunks)scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/compile/nodecompilation/FragmentParameterValidator.scala(5 hunks)scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/compile/nodecompilation/NodeCompiler.scala(4 hunks)scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/compile/nodecompilation/NodeDataValidator.scala(1 hunks)scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/definition/fragment/FragmentComponentDefinitionExtractor.scala(2 hunks)scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/definition/fragment/FragmentParameterTypingParser.scala(1 hunks)scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/definition/fragment/FragmentParametersDefinitionExtractor.scala(6 hunks)scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/definition/test/ModelDataTestInfoProvider.scala(1 hunks)scenario-compiler/src/test/scala/pl/touk/nussknacker/engine/compile/nodecompilation/FragmentParameterValidatorTest.scala(4 hunks)utils/flink-components-testkit/src/main/scala/pl/touk/nussknacker/engine/flink/util/test/FlinkProcessCompilerDataFactoryWithTestComponents.scala(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (16)
- designer/client/src/components/graph/node-modal/fragment-input-definition/FragmentInputDefinition.tsx
- designer/server/src/main/scala/pl/touk/nussknacker/ui/definition/AlignedComponentsDefinitionProvider.scala
- designer/server/src/test/scala/pl/touk/nussknacker/ui/definition/DefinitionsServiceSpec.scala
- designer/server/src/test/scala/pl/touk/nussknacker/ui/definition/EdgeTypesPreparerTest.scala
- designer/server/src/test/scala/pl/touk/nussknacker/ui/definition/component/ComponentsUsageHelperTest.scala
- engine/flink/executor/src/main/scala/pl/touk/nussknacker/engine/process/compiler/FlinkProcessCompilerDataFactory.scala
- engine/flink/executor/src/main/scala/pl/touk/nussknacker/engine/process/compiler/StubbedFlinkProcessCompilerDataFactory.scala
- scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/compile/ProcessCompiler.scala
- scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/compile/ProcessCompilerData.scala
- scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/compile/nodecompilation/NodeCompiler.scala
- scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/compile/nodecompilation/NodeDataValidator.scala
- scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/definition/fragment/FragmentComponentDefinitionExtractor.scala
- scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/definition/fragment/FragmentParameterTypingParser.scala
- scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/definition/test/ModelDataTestInfoProvider.scala
- scenario-compiler/src/test/scala/pl/touk/nussknacker/engine/compile/nodecompilation/FragmentParameterValidatorTest.scala
- utils/flink-components-testkit/src/main/scala/pl/touk/nussknacker/engine/flink/util/test/FlinkProcessCompilerDataFactoryWithTestComponents.scala
🔇 Additional comments (7)
scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/definition/fragment/FragmentParametersDefinitionExtractor.scala (2)
39-44: LGTM! Well-structured constructor enhancement.
The addition of the optional classDefinitions parameter with a default empty Set maintains backward compatibility while enabling enhanced typing capabilities. The immediate initialization of fragmentParameterTypingParser ensures proper setup for parsing operations.
Line range hint 145-156: LGTM! Robust error handling in type parsing.
The refactored implementation properly handles parsing failures and provides clear error messages. The use of Writer monad for error accumulation is particularly elegant.
Let's verify that all fragment parameter types used in the codebase are properly handled:
✅ Verification successful
Based on the search results, I can now verify the implementation. The codebase shows that:
- The fragment parameter types are properly handled through
FragmentParameterTypingParserwhich is used in the refactored code. - The supported types are defined in
FragmentParameterValidator.permittedTypesForEditorswhich includesBoolean,String, andLong. - The error handling in the reviewed code correctly handles parsing failures by returning
Unknowntype with descriptive error messages. - The implementation is used consistently across the codebase in fragment parameter validation and compilation.
LGTM! The type parsing implementation is robust and handles all supported fragment parameter types.
The error handling for unsupported types is particularly well done, providing clear error messages through FragmentParamClassLoadError.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for all fragment parameter type definitions to ensure they're supported
ast-grep --pattern 'class $_ extends FragmentParameter {
$$$
val typ: $_ = $_
$$$
}'
Length of output: 92
Script:
#!/bin/bash
# Let's try a different approach to find fragment parameter type usages
rg -A 5 "FragmentParameter" --type scala
Length of output: 101782
scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/compile/nodecompilation/FragmentParameterValidator.scala (1)
160-164: Improve error handling in type resolution.
The current implementation silently falls back to Unknown when type parsing fails, which could mask potential issues. Consider:
- Adding logging when falling back to Unknown type
- Propagating parsing errors instead of using
getOrElse(Unknown) - Adding validation for the parsed typing result
This issue was previously raised in a past review. The suggestion remains valid and should be addressed.
designer/server/src/test/scala/pl/touk/nussknacker/ui/definition/component/ComponentGroupsPreparerSpec.scala (1)
193-193:
Add test coverage for fragment input definitions with class definitions.
The empty set passed to FragmentComponentDefinitionExtractor doesn't test the new fragment input definition parsing functionality mentioned in the PR objectives. Consider adding test cases that:
- Pass non-empty class definitions to verify the parser's behavior
- Cover various scenarios of fragment input definitions
Let's verify if there are any existing tests for fragment input definitions:
designer/client/src/components/graph/node-modal/editors/expression/Table/TableEditor.tsx (3)
33-36: LGTM: Imports are well-organized and necessary.
The new imports support the implementation of the useTableEditorTypeOptions hook with Redux state management and utility functions.
93-112: 🛠️ Refactor suggestion
Enhance hook robustness and type safety.
The hook implementation could benefit from the following improvements:
-export function useTableEditorTypeOptions() {
+interface TypeOption {
+ value: SupportedType;
+ label: string;
+}
+
+interface TableEditorTypeOptions {
+ orderedTypeOptions: TypeOption[];
+ defaultTypeOption: TypeOption | null;
+}
+
+export function useTableEditorTypeOptions(): TableEditorTypeOptions {
const definitionData = useSelector(getProcessDefinitionData);
+
+ if (!definitionData?.classes) {
+ return {
+ orderedTypeOptions: [],
+ defaultTypeOption: null,
+ };
+ }
const typeOptions = useMemo(
() =>
- definitionData?.classes?.map((type) => ({
+ definitionData.classes.map((type) => ({
value: type.refClazzName as SupportedType,
label: ProcessUtils.humanReadableType(type),
})),
[definitionData?.classes],
);
const orderedTypeOptions = useMemo(() => orderBy(typeOptions, (item) => [item.label, item.value], ["asc"]), [typeOptions]);
- const defaultTypeOption = useMemo(() => find(typeOptions, { label: "String" }) || head(typeOptions), [typeOptions]);
+ const defaultTypeOption = useMemo(() => {
+ if (!typeOptions?.length) return null;
+ return find(typeOptions, { label: "String" }) || head(typeOptions);
+ }, [typeOptions]);
+
return {
orderedTypeOptions,
defaultTypeOption,
};
}Likely invalid or redundant comment.
Line range hint 93-124: Verify all usages of useTableEditorTypeOptions.
Please ensure that all other places where defaultTypeOption is used (e.g., onColumnAppend, pasteWithExpand) also handle the null case appropriately.
✅ Verification successful
defaultTypeOption is safely handled in all usages
The search results show that defaultTypeOption is used in two components:
-
In
TableEditor.tsx:- All usages access
defaultTypeOption.valuewithin callbacks that are dependent ondefaultTypeOptionin their dependency arrays - The value is guaranteed to be non-null as it's defined with a fallback:
find(typeOptions, { label: "String" }) || head(typeOptions)
- All usages access
-
In
FragmentInputDefinition.tsx:- Similar safe implementation with the same fallback pattern
- Used safely within the
addFieldcallback with proper dependency tracking
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for all usages of defaultTypeOption to ensure proper null checking
rg -A 5 "defaultTypeOption" --type ts --type tsx
Length of output: 76
Script:
#!/bin/bash
# Search for all usages of defaultTypeOption with proper file extension
rg "defaultTypeOption" -t typescript -t tsx -C 5
Length of output: 83
Script:
#!/bin/bash
# Search for all usages of defaultTypeOption without file type restrictions
rg "defaultTypeOption" -C 5
Length of output: 12825
designer/client/src/components/graph/node-modal/editors/expression/Table/TableEditor.tsx
Show resolved
Hide resolved
DeamonDev
deleted the
nu-7164-validate-fragment-input-params-on-backend
branch
)" This reverts commit d5154a3.
* Revert "Revert "[NU-7164] Add basic parser for fragment input definitions (#7167)" (#7202)" This reverts commit 383b4d5. * [NU-7164] Adjust fields parameter list * Revert "[NU-7164] Adjust fields parameter list" This reverts commit d4a7ccc. * add dicts bugfix * adjust type * correct validation rules --------- Co-authored-by: Piotr Rudnicki <[email protected]>
Co-authored-by: Piotr Rudnicki <[email protected]>
* Revert "Revert "[NU-7164] Add basic parser for fragment input definitions (#7167)" (#7202)" This reverts commit 383b4d5. * [NU-7164] Adjust fields parameter list * Revert "[NU-7164] Adjust fields parameter list" This reverts commit d4a7ccc. * add dicts bugfix * adjust type * correct validation rules --------- Co-authored-by: Piotr Rudnicki <[email protected]>
Describe your changes
Checklist before merge
Summary by CodeRabbit
Release Notes
New Features
FragmentParameterTypingParserfor enhanced parsing of class names into typing results.useTableEditorTypeOptionsfor dynamic type management in theTablecomponent.Improvements
NodeCompiler.ProcessValidator.FragmentParametersDefinitionExtractorto utilize class definitions for more robust parameter validation.Tablecomponent to integrate updated type options for better data management.Bug Fixes
Tests
DefinitionsServiceto validate scenario properties and component group management.