Reorganize case sensitivity for matching entities and filtering commits #1444
Conversation
Remove strtolower() from createRulesFromQueries() and entityMatchesSomeRule() to avoid case insensitivity for processing entities. Add strtolower() to createGitLogQueryFromRule() to ensure case insensitivity for converting rules related to filtering commits.
Processing entities should be case-sensitive, but converting rules for git log queries should be case-insensitive.
| } else { | ||
| $prefix = '\(X-VP-\|VP-\)'; | ||
| $key = '\(X-VP-\|VP-\)' . $key; |
There was a problem hiding this comment.
I stayed with capitalized characters here and changed queryLanguageUtilsGeneratesCorrectGitLogQuery test accordingly to follow VP-Action example (also capitalized letters) and made some small changes to allowing test to pass (assertEquals() is now case sensitive). But it can be also switched to lowercase characters.
Is that the goal though? I think the intent of #1439 was to:
Importantly, this would not change any user-facing behavior, just organize the code better and also fix the tests (for example, this change is highly desirable: https://github.com/versionpress/versionpress/pull/1444/files#diff-c2ff5a3b77d21156b05214cdb94eb655R107). If we keep case insensitivity as far as the user is concerned, many things in this PR will become simpler. |
Yes, you are right, I see it now. I probably moved the goal a little bit for this part. During work on this PR I realized that we also do not need using For Git searching is still used |
|
Yes, this PR does many good things and I think it's about 80% where it should be. It just wasn't clear to me what the goal regarding entity matching was as the issue mentions docs and their necessary update around "queries use the same syntax as search / filtering in the UI" – #1439 (comment). BTW, I've updated my previous comment to highlight that |
There was a problem hiding this comment.
@pavelevap, following up on the discussion starting with #1444 (comment), I left some comments directly in code to demonstrate what I meant. Hopefully we're on the same page.
| @@ -128,7 +128,7 @@ public function ignoredEntityCapitalizedIsCorrectlyIdentified() | |||
| $entity = [ | |||
| 'some_field' => 'b', | |||
| 'other_field' => 'a', | |||
| 'capitalized_value_field' => 'VaLuE', // comparison must be case insensitive | |||
There was a problem hiding this comment.
This is how the test should look – we indeed want to test that if VaLuE is provided, it is still matched by a rule that uses uppercase (or lowercase, or any-case) VALUE.
There was a problem hiding this comment.
Yes, we are providing VALUE in original $entitySchema, so we are also expecting VALUE. Comparison is not case insensitive now, so VaLuE would fail.
| @@ -101,8 +104,7 @@ public function ruleAndQueryProvider() | |||
| public function queryLanguageUtilsCreatesCorrectRules($query, $expectedRules) | |||
| { | |||
| $rules = QueryLanguageUtils::createRulesFromQueries($query); | |||
| // Perform case insensitive match | |||
| $this->assertEquals($expectedRules, $rules, '', 0, 10, false, true); | |||
| $this->assertEquals($expectedRules, $rules); | |||
There was a problem hiding this comment.
👍 this is a very welcome change, basically making tests actually test something :)
| [['x-vp-another-key' => ['Test value']], '-i --all-match --grep="^x-vp-another-key: \(Test value\)$"'], | ||
| [['vp-another-key' => ['Test value']], '-i --all-match --grep="^\(x-\)\?vp-another-key: \(Test value\)$"'], | ||
| [['another-key' => ['Test value']], '-i --all-match --grep="^\(x-vp-\|vp-\)another-key: \(Test value\)$"'], | ||
| [['X-vp-another-key' => ['Test value']], '-i --all-match --grep="^X-VP-another-key: \(Test value\)$"'], |
There was a problem hiding this comment.
I think that no case juggling is necessary here. If the input is X-vp-another-key, Git search should be ... --grep="^X-vp-another-key as well.
Similarly for the lines below.
There was a problem hiding this comment.
Yes, but we need it for QueryLanguageUtils::createGitLogQueryFromRule and also this test (and I believe that it was the original reason to make tests case-insensitive before).
Example: vp-another-key was automatically transformed into X-vp-another-key and test would fail because of expecting x-vp-another-key.
Result will be now capitalized everytime:
X-VP-another-key (see changes in QueryLanguageUtils::createGitLogQueryFromRule). But I am not sure if this is the best solution, see my previous comment: #1444 (review).
|
I am closing this PR, superseded by #1450. |
Resolves #1439
Case-insensitive comparison moved from
QueryLanguageUtils::createRulesFromQueriesandQueryLanguageUtils::entityMatchesSomeRuletoQueryLanguageUtils::createGitLogQueryFromRule. This ensures that entities comparison will be case-sensitive and filtering commits case-insensitive without losing any information related to capitalized characters.