Fix subtyping in dict service #7115
Conversation
Diamekod0221
force-pushed
the
fix_subtyping_in_DictService
branch
2 times, most recently
from
81e1d11
to
05da3cb
Compare
components-api/src/main/scala/pl/touk/nussknacker/engine/api/typed/TypeConversionHandler.scala
Outdated
Show resolved
Hide resolved
...onents-api/src/main/scala/pl/touk/nussknacker/engine/api/typed/CanBeSubclassDeterminer.scala
Outdated
Show resolved
Hide resolved
...onents-api/src/main/scala/pl/touk/nussknacker/engine/api/typed/CanBeSubclassDeterminer.scala
Outdated
Show resolved
Hide resolved
Diamekod0221
force-pushed
the
fix_subtyping_in_DictService
branch
from
5976b3b
to
43706dc
Compare
📝 WalkthroughWalkthroughThe pull request introduces significant changes to the type-checking and conversion logic across various components of the Nussknacker engine. Key modifications include the replacement of the Possibly related PRs
Suggested labels
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: 4
🧹 Outside diff range and nitpick comments (38)
components-api/src/main/scala/pl/touk/nussknacker/engine/api/typed/NumberTypeUtils.scala (1)
18-20: Consider enhancing fallback strategy documentationWhile the comment about Double being "quite safe" is helpful, consider adding more detailed documentation about the fallback strategy to help future maintainers understand the reasoning behind the order of checks.
// in case of some unions else if (typ.canBeConvertedTo(Typed[java.lang.Integer])) java.lang.Integer.valueOf(0) - // double is quite safe - it can be converted to any Number + // Fall back to Double as it safely handles both integral and floating-point numbers + // while maintaining sufficient precision for most use cases else if (typ.canBeConvertedTo(Typed[Number])) java.lang.Double.valueOf(0)scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/spel/SpelExpressionValidator.scala (1)
21-21: Consider documenting the SpelExpressionRepr special case.The
|| expectedType == Typed[SpelExpressionRepr]condition appears to be a special case in the type checking logic. Consider adding a brief comment explaining why this bypass of conversion checking is necessary.+ // SpelExpressionRepr is allowed as an expected type without conversion checking + // because [explain the reason here] case a: TypingResult if a.canBeConvertedTo(expectedType) || expectedType == Typed[SpelExpressionRepr] =>engine/flink/components/table/src/main/scala/pl/touk/nussknacker/engine/flink/table/sink/TableTypeOutputValidator.scala (1)
Line range hint
24-26: Consider documenting the LogicalType.toString decisionThe comment suggests uncertainty about using
LogicalType.toString. Consider documenting the rationale for usingexpectedTypingResult.displayinstead, or if the comment is no longer relevant, remove it.components-api/src/test/scala/pl/touk/nussknacker/engine/api/typed/SubclassDeterminerSpec.scala (2)
9-17: Add documentation explaining assignability rulesWhile the test cases are comprehensive, adding comments explaining why certain type conversions are allowed (true) or disallowed (false) would improve maintainability and help other developers understand the business rules.
Example improvement:
test("Should validate assignability for decimal types") { + // Long cannot be assigned to Integer as it may cause data loss StrictConversionDeterminer.isAssignable(classOf[java.lang.Long], classOf[java.lang.Integer]) shouldBe false + // Integer can be safely assigned to Long as there's no data loss risk StrictConversionDeterminer.isAssignable(classOf[Integer], classOf[java.lang.Long]) shouldBe true
27-39: Consider creating a tracking issue for lang3 library bugThe test effectively documents the known issues with lang3's
ClassUtils.isAssignable. To ensure future cleanup when the library bug is fixed, consider:
- Creating a tracking issue referencing the upstream lang3 bug
- Adding the issue number to the test comment
- Setting up automated checks to verify if newer versions of lang3 fix this issue
This will help maintain the workaround in
SubclassDeterminer.isAssignableappropriately.Would you like me to help create a tracking issue for this?
components-api/src/test/scala/pl/touk/nussknacker/engine/api/typed/TypingResultErrorMessagesSpec.scala (2)
Line range hint
20-40: LGTM with enhancement suggestion for error messagesThe test case correctly validates type conversion scenarios with clear error messages. Consider enhancing the error messages to include type conversion suggestions when applicable, which could help developers understand valid conversion paths.
Line range hint
41-51: Consider improving nested structure error messagesWhile the test case correctly validates nested type conversions, the error message could be more specific about which part of the nested structure is incompatible. This would help developers quickly identify the exact point of type mismatch in complex structures.
designer/server/src/main/scala/pl/touk/nussknacker/ui/api/DictApiHttpService.scala (1)
65-67: Excellent fix for the subtyping issues!The transition from
canBeSubclassOftocanBeStrictlyConvertedTodirectly addresses the core issue of incorrect type returns (e.g., Long for Integer requests). This stricter type checking should prevent type mismatches and ensure more accurate dictionary filtering.Consider adding logging for cases where type conversion is rejected to help with debugging and monitoring of the type conversion behavior in production.
scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/definition/clazz/MethodTypeInfoSubclassChecker.scala (2)
15-15: LGTM! Consider updating related error messages.The change from
canBeSubclassOftocanBeConvertedToaligns with the PR objectives and should help fix the subtyping issues. However, the error messages in theNotSubclassVarArgumentcase class still reference "subclass" terminology, which might be misleading given the new conversion-based approach.Consider updating the error message to reflect the new semantics:
- s"vararg argument has illegal type: ${found.display} cannot be subclass of ${expected.display}" + s"vararg argument has illegal type: ${found.display} cannot be converted to ${expected.display}"
Line range hint
1-90: Consider documenting the type conversion rules.The transition from subclass checking to type conversion checking is a significant architectural change. Consider adding documentation that clearly outlines:
- The rules for valid type conversions
- The precedence of different conversion paths
- Examples of supported and unsupported conversions
This will help maintainers understand the expected behavior and make future modifications with confidence.
scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/language/dictWithLabel/DictKeyWithLabelExpressionParser.scala (1)
Line range hint
79-81: Update error message to reflect type conversion approach.The error message still refers to "subclass" despite the implementation now using type conversion checks. Consider updating the message to:
- s"DictKeyExpression expected type: ${expectedType.display} is unsupported. It must be a subclass of Long, Boolean or String" + s"DictKeyExpression expected type: ${expectedType.display} is unsupported. It must be convertible to Long, Boolean or String"designer/server/src/main/scala/pl/touk/nussknacker/ui/api/description/DictApiEndpoints.scala (2)
19-20: Consider using explicit imports instead of wildcard importWhile the wildcard import
sttp.tapir.json.circe._works, explicit imports would make it clearer which specific JSON functionalities are being used. This improves code maintainability and makes dependencies more transparent.
Line range hint
26-98: Good architectural approach to type handling refactoringThe endpoint definitions maintain a clean separation of concerns while introducing more flexible type handling. The error handling structure, particularly the inclusion of
MalformedTypingResult, provides good coverage for potential issues with the new Json-based type representation.A few architectural considerations:
- The endpoint responses use appropriate HTTP status codes
- Security is properly maintained through the auth parameter
- Error cases are well-defined and documented
scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/definition/clazz/MethodDefinition.scala (1)
Line range hint
99-107: Consider using a more graceful error handling approach.While the type validation logic is correct, throwing an AssertionError might be too aggressive for runtime validation. Consider returning a ValidationNel instead, consistent with the method's return type.
- if (!typesFromStaticMethodInfo.exists(calculated.canBeConvertedTo)) { - val expectedTypesString = typesFromStaticMethodInfo.map(_.display).mkString("(", ", ", ")") - val argumentsString = arguments.map(_.display).mkString("(", ", ", ")") - throw new AssertionError( - s"Generic function $name returned type ${calculated.display} that does not match any of declared types $expectedTypesString when called with arguments $argumentsString" - ) - } + if (!typesFromStaticMethodInfo.exists(calculated.canBeConvertedTo)) { + val expectedTypesString = typesFromStaticMethodInfo.map(_.display).mkString("(", ", ", ")") + val argumentsString = arguments.map(_.display).mkString("(", ", ", ")") + convertError( + GenericFunctionTypingError.ReturnTypeError( + s"Generic function $name returned type ${calculated.display} that does not match any of declared types $expectedTypesString when called with arguments $argumentsString" + ), + arguments + ).invalidNel + } else { + calculated.validNel + }components-api/src/test/scala/pl/touk/nussknacker/engine/api/typed/TypedFromInstanceTest.scala (1)
85-85: Consider renaming the function to match new type checking approach.While the change to
canBeConvertedTois correct, the function namecheckNotASubclassOfOtherParamTypingResultstill refers to the old subclass-based approach. Consider renaming it to something likecheckNotConvertibleToOtherParamTypingResultfor better clarity and consistency.engine/flink/management/dev-model/src/main/scala/pl/touk/nussknacker/engine/management/sample/global/DocumentationFunctions.scala (1)
113-113: LGTM! Consider enhancing the error message.The change to use type conversion checking is appropriate for field access validation.
Consider making the error message more specific by including the actual type:
- OtherError("Expected string with known value").invalidNel + OtherError(s"Expected string with known value but got: ${x.display}").invalidNelengine/flink/components/table/src/main/scala/pl/touk/nussknacker/engine/flink/table/utils/ToTableTypeSchemaBasedEncoder.scala (3)
85-86: LGTM! Consider improving readability.The double-check mechanism (Number compatibility + target type compatibility) is a robust solution. However, the condition could be more readable.
Consider reformatting for better readability:
- if typ - .canBeConvertedTo(Typed[Number]) && typ.canBeConvertedTo(targetType.toTypingResult) => + if typ.canBeConvertedTo(Typed[Number]) && + typ.canBeConvertedTo(targetType.toTypingResult) =>
108-108: LGTM! Consider extracting common condition.The changes correctly ensure that multiset count values can be converted to Int. The implementation is consistent across both cases.
Consider extracting the common condition to reduce duplication:
private val canBeUsedAsCount = (valueType: TypingResult) => valueType.canBeConvertedTo(Typed[Int])Also applies to: 111-111
Line range hint
29-111: Consider adding documentation for the type conversion strategy.The changes successfully implement a more flexible type conversion approach, replacing subclass checks with conversion checks throughout the file. This is a significant architectural change that would benefit from documentation.
Consider adding:
- Comments explaining the type conversion strategy
- Examples of supported type conversions
- Documentation of edge cases and limitations
designer/server/src/test/scala/pl/touk/nussknacker/ui/api/DictApiHttpServiceSpec.scala (1)
82-95: Consider improving test data isolation.While the test correctly validates Long type handling, consider extracting the dictionary ID "long_dict" as a constant to improve maintainability and make the test's dependencies more explicit.
+ private val LongDictId = "long_dict" // Then use it in the test - "id" : "long_dict", + "id" : LongDictId,utils/schemed-kafka-components-utils/src/main/scala/pl/touk/nussknacker/engine/schemedkafka/encode/AvroSchemaOutputValidator.scala (1)
155-159: Consider documenting the type validation strategyThe codebase now uses both
canBeSubclassOfandcanBeConvertedTofor different validation scenarios. Consider adding documentation to clarify:
- When to use
canBeSubclassOfvscanBeConvertedTo- The rationale behind using conversion checks specifically for map keys
- Any performance implications of the different validation approaches
This will help maintain consistency as the codebase evolves and guide future modifications to type validation logic.
utils/json-utils/src/main/scala/pl/touk/nussknacker/engine/json/encode/JsonSchemaOutputValidator.scala (1)
373-373: Consider adding a comment explaining type conversion behavior.Since this is a critical change in type validation logic, consider adding a comment explaining which type conversions are allowed and any potential precision considerations. This will help future maintainers understand the implications of using
canBeConvertedTo.+ // We use canBeConvertedTo instead of canBeSubclassOf to allow valid type conversions + // (e.g., Integer → Long) while still maintaining type safety. This ensures correct + // handling of numeric types in JSON schema validation. condNel( typingResult.canBeConvertedTo(schemaAsTypedResult), (), OutputValidatorTypeError(path, typingResult, JsonSchemaExpected(schema, rootSchema)) )engine/flink/components/base-unbounded/src/main/scala/pl/touk/nussknacker/engine/flink/util/transformer/aggregate/aggregates.scala (2)
356-358: Improve readability of the conditionWhile the type check change is correct, the multi-line condition could be more readable. Consider reformatting:
- if inputFields.keySet == scalaFields.keySet && klass.canBeConvertedTo( - Typed[java.util.Map[String, _]] - ) => + if inputFields.keySet == scalaFields.keySet && + klass.canBeConvertedTo(Typed[java.util.Map[String, _]]) =>
442-442: LGTM! Consider documenting type conversion behaviorThe change to
canBeConvertedToin MathAggregator trait affects all implementing aggregators. Consider adding documentation about the expected type conversion behavior to help future maintainers understand the implications of this change.components-api/src/test/scala/pl/touk/nussknacker/engine/api/typed/TypingResultSpec.scala (3)
79-82: Consider adding more union type conversion test cases.While the current tests cover basic union type conversions, consider adding test cases for:
- Union types with different type parameters
- Nested union types
- Union types with incompatible types
133-141: Consider adding array covariance test cases.While the current array type conversion tests are good, consider adding test cases that specifically verify array covariance behavior with:
- Primitive type arrays
- Object arrays with inheritance
- Multi-dimensional arrays
349-353: Consider adding more date/time format test cases.While the current tests cover basic date/time string conversions, consider adding test cases for:
- Different date/time formats
- Invalid format handling
- Timezone handling
components-api/src/main/scala/pl/touk/nussknacker/engine/api/typed/StrictConversionDeterminer.scala (3)
51-51: Typo in variable namegivenSuperclasThe variable
givenSuperclasseems to be misspelled. Consider renaming it togivenSuperclassfor consistency and readability.
75-83: Consider renamingisAssignabletoisAssignableFromfor clarityThe method
isAssignablechecks if an instance offromcan be assigned toto, similar to the JavaClass.isAssignableFrom(Class)method. Renaming it toisAssignableFromwould enhance clarity and align with conventional naming.
64-67: Clarify error message inisStrictSubclassmethodThe error message
"${givenClass.display} and ${givenSuperclass.display} are not the same"may be unclear. Consider rephrasing it to provide more context, such as"Expected ${givenSuperclass.display}, but found ${givenClass.display}".components-api/src/main/scala/pl/touk/nussknacker/engine/api/typed/TypeConversionHandler.scala (3)
23-24: Correct grammatical errors in documentation commentsThe documentation comments contain grammatical errors that may reduce clarity. Consider rephrasing them for better readability.
Apply this diff to improve the comments:
- * Given to this we cannot be sure if conversion is safe or not based on type (without scale knowledge). - * So we have two options: force user to convert to some type without floating point (e.g. BigInteger) or be loose in this point. + * Given this, we cannot be sure if the conversion is safe based on type alone (without scale knowledge). + * Therefore, we have two options: force the user to convert to a non-floating-point type (e.g., BigInteger) or be lenient in this aspect.
96-98: Address the TODO regarding strict number precision checkingThe TODO comment indicates a pending decision about adding a feature flag for strict number precision checking.
Would you like assistance in implementing this feature flag? This could enhance configurability and allow users to opt-in for strict precision checks as needed.
100-103: Optimize helper methods withinhandleStrictNumberConversionsThe helper methods
isFloatingandisDecimalNumberencapsulate type checks.Consider moving these methods to a shared utility class if they are used elsewhere, or refining them to improve clarity. For example, you might inline them if they're only used once, to streamline the code.
components-api/src/main/scala/pl/touk/nussknacker/engine/api/typed/ImplicitConversionDeterminer.scala (5)
24-26: Update documentation to reflect method functionality changeThe method documentation still references "subclass" terminology, but the method now handles type conversion rather than subclass checking. Please update the comments to accurately describe the new functionality.
Here is the suggested change:
/** - * This method checks if `givenType` can by subclass of `superclassCandidate` - * It will return true if `givenType` is equals to `superclassCandidate` or `givenType` "extends" `superclassCandidate` + * This method checks if `givenType` can be converted to `superclassCandidate`. + * It will return true if `givenType` is equal to `superclassCandidate` or if a valid type conversion exists. */
49-50: Clarify comment regarding type compatibility checksThe comment references a "double exists" approach but could be clearer regarding the trade-offs made for type safety versus user intentions. Consider refining the comment for better readability and understanding.
Suggested improvement:
- // "double exists" looks like a good tradeoff + // Using "double exists" allows flexibility for user intentions, accepting possible types rather than restricting expressions that could fail at runtime
166-167: Rename variable for consistency with method namesThe variable
canBeSubclassdoes not align with the updated method names focused on conversion. Rename it tocanBeConvertedto maintain consistency and improve code readability.Apply this diff to rename the variable:
val canBeConverted = equalClassesOrCanAssign andThen (_ => typeParametersMatches(givenClass, givenSuperclass)) -canBeSubclass orElse canBeConvertedTo(givenType, givenSuperclass) +canBeConverted orElse canBeConvertedTo(givenType, givenSuperclass)
170-170: Update TODO comment to reflect renamed methodsThe TODO comment references the old method
canBeSubclassOf. Update it tocanBeConvertedToto reflect the method's new name and functionality.Suggested change:
// TODO: Conversions should be checked during typing, not during generic usage of TypingResult.canBeConvertedTo(...)
199-203: Correct error message formatting and grammarThe error message in the
condNelcall has minor grammatical issues. Improve the message for clarity.Suggested correction:
s"Key types of Maps ${givenKeyParam.display} and ${superclassKeyParam.display} are not equal"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (37)
components-api/src/main/scala/pl/touk/nussknacker/engine/api/typed/ImplicitConversionDeterminer.scala(6 hunks)components-api/src/main/scala/pl/touk/nussknacker/engine/api/typed/NumberTypeUtils.scala(1 hunks)components-api/src/main/scala/pl/touk/nussknacker/engine/api/typed/StrictConversionDeterminer.scala(1 hunks)components-api/src/main/scala/pl/touk/nussknacker/engine/api/typed/TypeConversionHandler.scala(3 hunks)components-api/src/main/scala/pl/touk/nussknacker/engine/api/typed/typing.scala(2 hunks)components-api/src/test/scala/pl/touk/nussknacker/engine/api/typed/SubclassDeterminerSpec.scala(1 hunks)components-api/src/test/scala/pl/touk/nussknacker/engine/api/typed/TypedFromInstanceTest.scala(2 hunks)components-api/src/test/scala/pl/touk/nussknacker/engine/api/typed/TypingResultErrorMessagesSpec.scala(3 hunks)components-api/src/test/scala/pl/touk/nussknacker/engine/api/typed/TypingResultSpec.scala(7 hunks)designer/client/src/http/HttpService.ts(1 hunks)designer/server/src/main/scala/pl/touk/nussknacker/ui/api/DictApiHttpService.scala(1 hunks)designer/server/src/main/scala/pl/touk/nussknacker/ui/api/description/DictApiEndpoints.scala(2 hunks)designer/server/src/main/scala/pl/touk/nussknacker/ui/process/test/ScenarioTestService.scala(2 hunks)designer/server/src/test/scala/pl/touk/nussknacker/ui/api/DictApiHttpServiceSpec.scala(2 hunks)designer/server/src/test/scala/pl/touk/nussknacker/ui/validation/UIProcessValidatorSpec.scala(1 hunks)docs-internal/api/nu-designer-openapi.yaml(1 hunks)engine/flink/components/base-tests/src/test/scala/pl/touk/nussknacker/engine/flink/util/transformer/aggregate/AggregatesSpec.scala(1 hunks)engine/flink/components/base-unbounded/src/main/scala/pl/touk/nussknacker/engine/flink/util/transformer/aggregate/aggregates.scala(4 hunks)engine/flink/components/base/src/main/scala/pl/touk/nussknacker/engine/flink/util/transformer/ForEachTransformer.scala(1 hunks)engine/flink/components/table/src/main/scala/pl/touk/nussknacker/engine/flink/table/sink/TableTypeOutputValidator.scala(1 hunks)engine/flink/components/table/src/main/scala/pl/touk/nussknacker/engine/flink/table/utils/ToTableTypeSchemaBasedEncoder.scala(3 hunks)engine/flink/management/dev-model/src/main/scala/pl/touk/nussknacker/engine/management/sample/global/DocumentationFunctions.scala(2 hunks)engine/flink/management/dev-model/src/main/scala/pl/touk/nussknacker/engine/management/sample/global/ExampleFunctions.scala(1 hunks)engine/lite/components/base/src/main/scala/pl/touk/nussknacker/engine/lite/components/ForEachTransformer.scala(1 hunks)scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/compile/nodecompilation/FragmentParameterValidator.scala(1 hunks)scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/definition/clazz/ClassDefinitionExtractor.scala(2 hunks)scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/definition/clazz/MethodDefinition.scala(2 hunks)scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/definition/clazz/MethodTypeInfoSubclassChecker.scala(2 hunks)scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/language/dictWithLabel/DictKeyWithLabelExpressionParser.scala(1 hunks)scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/spel/SpelExpressionSuggester.scala(1 hunks)scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/spel/SpelExpressionValidator.scala(1 hunks)scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/spel/Typer.scala(11 hunks)scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/spel/typer/MethodReferenceTyper.scala(1 hunks)utils/default-helpers/src/main/scala/pl/touk/nussknacker/engine/util/functions/collection.scala(1 hunks)utils/default-helpers/src/main/scala/pl/touk/nussknacker/engine/util/functions/numeric.scala(1 hunks)utils/json-utils/src/main/scala/pl/touk/nussknacker/engine/json/encode/JsonSchemaOutputValidator.scala(1 hunks)utils/schemed-kafka-components-utils/src/main/scala/pl/touk/nussknacker/engine/schemedkafka/encode/AvroSchemaOutputValidator.scala(1 hunks)
🔇 Additional comments (67)
components-api/src/main/scala/pl/touk/nussknacker/engine/api/typed/NumberTypeUtils.scala (1)
18-18: LGTM! Type conversion checks improve type safety
The transition from subclass checks to conversion checks aligns well with the PR objectives and provides more accurate type compatibility verification. The fallback strategy is logically sound:
- First attempting Integer conversion for whole numbers
- Then falling back to Double for broader number types
Also applies to: 20-20
scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/spel/SpelExpressionValidator.scala (1)
21-21: LGTM! Verify type conversion behavior.
The change from subclass checking to type conversion checking aligns well with the PR objectives and should fix the incorrect subtype handling issues. The implementation maintains proper error handling through the Invalid case.
Please ensure that all edge cases are covered in tests, particularly:
- Long to Integer conversions
- Number type handling for Long requests
- Cases where type conversion might result in data loss
engine/flink/components/table/src/main/scala/pl/touk/nussknacker/engine/flink/table/sink/TableTypeOutputValidator.scala (1)
17-17: LGTM! Verify type conversion behavior
The change from canBeSubclassOf to canBeConvertedTo aligns with the PR objective of fixing subtyping issues. This should properly handle cases where type conversion is possible without requiring strict subclass relationships.
engine/lite/components/base/src/main/scala/pl/touk/nussknacker/engine/lite/components/ForEachTransformer.scala (1)
45-47: LGTM! The type conversion change aligns with the PR objectives.
The switch to canBeConvertedTo from canBeSubclassOf is consistent with the PR's goal to fix subtyping issues in the dict service. This should help resolve the incorrect type handling problems mentioned in the PR description.
Please ensure that the test suite covers edge cases where:
- Collections with various element types are processed
- Non-collection types are handled (should return Unknown)
- Generic type parameters are properly preserved
components-api/src/test/scala/pl/touk/nussknacker/engine/api/typed/SubclassDeterminerSpec.scala (1)
1-7: LGTM! Well-structured test file setup
The file structure follows Scala best practices with clear package declaration and appropriate testing framework imports.
engine/flink/components/base/src/main/scala/pl/touk/nussknacker/engine/flink/util/transformer/ForEachTransformer.scala (1)
48-50: LGTM! The type conversion check aligns with the PR objectives.
The transition from subclass checking to type conversion checking is appropriate here and aligns with the broader goal of fixing subtyping issues. Please verify that this change correctly handles all relevant collection types (e.g., List, Set, Queue) to ensure consistent behavior across different collection implementations.
components-api/src/test/scala/pl/touk/nussknacker/engine/api/typed/TypingResultErrorMessagesSpec.scala (2)
16-16: LGTM: Import change aligns with the new type conversion approach
The change from subclass checks to conversion checks is consistent with the PR's objective to fix subtyping issues.
52-75: LGTM: Comprehensive coverage of special cases
The test cases thoroughly validate various edge cases and special scenarios, with clear and specific error messages for each case. The null handling is particularly well-implemented, ensuring type safety.
designer/server/src/main/scala/pl/touk/nussknacker/ui/api/DictApiHttpService.scala (1)
59-59: Verify the decoder behavior with the full expectedType object.
The change to pass the complete expectedType object instead of just its value looks correct, as it allows for more comprehensive type information during decoding.
scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/definition/clazz/MethodTypeInfoSubclassChecker.scala (2)
41-43: LGTM! Verify type conversion behavior.
The change to use canBeConvertedTo is consistent with the PR objectives and should fix the incorrect subtype handling. The surrounding logic properly maintains parameter position tracking for error reporting.
Please ensure that all expected type conversions (especially edge cases like Long to Integer) are covered by the test suite.
49-49: LGTM! Ensure comprehensive return type testing.
The change to canBeConvertedTo completes the consistent pattern of changes across all type checks. This should properly handle return type compatibility checks.
Consider adding test cases that specifically verify:
- Number type dictionary returns for Long requests
- Long type dictionary returns for Integer requests
- Edge cases where type conversion should be rejected
scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/language/dictWithLabel/DictKeyWithLabelExpressionParser.scala (1)
73-77: LGTM! Type conversion checks properly implemented.
The transition from canBeSubclassOf to canBeConvertedTo aligns with the PR objectives to fix subtyping issues. The implementation correctly handles type conversion for Long, Boolean, and String types.
designer/server/src/main/scala/pl/touk/nussknacker/ui/api/description/DictApiEndpoints.scala (2)
90-90: Verify the implications of using raw Json type
The change from TypingResultInJson to Json for expectedType makes the type handling more flexible, which aligns with fixing the subtyping issues. However, please ensure that:
- Proper validation is implemented where this DTO is processed
- Error handling adequately covers potential JSON parsing/validation failures
- Type conversion logic in the service layer correctly handles the raw Json input
98-100: LGTM! Well-structured schema definitions
The schema definitions are properly implemented with:
- Appropriate use of lazy evaluation
- Correct implicit declarations
- Clean usage of Schema.derived
scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/definition/clazz/MethodDefinition.scala (2)
38-41: LGTM! Verify array-to-list conversion edge cases.
The updated type conversion logic correctly handles both direct type conversion and array-to-list conversion scenarios. This change aligns well with the PR's objective of fixing subtyping issues.
Consider adding test cases for edge cases like:
- Empty arrays
- Arrays with null elements
- Nested arrays that could be converted to nested lists
46-46: LGTM! Clean and consistent type conversion check.
The vararg parameter type checking has been properly updated to use type conversion instead of subclass checking, maintaining consistency with the new type system approach.
components-api/src/test/scala/pl/touk/nussknacker/engine/api/typed/TypedFromInstanceTest.scala (3)
63-64: LGTM! Test updated to use new type conversion checks.
The change correctly implements the transition from subclass checks to type conversion checks for empty lists, maintaining coverage for both Scala and Java collections.
71-71: LGTM! Helper function updated to use new type conversion mechanism.
The change correctly updates the type checking mechanism in the helper function while maintaining its original validation purpose.
76-76: LGTM! Parameter type checking updated consistently.
The change correctly implements type conversion checks for generic type parameters, maintaining consistency with the new approach.
engine/flink/management/dev-model/src/main/scala/pl/touk/nussknacker/engine/management/sample/global/DocumentationFunctions.scala (1)
65-65: LGTM! Verify test coverage for type conversion scenarios.
The change from subclass checking to type conversion checking aligns well with the PR objectives and provides more accurate type compatibility validation for the plus operation.
Consider adding test cases to verify:
- Mixed numeric type operations (e.g., Int + Long, Float + Double)
- Edge cases with custom Number implementations
engine/flink/management/dev-model/src/main/scala/pl/touk/nussknacker/engine/management/sample/global/ExampleFunctions.scala (2)
150-150: LGTM! Change aligns with PR objectives.
The transition from canBeSubclassOf to canBeConvertedTo aligns with the PR's goal of fixing subtyping issues in the dict service. This change makes the type checking more flexible by allowing type conversions instead of strict subclass relationships.
150-150: Verify numeric type conversion safety.
While the change to canBeConvertedTo improves type flexibility, please verify:
- All possible numeric type conversions are safe and won't cause precision loss
- Error messages and validation behavior remain appropriate for all numeric types
- Edge cases (e.g., very large Long values being converted) are handled correctly
scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/spel/typer/MethodReferenceTyper.scala (1)
65-65: LGTM! The change aligns with the broader type-checking improvements.
The modification from canBeSubclassOf to canBeConvertedTo is consistent with the PR's objective of fixing subtyping issues. This change is part of a systematic refactoring across the codebase to improve type conversion handling, particularly in cases where the previous subclass checks were returning incorrect results.
Please ensure that all test cases cover edge cases where the previous subclass-based type checking might have failed, particularly with Number types and their conversions.
engine/flink/components/table/src/main/scala/pl/touk/nussknacker/engine/flink/table/utils/ToTableTypeSchemaBasedEncoder.scala (1)
29-29: LGTM! Verify number type conversion scenarios.
The change from subclass checking to conversion checking aligns well with the PR objectives and should fix the incorrect number type returns. The implementation is safe as it's followed by proper NumberUtils conversion.
Consider adding test cases for edge cases like:
- Converting between different number types (Long → Integer, Double → Long, etc.)
- Handling overflow scenarios
utils/default-helpers/src/main/scala/pl/touk/nussknacker/engine/util/functions/numeric.scala (1)
132-132: LGTM! The change improves type conversion handling.
The switch from canBeSubclassOf to canBeConvertedTo aligns well with the PR's objective of fixing subtype handling issues. This change makes the type checking more flexible while maintaining type safety, particularly in cases where strict subclass relationships were causing false negatives.
Please ensure that the test suite covers edge cases where the previous subclass-based check was failing, particularly with:
- Long type dictionaries for Integer requests
- Number type dictionaries for Long requests
designer/server/src/main/scala/pl/touk/nussknacker/ui/process/test/ScenarioTestService.scala (2)
10-10: LGTM! Import change aligns with the PR objectives.
The transition from CanBeSubclassDeterminer to ImplicitConversionDeterminer correctly implements the architectural shift from subclass checks to type conversion checks.
Line range hint 136-140: LGTM! Verify UI behavior with edge cases.
The switch to canBeConvertedTo improves type compatibility checking for UI parameters. This change aligns with the PR's goal of fixing incorrect type handling.
Please verify that the UI correctly handles edge cases, such as:
- Complex types that might have string representations
- Custom types with implicit conversions to string
- Parameters with null values
designer/server/src/test/scala/pl/touk/nussknacker/ui/api/DictApiHttpServiceSpec.scala (2)
20-39: LGTM! Consider adding edge cases.
The test case effectively validates the fix for incorrect subtype handling, ensuring Integer requests don't return Long dictionaries.
Consider adding edge cases with different numeric types (e.g., Short, Byte) to ensure comprehensive subtype handling validation.
97-105:
Resolve inconsistency between test name and implementation.
The test name mentions "BigDecimal" but the implementation uses "BigInteger". This inconsistency could lead to confusion. Either:
- Rename the test to match the implementation, or
- Update the implementation to actually test BigDecimal if that was the intention.
Additionally, verify if returning the same dictionary ("long_dict") for both Long and BigInteger types is intentional as part of the type conversion testing.
scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/compile/nodecompilation/FragmentParameterValidator.scala (1)
157-157: LGTM! The type compatibility check has been updated correctly.
The change from canBeSubclassOf to canBeConvertedTo aligns with the PR objectives to fix subtyping issues in the dict service. This modification should properly handle cases where dictionary value types need to be converted to fragment parameter types.
Please ensure that test coverage includes edge cases such as:
- Converting between different numeric types (Long to Integer, etc.)
- Handling null values
- Cases where type conversion should fail
engine/flink/components/base-tests/src/test/scala/pl/touk/nussknacker/engine/flink/util/transformer/aggregate/AggregatesSpec.scala (1)
77-80: LGTM! The type checking improvement looks good.
The change from canBeSubclassOf to canBeConvertedTo aligns well with the PR objectives to fix subtyping issues. This should properly handle cases where type conversion is possible even without direct inheritance.
Consider adding test cases for edge cases such as:
- Numeric type conversions (e.g., Long to Integer)
- Complex object type conversions
- Cases where conversion should explicitly fail
utils/schemed-kafka-components-utils/src/main/scala/pl/touk/nussknacker/engine/schemedkafka/encode/AvroSchemaOutputValidator.scala (1)
155-159: LGTM! Verify map key type conversion behavior
The change from canBeSubclassOf to canBeConvertedTo aligns with the PR objectives to fix subtyping issues. This modification should properly handle cases where map keys can be converted to strings, rather than strictly checking for subclass relationships.
Please verify that this change correctly handles all expected map key types, particularly:
- String-like types (e.g., CharSequence)
- Numeric types that should be convertible to strings
- Custom types with string conversion methods
utils/json-utils/src/main/scala/pl/touk/nussknacker/engine/json/encode/JsonSchemaOutputValidator.scala (1)
373-373: LGTM! Change aligns with PR objectives.
The switch from canBeSubclassOf to canBeConvertedTo correctly addresses the issue of improper subtype handling mentioned in the PR objectives. This change will allow valid type conversions (e.g., Integer → Long) that were previously being incorrectly rejected.
engine/flink/components/base-unbounded/src/main/scala/pl/touk/nussknacker/engine/flink/util/transformer/aggregate/aggregates.scala (2)
193-193: LGTM! Verify Boolean type conversion handling
The change from canBeSubclassOf to canBeConvertedTo improves type safety by ensuring proper type conversion capability. Please verify that all expected Boolean-convertible types are still accepted correctly.
242-242: LGTM! Verify numeric type conversion handling
The change to canBeConvertedTo ensures proper type conversion capability for numeric types. Please verify that all numeric type conversions work correctly, especially with the ForLargeFloatingNumbersOperation promotion strategy.
components-api/src/main/scala/pl/touk/nussknacker/engine/api/typed/typing.scala (2)
30-41: LGTM! Clear distinction between conversion types.
The introduction of two separate methods with clear documentation helps prevent precision loss issues. The implementation correctly delegates to appropriate determiners.
Please ensure that:
- All existing callers of the old
canBeSubclassOfmethod have been updated to use the appropriate new method based on their precision requirements - The determiners (
ImplicitConversionDeterminerandStrictConversionDeterminer) handle all edge cases correctly
472-474: LGTM! Consistent usage of new conversion method.
The update to use canBeConvertedTo aligns with the broader refactoring from subclass checks to conversion checks.
scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/spel/SpelExpressionSuggester.scala (1)
414-416: LGTM! The change from subclass to conversion checks aligns with PR objectives.
The modification to use canBeConvertedTo instead of canBeSubclassOf should correctly fix the subtyping issues mentioned in the PR objectives, particularly the cases where incorrect types (e.g., Long for Integer requests) were being returned.
Please ensure that the test suite includes cases that verify:
- Collection type conversions (e.g., List to Collection)
- Map type conversions
- Edge cases where type conversion should fail
components-api/src/test/scala/pl/touk/nussknacker/engine/api/typed/TypingResultSpec.scala (5)
35-61: LGTM! Basic type conversion tests are comprehensive.
The test cases effectively cover various scenarios for record type conversions, including:
- Subset/superset field relationships
- Type compatibility checks
- List type conversions
- Map type conversions
86-93: LGTM! Unknown type conversion tests are well-structured.
The test cases properly verify the behavior of Unknown type conversions with various target types.
145-150: LGTM! Numeric type conversion tests are thorough.
The test cases effectively cover various numeric type conversions and their relationships.
306-314: LGTM! Tagged value type conversion tests are comprehensive.
The test cases properly verify tagged value type conversions, including:
- Same tag, same type
- Different tags
- Tagged to untagged conversions
452-452: LGTM! Property-based tests provide good coverage.
The property-based tests effectively verify type conversion properties:
- Reflexivity: A type can be converted to itself
- Transitivity: If A can be converted to B and B to C, then A can be converted to C
Also applies to: 466-470, 486-491
utils/default-helpers/src/main/scala/pl/touk/nussknacker/engine/util/functions/collection.scala (1)
359-361: LGTM! Verify numeric type conversion behavior.
The change from canBeSubclassOf to canBeConvertedTo aligns with the PR's objective to fix subtyping issues. This modification should correctly handle cases where numeric types need conversion rather than strict subtype relationships.
Please verify that:
- Long type dictionaries are now correctly handled for Integer requests
- Number type dictionaries are properly returned for Long requests
- Edge cases with different numeric type combinations work as expected
designer/client/src/http/HttpService.ts (1)
857-857: LGTM! Simplified payload structure.
The simplified structure of expectedType in the request payload aligns with the broader changes transitioning from subclass checks to conversion checks.
Please ensure that this payload structure change is compatible with the corresponding backend API changes.
docs-internal/api/nu-designer-openapi.yaml (3)
Line range hint 1119-1127: LGTM! Dict schema properly supports type system
The Dict schema is well-structured with proper type definitions that support the subtyping system through SingleTypingResult.
Line range hint 1128-1201: Verify dictionary type compatibility after schema fix
The endpoint implementation looks correct, but its type compatibility checking will only work properly after fixing the DictListRequestDto schema. Please verify the endpoint's behavior with various type combinations once the schema is updated.
Line range hint 5084-5087: LGTM! Security scheme is well-implemented
The security implementation using HTTP Basic Auth is properly defined and consistently applied across endpoints, with good support for user impersonation.
components-api/src/main/scala/pl/touk/nussknacker/engine/api/typed/StrictConversionDeterminer.scala (1)
56-58: Address TODO: Improve TypedNull handling for typed objects
There's a TODO comment indicating that TypedNull should not be considered a subclass of TypedObjectWithValue when all values are assigned. Addressing this is important to prevent potential type system inconsistencies when dealing with null values.
Would you like assistance in implementing the necessary logic to handle this case?
components-api/src/main/scala/pl/touk/nussknacker/engine/api/typed/TypeConversionHandler.scala (2)
71-75: Ensure proper integration of canBeStrictlyConvertedTo method
The new method canBeStrictlyConvertedTo adds stricter conversion checks, which is beneficial for precise type handling.
Please verify that:
- All calls to type conversion checks are updated appropriately where strict conversions are required.
- The method complements existing functionality without introducing regressions.
92-117: 🛠️ Refactor suggestion
Review logic in handleStrictNumberConversions for completeness
The handleStrictNumberConversions method introduces stricter rules for number conversions. Ensure that the logic correctly handles all relevant number types and edge cases.
Consider the following:
- Floating-Point Numbers: Confirm that the check for floating-point numbers includes all relevant types and that conversions are handled appropriately.
- Decimal Numbers: Verify that the use of
StrictConversionDeterminer.isAssignablecorrectly enforces strict conversions for decimal numbers. - Default Case Handling: Ensure the default case (
case _ => false) properly addresses types that should not be converted, preventing unintended conversions.
If there are known limitations or exceptions, consider documenting them within the code comments for clarity.
components-api/src/main/scala/pl/touk/nussknacker/engine/api/typed/ImplicitConversionDeterminer.scala (4)
145-150: Review the logic of isStrictSubclass method
The isStrictSubclass method may not accurately reflect the new conversion-based logic. It still uses subclass terminology and checks for class equality or assignability, which may not align with type conversion semantics. Verify if this method needs adjustment or renaming.
182-183: Simplify condition in typeParametersMatches
The condition within typeParametersMatches uses a logical OR (||) between canBeConvertedTo in both directions. Considering the method's goal, verify if both checks are necessary or if the logic can be simplified to improve performance and readability.
217-219: Assess compatibility of ClassUtils.isAssignable with JDK versions
The comment mentions using an explicit autoboxing flag due to compatibility issues with JDK 11. Verify that this approach is the most effective solution and consider updating the commons-lang3 library if feasible.
218-219:
Consider thread safety for shared utility methods
The isAssignable method is a shared utility that may be used concurrently. Ensure that the ClassUtils.isAssignable method is thread-safe or consider synchronizing access if necessary.
scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/definition/clazz/ClassDefinitionExtractor.scala (1)
144-144: Ensure method deduplication aligns with new type conversion logic
The change from canBeSubclassOf to canBeConvertedTo alters the criteria for method selection during deduplication. Please verify that this adjustment maintains the intended behavior when selecting the most specific method among those with generic return types.
scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/spel/Typer.scala (10)
149-150: Verify the replacement of canBeSubclassOf with canBeConvertedTo
The method now uses canBeConvertedTo instead of canBeSubclassOf when checking child types. Ensure that this change aligns with the intended type-checking logic and does not introduce unintended type acceptance.
201-201: Check dynamic property access for string keys
The condition now checks if indexKey can be converted to Typed[String]. Please verify that this change correctly handles dynamic property access when dynamicPropertyAccessAllowed is true or false.
Line range hint 359-368: Validate arithmetic operation type checks
The conditions for arithmetic operations now use canBeConvertedTo(Typed[Number]). Confirm that this change correctly handles all numeric types and maintains proper error handling for non-numeric types.
398-409: Ensure correct handling of string concatenation and numeric addition
In the OpPlus operator, the logic now checks for type conversion to Typed[String] or Typed[Number]. Please verify that this correctly differentiates between string concatenation and numeric addition without introducing type coercion issues.
451-451: Confirm boolean evaluation in selection expressions
The condition checks if result can be converted to Typed[Boolean]. Ensure that this modification correctly evaluates selection predicates and does not affect the selection mechanism.
462-462: Validate ternary operator condition type conversion
The ternary operator now uses canBeConvertedTo(Typed[Boolean]) for its condition. Please confirm that this change correctly enforces boolean conditions and handles type conversions appropriately.
523-526: Review iterative type extraction logic with type conversion
The method extractIterativeType replaces canBeSubclassOf with canBeConvertedTo when handling collections and arrays. Verify that this change maintains correct type extraction for iterable types.
578-578: Ensure arithmetic operations handle numeric conversions properly
In checkTwoOperandsArithmeticOperation, both operands are checked with canBeConvertedTo(Typed[Number]). Confirm that this allows for proper numeric promotions and that invalid conversions are correctly handled.
597-597: Check single-operand arithmetic operation type validation
For unary operations like increment and decrement, the operand is now validated with canBeConvertedTo(Typed[Number]). Please ensure this change correctly restricts the operation to numeric types.
693-696: Validate type conversion in projection and selection
The conditions in extractIterativeType now use canBeConvertedTo for collections and maps. Verify that this change accurately identifies iterable types and does not introduce errors in projections or selections.
designer/server/src/test/scala/pl/touk/nussknacker/ui/validation/UIProcessValidatorSpec.scala (1)
1953-1953: Ensure consistent parameter types across tests
In this line, the parameter type for P1 has been changed to Integer. However, in other tests, such as the one starting at line 1969, P1 is still using Short. Consider updating all relevant tests to use the same parameter type to maintain consistency and avoid potential confusion.
components-api/src/test/scala/pl/touk/nussknacker/engine/api/typed/SubclassDeterminerSpec.scala
Outdated
Show resolved
Hide resolved
components-api/src/main/scala/pl/touk/nussknacker/engine/api/typed/TypeConversionHandler.scala
Outdated
Show resolved
Hide resolved
...er/src/main/scala/pl/touk/nussknacker/engine/definition/clazz/ClassDefinitionExtractor.scala
Show resolved
Hide resolved
…yped/SubclassDeterminerSpec.scala Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
|
||
| @JsonCodec | ||
| case class DictListRequestDto(expectedType: TypingResultInJson) | ||
| case class DictListRequestDto(expectedType: Json) |
There was a problem hiding this comment.
This change of expected request body type should be described in MigrationGuide under REST API changes section
| StrictConversionDeterminer.isAssignable(classOf[java.lang.Long], classOf[java.lang.Double]) shouldBe true | ||
| } | ||
|
|
||
| // to check if autoboxing lang3 is failing - we can remove our fallback from SubclassDeterminer.isAssignable if the lib works properly |
There was a problem hiding this comment.
it works correctly according to Java Spec rules. We could remove our fallback if we called ClassUtils.wrapperToPrimitive on to Class, but still wouldn't work for non-primitives - BigInteger and BigDecimal. I'd remove this test.
| val givenClass = givenType.runtimeObjType | ||
| val givenSuperclas = superclassCandidate.runtimeObjType | ||
|
|
||
| isStrictSubclass(givenClass, givenSuperclas) |
There was a problem hiding this comment.
We can make it so the only change in logic between a strict conversion check and loose conversion check is this singleCanBeConvertedTo. From what i understand, the only difference should be that we should not allow precision loss on converting numbers when doing a strict check. Just instead of calling TypeConversionHandler.canBeConvertedTo i TypeConversionHandler.canBeStrictlyConvertedTo and just handle the BigDecimal and BigInteger conversions there.
Right now because theres no reuse of rest of logic we dont handle our whole type system like records or just typed object with values.
Ideally I think we'd have a single object like ImplicitConversionDeterminer that has only 2 public defs - canBeConvertedTo and canBeStrictlyconvertedTo . Everything else should be private so that no one else reuses it in context we dont control.
There was a problem hiding this comment.
Refactroed to a single AssignabilityDeterminer as per your PoC
| class SubclassDeterminerSpec extends AnyFunSuite with Matchers { | ||
|
|
||
| test("Should validate assignability for decimal types") { | ||
| StrictConversionDeterminer.isAssignable(classOf[java.lang.Long], classOf[java.lang.Integer]) shouldBe false |
There was a problem hiding this comment.
I think we shouldn't ever check .isAssignable - the only things that should be public in a ConversionDeterminer is just the check taking 2 values which are types from out typing system.
There was a problem hiding this comment.
I have doubts whether being very strict with public/private here is the correct way, we could possibly end up in a situation where the next person is also surprised by how this method works. I will remove the test and add a comment in isAssignable to prevent that
There was a problem hiding this comment.
Why leave it public? Its not / shouldn't be used in this project or outside it.
There was a problem hiding this comment.
Just for testing, i agree that it shouldn't be used outside
|
|
||
| class SubclassDeterminerSpec extends AnyFunSuite with Matchers { | ||
|
|
||
| test("Should validate assignability for decimal types") { |
There was a problem hiding this comment.
It would be great to have a precise comparison of our 2 ways of comparing types (currently from StrictConversionDeterminer and ImplcitConversionDeterminer). I'd prefer to do it on a one test data table and use it in 2 parameterized TableDrivenPropertyChecks tests. Then we have a proof that the strict checker work on other data types like maps, lists, records, tagged types.
There was a problem hiding this comment.
I have added such tests in AssignabilityDeterminerSpec. I only compare the numerical types that actually differ in those two Determiners. The rest is already tested in typingResultSpec.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (12)
components-api/src/main/scala/pl/touk/nussknacker/engine/api/typed/StrictConversionDeterminer.scala (4)
32-35: Consider elevating the type safety tradeoff explanation to documentation.The comment explains an important design decision about type safety vs usability tradeoff. Consider moving this explanation to scaladoc to make it more visible and maintainable.
+/** + * This implementation chooses usability over strict type safety. Instead of requiring + * givenTypes.forAll(... superclassCandidates.exists ...) which would be safer but could + * block valid use cases, it uses "double exists" as a practical tradeoff. This allows + * scenarios where the user knows only compatible types will appear at runtime. + */ def canBeConvertedTo(
46-54: Add documentation for singleCanBeConvertedTo method.This method plays a crucial role in type conversion determination. Consider adding scaladoc to explain its purpose and relationship with isStrictSubclass.
+/** + * Determines if a single type can be converted to another by comparing their runtime + * object types using strict subclass rules. + * + * @param givenType The type to check for conversion + * @param superclassCandidate The target type to convert to + * @return Validation result indicating if conversion is possible + */ def singleCanBeConvertedTo(
57-58: Address TODO for Null handling with TypedMap.The TODO indicates that Null handling for typed maps with assigned values needs implementation. This aligns with the overall goal of improving type safety.
Would you like help implementing the TypedMap null check or should I create an issue to track this?
62-84: Consider architectural improvements for conversion handling.As suggested in the previous review, consider:
- Creating a single
ImplicitConversionDeterminerobject with only two public methods:canBeConvertedToandcanBeStrictlyConvertedTo- Making all helper methods private to prevent uncontrolled reuse
- Consolidating the number type conversion logic in
TypeConversionHandlerThis would improve maintainability and ensure consistent behavior across the type system.
components-api/src/main/scala/pl/touk/nussknacker/engine/api/typed/ImplicitConversionDeterminer.scala (4)
Line range hint
16-23: Update class documentation to reflect the new conversion-focused approachThe class documentation still refers to subclass determination while the implementation has been refactored to focus on type conversion. Consider updating it to reflect this change and explain the new conversion-based approach.
50-68: Consider reducing code duplication with canBeConvertedToThe method shares significant logic with
canBeConvertedTo. Consider extracting the common pattern matching logic into a private helper method that takes a function parameter for the different conversion strategies (strict vs. non-strict).+ private def handleTypeConversion( + givenType: TypingResult, + superclassCandidate: TypingResult, + singleTypeHandler: (SingleTypingResult, SingleTypingResult) => ValidatedNel[String, Unit] + ): ValidatedNel[String, Unit] = { + (givenType, superclassCandidate) match { + case (_, Unknown) => ().validNel + case (Unknown, _) => ().validNel + case (TypedNull, other) => canNullBeConvertedTo(other) + case (_, TypedNull) => s"No type can be subclass of ${TypedNull.display}".invalidNel + case (given: SingleTypingResult, superclass: TypedUnion) => + canBeConvertedTo(NonEmptyList.one(given), superclass.possibleTypes) + case (given: TypedUnion, superclass: SingleTypingResult) => + canBeConvertedTo(given.possibleTypes, NonEmptyList.one(superclass)) + case (given: SingleTypingResult, superclass: SingleTypingResult) => + singleTypeHandler(given, superclass) + case (given: TypedUnion, superclass: TypedUnion) => + canBeConvertedTo(given.possibleTypes, superclass.possibleTypes) + } + } def canBeConvertedTo(...): ValidatedNel[String, Unit] = - (givenType, superclassCandidate) match { ... } + handleTypeConversion(givenType, superclassCandidate, singleCanBeConvertedTo) def canBeStrictlyConvertedTo(...): ValidatedNel[String, Unit] = - (givenType, superclassCandidate) match { ... } + handleTypeConversion(givenType, superclassCandidate, singleCanBeStrictlyConvertedTo)
221-221: Track architectural improvement for type conversion checksThe TODO comment indicates an important architectural improvement to move type conversion checks to the typing phase. This would help prevent type-related issues earlier in the process.
Would you like me to create a GitHub issue to track this architectural improvement? The issue would cover moving type conversion checks from runtime to the typing phase.
Line range hint
239-243: Consider extracting array type conversion logicAs noted in the comment, array type conversions should be handled separately from the generic type parameter matching logic. This would improve code organization and make the type system more maintainable.
Consider creating a separate handler for array type conversions that can be composed with the generic type parameter matching logic.
docs/MigrationGuide.md (4)
608-609: Fix formatting and improve clarity of DictApiEndpoints changesThe changes for DictApiEndpoints are not properly formatted and lack context. Consider expanding this section to:
* [#7115](https://github.com/TouK/nussknacker/pull/7115) Changes in DictApiEndpoints: * Changed `DictListRequestDto.expectedType` type from `TypingResultInJson` to `Json` * This change affects dictionary API request format
Line range hint
1-24: Consider adding version compatibility matrixThe migration guide would benefit from having a compatibility matrix at the top showing:
- Supported Scala versions
- Supported Flink versions
- Supported Java versions
- Required database schema versions
This would help users quickly determine if they can upgrade and what dependencies need to be updated.
Line range hint
25-50: Improve organization of configuration changes sectionThe configuration changes section could be better organized by grouping related changes together, for example:
- Authentication/Security changes
- Logging changes
- Deployment changes
- UI/Frontend changes
This would make it easier for users to find relevant changes for their use case.
Based on the gathered information, I can now generate the final response:
Add verification steps for critical API changes
The migration guide should include the following verification steps after upgrade:
- Verify DeploymentManager API changes:
- Check if custom DeploymentManager implementations are updated to use the new
DeploymentUpdateStrategyinstead ofsavepointPath- Verify state restoration works correctly after deployment
- Verify API endpoints:
- Test the new
/api/scenarioTesting/{scenarioName}/adhoc/validateendpoint- Confirm PUT
/api/processes/{processName}works with the newscenarioLabelsfield- Verify GET
/api/processDefinitionData/*with the newenrichedWithUiConfigparameter🔗 Analysis chain
Line range hint
51-100: Add migration verification stepsFor critical API changes like DeploymentManager modifications, it would be helpful to add verification steps that users can follow to ensure their upgrade was successful. Consider adding a "Verification" subsection.
Let me gather more information about the API changes and migration steps.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for usage of deprecated APIs rg -l "DeploymentManagerDependencies"Length of output: 3066
Script:
#!/bin/bash # Check for recent changes in DeploymentManager API rg -A 5 "DeploymentManagerDependencies" designer/deployment-manager-api/src/main/scala/pl/touk/nussknacker/engine/DeploymentManagerDependencies.scala # Look for existing verification steps in migration guide rg -i "verif|check|test" docs/MigrationGuide.mdLength of output: 17093
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
components-api/src/main/scala/pl/touk/nussknacker/engine/api/typed/ImplicitConversionDeterminer.scala(6 hunks)components-api/src/main/scala/pl/touk/nussknacker/engine/api/typed/StrictConversionDeterminer.scala(1 hunks)components-api/src/test/scala/pl/touk/nussknacker/engine/api/typed/SubclassDeterminerSpec.scala(1 hunks)docs/MigrationGuide.md(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- components-api/src/test/scala/pl/touk/nussknacker/engine/api/typed/SubclassDeterminerSpec.scala
🔇 Additional comments (3)
components-api/src/main/scala/pl/touk/nussknacker/engine/api/typed/StrictConversionDeterminer.scala (1)
75-82: Verify fix for incorrect subtype handling.
The implementation includes special handling for number types, which should address the issues mentioned in the PR objectives. Let's verify this fixes the specific cases.
✅ Verification successful
Based on the search results and the implementation shown in the review comment, I can now generate the final response:
Implementation correctly handles number type conversions
The implementation properly addresses number type conversions through:
- Primary check using
ClassUtils.isAssignablefor strict assignability - Secondary check specifically for number types using
AllNumbersindex comparison, which ensures proper handling of widening conversions (e.g., Int to Long)
The test results show various type conversion scenarios being tested across the codebase, including numeric conversions in NumericUtilsSpec and type conversion validations in TypingResultSpec, confirming the robustness of the implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for test cases verifying number type conversions
rg -A 5 "test.*Long.*Integer|test.*Number.*Long" --type scala
# Search for related dict service tests
rg -A 5 "DictApiHttpService.*test" --type scala
Length of output: 113
Script:
#!/bin/bash
# Search for test files related to StrictConversionDeterminer
fd "StrictConversionDeterminer.*Test.*\.scala$"
# Search for test cases involving number type conversions in the test files
rg -A 10 "StrictConversionDeterminer" --type scala
# Look for the actual implementation to understand the number type handling
ast-grep --pattern 'object AllNumbers {
$$$
}'
Length of output: 5792
Script:
#!/bin/bash
# Search for test files in the entire codebase
fd "Test.*\.scala$" --exec rg -l "StrictConversionDeterminer|number.*conversion|type.*conversion" {}
# Look for the AllNumbers definition and its usage
rg -A 5 "object AllNumbers" --type scala
# Search for any related conversion test cases
rg -A 10 "test.*conversion" --type scala
Length of output: 26120
components-api/src/main/scala/pl/touk/nussknacker/engine/api/typed/ImplicitConversionDeterminer.scala (2)
34-48: LGTM! Comprehensive type conversion handling
The implementation properly handles all type cases with exhaustive pattern matching and clear conversion logic.
Line range hint 34-270: Verify the fix addresses the original dict service issues
The implementation looks solid and should address the subtyping issues mentioned in the PR objectives. However, let's verify that this fixes the specific cases mentioned in the PR description.
✅ Verification successful
Implementation correctly handles dict type conversions
Based on the DictApiHttpServiceSpec tests, the implementation properly handles type conversions for dictionaries, including:
- Subtyping checks for Integer types (empty list case)
- Long type dictionary handling (returns long_dict)
- Numeric type hierarchy (BigInteger can access long_dict)
- String type dictionaries (rgb, bc, dict)
The tests verify that the type conversion logic in ImplicitConversionDeterminer works correctly for the dict service use cases, particularly for numeric type hierarchies which was a key concern.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for dict service tests that verify Long/Integer type handling
rg -l "DictApiHttpService.*Long.*Integer" --type scala
# Search for related test cases
ast-grep --pattern 'test("$_") { $$$
$$$
DictApiHttpService
$$$
}'
Length of output: 131
Script:
#!/bin/bash
# Let's try a broader search for dict-related test files and type conversion tests
rg -l "DictApiHttpService" --type scala
# Also search for any test cases involving type conversion or subtyping
rg -l "TypeConversionHandler|canBeConvertedTo|isSubclassOf" --type scala --type test
# Look for dict-related type conversion tests
rg "Dict.*Type.*test" --type scala -A 5 -B 5
Length of output: 466
Script:
#!/bin/bash
# Let's examine the DictApiHttpServiceSpec for type conversion tests
cat designer/server/src/test/scala/pl/touk/nussknacker/ui/api/DictApiHttpServiceSpec.scala
# Also search for type conversion tests in the codebase
rg -l "TypeConversionHandler|canBeConvertedTo|isSubclassOf" --type scala
# Look for tests related to ImplicitConversionDeterminer
rg "ImplicitConversionDeterminer.*test|test.*ImplicitConversionDeterminer" --type scala -A 5 -B 5
Length of output: 10763
...s-api/src/main/scala/pl/touk/nussknacker/engine/api/typed/ImplicitConversionDeterminer.scala
Outdated
Show resolved
Hide resolved
| def isAssignable(from: Class[_], to: Class[_]): Boolean = | ||
| ClassUtils.isAssignable(from, to, true) |
There was a problem hiding this comment.
💡 Codebase verification
Comment about JDK 11 compatibility can be removed
The comment about JDK 11 compatibility with commons-lang3:3.3 is outdated and no longer relevant. The project is currently using commons-lang3 version 3.12.0, which fully supports JDK 11 and has resolved the earlier compatibility issues. The explicit autoboxing flag is no longer needed as a workaround.
components-api/src/main/scala/pl/touk/nussknacker/engine/api/typed/ImplicitConversionDeterminer.scala: Remove the outdated comment about JDK 11 compatibility with commons-lang3:3.3
🔗 Analysis chain
Verify commons-lang3 version compatibility
The comment indicates a workaround for JDK 11 compatibility with commons-lang3:3.3. Let's verify the current commons-lang3 version and its compatibility.
Let me gather more information about the commons-lang3 version being used.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check commons-lang3 version in the project
rg -A 1 'commons-lang3' --glob 'build.{gradle,sbt}'
# Check if there are any reported issues with commons-lang3 and JDK 11
gh api graphql -f query='
{
search(query: "org:apache repo:commons-lang type:issue JDK 11 ClassUtils isAssignable", type: ISSUE, first: 5) {
nodes {
... on Issue {
title
url
state
}
}
}
}'
Length of output: 1273
Script:
#!/bin/bash
# Check the value of flinkCommonsLang3V in build.sbt or other configuration files
rg "flinkCommonsLang3V\s*:?=" --glob '*.{sbt,scala}'
# Also check for any comments or documentation about JDK compatibility
rg -B 2 -A 2 "JDK.*11" --glob '*.{scala,sbt,md}'
Length of output: 3667
* simplify conversion check * changes to TypeConversionHandler * test fixes --------- Co-authored-by: Marcel Philipiak <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (6)
components-api/src/main/scala/pl/touk/nussknacker/engine/api/typed/typing.scala (2)
37-41: Consider enhancing documentation with examplesThe implementation looks good and provides a strict type conversion check. Consider adding examples to the documentation to illustrate the difference between strict and loose conversions (e.g., Int→Long vs Long→Int).
/** - * Checks if the conversion to a given typingResult can be made without loss of precision + * Checks if the conversion to a given typingResult can be made without loss of precision. + * + * Examples: + * - Int → Long: true (no precision loss) + * - Long → Int: false (potential precision loss) + * - Float → Double: true (no precision loss) + * - Double → Float: false (potential precision loss) */
30-41: Consider type conversion hierarchy documentationThe introduction of both loose and strict conversion checks creates a hierarchy of type compatibility. Consider documenting this relationship in a central location (e.g., package documentation) to help developers choose the appropriate method for their use case. This could include:
- When to use
canBeConvertedTovscanBeStrictlyConvertedTo- Impact on pattern matching
- Examples of type hierarchies and their conversion relationships
components-api/src/main/scala/pl/touk/nussknacker/engine/api/typed/TypeConversionHandler.scala (1)
24-24: Typo in comment: Correct grammatical errorPlease correct the grammatical error in the comment for clarity:
- * Given to this we cannot be sure if conversion is safe or not based on type (without scale knowledge). + * Given this, we cannot be sure if conversion is safe or not based on type (without scale knowledge).components-api/src/main/scala/pl/touk/nussknacker/engine/api/typed/AssignabilityDeterminer.scala (3)
10-16: Correct grammatical errors in the class documentation comments.The class documentation contains grammatical errors that affect readability. Consider updating the comments as follows:
- * This class determine if type can be subclass of other type. It basically based on fact that TypingResults are + * This class determines if a type can be a subclass of another type. It is basically based on the fact that TypingResults are
37-38: Fix the typo in the method name 'isNullAsignableTo'.The method name
isNullAsignableTocontains a typo. It should beisNullAssignableToto reflect the correct spelling.Apply this diff to correct the method name:
- private def isNullAsignableTo(to: TypingResult): ValidatedNel[String, Unit] = to match { + private def isNullAssignableTo(to: TypingResult): ValidatedNel[String, Unit] = to match {And update the method calls accordingly:
- case (TypedNull, other) => isNullAsignableTo(other) + case (TypedNull, other) => isNullAssignableTo(other)Also applies to: 50-50
51-51: Address the TODO regarding null assignability to typed maps.There's a TODO comment indicating that
Nullshould not be a subclass of typed maps with all values assigned. Implementing this logic will improve type safety and prevent potential runtime errors.Would you like assistance in implementing the necessary checks for this case?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
components-api/src/main/scala/pl/touk/nussknacker/engine/api/typed/AssignabilityDeterminer.scala(1 hunks)components-api/src/main/scala/pl/touk/nussknacker/engine/api/typed/TypeConversionHandler.scala(4 hunks)components-api/src/main/scala/pl/touk/nussknacker/engine/api/typed/typing.scala(2 hunks)components-api/src/test/scala/pl/touk/nussknacker/engine/api/typed/TypingResultErrorMessagesSpec.scala(4 hunks)designer/server/src/main/scala/pl/touk/nussknacker/ui/process/test/ScenarioTestService.scala(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- components-api/src/test/scala/pl/touk/nussknacker/engine/api/typed/TypingResultErrorMessagesSpec.scala
- designer/server/src/main/scala/pl/touk/nussknacker/ui/process/test/ScenarioTestService.scala
🔇 Additional comments (4)
components-api/src/main/scala/pl/touk/nussknacker/engine/api/typed/typing.scala (2)
30-35: Well-documented type conversion method!
The implementation correctly delegates to AssignabilityDeterminer and the documentation clearly explains that this method allows for precision loss during conversion.
472-474: Verify impact of looser type matching
The change from canBeSubclassOf to canBeConvertedTo allows for more flexible type matching but could potentially introduce precision loss during pattern matching. We should verify all usages of CastTypedValue to ensure this change doesn't lead to unexpected behavior.
✅ Verification successful
Let me gather more information about the type conversion behavior to better understand the impact.
Based on the gathered information, I can now provide the final response:
Change from canBeSubclassOf to canBeConvertedTo is safe
The change is actually an improvement in type safety. Looking at the implementations:
canBeConvertedTousesAssignabilityDeterminer.isAssignableLoosewhich is the correct behavior for pattern matching- The only usage of
CastTypedValueis inTyper.scalafor binary operations where looser type matching is desirable - There's a separate
canBeStrictlyConvertedTomethod available when strict type conversion is needed
The change aligns with type system best practices where pattern matching should be more permissive than direct assignments, and the implementation properly handles type conversion safety through AssignabilityDeterminer.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all usages of CastTypedValue pattern matching to assess impact
# Search for CastTypedValue pattern matching usage
echo "=== CastTypedValue pattern matching usage ==="
rg -A 5 "CastTypedValue"
# Search for specific pattern matching cases that might be affected
echo -e "\n=== Numeric type pattern matching ==="
rg "case\s+(\w+:\s*)?(Long|Int|Double|Float)\s*=>"
Length of output: 4836
Script:
#!/bin/bash
# Search for canBeConvertedTo and canBeSubclassOf implementations and usages
echo "=== Type conversion implementations ==="
rg -A 10 "def canBeConvertedTo|def canBeSubclassOf"
# Search for TypedValue usage in pattern matching contexts
echo -e "\n=== TypedValue pattern matching ==="
ast-grep --pattern 'case $_ @ TypedValue($$$) =>'
Length of output: 3562
components-api/src/main/scala/pl/touk/nussknacker/engine/api/typed/TypeConversionHandler.scala (2)
6-6: Approved: Importing AllNumbers for strict number comparison
The import of AllNumbers is necessary for the updated logic in strict number conversions.
97-103: Verify the correctness of new strict conversion methods
The introduction of canBeStrictlyConvertedTo and handleStrictConversion enhances type checking. Please ensure that:
- All necessary cases are thoroughly handled in these methods.
- Unit tests are updated or added to cover the new logic.
- There are no regressions or unintended side effects.
I can help review the test coverage or suggest additional test cases if needed.
| // TODO: This is probably wrong - relying on index of AllNumbers | ||
| (boxedGivenClass, boxedSuperclassCandidate) match { | ||
| case (f, t) if ClassUtils.isAssignable(f, t, true) => true | ||
| case (f, t) if (AllNumbers.contains(f) && AllNumbers.contains(t)) => | ||
| AllNumbers.indexOf(f) >= AllNumbers.indexOf(t) | ||
| case _ => false |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Critical Issue: Unreliable type comparison using AllNumbers.indexOf
The logic in handleStrictNumberConversions relies on the index of AllNumbers to compare types:
AllNumbers.indexOf(f) >= AllNumbers.indexOf(t)This approach is fragile and can lead to incorrect results if AllNumbers changes or if types are reordered.
Consider using a more robust method for comparing numeric types, such as:
- Defining an explicit type hierarchy or precedence mapping.
- Utilizing existing utility methods for type comparison without relying on collection indices.
- Implementing a comparison function that accurately reflects the desired conversion rules.
Would you like assistance in proposing an alternative solution or opening a GitHub issue to address this?
| condNel( | ||
| isAssignable(givenFieldType, typ, conversionChecker).isValid, | ||
| (), | ||
| s"Field '$name' is of the wrong type. Expected: ${givenFieldType.display}, actual: ${typ.display}" |
There was a problem hiding this comment.
Correct the order of expected and actual types in the error message.
In the error message for field type mismatch, the expected and actual types are reversed. The expected type should be from typ, and the actual type is givenFieldType. Correcting this will enhance clarity during debugging.
Apply this diff to fix the error message:
- s"Field '$name' is of the wrong type. Expected: ${givenFieldType.display}, actual: ${typ.display}"
+ s"Field '$name' is of the wrong type. Expected: ${typ.display}, actual: ${givenFieldType.display}"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| s"Field '$name' is of the wrong type. Expected: ${givenFieldType.display}, actual: ${typ.display}" | |
| s"Field '$name' is of the wrong type. Expected: ${typ.display}, actual: ${givenFieldType.display}" |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
components-api/src/test/scala/pl/touk/nussknacker/engine/api/typed/AssignabilityDeterminerSpec.scala (3)
9-9: Remove redundant importThe import
pl.touk.nussknacker.engine.api.typed._is redundant as it's importing from the same package where this file is located.-import pl.touk.nussknacker.engine.api.typed._
14-22: Consider adding more edge cases to the test suiteThe current test cases cover basic scenarios well. Consider enhancing the test coverage by adding:
- Null type handling cases
- Array type conversion cases
- Custom type hierarchy cases
- Generic type invariance cases
Example additions:
// Add these cases to strictConversionCases (typing.Typed[Null], typing.Typed[AnyRef], Valid(()), Valid(())), // Null handling (typing.Typed[Array[Int]], typing.Typed[Array[Number]], Valid(()), Valid()), // Array variance (typing.Typed[Dog], typing.Typed[Animal], Valid(()), Valid(())) // Custom hierarchy
48-52: Enhance test assertions with more specific matchersThe current pattern matching with generic succeed/fail could be replaced with more specific ScalaTest matchers for better error reporting and clearer intent:
// Replace the pattern matching blocks with: result should matchPattern { case Valid(_) if expectedStrict.isValid => case Invalid(NonEmptyList(msg, _)) if expectedStrict.isInvalid => msg should not be empty msg should include("Cannot") }This would:
- Provide more specific failure messages
- Validate error message content
- Make the test intentions clearer
Also applies to: 58-63
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
components-api/src/test/scala/pl/touk/nussknacker/engine/api/typed/AssignabilityDeterminerSpec.scala(1 hunks)
🔇 Additional comments (1)
components-api/src/test/scala/pl/touk/nussknacker/engine/api/typed/AssignabilityDeterminerSpec.scala (1)
1-67: Verify integration with modified components
While the tests are well-structured, let's verify the integration with the modified components mentioned in the PR objectives:
✅ Verification successful
Tests are well-integrated with the type conversion system
The test suite in AssignabilityDeterminerSpec is properly integrated with the type conversion system and covers the essential scenarios:
- The test cases align with
TypeConversionHandler's implementation, covering both strict and loose conversions - Numeric type conversions are tested according to the
NumberTypesPromotionStrategyrules - The test suite verifies primitive type widening, generic type variance, and collection type compatibility
The implementation shows that:
TypeConversionHandleris the core component handling conversion logicAssignabilityDeterminertests cover both strict and loose conversion paths- The test cases match the actual implementation's behavior for number type promotions and conversions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for related changes in TypeConversionHandler and CanBeSubclassDeterminer
echo "Checking for related component changes..."
rg -l "TypeConversionHandler|CanBeSubclassDeterminer" --type scala
# Verify test coverage for the specific bug fixes mentioned in PR
echo "Verifying test coverage for mentioned bugs..."
rg -l "lang3.*canBeSubclassOf|TypeConversionHandler.*true" --type scala
Length of output: 831
Script:
#!/bin/bash
# Let's check the content of AssignabilityDeterminer to understand its relationship with TypeConversionHandler
echo "Checking AssignabilityDeterminer implementation..."
rg -A 10 "class AssignabilityDeterminer" --type scala
# Let's also check for any test files related to TypeConversionHandler
echo "Checking for TypeConversionHandler tests..."
fd -e scala -e java TypeConversionHandler.*Test
fd -e scala -e java TypeConversionHandler.*Spec
# Let's examine the actual implementation of TypeConversionHandler
echo "Checking TypeConversionHandler implementation..."
cat components-api/src/main/scala/pl/touk/nussknacker/engine/api/typed/TypeConversionHandler.scala
Length of output: 8137
| val looseConversionCases = Table( | ||
| ("sourceType", "targetType", "expectedStrict", "expectedLoose"), | ||
| (typing.Typed[Long], typing.Typed[Int], Invalid(NonEmptyList.of("")), Valid(())), | ||
| (typing.Typed[Double], typing.Typed[Int], Invalid(NonEmptyList.of("")), Invalid(NonEmptyList.of(""))), | ||
| (typing.Typed[BigDecimal], typing.Typed[Int], Invalid(NonEmptyList.of("")), Invalid(NonEmptyList.of(""))) | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance numeric type conversion test coverage
Consider adding more comprehensive numeric type conversion cases:
// Add these cases to looseConversionCases
(typing.Typed[Float], typing.Typed[Double], Valid(()), Valid()), // Precision widening
(typing.Typed[Double], typing.Typed[Float], Invalid(...), Invalid(...)), // Precision loss
(typing.Typed[Int], typing.Typed[Short], Invalid(...), Invalid(...)), // Range overflow
(typing.Typed[BigInt], typing.Typed[Long], Invalid(...), Invalid(...)) // Large number handlingThese additional cases would help ensure proper handling of numeric type conversions, especially for edge cases involving precision and range limitations.
| (typing.Typed[Long], typing.Typed[Int], Invalid(NonEmptyList.of("")), Valid(())), | ||
| (typing.Typed[Double], typing.Typed[Int], Invalid(NonEmptyList.of("")), Invalid(NonEmptyList.of(""))), | ||
| (typing.Typed[BigDecimal], typing.Typed[Int], Invalid(NonEmptyList.of("")), Invalid(NonEmptyList.of(""))) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error messages in Invalid cases
The current implementation uses empty strings for error messages. Consider adding meaningful error messages to improve test documentation and debugging:
-(typing.Typed[Long], typing.Typed[Int], Invalid(NonEmptyList.of("")), Valid(())),
+(typing.Typed[Long], typing.Typed[Int], Invalid(NonEmptyList.of("Cannot strictly convert Long to Int due to potential data loss")), Valid(())),
-(typing.Typed[Double], typing.Typed[Int], Invalid(NonEmptyList.of("")), Invalid(NonEmptyList.of(""))),
+(typing.Typed[Double], typing.Typed[Int], Invalid(NonEmptyList.of("Cannot convert Double to Int due to decimal precision loss")), Invalid(NonEmptyList.of("Double to Int conversion not supported even in loose mode"))),
-(typing.Typed[BigDecimal], typing.Typed[Int], Invalid(NonEmptyList.of("")), Invalid(NonEmptyList.of("")))
+(typing.Typed[BigDecimal], typing.Typed[Int], Invalid(NonEmptyList.of("Cannot convert BigDecimal to Int due to potential range and precision loss")), Invalid(NonEmptyList.of("BigDecimal to Int conversion not supported")))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| (typing.Typed[Long], typing.Typed[Int], Invalid(NonEmptyList.of("")), Valid(())), | |
| (typing.Typed[Double], typing.Typed[Int], Invalid(NonEmptyList.of("")), Invalid(NonEmptyList.of(""))), | |
| (typing.Typed[BigDecimal], typing.Typed[Int], Invalid(NonEmptyList.of("")), Invalid(NonEmptyList.of(""))) | |
| (typing.Typed[Long], typing.Typed[Int], Invalid(NonEmptyList.of("Cannot strictly convert Long to Int due to potential data loss")), Valid(())), | |
| (typing.Typed[Double], typing.Typed[Int], Invalid(NonEmptyList.of("Cannot convert Double to Int due to decimal precision loss")), Invalid(NonEmptyList.of("Double to Int conversion not supported even in loose mode"))), | |
| (typing.Typed[BigDecimal], typing.Typed[Int], Invalid(NonEmptyList.of("Cannot convert BigDecimal to Int due to potential range and precision loss")), Invalid(NonEmptyList.of("BigDecimal to Int conversion not supported"))) |
Describe your changes
DictApiHttpService sometimes returns improper subtypes like Long type Dicts for Integer or won't return proper types like Number type dict for Long.
Issue is caused by bugs in lang3 lib used in CanBeSubclassDeterminer - the lib would sometimes return incorrect false, and our faulty logic in TypeConversionHandler - the code would return true even if no conversion was possible.
Fixed by adding a double check in SubclassDeterminer and fixing the logic in ConversionHandler.
Checklist before merge
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
AssignabilityDeterminerclass, covering strict and loose conversion cases.