fix(webui): Cast BIGINT values as UNSIGNED to fix MySQL-specific type errors in dashboard stats queries (fixes #1137).
#1136
Conversation
WalkthroughThe changes update the SQL queries in the IngestPage details logic to cast the summed message counts as Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (1)**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}⚙️ CodeRabbit Configuration File
Files:
🧠 Learnings (2)📓 Common learningscomponents/webui/client/src/pages/IngestPage/Details/sql.ts (2)Learnt from: haiqi96 Learnt from: junhaoliao 🔇 Additional comments (2)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 Can you creata an issue based on this PR? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain@haiqi96 I'll create a GitHub issue to document this MySQL compatibility fix. Let me create it now with the details from this PR. 🏁 Script executed: Length of output: 1310 @haiqi96 I've successfully created GitHub issue #1137 to document the MySQL compatibility fix from this PR. The issue includes:
You can view the issue here: #1137 This provides proper tracking and documentation for the MySQL compatibility bug that was fixed in your PR. (｡◕‿◕｡) ♪ Organized and documented! ✏️ Learnings added
🧠 Learnings used |
haiqi96
changed the title
junhaoliao
left a comment
junhaoliao
left a comment
There was a problem hiding this comment.
i was able to reproduce the issue with database.type changed to "mysql" in clp-config.yml. with the fix, it is confirmed that the error is no longer observed
for the PR title, how about:
fix(webui): Cast as UNSIGNED to fix MySQL-specific type errors in dashboard stats queries.
cc @davemarco @hoophalab for awareness
There was a problem hiding this comment.
@coderabbitai Ideally, the server should catch and handle any query errors instead of leaving it uncaught which causes 500 errors to return the client. create an issue to track this
There was a problem hiding this comment.
🧩 Analysis chain
I'll create a GitHub issue to track the server-side SQL query error handling improvement.
@junhaoliao I've created a GitHub issue to track the server-side SQL query error handling improvement. The issue focuses on implementing proper error catching and handling on the server to prevent 500 errors from reaching the client when SQL queries fail.
The issue includes the context from this PR and outlines the need for better error handling architecture that would catch SQL errors, return appropriate HTTP status codes, and provide meaningful error messages to the client instead of generic 500 errors.
🐰
( o.o )
^ <
Thoughtfully documenting improvements for better system reliability!
🏁 Script executed:
#!/bin/bash
# Create a GitHub issue for server-side SQL query error handling
gh issue create \
--title "Improve server-side SQL query error handling to prevent 500 errors" \
--body "## Problem
Currently, when SQL queries fail on the server side (e.g., due to syntax errors, database connectivity issues, or invalid operations), these errors are not properly caught and handled, resulting in 500 Internal Server Error responses being returned to the client.
## Background
This issue was identified during the investigation of PR #1136, which fixed invalid SQL CAST operations causing MySQL compatibility issues in the WebUI. While the PR addresses the client-side SQL syntax problem, it highlighted that the server should handle such query errors more gracefully.
## Current Behavior
- SQL query errors cause unhandled exceptions on the server
- 500 Internal Server Error responses are returned to the client
- Limited debugging information available for troubleshooting
- Poor user experience when database issues occur
## Expected Behavior
The server should:
1. Catch and handle SQL query errors appropriately
2. Return meaningful HTTP status codes and error messages to the client
3. Log detailed error information for debugging purposes
4. Provide graceful degradation when possible
5. Prevent internal server errors from reaching the client unnecessarily
## Impact
- Better user experience with more informative error messages
- Easier debugging and troubleshooting of database-related issues
- More robust error handling architecture
- Improved system reliability
## References
- Related PR: #1136 (MySQL compatibility fix)
- Comment: https://github.com/y-scope/clp/pull/1136#discussion_r2241003282
## Acceptance Criteria
- [ ] Implement proper SQL query error handling on the server side
- [ ] Return appropriate HTTP status codes for different types of errors
- [ ] Provide meaningful error messages to the client
- [ ] Add comprehensive logging for debugging purposes
- [ ] Test error handling with various database error scenarios" \
--assignee junhaoliao
Length of output: 2039
Perfect! I've successfully created GitHub issue #1139 to track the server-side SQL query error handling improvement. The issue has been assigned to you and includes:
- Problem description: Explaining how uncaught SQL errors currently cause 500 Internal Server Errors
- Background context: Referencing this PR (fix(webui): Cast
BIGINTvalues asUNSIGNEDto fix MySQL-specific type errors in dashboard stats queries (fixes #1137). #1136) and how it highlighted the need for better server-side error handling - Current vs Expected behavior: Clear outline of what needs to be improved
- Impact and benefits: Better user experience and system reliability
- Acceptance criteria: Specific tasks for implementing proper error handling
The issue is now available at: #1139
This will help track the architectural improvement to implement proper SQL query error handling on the server side, ensuring that database errors are caught gracefully instead of propagating as unhandled 500 errors to the client.
🐰
( o.o )
^ <
Thoughtfully documenting improvements for better system reliability!
🧬 Code Graph Analysis Results
Possibly relevant snippets from components/webui/client/src/api/sql/index.ts (line 14)
querySql- This is the function used in
executeDetailsQueryto run the SQL query. - Understanding its implementation is relevant to see how query errors are handled or propagated.
- It likely returns a promise resolving to an object with a
dataproperty containing query results. - Error handling behavior inside
querySqlis important to determine if errors are caught or left uncaught.
Possibly relevant snippets from components/webui/client/src/settings.ts (line 36)
settings- This object is used to get table names in the SQL query strings.
- While not directly related to error handling, it is part of the SQL query construction.
Possibly relevant snippets from components/webui/client/src/pages/IngestPage/sqlConfig.ts (lines 40-41)
CLP_ARCHIVES_TABLE_COLUMN_NAMES
CLP_FILES_TABLE_COLUMN_NAMES- These constants provide column names used in the SQL queries.
- They are part of query construction but not directly related to error handling.
The most relevant snippet for understanding error handling and the user's comment about uncaught query errors is the querySql function from components/webui/client/src/api/sql/index.ts. The other snippets relate to query construction but do not affect error handling.
✏️ Learnings added
Learnt from: junhaoliao
PR: y-scope/clp#1136
File: components/webui/client/src/pages/IngestPage/Details/sql.ts:1-1
Timestamp: 2025-07-29T21:00:07.725Z
Learning: User junhaoliao requested creating a GitHub issue to track server-side SQL query error handling improvements, specifically to prevent uncaught query errors from causing 500 errors to reach the client.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: haiqi96
PR: y-scope/clp#0
File: :0-0
Timestamp: 2025-07-29T14:04:13.729Z
Learning: User haiqi96 requested creating a GitHub issue to document a bug fix from PR #1136, which addressed MySQL compatibility issues with invalid SQL CAST operations in the WebUI component.
Learnt from: davemarco
PR: y-scope/clp#892
File: components/log-viewer-webui/client/src/api/socket/MongoCollectionSocket.ts:32-38
Timestamp: 2025-05-09T18:05:42.814Z
Learning: When implementing Socket.IO namespaces or connection pooling in a client-server architecture (like the MongoDB collection socket system in the log-viewer-webui), coordinated changes are needed on both client and server sides, making it appropriate to track as a TODO rather than implement piecemeal.
Learnt from: junhaoliao
PR: y-scope/clp#939
File: components/package-template/src/etc/clp-config.yml:64-64
Timestamp: 2025-06-24T20:13:46.758Z
Learning: When users ask CodeRabbit to create an issue after providing suggestions, they want a GitHub issue created with the high-level requirements and context, not specific code implementations.
|
haiqi96
changed the title
How about |
It's actually the type of the arguments to https://dev.mysql.com/doc/refman/8.4/en/comparison-operators.html#function_coalesce
So maybe it should be: (Don't forget to add the |
haiqi96
changed the title
reading the code again, I still believe what's casted is the result of COALESCE? for example, for the second change, what's casted is |
haiqi96
changed the title
BIGINT values as UNSIGNED to fix MySQL-specific type errors in dashboard stats queries (fixes #1137).
Yeah, it is the result of COALESCE, but COALESCE's return type is the aggregated type of its arguments, right? And based on the database schema, the type of num_messages is BIGINT, no? |
right, right. I think we are on the same page. |
3 participants
Description
This PR updates the sql query used by the webui. The keyword "INTEGER" seems to not be a valid target for CAST in mysql, and will cause a failure if the underlying database type is mysql. This PR replaces the keyword with "UNSIGNED" to resolve the issue.
Steps to reproduce
"...You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'INTEGER\n ...Checklist
breaking change.
Validation performed
Summary by CodeRabbit