[Improve][ISSUE-3472] Improve streampark-console module Controller

[zzzk1]

What changes were proposed in this pull request

Issue Number: close #3472

For controllers that use only a single field, but whose argument is an object, use a single field instead of an object.

Brief change log

• AlertController.java.
• ApplicationController.java.

Verifying this change

This change is a trivial rework / code cleanup without any test coverage.

Does this pull request potentially affect one of the following parts

• Dependencies (does it add or upgrade a dependency): (no)

[zzzk1]

still working...

[zzzk1]

@wolfboys pls take a look.

[caicancai]

still working...

Hello, thank you for your contribution
If it is still working in the future, you can add the [WIP] prefix to the PR title, or set the PR to draft. This is the default rule of the community.

[caicancai]

I'm not sure if its scalability is a consideration here
I don't think it's good to change it to a parameter

[caicancai]

@RocMarshal If you have time, can you help review this PR? Thank you.

[RocMarshal]

Hi, @zzzk1 Thanks for your contribution and for @caicancai review.

To be honest, I'm more inclined towards the scope of responsibilities in this proposal-1, so I would prefer to discard changes that are not within the scope of the proposal in the current PR.

Of course, we'd be glad to consider accepting changes beyond the scope, but we need to make this decision through some necessary discussions.

So, in short, if you want to quickly merge this PR, you can remove changes that are not within the proposal scope; If you wish to merge all the contents of this PR, we look forward to you initiating this discussion to determine whether new rules can be introduced. Please relax, this does not mean that additional changes are incorrect, it just requires a community oriented discussion process.

Please let me know what's your opinion~

[zzzk1]

Hi, @zzzk1 Thanks for your contribution and for @caicancai review.

To be honest, I'm more inclined towards the scope of responsibilities in this proposal-1, so I would prefer to discard changes that are not within the scope of the proposal in the current PR.

Of course, we'd be glad to consider accepting changes beyond the scope, but we need to make this decision through some necessary discussions.

So, in short, if you want to quickly merge this PR, you can remove changes that are not within the proposal scope; If you wish to merge all the contents of this PR, we look forward to you initiating this discussion to determine whether new rules can be introduced. Please relax, this does not mean that additional changes are incorrect, it just requires a community oriented discussion process.

Please let me know what's your opinion~

Hi @RocMarshal , I think the following coding rules can be introduced

• If the argument passed by the front end is single, the Controller uses that single argument to receive without using object wrappers

• If the Controller passes an object to the Service, and only a single field in the object is used in the Service, the object is replaced with a single parameter

  /* before */
  /* controller */
  public RestResponse delete(Member member) {
      this.memberService.remove(member);
      return RestResponse.success();
  }
  /* service */
  public void remove(Member memberArg) {
      Member member =
          Optional.ofNullable(this.getById(memberArg.getId()))
              .orElseThrow(
                  () ->
                      new ApiAlertException(
                          String.format("The member [id=%s] not found", memberArg.getId())));
      this.removeById(member);
      userService.clearLastTeam(member.getUserId(), member.getTeamId());
    }
  
  
  /* after */
  /* controller */
  public RestResponse delete(Long id) {
      this.memberService.remove(id);
      return RestResponse.success();
  }
  
  /* service */
  public void remove(Long id) {
  	Member member =
      	Optional.ofNullable(this.getById(id))
          	.orElseThrow(
              	() -> new ApiAlertException(String.format("The member [id=%s] not found", id)));
  	this.removeById(member);
  	userService.clearLastTeam(member.getUserId(), member.getTeamId());
  }

• Use distinct and well-defined parameter names in Service to avoid ambiguity

  /* Bad case */
  List<ApplicationConfig> list(Long id);
  
  /* good case */
  List<ApplicationConfig> list(Long appId);

[caicancai]

Hi, @zzzk1 Thanks for your contribution and for @caicancai review.
To be honest, I'm more inclined towards the scope of responsibilities in this proposal-1, so I would prefer to discard changes that are not within the scope of the proposal in the current PR.
Of course, we'd be glad to consider accepting changes beyond the scope, but we need to make this decision through some necessary discussions.
So, in short, if you want to quickly merge this PR, you can remove changes that are not within the proposal scope; If you wish to merge all the contents of this PR, we look forward to you initiating this discussion to determine whether new rules can be introduced. Please relax, this does not mean that additional changes are incorrect, it just requires a community oriented discussion process.
Please let me know what's your opinion~

Hi @RocMarshal , I think the following coding rules can be introduced

* If the argument passed by the front end is single, the **Controller** uses that single argument to receive without using object wrappers

* If the **Controller** passes an object to the **Service**, and only a single field in the object is used in the Service, the object is replaced with a single parameter
  ```java
  /* before */
  /* controller */
  public RestResponse delete(Member member) {
      this.memberService.remove(member);
      return RestResponse.success();
  }
  /* service */
  public void remove(Member memberArg) {
      Member member =
          Optional.ofNullable(this.getById(memberArg.getId()))
              .orElseThrow(
                  () ->
                      new ApiAlertException(
                          String.format("The member [id=%s] not found", memberArg.getId())));
      this.removeById(member);
      userService.clearLastTeam(member.getUserId(), member.getTeamId());
    }
  
  
  /* after */
  /* controller */
  public RestResponse delete(Long id) {
      this.memberService.remove(id);
      return RestResponse.success();
  }
  
  /* service */
  public void remove(Long id) {
  	Member member =
      	Optional.ofNullable(this.getById(id))
          	.orElseThrow(
              	() -> new ApiAlertException(String.format("The member [id=%s] not found", id)));
  	this.removeById(member);
  	userService.clearLastTeam(member.getUserId(), member.getTeamId());
  }
  ```

* Use distinct and well-defined parameter names in **Service** to avoid ambiguity
  ```java
  /* Bad case */
  List<ApplicationConfig> list(Long id);
  
  /* good case */
  List<ApplicationConfig> list(Long appId);
  ```

Here I think we can’t just look at the current interface design of sp. We must consider the scalability of the sp controller interface. You have to ensure that other parameters in the entire object will not be used in the future. I have to say that sp still has a lot of potential here. There is room for improvement. I cannot guarantee that the parameters in the object will not be used in the future.

[wolfboys]

hi @zzzk1
Thanks very much for your hard work and contribution, In the controller layer, we cannot change the previous form of using beans to receive parameters to the current form. e.g:

 @Operation(summary = "Force stop application")
  @PermissionAction(id = "#app.id", type = PermissionType.APP)
  @PostMapping("forcedStop")
  @RequiresPermissions("app:cancel")
  public RestResponse forcedStop(Application app) {
    applicationService.forcedStop(app);
    return RestResponse.success();
  }

  @Operation(summary = "Delete member")
  @PermissionAction(id = "#member.teamId", type = PermissionType.TEAM)
  @DeleteMapping("delete")
  @RequiresPermissions("member:delete")
  public RestResponse delete(Member member) {
    this.memberService.deleteMember(member);
    return RestResponse.success();
  }

PermissionAction is an annotation used to verify user permissions, This annotation, in conjunction with StreamParkAspect, obtains the required parameters, such as appid and teamid, before the execution of the controller method, and performs logical judgment and interception in the aspect.

Therefore, the controller layer still needs to use beans to receive parameters from the frontend and cannot be change like this PR, But when the controller layer calls a method in the service layer, we can pass specific parameters(e.g: Long id) instead of a bean. I think this part can be improved. I'd love to hear your further thoughts on this. 😊

[wolfboys]

We can't make this change because it would cause the annotations to not work. 😕