-
Notifications
You must be signed in to change notification settings - Fork 641
[ISSUE #4494] RESTful API framework for EventMeshAdmin #4498
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
Conversation
Codecov Report
@@ Coverage Diff @@
## master #4498 +/- ##
============================================
+ Coverage 15.46% 15.47% +0.01%
Complexity 1452 1452
============================================
Files 691 691
Lines 28101 28106 +5
Branches 2624 2626 +2
============================================
+ Hits 4345 4349 +4
- Misses 23309 23312 +3
+ Partials 447 445 -2 see 8 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
| public static final String CONFIGS_RESP_CONTENT = "content"; // json page data field | ||
| public static final String CONFIGS_RESP_PAGES = "pagesAvailable"; // json total pages field | ||
| public static final String CONFIGS_RESP_DATAID = "dataId"; | ||
| public static final String CONFIGS_RESP_GROUP = "group"; |
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.
Is it necessary to have 2 accessToken constants, 2 dataId constants and 2 group constants?
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.
Yes, because their "meanings" belong to the request body and the response body respectively, it's just that they happen to “share” the same name. If the field names in the request body are changed, and the field names in the response body remain unchanged, then the field names in both the request body and the response body should not be modified at the same time.
是的,因为它们的“含义”分别属于请求体和响应体,只不过“恰好”重名了而已。如果请求体中的字段名称发生了更改,而响应体中的字段名称没有发生更改,那么就不应该同时修改请求体和响应体的字段名称。
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.
If understood in this way, is it true that some of the static constant repetition problems pointed out in issue #4474 are not repeated? Because many variables that happen to have the same name are actually used in different modules and scenarios.
这样理解的话,那其实issue #4474中指出的静态常量重复问题,其实很多是不是不重复?因为不少“恰好”同名的变量其实分别用在不同的模块,不同场景的。
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.
This depends on the scope of the field's impact. If the field is a "concept" for the entire project, then defining it in multiple places naturally does not make it easy to synchronise changes. I think this is the case with eventmesh's protocol. But #4474 has some other problems: #4495 (review)
这取决于字段的影响范围。如果这个字段是整个项目的一个“概念”,那定义在多个地方自然不便于同步修改。我觉得eventmesh的协议属于这种情况。但是#4474 还存在一些其它问题:#4495 (review)
| @Getter | ||
| public enum Errors { | ||
|
|
||
| SUCCESS(HttpStatus.OK, Types.SUCCESS, "Operation success."), |
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.
Is it reasonable to have an object named SUCCESS in Errors class?
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.
The naming of the Errors class is appropriate for its usage because it needs to be propagated within the BaseException, so this naming is the most suitable.
The SUCCESS enumeration is always statically referenced, so there won't be a usage like Errors.SUCCESS. The usage of the SUCCESS enumeration is consistent with other XXX_ERRs, so there's no need to create a separate file for it, and I think this approach is appropriate.
Errors类的命名符合它的用法,因为它需要在BaseException中被传播,因此这个命名是最合适的。
SUCCESS枚举总是静态引用的,因此不会出现Errors.SUCCESS的用法。SUCCESS枚举的用法和其它XXX_ERR是一致的,没有必要为它单独新建一个文件,因此我觉得这样是合适的。
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.
I think this is what we often call 'bad smell'. Additionally, the explanation of 'SUCCESS enumeration is always statically referenced, so there will be no usage of Errors. SUCCESS' is too farfetched.
我觉得这就是我们常说的"bad smell"。另外,“SUCCESS枚举总是静态引用的,因此不会出现Errors.SUCCESS的用法”,这样的解释太牵强了。
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.
OK, I will rename it to 'Status,' which was its original name at first.
好。我会将它改名为Status,这也是它一开始的命名。
eventmesh-admin/src/main/java/org/apache/eventmesh/admin/exception/GlobalExceptionHandler.java
Show resolved
Hide resolved
…4498) * restful response optimize part1 * restful response optimize part2 * restful response optimize part3 * restful response optimize part4 * exception handling * exception handling part2 * exception handling part3 * exception handling part4 * exception handling part5 * add error type to error displaying * warp json message response instead of string
Fixes #4494.
Motivation
Optimize the EventMeshAdmin API into a RESTful style to enhance its capability to provide detailed error messages and HTTP status codes to the caller, enable the propagation of error messages as exceptions, and support the extension of error message types and usage.
The framework can support a wide range of error types and categories, reducing redundant error reporting for similar issues and offering reusability across multiple scenarios.
Modifications
Errors: An enumeration class that conforms to the RESTful specifications and custom error reporting requirements.Result: A RESTful response DTO.BaseException: Customized error reporting using enums and exceptionsGlobalExceptionHandler: handle BaseExceptionDocumentation