[NU-1893] Fix "Failed to get node validation" when opening fragment node details for referencing non-existing fragment #7190
Conversation
…ode details for referencing non-existing fragment
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces significant modifications to the Additionally, a new test case has been added to the Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🔇 Additional comments (1)docs/Changelog.md (1)
The changelog entry accurately describes the bug fix and follows the standard format with proper PR reference. 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 (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
designer/server/src/test/scala/pl/touk/nussknacker/ui/process/fragment/FragmentRepositorySpec.scala (1)
40-42: Consider adding more test cases for comprehensive coverage.The test case effectively verifies the new behavior for non-existing fragments. However, consider adding these additional test cases to ensure robust error handling:
- Fragment name with special characters
- Empty fragment name
- Multiple concurrent requests for the same non-existing fragment
Example test case:
it should "handle fragment names with special characters" in { fragmentRepository .fetchLatestFragment(ProcessName("non/existing@fragment"))(adminUser) .futureValue shouldBe None }docs/Changelog.md (1)
106-106: LGTM! Consider adding more details about the fix.The changelog entry correctly documents the fix for fragment node validation. However, consider expanding the description to provide more context about:
- What caused the validation failure
- How it was fixed
- Impact on users who may be affected by this issue
Example of more detailed entry:
-* [#7190](https://github.com/TouK/nussknacker/pull/7190) Fix "Failed to get node validation" when opening fragment node details for referencing non-existing fragment +* [#7190](https://github.com/TouK/nussknacker/pull/7190) Fix "Failed to get node validation" error that occurred when opening fragment node details that referenced a non-existing fragment. The validation now properly handles missing fragment references and provides a clear error message to users.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
designer/server/src/main/scala/pl/touk/nussknacker/ui/process/fragment/FragmentRepository.scala(2 hunks)designer/server/src/test/scala/pl/touk/nussknacker/ui/process/fragment/FragmentRepositorySpec.scala(2 hunks)docs/Changelog.md(1 hunks)scenario-compiler/src/test/scala/pl/touk/nussknacker/engine/compile/NodeDataValidatorSpec.scala(1 hunks)
🔇 Additional comments (4)
designer/server/src/test/scala/pl/touk/nussknacker/ui/process/fragment/FragmentRepositorySpec.scala (1)
8-8: LGTM!
The import is correctly placed and necessary for the new test case.
designer/server/src/main/scala/pl/touk/nussknacker/ui/process/fragment/FragmentRepository.scala (2)
3-3: LGTM: Appropriate addition of cats traverse operations
The addition of cats traverse operations aligns well with the shift towards more functional error handling.
48-50: LGTM: Improved error handling with functional approach
The new implementation elegantly handles non-existing fragments using functional programming constructs. This fixes the validation failure issue by properly propagating None instead of throwing exceptions.
Consider adding a comment explaining the transformation chain for better maintainability:
override def fetchLatestFragment(
fragmentName: ProcessName
)(implicit user: LoggedUser): Future[Option[CanonicalProcess]] = {
processRepository
.fetchProcessId(fragmentName)
+ // Transform Future[Option[ProcessId]] to Future[Option[CanonicalProcess]] by:
+ // 1. For each ProcessId, fetch process details
+ // 2. Sequence the nested Option to handle missing processes
+ // 3. Flatten the result to remove unnecessary nesting
.flatMap(_.map(processRepository.fetchLatestProcessDetailsForProcessId[CanonicalProcess]).sequence.map(_.flatten))
.map(_.map(_.json))
}Let's verify the return types of the repository methods to ensure our assumptions are correct:
✅ Verification successful
Verified: Return types match the implementation assumptions
The search results confirm that both methods return Future[Option[_]]:
fetchProcessIdreturnsFuture[Option[ProcessId]](seeFetchingProcessRepositorytrait)fetchLatestProcessDetailsForProcessIdreturnsFuture[Option[ScenarioWithDetailsEntity[PS]]](seeFetchingProcessRepositorytrait)
This validates that the functional transformation chain in the implementation is correct and properly handles the Option monad through the sequence operation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the method declarations in the repository
ast-grep --pattern 'def fetchProcessId($_): Future[Option[$_]]'
ast-grep --pattern 'def fetchLatestProcessDetailsForProcessId[$_]($_): Future[Option[$_]]'
Length of output: 158
Script:
#!/bin/bash
# Let's try a broader search for these method signatures
rg -A 2 "def fetchProcessId"
rg -A 2 "def fetchLatestProcessDetailsForProcessId"
# Also search for trait/class definitions containing these methods
ast-grep --pattern 'trait ProcessRepository {
$$$
}'
ast-grep --pattern 'class $_ extends ProcessRepository {
$$$
}'
Length of output: 4819
scenario-compiler/src/test/scala/pl/touk/nussknacker/engine/compile/NodeDataValidatorSpec.scala (1)
500-519: Well-structured test case that addresses the core issue!
The test effectively verifies the handling of non-existing fragment references, ensuring that:
- A proper business error (
UnknownFragment) is returned instead of a technical failure - The error contains both the fragment ID and the node ID for clear error reporting
This aligns perfectly with the PR's objective of fixing the "Failed to get node validation" error.
|
created: #7191 |
…ode details for referencing non-existing fragment
Describe your changes
Checklist before merge
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
fetchLatestFragmentmethod, returningNoneinstead of throwing an error.Documentation