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

[Task #3011]The JwtRuleHandle should be coustomerd #4133

Merged
merged 16 commits into from
Nov 11, 2022

Conversation

tian-pengfei
Copy link
Contributor

@tian-pengfei tian-pengfei commented Oct 26, 2022

Make sure that:

  • You have read the contribution guidelines.
  • You submit test cases (unit or integration tests) that back your changes.
  • Your local test passed ./mvnw clean install -Dmaven.javadoc.skip=true.

finished task #3011

client could be customed JwtRuleHandle
for example

/**
 * isSingleton is false  , because JwtRuleHandle is stateful.
 */
@Join(isSingleton = false)
public class CustomJwtRuleHandle implements JwtRuleHandle {
    private String handleJson;

    @Override
    public void init(final String handleJson) {
        this.handleJson = handleJson;
    }

    @Override
    public ServerWebExchange execute(final ServerWebExchange exchange, final Map<String, Object> jwtBody) {
        // handle exchange by config(handleJson),return new exchange
        return exchange;
    }

    @Override
    public String toJson() {
        return handleJson;
    }
}
custom=org.apache.shenyu.plugin.jwt.rule.CustomJwtRuleHandle

handle format is

{"handleType":custom","customConfig":"customConfig"}
Siteproxy

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@tian-pengfei tian-pengfei reopened this Oct 26, 2022
@codecov-commenter
Copy link

codecov-commenter commented Oct 26, 2022

Codecov Report

Merging #4133 (aebce40) into master (5ed30ca) will decrease coverage by 0.07%.
The diff coverage is 70.68%.

@@             Coverage Diff              @@
##             master    #4133      +/-   ##
============================================
- Coverage     70.33%   70.25%   -0.08%     
- Complexity     7172     7174       +2     
============================================
  Files           965      968       +3     
  Lines         26934    26962      +28     
  Branches       2399     2403       +4     
============================================
- Hits          18944    18943       -1     
- Misses         6538     6563      +25     
- Partials       1452     1456       +4     
Impacted Files Coverage Δ
...n/java/org/apache/shenyu/plugin/jwt/JwtPlugin.java 51.21% <45.45%> (-11.75%) ⬇️
...plugin/jwt/strategy/DefaultJwtConvertStrategy.java 73.07% <73.07%> (ø)
...g/apache/shenyu/plugin/jwt/rule/JwtRuleHandle.java 75.00% <75.00%> (ø)
...plugin/jwt/strategy/JwtConvertStrategyFactory.java 80.00% <80.00%> (ø)
...shenyu/plugin/jwt/handle/JwtPluginDataHandler.java 100.00% <100.00%> (ø)
...e/shenyu/plugin/jwt/rule/DefaultJwtRuleHandle.java 41.66% <100.00%> (ø)
...controller/ShenyuClientHttpRegistryController.java 77.77% <0.00%> (-22.23%) ⬇️
...ruptor/RegisterClientServerDisruptorPublisher.java 52.94% <0.00%> (-11.77%) ⬇️
.../plugin/grpc/loadbalance/AbstractLoadBalancer.java 70.65% <0.00%> (-8.70%) ⬇️
... and 2 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

li-keguo
li-keguo previously approved these changes Oct 26, 2022
@moremind
Copy link
Member

why do you do like this?

@tian-pengfei
Copy link
Contributor Author

@moremind @li-keguo Hi guys,I have refactored it again , please check and give me some advice

@li-keguo li-keguo requested a review from lishuo5263 October 28, 2022 13:07
li-keguo
li-keguo previously approved these changes Oct 28, 2022
* @param handleJson handleJson from rule
* @return jwtRuleHandle
*/
T parseHandleJson(String handleJson);
Copy link
Member

Choose a reason for hiding this comment

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

You can skip generics here,

JwtRuleHandle parseHandleJson(String handleJson);

This also returns a subclass without warning

no SuppressWarnings

Copy link
Contributor Author

@tian-pengfei tian-pengfei Oct 29, 2022

Choose a reason for hiding this comment

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

Thank you for the advice,but I think it has a problem . If you replace generics by JwtRuleHandle, it can happen class type of the result of executing parseHandleJson is diffrent from first parameter of ServerWebExchange convert. I think it is unreasonable for this to happen

Copy link
Member

Choose a reason for hiding this comment

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

parseHandleJson must return : JwtRuleHandle or ? extent JwtRuleHandle

Generics are weakly constrained and type-ambiguous.
It's better to have a strong constraint here

eg:

@SuppressWarnings
public class MyJwtConvertStrategy implements JwtConvertStrategy {

   Object parseHandleJson(String handleJson){
    // ... ... 
    }
}

@li-keguo
Copy link
Member

Looks like a lot of good

@tian-pengfei
Copy link
Contributor Author

Looks like a lot of good

Thanks <3

@moremind
Copy link
Member

moremind commented Nov 9, 2022

you maybe know spi

@tian-pengfei tian-pengfei requested review from moremind and removed request for lishuo5263 November 9, 2022 02:42
@moremind moremind requested a review from impactCn November 10, 2022 11:19
@moremind moremind requested a review from li-keguo November 10, 2022 11:19
@moremind moremind added this to the 2.5.1 milestone Nov 10, 2022
* @param handleJson handleJson from rule
* @return jwtRuleHandle
*/
T parseHandleJson(String handleJson);
Copy link
Member

Choose a reason for hiding this comment

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

parseHandleJson must return : JwtRuleHandle or ? extent JwtRuleHandle

Generics are weakly constrained and type-ambiguous.
It's better to have a strong constraint here

eg:

@SuppressWarnings
public class MyJwtConvertStrategy implements JwtConvertStrategy {

   Object parseHandleJson(String handleJson){
    // ... ... 
    }
}

@li-keguo li-keguo merged commit 08ff2aa into apache:master Nov 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants