-
Notifications
You must be signed in to change notification settings - Fork 73
fix: code check #293
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: code check #293
Conversation
WalkthroughFour Java classes updated: locale-aware lowercasing added, exception handling standardized to throw ServiceException with specific codes, and input validation added for dynamic table drop SQL generation. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@base/src/main/java/com/tinyengine/it/login/config/SSOInterceptor.java`:
- Around line 115-119: The current catch in SSOInterceptor is catching all
Exceptions and converting ServiceException instances (CM339/CM340/CM341/CM342)
into CM336; update the error handling so ServiceException is preserved: add a
specific catch (ServiceException se) in the same method in SSOInterceptor that
calls DefaultLoginUserContext.clear(), logs the exception, and rethrows se, then
keep the existing catch (Exception e) to handle unexpected errors (log, clear
context, and throw CM336); ensure logging messages reference the thrown
exception in both catches.
In
`@base/src/main/java/com/tinyengine/it/service/material/impl/ModelServiceImpl.java`:
- Around line 192-193: The error handling in updateModelById is wrong: the log
call uses a generic message and the ServiceException uses getResultCode() twice;
update the log inside updateModelById to a descriptive message like "failed to
update model by id" and pass the exception (e) to log.error, and change the
thrown ServiceException to use ExceptionEnum.CM001.getResultCode() as the first
arg and ExceptionEnum.CM001.getResultMsg() as the second arg so the message is
returned correctly.
- Around line 155-157: Fix the error handling in
ModelServiceImpl.deleteModelById: change the ServiceException construction to
pass ExceptionEnum.CM001.getResultMsg() as the second argument (message) instead
of getResultCode(), and improve the log.error call to include a descriptive
message and the caught exception and model id (e.g., log.error("Failed to delete
model by id {}:", modelId, e)) so the log provides context and the exception is
recorded; update references to ServiceException, ExceptionEnum.CM001,
deleteModelById, and the log.error invocation accordingly.
🧹 Nitpick comments (1)
base/src/main/java/com/tinyengine/it/service/material/impl/ModelServiceImpl.java (1)
148-150: Pre-existingServiceExceptionbug at line 150.This line has the same
getResultCode()/getResultCode()issue but wasn't changed in this PR. Consider fixing it for consistency.♻️ Suggested fix for consistency
if (result != 1) { - throw new ServiceException(ExceptionEnum.CM001.getResultCode(), ExceptionEnum.CM001.getResultCode()); + throw new ServiceException(ExceptionEnum.CM001.getResultCode(), ExceptionEnum.CM001.getResultMsg()); }
base/src/main/java/com/tinyengine/it/service/material/impl/ModelServiceImpl.java
Show resolved
Hide resolved
base/src/main/java/com/tinyengine/it/service/material/impl/ModelServiceImpl.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
base/src/main/java/com/tinyengine/it/dynamic/service/DynamicModelService.java (1)
62-70:⚠️ Potential issue | 🟡 MinorAlign table-name regex with
validateTableAndDatafor consistency.
generateDropTableSQLuses^[a-zA-Z0-9_]+$whilevalidateTableAndDatauses^[a-zA-Z_][a-zA-Z0-9_]*$. The current pattern also allows all-digit identifiers, which are invalid in MySQL. Aligning to the stricter pattern ensures consistency across the codebase.♻️ Suggested fix
- if (!tableName.matches("^[a-zA-Z0-9_]+$")) { + if (!tableName.matches("^[a-zA-Z_][a-zA-Z0-9_]*$")) {
🤖 Fix all issues with AI agents
In
`@base/src/main/java/com/tinyengine/it/dynamic/service/DynamicModelService.java`:
- Around line 56-58: In the catch blocks inside DynamicModelService (e.g., the
one in createDynamicTable and the other at lines ~86-88), the ServiceException
is being constructed with ExceptionEnum.CM001.getResultCode() passed twice;
change the second argument to ExceptionEnum.CM001.getResultMsg() so the
human-readable message is preserved when throwing new ServiceException; update
both occurrences where getResultCode() is duplicated to use getResultMsg() as
the message parameter.
English | 简体中文
PR
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
Background and solution
What is the current behavior?
Issue Number: N/A
What is the new behavior?
Does this PR introduce a breaking change?
Other information
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.