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

Refactoring the message splicing of internal Exceptions #4571

Merged
merged 59 commits into from
Sep 18, 2022

Conversation

klboke
Copy link
Contributor

@klboke klboke commented Sep 15, 2022

What's the purpose of this PR

Refactoring the message splicing of internal Exceptions

  • As defined in AbstractApolloHttpException, the message splicing operation can be simplified when using
public abstract class AbstractApolloHttpException extends RuntimeException{

  private static final long serialVersionUID = -1713129594004951820L;
  
  protected HttpStatus httpStatus;

  /**
   * When args not empty, use {@link com.google.common.base.Strings#lenientFormat(String, Object...)}
   * to replace %s in msgTpl with args to set the error message. Otherwise, use msgTpl
   * to set the error message. e.g.: 
   * <pre>{@code new NotFoundException("... %s ... %s ... %s", "str", 0, 0.1)}</pre>
   * If the number of '%s' in `msgTpl` does not match args length, the '%s' string will be printed.
   */
  public AbstractApolloHttpException(String msgTpl, Object... args){
    super(args == null || args.length == 0 ? msgTpl : Strings.lenientFormat(msgTpl, args));
  }
  
  public AbstractApolloHttpException(String msg, Exception e){
    super(msg,e);
  }

  protected void setHttpStatus(HttpStatus httpStatus){
    this.httpStatus = httpStatus;
  }

  public HttpStatus getHttpStatus() {
    return httpStatus;
  }
}

klboke and others added 30 commits May 16, 2019 11:07
@klboke klboke requested review from mghio and nobodyiam September 15, 2022 09:21
@codecov-commenter
Copy link

Codecov Report

Merging #4571 (a711012) into master (d6fd359) will decrease coverage by 0.01%.
The diff coverage is 15.15%.

@@             Coverage Diff              @@
##             master    #4571      +/-   ##
============================================
- Coverage     53.34%   53.32%   -0.02%     
+ Complexity     2713     2711       -2     
============================================
  Files           495      495              
  Lines         15440    15440              
  Branches       1599     1599              
============================================
- Hits           8236     8234       -2     
  Misses         6646     6646              
- Partials        558      560       +2     
Impacted Files Coverage Δ
...ctrip/framework/apollo/biz/service/AppService.java 48.71% <0.00%> (ø)
...trip/framework/apollo/biz/service/ItemService.java 12.94% <0.00%> (ø)
...p/framework/apollo/biz/service/ItemSetService.java 8.95% <0.00%> (ø)
.../common/exception/AbstractApolloHttpException.java 42.85% <0.00%> (ø)
...mework/apollo/openapi/service/ConsumerService.java 58.46% <0.00%> (ø)
...pollo/openapi/v1/controller/ClusterController.java 23.52% <0.00%> (ø)
...k/apollo/openapi/v1/controller/ItemController.java 12.19% <0.00%> (ø)
...k/apollo/portal/controller/ConsumerController.java 10.52% <0.00%> (ø)
...o/portal/controller/NamespaceBranchController.java 18.91% <0.00%> (ø)
.../apollo/portal/controller/NamespaceController.java 11.82% <0.00%> (ø)
... and 14 more

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

Copy link
Contributor

@mghio mghio left a comment

Choose a reason for hiding this comment

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

I think the format of String.format can be preserved, because it can warn when %s does not match the number of arguments. eg : Too few arguments with format string (found: 0 , expected: 1) .

@klboke
Copy link
Contributor Author

klboke commented Sep 16, 2022

  • @mghio when discussing this, does it mean choosing Strings.lenientFormat or String.format, whichever is more appropriate.

This pr (#3999) has instructions

@nobodyiam
Copy link
Member

I think the format of String.format can be preserved, because it can warn when %s does not match the number of arguments. eg : Too few arguments with format string (found: 0 , expected: 1) .

I think it's all right. As we could replace Strings.lenientFormat with String.format in AbstractApolloHttpException if we do want to warn the mismatched number of arguments.

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

Copy link
Contributor

@mghio mghio 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 70b70c3 into apolloconfig:master Sep 18, 2022
@github-actions github-actions bot locked and limited conversation to collaborators Sep 18, 2022
@nobodyiam nobodyiam added this to the 2.1.0 milestone Feb 5, 2023
@klboke klboke deleted the exception-logs branch March 21, 2023 03:51
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.

4 participants