Revert "[NU-7164] Add basic parser for fragment input definitions (#7167) #7202
Conversation
)" This reverts commit d5154a3.
📝 WalkthroughWalkthroughThe pull request introduces significant changes across multiple files, primarily focusing on the refactoring of how type options are managed within the application. In the In the server-side code, modifications to the Overall, the changes simplify the instantiation of various components by removing unnecessary parameters and dependencies, while maintaining the core functionalities across the affected files. Possibly related PRs
Suggested reviewers
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (7)
designer/client/src/components/graph/node-modal/fragment-input-definition/FragmentInputDefinition.tsx (1)
Line range hint
17-43: Add test coverage for the new type options implementationThe changes introduce new type handling logic but lack corresponding test coverage. We should ensure:
- Edge cases are tested (empty classes, null values)
- Type conversion behavior is verified
- Integration with FragmentInputDefinition component is tested
Consider adding tests for:
- Default type option selection
- Ordering of type options
- Handling of missing or malformed data
- Generic type parameter behavior
Would you like me to help create a test suite for these scenarios?
utils/flink-components-testkit/src/main/scala/pl/touk/nussknacker/engine/flink/util/test/FlinkProcessCompilerDataFactoryWithTestComponents.scala (1)
Line range hint
64-83: Consider adding documentation about parameter removal.Since this is part of a revert operation, consider adding a comment explaining why the
classDefinitionsparameter was removed to prevent future confusion or accidental reintroduction.Here's a suggested documentation addition:
override protected def adjustDefinitions( originalModelDefinition: ModelDefinition, definitionContext: ComponentDefinitionContext + // Note: classDefinitions parameter was removed as part of NU-7164 revert + // as it was not utilized in the implementation ): ModelDefinition = {scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/compile/ProcessCompilerData.scala (1)
56-56: Ensure comprehensive testing of fragment parameter handlingSince this change affects the core compilation logic, particularly around fragment parameters, thorough testing is crucial. The revert should be validated against existing fragment-related test cases.
Consider adding specific test cases that verify:
- Fragment parameter type resolution
- Compilation of nested fragments
- Edge cases in fragment parameter handling
scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/definition/fragment/FragmentParametersDefinitionExtractor.scala (1)
Line range hint
35-145: Consider documenting the architectural changesWhile the code changes look good, consider:
- Updating the class documentation to reflect the new validation approach
- Adding a comment explaining why
classDefinitionswas removed- Documenting the rationale for switching to runtime class resolution
This will help future maintainers understand the architectural decisions made here.
scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/compile/nodecompilation/NodeDataValidator.scala (2)
Line range hint
48-53: Consider architectural implications of simplified fragment parameter extraction.The removal of class definitions from
FragmentParametersDefinitionExtractormight affect type checking during fragment parameter validation. This change could have implications for:
- Type safety during fragment parameter validation
- Fragment input resolution
- Output variable handling
Please ensure that:
- Type checking is still properly handled without the class definitions
- Fragment parameter validation remains robust
- No type-related issues are introduced in fragment processing
Line range hint
39-39: Consider addressing the TODO comment.There's a TODO to remove
ValidationNotPerformed. This would require changes to how certain node types (Split,FragmentUsageOutput,BranchEndData) are handled in the validation logic.Would you like me to help create a GitHub issue to track the removal of
ValidationNotPerformedand propose alternative validation handling for these node types?scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/compile/nodecompilation/NodeCompiler.scala (1)
168-169: LGTM! Good refactoring to remove instance state.The refactoring improves the code by:
- Removing unnecessary instance state
- Making validation logic more direct and easier to understand
- Following functional programming principles
Consider extracting the validation logic into a separate private method to improve readability and reusability:
+ private def validateFragmentInput( + parameterDefinitions: List[Parameter], + fragmentInputDefinition: FragmentInputDefinition, + validationContext: ValidationContext + )(implicit nodeId: NodeId): ValidatedNel[ProcessCompilationError, Unit] = { + val parameterNameValidation = FragmentParameterValidator.validateParameterNames(parameterDefinitions) + val fixedValuesValidation = fragmentInputDefinition.parameters + .map(FragmentParameterValidator.validateFixedExpressionValues(_, validationContext, expressionCompiler)) + .sequence + .map(_ => ()) + val dictValueEditorValidation = fragmentInputDefinition.parameters + .map(FragmentParameterValidator.validateValueInputWithDictEditor(_, expressionConfig.dictionaries, classLoader)) + .sequence + .map(_ => ()) + + parameterNameValidation |+| fixedValuesValidation |+| dictValueEditorValidation + }Also applies to: 198-203, 209-210
📜 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(2 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(0 hunks)designer/server/src/test/scala/pl/touk/nussknacker/ui/definition/DefinitionsServiceSpec.scala(0 hunks)designer/server/src/test/scala/pl/touk/nussknacker/ui/definition/EdgeTypesPreparerTest.scala(0 hunks)designer/server/src/test/scala/pl/touk/nussknacker/ui/definition/component/ComponentGroupsPreparerSpec.scala(0 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(2 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(2 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(1 hunks)scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/definition/fragment/FragmentParameterTypingParser.scala(0 hunks)scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/definition/fragment/FragmentParametersDefinitionExtractor.scala(3 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(1 hunks)
💤 Files with no reviewable changes (5)
- 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/ComponentGroupsPreparerSpec.scala
- scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/definition/fragment/FragmentParameterTypingParser.scala
🧰 Additional context used
📓 Learnings (4)
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.
engine/flink/executor/src/main/scala/pl/touk/nussknacker/engine/process/compiler/StubbedFlinkProcessCompilerDataFactory.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.
scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/compile/ProcessCompilerData.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.
utils/flink-components-testkit/src/main/scala/pl/touk/nussknacker/engine/flink/util/test/FlinkProcessCompilerDataFactoryWithTestComponents.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 (27)
scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/compile/nodecompilation/FragmentParameterValidator.scala (2)
17-17: LGTM!
The necessary imports for Typed and Unknown are appropriately added.
150-152: Simplified type resolution in validateValueInputWithDictEditor
The changes streamline the retrieval of the fragment parameter's typing result by directly using toRuntimeClass and mapping it to Typed, defaulting to Unknown when the class cannot be determined. This enhances readability and reduces complexity.
scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/definition/fragment/FragmentComponentDefinitionExtractor.scala (1)
25-25: LGTM! Verify revert completeness.
The simplified instantiation of FragmentParametersDefinitionExtractor aligns with the removal of classDefinitions parameter.
Let's verify this revert matches the previous state:
✅ Verification successful
Constructor signature matches the instantiation - change is correct
The FragmentParametersDefinitionExtractor class constructor only requires a classLoader parameter, which matches exactly how it's being instantiated in the reviewed line. This confirms the change is correct and consistent with the class definition.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the revert matches the state before the original change
# Get the content of the specific line before the original change
# First, get the commit being reverted
ORIGINAL_COMMIT=$(git rev-parse d5154a3e7053d485fe766a021adee0401986411e^)
# Check the file content at that commit
git show $ORIGINAL_COMMIT:scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/definition/fragment/FragmentComponentDefinitionExtractor.scala | grep -A 1 "parametersExtractor ="
Length of output: 630
Script:
#!/bin/bash
# Let's first check the git history for this file to understand the changes
git log --oneline -- scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/definition/fragment/FragmentComponentDefinitionExtractor.scala
# Then let's examine the FragmentParametersDefinitionExtractor class to understand its constructor
rg "class FragmentParametersDefinitionExtractor" -A 3
Length of output: 694
designer/client/src/components/graph/node-modal/fragment-input-definition/FragmentInputDefinition.tsx (2)
17-24: 🛠️ Refactor suggestion
Improve type safety and handle potential runtime issues
The current implementation has several concerns:
- The type assertion
as Valuebypasses TypeScript's type checking, which could lead to runtime errors - No null checks for
definitionData?.classes?.map - The TODO comment indicates incomplete type safety for
refClazzName
Consider this safer implementation:
-export function useTypeOptions<Value = string>() {
+export function useTypeOptions<Value extends string = string>() {
const definitionData = useSelector(getProcessDefinitionData);
const typeOptions = useMemo(
() =>
- definitionData?.classes?.map((type) => ({
- // TODO: Instead of using type assertion type, set refClazzName as a union of available clazzNames
- value: type.refClazzName as Value,
+ (definitionData?.classes || []).map((type) => ({
+ value: type.refClazzName,
label: ProcessUtils.humanReadableType(type),
})),
[definitionData?.classes],
);Let's verify the usage of refClazzName across the codebase:
✅ Verification successful
Based on the search results, I can now provide a final response regarding the type safety concerns in the useTypeOptions hook:
Type assertion is safe and matches the codebase's type system
The type assertion as Value in useTypeOptions is actually safe because:
refClazzNameis consistently defined asstringacross the codebase (seeReturnedTypeandTypedClassinterfaces intypes/definition.ts)- The hook's generic parameter
Valuedefaults tostring, aligning with the actual type - The value is only used for select/dropdown options where string values are expected
The TODO comment can be safely removed as the current implementation is type-safe and follows the codebase's type conventions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Find all usages of refClazzName to understand its type constraints
rg "refClazzName" --type ts --type tsx
Length of output: 66
Script:
#!/bin/bash
# Let's try without file type filters since the extension might not be recognized
rg "refClazzName"
Length of output: 133214
Script:
#!/bin/bash
# Let's also check for the type definition of the class property
ast-grep --pattern 'interface $_ {
$$$
refClazzName: $_
$$$
}'
Length of output: 1104
Script:
#!/bin/bash
# Search for type definitions containing refClazzName
fd -e ts -e tsx | xargs cat | grep -A 5 -B 5 "refClazzName"
Length of output: 23509
43-43: Verify revert implications
This change from the previous implementation to useTypeOptions seems to contradict the PR's objective of reverting changes. According to the PR description, this should be reverting "[NU-7164] Add basic parser for fragment input definitions".
Let's verify the original implementation:
scenario-compiler/src/test/scala/pl/touk/nussknacker/engine/compile/nodecompilation/FragmentParameterValidatorTest.scala (4)
Line range hint 42-49: LGTM! Consistent with the refactoring pattern.
The change to static method call maintains test behavior while aligning with the overall refactoring strategy.
Line range hint 58-65: LGTM! Error case handling preserved.
The static method call correctly maintains the negative test case behavior for invalid parameter types.
Line range hint 74-84: LGTM! Comprehensive test coverage maintained.
The static method refactoring is consistent, and the test suite maintains good coverage of both valid and invalid scenarios for different parameter editor types.
Line range hint 26-33: LGTM! Verify related class changes.
The change from instance method to static method call is correct and maintains the test's original behavior.
Let's verify the corresponding changes in the FragmentParameterValidator class:
✅ Verification successful
The change is correct and consistent with the codebase
The verification confirms that the test's change from instance to static method call is correct. The FragmentParameterValidator has been refactored to be a singleton object (Scala's equivalent of static methods) with the validateAgainstClazzRefAndGetEditor method defined exactly as used in the test. The implementation maintains the same behavior and parameter types as expected by the test.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the class definition and its companion object
ast-grep --pattern 'class FragmentParameterValidator'
ast-grep --pattern 'object FragmentParameterValidator'
Length of output: 28724
utils/flink-components-testkit/src/main/scala/pl/touk/nussknacker/engine/flink/util/test/FlinkProcessCompilerDataFactoryWithTestComponents.scala (1)
64-64: LGTM! Verify complete revert operation.
The removal of the classDefinitions parameter from adjustDefinitions looks correct. Based on the learnings, this method has only one usage, making this change safe.
Let's verify if the revert operation is complete:
✅ Verification successful
Revert operation is complete and consistent
The verification shows that the adjustDefinitions method signature is consistent across all implementations:
- Base class in
FlinkProcessCompilerDataFactory.scala - Test implementation in
FlinkProcessCompilerDataFactoryWithTestComponents.scala - Stubbed implementation in
StubbedFlinkProcessCompilerDataFactory.scala
All implementations have the same two parameters (originalModelDefinition and definitionContext), with no references to the removed classDefinitions parameter, confirming the revert operation is complete.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining references to classDefinitions in adjustDefinitions
# across the codebase to ensure complete revert
# Search for any remaining references to classDefinitions in adjustDefinitions
rg "adjustDefinitions.*classDefinitions"
# Search for any TODO or FIXME comments related to this change
rg -i "todo.*classDefinitions|fixme.*classDefinitions"
Length of output: 99
Script:
#!/bin/bash
# Let's try a broader search to understand the usage of adjustDefinitions
# and verify the complete revert operation
# Find all occurrences of adjustDefinitions method
rg "adjustDefinitions" -A 3 -B 3
# Find all files containing FlinkProcessCompilerDataFactoryWithTestComponents
# to understand its usage
fd "FlinkProcessCompilerDataFactoryWithTestComponents"
# Search for any test files that might be using this class
fd -e scala -e java ".*Test.*" --exec rg "FlinkProcessCompilerDataFactoryWithTestComponents"
Length of output: 4657
scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/compile/ProcessCompilerData.scala (2)
Line range hint 55-62: Verify type resolution in NodeCompiler after parameter removal
The NodeCompiler initialization still has access to type information through definitionWithTypes.modelDefinition, but we should verify that fragment parameter type resolution works correctly after removing the direct class definitions access.
Let's check the type resolution flow:
✅ Verification successful
Let me gather more information about the fragment parameter handling to verify the type resolution.
Based on the code analysis, I can now generate the final response since I have sufficient information about how fragment parameter type resolution works:
Type resolution is properly handled through FragmentParametersDefinitionExtractor
The type resolution for fragment parameters is correctly maintained through:
FragmentParametersDefinitionExtractor.getParamTypingResult()which resolves types using the classloaderNodeCompilervalidates parameter types throughFragmentParameterValidator- Test coverage confirms type resolution works for both required and optional parameters
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check how type resolution is handled in NodeCompiler and related classes
rg -A 5 "def resolve.*Type"
# Look for any type-related test cases
fd -e scala -e java | grep -i "test" | xargs rg "fragment.*type"
Length of output: 2411
Script:
#!/bin/bash
# Check FragmentParametersDefinitionExtractor implementation
ast-grep --pattern 'class FragmentParametersDefinitionExtractor' -A 10
# Check NodeCompiler's fragment parameter handling
rg -A 10 "fragment.*parameter" scenario-compiler/
# Check test cases for fragment parameter validation
rg -A 10 "test.*fragment.*parameter" scenario-compiler/src/test/
Length of output: 40484
56-56: Verify the impact of simplified FragmentParametersDefinitionExtractor initialization
The change simplifies the FragmentParametersDefinitionExtractor constructor by removing the class definitions parameter. Since this is a revert PR, we should ensure this doesn't break fragment parameter handling.
Let's verify the usage and implementation:
✅ Verification successful
Simplified FragmentParametersDefinitionExtractor initialization is safe
The change to simplify the FragmentParametersDefinitionExtractor constructor by removing the class definitions parameter is consistent with its implementation and usage across the codebase. The class only requires a ClassLoader for its functionality, and all instances across the codebase follow this pattern. The class definitions parameter was not used in the implementation.
Key findings:
- The class's implementation shows it only needs
ClassLoaderfor type resolution - All other usages in the codebase consistently use the single-parameter constructor
- No functionality is lost by removing the unused parameter
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if FragmentParametersDefinitionExtractor is used consistently across the codebase
ast-grep --pattern 'new FragmentParametersDefinitionExtractor($$$)'
# Check for any direct references to classDefinitions in fragment-related code
rg -l "classDefinitions.*Fragment"
Length of output: 4098
Script:
#!/bin/bash
# Let's check the implementation of FragmentParametersDefinitionExtractor to understand its constructor parameters
ast-grep --pattern 'class FragmentParametersDefinitionExtractor'
# Also check for any references to classDefinitions in fragment-related files
rg "classDefinitions" -A 3 -B 3 --type scala | grep -v "test"
Length of output: 37490
Script:
#!/bin/bash
# Let's check the implementation of FragmentParametersDefinitionExtractor to understand its usage
rg -A 5 -B 5 "FragmentParametersDefinitionExtractor" --type scala | grep -v "test"
# Also check for any changes in fragment-related files in this PR
ast-grep --pattern 'class Fragment'
Length of output: 24700
scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/definition/fragment/FragmentParametersDefinitionExtractor.scala (3)
Line range hint 76-85: LGTM! Good refactoring of validation logic
The change to use static validation through FragmentParameterValidator.validateAgainstClazzRefAndGetEditor is a good improvement that:
- Centralizes validation logic
- Reduces instance state
- Maintains consistent error handling
136-139: Verify runtime class resolution behavior
The change from class name parsing to runtime class resolution looks good, but we should verify:
- Performance impact of runtime class resolution
- Handling of edge cases (e.g., missing classes)
✅ Verification successful
Let me gather more information about error handling and test coverage.
Let me try one more search with corrected paths:
Based on the code analysis, I can now provide the final response:
Runtime class resolution implementation is robust and well-handled
The implementation shows:
- Proper error handling with fallback to
Unknowntype - Clear error reporting via
FragmentParamClassLoadErrorwith detailed context - Efficient use of
Trymonad for class resolution - Appropriate error propagation through the Writer monad
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for test coverage of runtime class resolution
rg -A 5 "toRuntimeClass.*classLoader"
# Look for error handling test cases
ast-grep --pattern 'test($$$) {
$$$
toRuntimeClass($$$)
$$$
}'
Length of output: 2555
Script:
#!/bin/bash
# Search for test files related to FragmentParametersDefinitionExtractor
fd "FragmentParametersDefinitionExtractor.*Test.*\.scala$"
# Look for test cases involving class loading errors
rg -B 2 -A 5 "classLoader.*toRuntimeClass" ".*Test.*\.scala$"
# Check implementation of error handling in FragmentParametersDefinitionExtractor
rg -B 5 -A 5 "Writer.*Unknown" FragmentParametersDefinitionExtractor.scala
Length of output: 342
Script:
#!/bin/bash
# Look for test files and test cases
rg -B 2 -A 5 "toRuntimeClass" ".*[Tt]est.*\.scala$"
# Check error handling in the implementation
rg -B 5 -A 5 "Unknown" scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/definition/fragment/FragmentParametersDefinitionExtractor.scala
Length of output: 1405
35-35: Verify the impact of removing classDefinitions parameter
The constructor has been simplified by removing the classDefinitions parameter. While this reduces coupling, we should ensure that all existing usages have been properly updated.
✅ Verification successful
Constructor change is safe and all usages have been updated
All existing usages of FragmentParametersDefinitionExtractor have been found and they only pass the classLoader parameter, showing that the codebase has been properly updated to match the simplified constructor signature. No instances were found where classDefinitions is still being passed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining references to classDefinitions in constructor calls
rg -A 2 "new FragmentParametersDefinitionExtractor.*classDefinitions"
# Search for any potential compilation issues in related files
ast-grep --pattern 'new FragmentParametersDefinitionExtractor($$$)'
Length of output: 4133
engine/flink/executor/src/main/scala/pl/touk/nussknacker/engine/process/compiler/StubbedFlinkProcessCompilerDataFactory.scala (2)
45-45: LGTM! Parameter removal aligns with parent class changes.
The removal of the classDefinitions parameter is safe as this method has only one usage in the codebase, as confirmed by previous learnings. This change maintains consistency with similar modifications in parent classes.
64-64: Verify fragment parameter extraction behavior.
The simplified constructor call to FragmentParametersDefinitionExtractor removes the classDefinitions dependency. While this aligns with the broader refactoring, we should verify that fragment parameter extraction still works as expected.
✅ Verification successful
Constructor change is consistent across the codebase
The verification shows that all usages of FragmentParametersDefinitionExtractor across the codebase have been updated to use only the classLoader parameter. The implementation in FragmentParametersDefinitionExtractor.scala confirms that the class is designed to work with just the classLoader parameter. The change maintains consistency with the broader refactoring and all current usages follow the same pattern.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the usage of FragmentParametersDefinitionExtractor across the codebase
# to ensure consistent constructor usage and no remaining classDefinitions dependencies
# Check for any remaining usages of classDefinitions with FragmentParametersDefinitionExtractor
ast-grep --pattern 'new FragmentParametersDefinitionExtractor($$$classDefinitions$$$)'
# Look for test cases that verify fragment parameter extraction
rg -l "FragmentParametersDefinitionExtractor.*Test"
Length of output: 2471
Script:
#!/bin/bash
# Let's try a different approach to verify the changes
# Check current usage of FragmentParametersDefinitionExtractor constructor
rg "new FragmentParametersDefinitionExtractor" -A 2
# Check the implementation of FragmentParametersDefinitionExtractor
ast-grep --pattern 'class FragmentParametersDefinitionExtractor'
# Look for tests containing "fragment" and "parameter" to catch relevant test files
rg -l "fragment.*parameter" -g "**/*Test*"
Length of output: 22367
scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/compile/nodecompilation/NodeDataValidator.scala (1)
48-48: Verify the revert changes and their impact on fragment parameter validation.
The simplified constructor call to FragmentParametersDefinitionExtractor removes the class definitions parameter. This change could affect how fragment parameters are validated and extracted.
Let's verify the revert changes and their impact:
✅ Verification successful
Based on the code inspection, I can now generate the final response:
The removal of classDefinitions parameter is safe and aligns with the implementation
The FragmentParametersDefinitionExtractor class doesn't use the removed classDefinitions parameter in its implementation. The class only requires the classLoader to resolve runtime classes for parameter typing. All parameter validation, including type checking and editor validation, is handled through the FragmentParameterValidator which also doesn't depend on class definitions.
Key observations:
- The parameter typing is done via
toRuntimeClass(classLoader)method - Editor validation is performed through
FragmentParameterValidatorwhich uses runtime class validation - No direct usage of class definitions in the parameter extraction logic
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the original implementation and related changes
# Check the original commit being reverted
git show d5154a3e7053d485fe766a021adee0401986411e -- 'scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/compile/nodecompilation/NodeDataValidator.scala'
# Check for any related changes in FragmentParametersDefinitionExtractor
rg -l "FragmentParametersDefinitionExtractor" | xargs git diff d5154a3e7053d485fe766a021adee0401986411e HEAD --
# Look for any tests that might be affected
fd -e scala -e java -x rg -l "FragmentParametersDefinitionExtractor"
Length of output: 23815
Script:
#!/bin/bash
# Let's check the implementation of FragmentParametersDefinitionExtractor and related validation logic
# Check the implementation of FragmentParametersDefinitionExtractor
ast-grep --pattern 'class FragmentParametersDefinitionExtractor {
$$$
}'
# Check the implementation of FragmentParameterValidator
ast-grep --pattern 'object FragmentParameterValidator {
$$$
}'
# Check for any tests related to fragment parameter validation
rg -A 10 "FragmentParametersDefinitionExtractor.*Test"
Length of output: 28812
Script:
#!/bin/bash
# Let's check the implementation of FragmentParametersDefinitionExtractor and its usage
# Check the implementation of FragmentParametersDefinitionExtractor
ast-grep --pattern 'class FragmentParametersDefinitionExtractor {
$$$
}'
# Check for any usages of classDefinitions in the extractor
rg -A 5 "classDefinitions" scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/definition/fragment/FragmentParametersDefinitionExtractor.scala
# Check for any tests that might verify the behavior
rg -l "FragmentParametersDefinitionExtractor.*Spec"
Length of output: 281
Script:
#!/bin/bash
# Let's check the actual file content and its changes
# Check the content of FragmentParametersDefinitionExtractor
cat scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/definition/fragment/FragmentParametersDefinitionExtractor.scala
# Check the git history for this specific file to understand the changes
git log -p -- scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/definition/fragment/FragmentParametersDefinitionExtractor.scala
# Check for any related test files
fd -e scala -x rg -l "class.*FragmentParametersDefinitionExtractor"
Length of output: 6323
engine/flink/executor/src/main/scala/pl/touk/nussknacker/engine/process/compiler/FlinkProcessCompilerDataFactory.scala (1)
12-12: LGTM!
The import for ClassDefinitionSet is required as it's used in the ComponentDefinitionContext case class definition.
scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/definition/test/ModelDataTestInfoProvider.scala (1)
27-27: Verify the impact of removing classDefinitions parameter
The removal of modelData.modelDefinitionWithClasses.classDefinitions.all from FragmentParametersDefinitionExtractor constructor could affect fragment parameter handling. Since this is part of a revert operation (PR #7202), we should ensure this change doesn't introduce any regressions.
✅ Verification successful
Change is safe - constructor parameter removal is consistent across codebase
Based on the verification results:
- The
FragmentParametersDefinitionExtractorclass consistently accepts onlyClassLoaderas a constructor parameter across all usages in the codebase. - All instances of
FragmentParametersDefinitionExtractorconstruction have been updated to use only the classloader parameter:- ModelDataTestInfoProvider.scala
- FragmentComponentDefinitionExtractor.scala
- ProcessCompilerData.scala
- ProcessCompiler.scala
- NodeDataValidator.scala
- The implementation in
FragmentParametersDefinitionExtractor.scalashows that the class only requires the classloader for resolving parameter types, and doesn't use class definitions for any other purpose.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other instances of FragmentParametersDefinitionExtractor to verify consistent parameter removal
ast-grep --pattern 'new FragmentParametersDefinitionExtractor($$$)'
# Look for any tests related to fragment parameters that might need adjustment
rg -l "FragmentParametersDefinitionExtractor.*Test"
# Check for any fragment-related configuration or documentation that might need updates
rg -l "fragment.*parameters.*definition" -g "*.{md,conf,properties,yaml,yml}"
Length of output: 4195
Script:
#!/bin/bash
# Let's check the implementation of FragmentParametersDefinitionExtractor to understand the constructor parameters
ast-grep --pattern 'class FragmentParametersDefinitionExtractor'
# Check the git history of this specific file to understand the revert context
git log -n 5 --oneline -- scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/definition/test/ModelDataTestInfoProvider.scala
# Look for tests that might verify fragment parameter handling
rg "FragmentParametersDefinitionExtractor" -A 5 -g "*.scala" | grep -i "test"
Length of output: 23164
scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/compile/ProcessCompiler.scala (1)
355-355: Verify the impact of simplified FragmentParametersDefinitionExtractor initialization
The removal of definitionWithTypes.classDefinitions.all from FragmentParametersDefinitionExtractor initialization appears to be part of a larger refactoring effort. While the change simplifies the initialization, we should verify that fragment parameter validation still works as expected.
Let's verify the usage and implementation:
✅ Verification successful
Let me verify the implementation of FragmentParametersDefinitionExtractor to ensure the change is safe.
Simplified FragmentParametersDefinitionExtractor initialization is correct
After reviewing the implementation and all usages across the codebase, the removal of definitionWithTypes.classDefinitions.all is safe because:
- The
FragmentParametersDefinitionExtractorclass only requires aClassLoaderin its constructor - All other usages in the codebase consistently pass only the classloader
- The implementation shows that class definitions are not used internally, as parameter type resolution is done directly via the classloader
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for other usages of FragmentParametersDefinitionExtractor and its implementation
# Check the implementation to confirm if classDefinitions is still needed
ast-grep --pattern 'class FragmentParametersDefinitionExtractor {
$$$
}'
# Check for other instantiations to ensure consistency
rg -A 2 'new FragmentParametersDefinitionExtractor'
Length of output: 3433
Script:
#!/bin/bash
# Check the implementation of FragmentParametersDefinitionExtractor
fd FragmentParametersDefinitionExtractor.scala --exec cat {}
Length of output: 6062
designer/client/src/components/graph/node-modal/editors/expression/Table/TableEditor.tsx (4)
20-20: LGTM: Import change aligns with type options refactoring
The import of useTypeOptions from FragmentInputDefinition indicates proper centralization of type options management.
Line range hint 104-114: LGTM: Component maintains consistency with new type options implementation
The Table component correctly integrates the new useTypeOptions hook while maintaining its core functionality. The type options are consistently used across column operations and initial setup.
Also applies to: 290-297
100-101: Verify type options integration across the application
The switch to useTypeOptions looks good, but let's verify the consistency of type options handling across the application since this is a revert PR.
✅ Verification successful
Type options integration is correctly implemented
The verification shows that:
- The
useTypeOptionshook is properly defined inFragmentInputDefinition.tsxand reused inTableEditor.tsx - No references to the old
useTableEditorTypeOptionsremain in the codebase - Type filtering is localized to
TableEditor.tsxwhereSUPPORTED_TYPESis defined and used consistently - The generic type parameter
<SupportedType>ensures type safety
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining references to the old type options pattern
# and verify consistent usage of the new useTypeOptions hook
# Check for any remaining references to old pattern
rg -l "useTableEditorTypeOptions"
# Verify consistent usage of new pattern
rg -l "useTypeOptions.*SupportedType"
Length of output: 162
Script:
#!/bin/bash
# Let's check for all usages of useTypeOptions to ensure consistent pattern
rg "useTypeOptions" -A 2
# Also check for any other components using type options to verify consistency
rg "SUPPORTED_TYPES" -A 2
Length of output: 3084
Line range hint 1-1: Verify PR intention: Revert vs. Refactor
There appears to be a discrepancy between the PR description (which indicates this is a revert PR) and the actual changes (which implement a type options refactoring). Please clarify if this should be a revert PR or if the PR description needs to be updated.
designer/server/src/test/scala/pl/touk/nussknacker/ui/definition/component/ComponentsUsageHelperTest.scala (1)
179-179: LGTM! Verify fragment component extraction.
The constructor parameter change from Set.empty to Some(_) looks correct as part of the revert operation.
Let's verify the consistency of FragmentComponentDefinitionExtractor usage across the codebase:
✅ Verification successful
Constructor parameter change is consistent across the codebase
All instances of FragmentComponentDefinitionExtractor constructor usage have been updated consistently to use Some(_) as the second parameter:
DefinitionsServiceSpec.scalaComponentsUsageHelperTest.scalaEdgeTypesPreparerTest.scalaComponentGroupsPreparerSpec.scalaAlignedComponentsDefinitionProvider.scala(production code)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for other instances of FragmentComponentDefinitionExtractor construction
# to ensure consistent parameter usage
# Search for FragmentComponentDefinitionExtractor constructor calls
rg -A 2 "new FragmentComponentDefinitionExtractor\("
# Search for potential fragment-related test files that might need similar updates
fd -e scala -e java . | rg -l "Fragment.*Test"
Length of output: 2312
scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/compile/nodecompilation/NodeCompiler.scala (1)
17-17: LGTM! Clean import change.
The import change aligns with the refactoring to use static methods from FragmentParameterValidator.
...main/scala/pl/touk/nussknacker/engine/process/compiler/FlinkProcessCompilerDataFactory.scala
Show resolved
Hide resolved
raphaelsolarski
changed the title
* 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]>
* 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]>
…167)"
This reverts commit d5154a3.
Describe your changes
Checklist before merge
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests