Skip to content

#11397 fix bug of wrong route result#11626

Closed
vince7839 wants to merge 1 commit intoapache:masterfrom
vince7839:issue#11397
Closed

#11397 fix bug of wrong route result#11626
vince7839 wants to merge 1 commit intoapache:masterfrom
vince7839:issue#11397

Conversation

@vince7839
Copy link
Contributor

@vince7839 vince7839 commented Aug 3, 2021

Fixes #11397.

Changes proposed in this pull request:

  • fix bug of wrong route result when an "OR" condition before "AND" confition

Copy link
Contributor

@tristaZero tristaZero left a comment

Choose a reason for hiding this comment

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

@vince7839 Hi, thanks for your bug-fix. I am glad to see your feedback after you raised the issue. Could you add a UT in ExpressionBuilderTest to verify your modification?
@strongduanmu Could you have a review here?

@strongduanmu strongduanmu added this to the 5.0.0-RC1 milestone Aug 5, 2021
@strongduanmu
Copy link
Member

@vince7839 Thank you for your pr, can you add some test case(unit test, rewrite test) for it?

result.getAndPredicates().add(createAndPredicate(eachLeft, eachRight));
}
}
processAndExpression(result);
Copy link
Member

Choose a reason for hiding this comment

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

@vince7839 Maybe result.getAndPredicates().addAll(createAndPredicates()) is better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean changing the method of "processAndExpression()" to "createAndPredicates()"?

Copy link
Member

Choose a reason for hiding this comment

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

Do you mean changing the method of "processAndExpression()" to "createAndPredicates()"?

@vince7839 Yes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay,I'll do it and add UT

@vince7839 vince7839 closed this Aug 6, 2021
@vince7839 vince7839 deleted the issue#11397 branch August 6, 2021 17:05
@menghaoranss menghaoranss modified the milestones: 5.0.0-RC1, 5.0.0 Oct 26, 2021
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.

route result is wrong

4 participants

Comments