Skip to content
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

Refactor NotFoundException、BadRequestException #4812

Merged
merged 88 commits into from Mar 26, 2023

Conversation

klboke
Copy link
Contributor

@klboke klboke commented Mar 21, 2023

What's the purpose of this PR

  • Give a unified description to exceptions of the same class

klboke and others added 30 commits May 16, 2019 11:07
shoothzj
shoothzj previously approved these changes Mar 21, 2023
@codecov
Copy link

codecov bot commented Mar 21, 2023

Codecov Report

Merging #4812 (0fe74cf) into master (5903691) will increase coverage by 0.17%.
The diff coverage is 26.42%.

@@             Coverage Diff              @@
##             master    #4812      +/-   ##
============================================
+ Coverage     47.81%   47.98%   +0.17%     
- Complexity     1672     1703      +31     
============================================
  Files           346      346              
  Lines         10665    10694      +29     
  Branches       1065     1066       +1     
============================================
+ Hits           5099     5132      +33     
+ Misses         5251     5246       -5     
- Partials        315      316       +1     
Impacted Files Coverage Δ
...o/adminservice/aop/NamespaceAcquireLockAspect.java 85.96% <0.00%> (ø)
...apollo/adminservice/aop/NamespaceUnlockAspect.java 81.13% <0.00%> (ø)
...dminservice/controller/AppNamespaceController.java 34.61% <0.00%> (ø)
...llo/adminservice/controller/ClusterController.java 56.00% <0.00%> (ø)
...apollo/adminservice/controller/ItemController.java 42.50% <0.00%> (ø)
...nservice/controller/NamespaceBranchController.java 11.36% <0.00%> (ø)
...o/adminservice/controller/NamespaceController.java 15.62% <0.00%> (ø)
...minservice/controller/NamespaceLockController.java 35.71% <0.00%> (ø)
...llo/adminservice/controller/ReleaseController.java 23.72% <0.00%> (ø)
...framework/apollo/biz/service/AccessKeyService.java 40.00% <0.00%> (ø)
... and 31 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@klboke klboke changed the title Refactor NotFoundException Refactor NotFoundException、BadRequestException Mar 22, 2023
Copy link
Member

@nobodyiam nobodyiam left a comment

Choose a reason for hiding this comment

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

This is a good try to narrow down the messages. However, there seem 2 patterns to handle the exception:

  1. BadRequestException.xxx
throw BadRequestException.itemNotExists(itemId);
throw BadRequestException.namespaceNotExists();
  1. new xxxNotFoundException
throw new AppNotFountException(appId);
throw new ClusterNotFoundException(appId, clusterName);

Is there a special consideration for this? Or maybe we could unify them to one pattern?

@klboke
Copy link
Contributor Author

klboke commented Mar 23, 2023

BadRequestException's HTTP response code is 400, while NotFoundException's HTTP response code is 404.

Is there a special consideration for this? Or maybe we could unify them to one pattern?

Yes, it's unified now.

throw BadRequestException.userNotExists(operator);
throw NotFoundException.itemNotFound(entity.getKey());

@klboke klboke requested a review from nobodyiam March 23, 2023 03:01
Copy link
Member

@nobodyiam nobodyiam left a comment

Choose a reason for hiding this comment

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

LGTM

@nobodyiam nobodyiam merged commit a556681 into apolloconfig:master Mar 26, 2023
8 of 9 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Mar 26, 2023
@klboke klboke deleted the eception branch March 28, 2023 09:14
@nobodyiam nobodyiam added this to the 2.2.0 milestone Aug 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants