Skip to content

Conversation

@Pil0tXia
Copy link
Member

@Pil0tXia Pil0tXia commented Oct 19, 2023

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 exceptions
  • GlobalExceptionHandler: handle BaseException

image

Documentation

  • Does this pull request introduce a new feature? (no)
  • If yes, how is the feature documented? (not applicable / docs / JavaDocs / not documented)
  • If a feature is not applicable for documentation, explain why?
  • If a feature is not documented yet in this PR, please create a followup issue for adding the documentation

@codecov
Copy link

codecov bot commented Oct 20, 2023

Codecov Report

Merging #4498 (e5c1809) into master (f629730) will increase coverage by 0.01%.
Report is 1 commits behind head on master.
The diff coverage is n/a.

❗ Current head e5c1809 differs from pull request most recent head cec6a20. Consider uploading reports for the commit cec6a20 to get more accurate results

@@             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

@xwm1992 xwm1992 merged commit 62935c4 into apache:master Oct 20, 2023
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";
Copy link
Member

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?

Copy link
Member Author

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.


是的,因为它们的“含义”分别属于请求体和响应体,只不过“恰好”重名了而已。如果请求体中的字段名称发生了更改,而响应体中的字段名称没有发生更改,那么就不应该同时修改请求体和响应体的字段名称。

Copy link
Member

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中指出的静态常量重复问题,其实很多是不是不重复?因为不少“恰好”同名的变量其实分别用在不同的模块,不同场景的。

Copy link
Member Author

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."),
Copy link
Member

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?

Copy link
Member Author

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是一致的,没有必要为它单独新建一个文件,因此我觉得这样是合适的。

Copy link
Member

@pandaapo pandaapo Oct 20, 2023

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的用法”,这样的解释太牵强了。

Copy link
Member Author

@Pil0tXia Pil0tXia Oct 20, 2023

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,这也是它一开始的命名。

@Pil0tXia Pil0tXia mentioned this pull request Oct 30, 2023
3 tasks
@pandaapo pandaapo added this to the 1.10 milestone Dec 5, 2023
@Pil0tXia Pil0tXia deleted the pil0txia_enhance_4494 branch January 4, 2024 05:05
xuhongjia pushed a commit to Deckers-Ohana/eventmesh that referenced this pull request Mar 13, 2025
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Enhancement] RESTful API framework for EventMeshAdmin

5 participants