Fix label migration for branch patterns in submit requirements Current situation: The label migration API converts labels with function = MaxWithBlock into submit requirements. However, branch patterns are currently migrated by embedding the original branch value directly into the applicableIf expression. For example: [label "rest-authz"] branch = refs/heads/* function = MaxWithBlock was migrated to: [submit-requirement "rest-authz"] applicableIf = branch:\\\"refs/heads/*\\\" Motivation: This is not a valid Gerrit query expression. While refs/heads/* is a valid label branch pattern, the branch: operator in submit requirement expressions expects either an exact branch name or a regular expression. Fix: This change converts branch patterns during migration as follows: refs/heads/master -> branch:"refs/heads/master" refs/heads/* -> branch:^refs/heads/.* refs/* -> branch:^refs/.* ^refs/heads/.* -> branch:^refs/heads/.* refs/heads/hi\"ch -> branch:"refs/heads/hi\"ch" refs/heads/hi#ch -> branch:"refs/heads/hi#ch" As a result, the generated applicableIf expressions are valid and preserve the semantics of the original label branch restrictions. NOTE: After migration, When Jgit persists the data into config, it is persisted with escaping characters. E.g. [label "rest-authz-hash"] branch = "refs/heads/hi#ch" -> persisted after migration to [submit-requirement "rest-authz-hash"] applicableIf = branch:\"refs/heads/hi\\\"ch\" But during runtime, it is interpreted as `branch:"refs/heads/hi\"ch"` Change-Id: I80aaa87019a9e8412be5e26949cd150c9ffe6515 Release-Notes: Fix label migration for branch patterns in submit requirements
diff --git a/java/com/google/gerrit/server/schema/MigrateLabelFunctionsToSubmitRequirement.java b/java/com/google/gerrit/server/schema/MigrateLabelFunctionsToSubmitRequirement.java index 06c2037..305e476 100644 --- a/java/com/google/gerrit/server/schema/MigrateLabelFunctionsToSubmitRequirement.java +++ b/java/com/google/gerrit/server/schema/MigrateLabelFunctionsToSubmitRequirement.java
@@ -40,6 +40,7 @@ import java.util.Locale; import java.util.Map; import java.util.Optional; +import java.util.regex.Pattern; import java.util.stream.Collectors; import org.eclipse.jgit.errors.ConfigInvalidException; import org.eclipse.jgit.lib.Config; @@ -329,12 +330,29 @@ String.join( " OR ", attributes.refPatterns().stream() - .map(b -> "branch:\\\"" + b + "\\\"") + .map(MigrateLabelFunctionsToSubmitRequirement::toApplicableIfExpression) .collect(Collectors.toList())))); } return builder.build(); } + private static String toApplicableIfExpression(String branchRef) { + // Reqex -> migrate as it is. + if (branchRef.startsWith("^")) { + return "branch:" + branchRef; + } + // Wildcard -> needs to converted into gerrit regex. + if (branchRef.endsWith("/*")) { + String prefix = branchRef.substring(0, branchRef.length() - 1); + String regex = "^" + Pattern.quote(prefix) + ".*"; + return "branch:" + regex; + } + // If branch with " -> need to escape " + branchRef = branchRef.replace("\"", "\\\""); + // Other cases e.g. branch with # or " -> needs to be quoted + return "branch:\"" + branchRef + "\""; + } + private static boolean isBlockingOrRequiredLabel(String function) { return function.equals("AnyWithBlock") || function.equals("MaxWithBlock")
diff --git a/javatests/com/google/gerrit/acceptance/pgm/MigrateLabelFunctionsToSubmitRequirementIT.java b/javatests/com/google/gerrit/acceptance/pgm/MigrateLabelFunctionsToSubmitRequirementIT.java index 5a6c5c5..4e871bb 100644 --- a/javatests/com/google/gerrit/acceptance/pgm/MigrateLabelFunctionsToSubmitRequirementIT.java +++ b/javatests/com/google/gerrit/acceptance/pgm/MigrateLabelFunctionsToSubmitRequirementIT.java
@@ -277,7 +277,7 @@ assertExistentSr( /* srName */ "Foo", - /* applicabilityExpression= */ "branch:\\\"refs/heads/master\\\"", + /* applicabilityExpression= */ "branch:\"refs/heads/master\"", /* submittabilityExpression= */ "label:Foo=MAX AND -label:Foo=MIN", /* canOverride= */ true); assertLabelFunction("Foo", "NoBlock"); @@ -299,8 +299,52 @@ assertExistentSr( /* srName */ "Foo", - /* applicabilityExpression= */ "branch:\\\"refs/heads/master\\\" " - + "OR branch:\\\"refs/heads/develop\\\"", + /* applicabilityExpression= */ "branch:\"refs/heads/master\" " + + "OR branch:\"refs/heads/develop\"", + /* submittabilityExpression= */ "label:Foo=MAX AND -label:Foo=MIN", + /* canOverride= */ true); + assertLabelFunction("Foo", "NoBlock"); + } + + @Test + public void migrateBlockingLabel_withQuotesInBranchNameAttribute() throws Exception { + createLabelWithBranch( + "Foo", + "MaxWithBlock", + /* ignoreSelfApproval= */ false, + ImmutableList.of("refs/heads/gerr\"it")); + + assertNonExistentSr(/* srName= */ "Foo"); + + TestUpdateUI updateUI = runMigration(/* expectedResult= */ Status.MIGRATED); + assertThat(updateUI.newlyCreatedSrs).isEqualTo(1); + assertThat(updateUI.existingSrsMismatchingWithMigration).isEqualTo(0); + + assertExistentSr( + /* srName */ "Foo", + /* applicabilityExpression= */ "branch:\"refs/heads/gerr\\\"it\"", + /* submittabilityExpression= */ "label:Foo=MAX AND -label:Foo=MIN", + /* canOverride= */ true); + assertLabelFunction("Foo", "NoBlock"); + } + + @Test + public void migrateBlockingLabel_withHashInBranchNameAttribute() throws Exception { + createLabelWithBranch( + "Foo", + "MaxWithBlock", + /* ignoreSelfApproval= */ false, + ImmutableList.of("refs/heads/gerr#it")); + + assertNonExistentSr(/* srName= */ "Foo"); + + TestUpdateUI updateUI = runMigration(/* expectedResult= */ Status.MIGRATED); + assertThat(updateUI.newlyCreatedSrs).isEqualTo(1); + assertThat(updateUI.existingSrsMismatchingWithMigration).isEqualTo(0); + + assertExistentSr( + /* srName */ "Foo", + /* applicabilityExpression= */ "branch:\"refs/heads/gerr#it\"", /* submittabilityExpression= */ "label:Foo=MAX AND -label:Foo=MIN", /* canOverride= */ true); assertLabelFunction("Foo", "NoBlock"); @@ -322,7 +366,29 @@ assertExistentSr( /* srName */ "Foo", - /* applicabilityExpression= */ "branch:\\\"^refs/heads/main-.*\\\"", + /* applicabilityExpression= */ "branch:^refs/heads/main-.*", + /* submittabilityExpression= */ "label:Foo=MAX AND -label:Foo=MIN", + /* canOverride= */ true); + assertLabelFunction("Foo", "NoBlock"); + } + + @Test + public void migrateBlockingLabel_withWildcardBranchAttribute() throws Exception { + createLabelWithBranch( + "Foo", + "MaxWithBlock", + /* ignoreSelfApproval= */ false, + ImmutableList.of("refs/heads/release/*")); + + assertNonExistentSr(/* srName= */ "Foo"); + + TestUpdateUI updateUI = runMigration(/* expectedResult= */ Status.MIGRATED); + assertThat(updateUI.newlyCreatedSrs).isEqualTo(1); + assertThat(updateUI.existingSrsMismatchingWithMigration).isEqualTo(0); + + assertExistentSr( + /* srName */ "Foo", + /* applicabilityExpression= */ "branch:^\\Qrefs/heads/release/\\E.*", /* submittabilityExpression= */ "label:Foo=MAX AND -label:Foo=MIN", /* canOverride= */ true); assertLabelFunction("Foo", "NoBlock"); @@ -344,8 +410,8 @@ assertExistentSr( /* srName */ "Foo", - /* applicabilityExpression= */ "branch:\\\"refs/heads/master\\\" " - + "OR branch:\\\"^refs/heads/main-.*\\\"", + /* applicabilityExpression= */ "branch:\"refs/heads/master\" " + + "OR branch:^refs/heads/main-.*", /* submittabilityExpression= */ "label:Foo=MAX AND -label:Foo=MIN", /* canOverride= */ true); assertLabelFunction("Foo", "NoBlock");