Skip to content

api server exception management and code optimization (#397)#2397

Merged
davidzollo merged 4 commits intoapache:devfrom
hgaol:dev
Apr 12, 2020
Merged

api server exception management and code optimization (#397)#2397
davidzollo merged 4 commits intoapache:devfrom
hgaol:dev

Conversation

@hgaol
Copy link
Copy Markdown
Member

@hgaol hgaol commented Apr 10, 2020 •
edited
Loading

Tips

What is the purpose of the pull request

Currently, there are many try-catch code in api-server's controller. This pr clean the code, use an annotation to point out which Status should be return if there is an exception.

I just update AccessTokenController firstly, to avoid conflict. If this PR is accepted, maybe I could update for other controllers :)

i.e.
Before

public Result generateToken(@RequestAttribute(value = Constants.SESSION_USER) User loginUser,
                              @RequestParam(value = "userId") int userId,
                              @RequestParam(value = "expireTime") String expireTime){
    logger.info("login user {}, generate token , userId : {} , token expire time : {}",loginUser,userId,expireTime);
    try {
        Map<String, Object> result = accessTokenService.generateToken(userId, expireTime);
        return returnDataList(result);
    }catch (Exception e){
        logger.error(GENERATE_TOKEN_ERROR.getMsg(),e);
        return error(GENERATE_TOKEN_ERROR.getCode(), GENERATE_TOKEN_ERROR.getMsg());
    }
}

After

@ControllerException(GENERATE_TOKEN_ERROR)
public Result generateToken(@RequestAttribute(value = Constants.SESSION_USER) User loginUser,
                            @RequestParam(value = "userId") int userId,
                            @RequestParam(value = "expireTime") String expireTime) {
    logger.info("login user {}, generate token , userId : {} , token expire time : {}", loginUser, userId, expireTime);

    Map<String, Object> result = accessTokenService.generateToken(userId, expireTime);
    return returnDataList(result);
}

Brief change log

  • Add ControllerException annotation in org.apache.dolphinscheduler.api.exceptions.
  • Add ApiExceptionHandler controller advice to deal with exceptions.
  • Replace AccessTokenController functions which has try-catch style with @ControllerException annotation.

Verify this pull request

I've debugged the code locally, and it works well. But it's difficult to write tests for this code, because I should find how to throw an exception. It will be easy if I can write some test controllers to test this PR. Pls tell me if I can do that, or is there any other way for testing.

Thanks in advance!

Hope it has no conflict.

@hgaol
Copy link
Copy Markdown
Member Author

hgaol commented Apr 10, 2020 •
edited
Loading

Hi, is there anyone can help me on the unit test issue? How could I do to solve it... To this situation. @dailidong

@davidzollo
Copy link
Copy Markdown
Contributor

davidzollo commented Apr 10, 2020

good job， the code becomes more concise. I think you can refer the QueueControllerTest , about Exception Test , refer the following code:
try {
mockMvc.perform(post("/api/athlete").contentType(contentType).
content(TestUtil.convertObjectToJsonBytes(wAthleteFTP)))
.andExpect(status().isInternalServerError())
.andExpect(content().string("..."))
.andDo(print());
} catch (Exception e){
//sink it
}

I want to say I'm not very familar with MockMvc, you can contact me by mail( lidongdai@apache.org ) or add my wechat(510570367), when added , please tell me your github id is hgaol

@hgaol
Copy link
Copy Markdown
Member Author

hgaol commented Apr 11, 2020

good job， the code becomes more concise. I think you can refer the QueueControllerTest , about Exception Test , refer the following code:
try {
mockMvc.perform(post("/api/athlete").contentType(contentType).
content(TestUtil.convertObjectToJsonBytes(wAthleteFTP)))
.andExpect(status().isInternalServerError())
.andExpect(content().string("..."))
.andDo(print());
} catch (Exception e){
//sink it
}

I want to say I'm not very familar with MockMvc, you can contact me by mail( lidongdai@apache.org ) or add my wechat(510570367), when added , please tell me your github id is hgaol

OK, let me have a try. thank you!

@codecov-io
Copy link
Copy Markdown

codecov-io commented Apr 11, 2020 •
edited
Loading

Codecov Report

Merging #2397 into dev will increase coverage by 0.09%.
The diff coverage is 50.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##                dev    #2397      +/-   ##
============================================
+ Coverage     32.81%   32.91%   +0.09%     
- Complexity     2268     2276       +8     
============================================
  Files           422      423       +1     
  Lines         19830    19839       +9     
  Branches       2363     2366       +3     
============================================
+ Hits           6508     6530      +22     
+ Misses        12741    12725      -16     
- Partials        581      584       +3
Impacted Files Coverage Δ Complexity Δ
...lphinscheduler/api/service/AccessTokenService.java 88.13% <0.00%> (-3.10%) 10.00 <0.00> (ø)
...cheduler/api/controller/AccessTokenController.java 8.33% <5.55%> (+3.20%) 2.00 <1.00> (ø)
...nscheduler/api/exceptions/ApiExceptionHandler.java 75.00% <75.00%> (ø) 3.00 <3.00> (?)
.../org/apache/dolphinscheduler/api/utils/Result.java 92.85% <93.33%> (-0.48%) 13.00 <7.00> (+5.00) ⬇️
.../org/apache/dolphinscheduler/api/enums/Status.java 100.00% <100.00%> (ø) 5.00 <0.00> (ø)
...e/dolphinscheduler/remote/NettyRemotingClient.java 53.23% <0.00%> (-0.72%) 10.00% <0.00%> (-1.00%)
...he/dolphinscheduler/common/thread/ThreadUtils.java 81.66% <0.00%> (+6.66%) 15.00% <0.00%> (+1.00%)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 320469d...8176169. Read the comment docs.

@hgaol
Copy link
Copy Markdown
Member Author

hgaol commented Apr 11, 2020

I've added some tests, but seems it doesn't run. Does anyone has some ideas on that?

@hgaol
Copy link
Copy Markdown
Member Author

hgaol commented Apr 11, 2020

At last, it works, thanks @dailidong for your help.

Technoboy-
Technoboy- approved these changes Apr 11, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@Technoboy- Technoboy- left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud bot commented Apr 11, 2020

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 0 Code Smells

83.9% 83.9% Coverage
0.0% 0.0% Duplication

davidzollo
davidzollo approved these changes Apr 12, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@davidzollo davidzollo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

@davidzollo davidzollo merged commit 3c9ba0a into apache:dev Apr 12, 2020
@davidzollo
Copy link
Copy Markdown
Contributor

davidzollo commented Apr 12, 2020

good job. thanks for your first contribution, looking forward for your next conttribution, haha

@hgaol hgaol mentioned this pull request Apr 12, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Technoboy- Technoboy- Technoboy- approved these changes

@davidzollo davidzollo davidzollo approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@hgaol @davidzollo @codecov-io @Technoboy-