abzymeinsjtu commented on PR #15321: URL: https://github.com/apache/dolphinscheduler/pull/15321#issuecomment-1873243325
> If this PR aims to be an example of the API V2 refactoring PoC, I'd expect some more changes: > > 1. Remove all the `@ApiException` annotations. It's an unnecessary workload to maintain the huge list of `Status` codes, because: > a. these status codes basically replicate the HTTP status codes, I can name some examples: `XXX_NOT_EXITS == HTTP 404 not found`, `XXX_EXISTS == HTTP 409 conflict`, `XXX_NOT_NULL == HTTP 400 bad request`, etc., and I don't think we need so many detailed statuses, it brings nothing but maintenance burden > b. these status codes are not used anywhere in the project, only used on the UI to display error message, but as I said in (a), the error codes can be easily understood with the HTTP status semantics > 2. Replace all the `@Auth` and `ResourcePermissionCheckService`, add roles and permissions to the DS users, then leverage the [Spring method security](https://docs.spring.io/spring-security/reference/servlet/authorization/method-security.html) to do permission check. This also requires some refactoring work in the authorization and authentication path so I think we can do that first in another PR. BTW refactoring the authorization/authentication also brings many benefits in terms integrations with many existing authorization parties, but we can do it in another PR. > 3. Remove all swagger related annotations, dependencies. Packaging a swagger console in the web server by default is not a good idea in my opinion, it can be a vulnerable attack surface. To mitigate the situation where we don't have API doc after removing Swagger, I propose to use [Spring RestDoc](https://docs.spring.io/spring-restdocs/docs/current/reference/htmlsingle/#getting-started), which is a test-driven documentation framework, with RestDoc, we can make sure that: > a. Every API is tested and documented, even every field in the request and the response is documented as well > b. The documentation is always up to date as soon as anyone change the codes. > > I have been discussing these with @EricGao888 and I saw you @abzymeinsjtu already sent an email to the mailing list, so these are what we expected to have in API v2 and what I wanted to highlight, refactoring API to v2 will likely be a breaking change so we don't want to just implement API v1.5 or API v1.7 and then have to break it again in the short-term future @kezhenxu94 sorry for more than a week delay. As a POC PR, i agree that it makes sense to include sufficient new stuff instead of compatibility with old version. What u have mentioned is now WIP and will let u know when finished. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
