Remove debug logging for dataset listing #8256
Conversation
This reverts commit 0a2afa4.
📝 WalkthroughWalkthroughThe pull request introduces several modifications across multiple files, primarily focusing on enhancing error handling, simplifying control flow, and refining validation processes. Key changes include refactoring methods in the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 (
|
…ert-8251-add-debug-logging-for-ds-listing
|
@fm3 I added / refactored some of the error messages by putting them into the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
app/controllers/DatasetController.scala (1)
222-242: Consider adding error handling for empty results.While the error handling for failures has been improved, consider adding specific handling for empty result sets to provide better feedback to users.
def listGrouped(datasets: List[Dataset], requestingUser: Option[User])( implicit ctx: DBAccessContext, m: MessagesProvider): Fox[List[JsObject]] = for { + _ <- bool2Fox(datasets.nonEmpty) ?~> "dataset.list.empty" requestingUserTeamManagerMemberships <- Fox.runOptional(requestingUser)(user => userService.teamManagerMembershipsFor(user._id)) groupedByOrga = datasets.groupBy(_._organization).toList
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
app/controllers/DatasetController.scala(1 hunks)app/models/dataset/DataStore.scala(1 hunks)app/models/dataset/Dataset.scala(2 hunks)app/models/dataset/DatasetService.scala(1 hunks)app/models/user/UserService.scala(1 hunks)conf/messages(2 hunks)
🔇 Additional comments (9)
app/models/dataset/Dataset.scala (2)
223-223: LGTM!
The removal of error message from the parsing operation is consistent with the PR objective of removing debug logging.
118-123: Verify error message handling for parsing operations.
While removing error messages aligns with the PR objective, ensure that appropriate error messages for parsing failures are available in conf/messages for:
- voxelSize parsing
- defaultViewConfiguration parsing
- adminViewConfiguration parsing
✅ Verification successful
Error handling is properly implemented for dataset parsing operations
The verification shows that appropriate error handling is in place:
- Generic dataset error messages exist in
conf/messages, including:dataset.type.invaliddataset.upload.validation.faileddataset.name.invalid.*
- The
JsonHelper.parseAndValidateJsonis consistently used across the codebase for parsing JSON data, including dataset configurations - Error handling for parsing failures is handled through the
Foxmonad, with proper error propagation as seen in other similar operations in the codebase
The removal of debug logging messages is appropriate as the system maintains proper error handling through the Fox monad and standardized error messages in conf/messages.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if error messages for parsing operations exist in conf/messages
# Search for relevant error messages
rg "dataset.*parse|parse.*dataset|voxel.*size|view.*config" conf/messages
Length of output: 75
Script:
#!/bin/bash
# Let's try a broader search for error messages and error handling
# Search for error handling in Dataset.scala and related files
rg -A 3 "Fox\.failure|Fox\.error|JsonHelper.*Error|parse.*Error" app/models/dataset/
# Search for error messages in messages file with broader terms
rg "error|invalid|fail" conf/messages
# Check if there are any other message files
fd messages conf/
# Look for error handling in JsonHelper
rg -A 3 "parseAndValidateJson" app/
Length of output: 13551
app/models/dataset/DataStore.scala (1)
117-117: LGTM: Error message enhancement
The addition of the "dataStore.notFound" error message improves error handling clarity.
Let's verify the error message is properly defined:
✅ Verification successful
Error message is properly defined in messages file
The "dataStore.notFound" message is correctly defined in conf/messages as "DataStore not found."
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the error message is defined in conf/messages
rg "dataStore.notFound" conf/messages
Length of output: 77
app/models/dataset/DatasetService.scala (1)
352-362: LGTM: Comprehensive error handling
The changes add specific error messages for each operation in the publicWrites method, improving error reporting and debugging capabilities. The messages are descriptive and follow a consistent pattern.
app/models/user/UserService.scala (1)
282-286: LGTM: Consistent error handling
The addition of error messages for team membership operations improves error reporting consistency. Using the same error message "user.team.memberships.failed" for both operations is appropriate as they're closely related.
Let's verify the error message is defined:
✅ Verification successful
Error message is properly defined and used consistently
The error message "user.team.memberships.failed" is correctly defined in conf/messages with a clear, user-friendly message: "Failed to retrieve team memberships for user". The usage in both team membership operations in UserService.scala aligns with this definition.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the error message is defined in conf/messages
rg "user.team.memberships.failed" conf/messages
Length of output: 121
conf/messages (2)
71-71: LGTM: Clear and consistent error message.
The new error message follows the established pattern and provides clear feedback for team membership retrieval failures.
92-93: LGTM: Improved error messages for dataset operations.
The changes improve consistency by:
- Using "datasets" instead of "data sets"
- Adding a specific error message for grouping failures
app/controllers/DatasetController.scala (2)
179-214: LGTM: Improved error handling and code organization in list method.
The changes effectively:
- Handle both compact and detailed listing modes
- Validate input parameters properly
- Use appropriate error messages
- Update user activity correctly
179-214: Verify error message usage across the codebase.
Let's verify that the error messages are consistently used and that there are no missing cases.
Also applies to: 222-242
✅ Verification successful
Error messages are consistently used and properly scoped
The verification shows that the error messages are used consistently and appropriately:
dataset.list.failedis used only in the dataset listing operationdataset.list.grouping.failedis properly chained after the listing operation- Both messages are scoped to the DatasetController and not duplicated elsewhere
- The error handling follows the same pattern across similar operations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent error message usage in dataset listing operations
# Check for dataset listing error message usage
echo "Checking dataset listing error message usage..."
rg -A 2 "dataset\.list\.failed" --type scala
# Check for dataset grouping error message usage
echo "Checking dataset grouping error message usage..."
rg -A 2 "dataset\.list\.grouping\.failed" --type scala
# Check for team membership error message usage
echo "Checking team membership error message usage..."
rg -A 2 "user\.team\.memberships\.failed" --type scala
Length of output: 1561
MichaelBuessemeyer
deleted the
revert-8251-add-debug-logging-for-ds-listing
branch
MichaelBuessemeyer
changed the title
Simply removes the logging introduced in #8251
Some of the improved error messages are kept.