SONAR-10361 Handle assigning issue to a user with commas in name (#903)

server/sonar-db-dao/src/main/java/org/sonar/db/issue/IssueChangeDto.java

@@ -72,7 +72,7 @@ public final class IssueChangeDto implements Serializable {
   public static IssueChangeDto of(String issueKey, FieldDiffs diffs) {
     IssueChangeDto dto = newDto(issueKey);
     dto.setChangeType(IssueChangeDto.TYPE_FIELD_CHANGE);
-    dto.setChangeData(diffs.toString());
+    dto.setChangeData(diffs.toEncodedString());
     dto.setUserUuid(diffs.userUuid());
     Date createdAt = requireNonNull(diffs.creationDate(), "Diffs created at must not be null");
     dto.setIssueChangeCreationDate(createdAt.getTime());

server/sonar-server/src/main/java/org/sonar/server/issue/ws/ChangelogAction.java

@@ -142,14 +142,16 @@ public class ChangelogAction implements IssuesWsAction {
       FieldDiffs.Diff value = diff.getValue();
       Changelog.Diff.Builder diffBuilder = Changelog.Diff.newBuilder();
       String key = diff.getKey();
+      String oldValue = value.oldValue() != null ? value.oldValue().toString() : null;
+      String newValue = value.newValue() != null ? value.newValue().toString() : null;
       if (key.equals(FILE)) {
         diffBuilder.setKey(key);
-        setNullable(results.getFileLongName(emptyToNull(value.newValue().toString())), diffBuilder::setNewValue);
-        setNullable(results.getFileLongName(emptyToNull(value.oldValue().toString())), diffBuilder::setOldValue);
+        setNullable(results.getFileLongName(emptyToNull(newValue)), diffBuilder::setNewValue);
+        setNullable(results.getFileLongName(emptyToNull(oldValue)), diffBuilder::setOldValue);
       } else {
         diffBuilder.setKey(key.equals(TECHNICAL_DEBT) ? EFFORT_CHANGELOG_KEY : key);
-        setNullable(emptyToNull(value.newValue().toString()), diffBuilder::setNewValue);
-        setNullable(emptyToNull(value.oldValue().toString()), diffBuilder::setOldValue);
+        setNullable(emptyToNull(newValue), diffBuilder::setNewValue);
+        setNullable(emptyToNull(oldValue), diffBuilder::setOldValue);
       }
       return diffBuilder.build();
     };

sonar-core/src/main/java/org/sonar/core/issue/FieldDiffs.java

@@ -22,11 +22,14 @@ package org.sonar.core.issue;
 import com.google.common.base.Splitter;
 import com.google.common.collect.Maps;
 import java.io.Serializable;
+import java.nio.charset.StandardCharsets;
+import java.util.Base64;
 import java.util.Date;
 import java.util.Map;
 import java.util.Objects;
 import javax.annotation.CheckForNull;
 import javax.annotation.Nullable;
+import org.apache.commons.lang.StringUtils;
 
 import static com.google.common.base.Strings.emptyToNull;
 import static com.google.common.base.Strings.isNullOrEmpty;
@@ -39,18 +42,30 @@ import static com.google.common.base.Strings.isNullOrEmpty;
 public class FieldDiffs implements Serializable {
 
   public static final Splitter FIELDS_SPLITTER = Splitter.on(',').omitEmptyStrings();
+  private static final String CHAR_TO_ESCAPE = "|,{}=:";
 
   private String issueKey;
   private String userUuid;
   private Date creationDate;
+  public static final String ASSIGNEE = "assignee";
+  public static final String ENCODING_PREFIX = "{base64:";
+  public static final String ENCODING_SUFFIX = "}";
 
   private final Map<String, Diff> diffs = Maps.newLinkedHashMap();
 
   public Map<String, Diff> diffs() {
+    if (diffs.containsKey(ASSIGNEE)) {
+      Map<String, Diff> result = Maps.newLinkedHashMap(diffs);
+      result.put(ASSIGNEE, decode(result.get(ASSIGNEE)));
+      return result;
+    }
     return diffs;
   }
 
   public Diff get(String field) {
+    if (field.equals(ASSIGNEE)) {
+      return decode(diffs.get(ASSIGNEE));
+    }
     return diffs.get(field);
   }
 
@@ -85,6 +100,10 @@ public class FieldDiffs implements Serializable {
   @SuppressWarnings("unchecked")
   public FieldDiffs setDiff(String field, @Nullable Serializable oldValue, @Nullable Serializable newValue) {
     Diff diff = diffs.get(field);
+    if (field.equals(ASSIGNEE)) {
+      oldValue = encodeField(oldValue);
+      newValue = encodeField(newValue);
+    }
     if (diff == null) {
       diff = new Diff(oldValue, newValue);
       diffs.put(field, diff);
@@ -94,8 +113,16 @@ public class FieldDiffs implements Serializable {
     return this;
   }
 
+  public String toEncodedString() {
+    return serialize(true);
+  }
+
   @Override
   public String toString() {
+    return serialize(false);
+  }
+
+  private String serialize(boolean shouldEncode) {
     StringBuilder sb = new StringBuilder();
     boolean notFirst = false;
     for (Map.Entry<String, Diff> entry : diffs.entrySet()) {
@@ -106,7 +133,11 @@ public class FieldDiffs implements Serializable {
       }
       sb.append(entry.getKey());
       sb.append('=');
-      sb.append(entry.getValue().toString());
+      if (shouldEncode) {
+        sb.append(entry.getValue().toEncodedString());
+      } else {
+        sb.append(entry.getValue().toString());
+      }
     }
     return sb.toString();
   }
@@ -118,22 +149,60 @@ public class FieldDiffs implements Serializable {
     }
     Iterable<String> fields = FIELDS_SPLITTER.split(s);
     for (String field : fields) {
-      String[] keyValues = field.split("=");
+      String[] keyValues = field.split("=", 2);
       if (keyValues.length == 2) {
         String values = keyValues[1];
         int split = values.indexOf('|');
         if (split > -1) {
-          diffs.setDiff(keyValues[0], values.substring(0, split), values.substring(split +1));
+          diffs.setDiff(keyValues[0], emptyToNull(values.substring(0, split)), emptyToNull(values.substring(split + 1)));
         } else {
-          diffs.setDiff(keyValues[0], "", emptyToNull(values));
+          diffs.setDiff(keyValues[0], null, emptyToNull(values));
         }
       } else {
-        diffs.setDiff(keyValues[0], "", "");
+        diffs.setDiff(keyValues[0], null, null);
       }
     }
     return diffs;
   }
 
+  @SuppressWarnings("unchecked")
+  Diff decode(Diff encoded) {
+    return new Diff(
+        decodeField(encoded.oldValue),
+        decodeField(encoded.newValue)
+    );
+  }
+
+  private static Serializable decodeField(@Nullable Serializable field) {
+    if (field == null || !isEncoded(field)) {
+      return field;
+    }
+
+    String encodedField = field.toString();
+    String value = encodedField.substring(ENCODING_PREFIX.length(), encodedField.indexOf(ENCODING_SUFFIX));
+    return new String(Base64.getDecoder().decode(value), StandardCharsets.UTF_8);
+  }
+
+  private static Serializable encodeField(@Nullable Serializable field) {
+    if (field == null || !shouldEncode(field.toString())) {
+      return field;
+    }
+
+    return ENCODING_PREFIX + Base64.getEncoder().encodeToString(field.toString().getBytes(StandardCharsets.UTF_8)) + ENCODING_SUFFIX;
+  }
+
+  private static boolean shouldEncode(String field) {
+    return !field.isEmpty() && !isEncoded(field) && containsCharToEscape(field);
+  }
+
+  private static boolean containsCharToEscape(Serializable s) {
+    return StringUtils.containsAny(s.toString(), CHAR_TO_ESCAPE);
+  }
+
+  private static boolean isEncoded(Serializable field) {
+    return field.toString().startsWith(ENCODING_PREFIX) && field.toString().endsWith(ENCODING_SUFFIX);
+  }
+
   public static class Diff<T extends Serializable> implements Serializable {
     private T oldValue;
     private T newValue;
@@ -163,7 +232,7 @@ public class FieldDiffs implements Serializable {
       return toLong(newValue);
     }
 
-    void setNewValue(T t) {
+    void setNewValue(@Nullable T t) {
       this.newValue = t;
     }
 
@@ -179,16 +248,23 @@ public class FieldDiffs implements Serializable {
       return null;
     }
 
+    private String toEncodedString() {
+      return serialize(true);
+    }
+
     @Override
     public String toString() {
-      // TODO escape , and | characters
+      return serialize(false);
+    }
+
+    private String serialize(boolean shouldEncode) {
       StringBuilder sb = new StringBuilder();
       if (oldValue != null) {
-        sb.append(oldValue.toString());
+        sb.append(shouldEncode ? oldValue : decodeField(oldValue));
         sb.append('|');
       }
       if (newValue != null) {
-        sb.append(newValue.toString());
+        sb.append(shouldEncode ? newValue : decodeField(newValue));
       }
       return sb.toString();
     }

sonar-core/src/test/java/org/sonar/core/issue/FieldDiffsTest.java

@@ -25,7 +25,10 @@ import static org.assertj.core.api.Assertions.assertThat;
 
 public class FieldDiffsTest {
 
-  FieldDiffs diffs = new FieldDiffs();
+  private static final String NAME_WITH_RESERVED_CHARS = "name,with|reserved=chars:";
+  private static final String ENCODED_NAME_WITH_RESERVED_CHARS = "{base64:bmFtZSx3aXRofHJlc2VydmVkPWNoYXJzOg==}";
+
+  private FieldDiffs diffs = new FieldDiffs();
 
   @Test
   public void diffs_should_be_empty_by_default() {
@@ -50,11 +53,11 @@ public class FieldDiffsTest {
 
   @Test
   public void diff_with_long_values() {
-    diffs.setDiff("technicalDebt", 50l, "100");
+    diffs.setDiff("technicalDebt", 50L, "100");
 
     FieldDiffs.Diff diff = diffs.diffs().get("technicalDebt");
-    assertThat(diff.oldValueLong()).isEqualTo(50l);
-    assertThat(diff.newValueLong()).isEqualTo(100l);
+    assertThat(diff.oldValueLong()).isEqualTo(50L);
+    assertThat(diff.newValueLong()).isEqualTo(100L);
   }
 
   @Test
@@ -66,6 +69,33 @@ public class FieldDiffsTest {
     assertThat(diff.newValueLong()).isNull();
   }
 
+  @Test
+  public void diff_with_assignee() {
+    diffs.setDiff("assignee", "oldAssignee", NAME_WITH_RESERVED_CHARS);
+
+    FieldDiffs.Diff diff = diffs.diffs().get("assignee");
+    assertThat(diff.oldValue()).isEqualTo("oldAssignee");
+    assertThat(diff.newValue()).isEqualTo(NAME_WITH_RESERVED_CHARS);
+  }
+
+  @Test
+  public void get() {
+    diffs.setDiff("severity", "BLOCKER", "INFO");
+
+    FieldDiffs.Diff diff = diffs.get("severity");
+    assertThat(diff.oldValue()).isEqualTo("BLOCKER");
+    assertThat(diff.newValue()).isEqualTo("INFO");
+  }
+
+  @Test
+  public void get_with_assignee() {
+    diffs.setDiff("assignee", "oldAssignee", NAME_WITH_RESERVED_CHARS);
+
+    FieldDiffs.Diff diff = diffs.get("assignee");
+    assertThat(diff.oldValue()).isEqualTo("oldAssignee");
+    assertThat(diff.newValue()).isEqualTo(NAME_WITH_RESERVED_CHARS);
+  }
+
   @Test
   public void test_diff_by_key() {
     diffs.setDiff("severity", "BLOCKER", "INFO");
@@ -99,6 +129,14 @@ public class FieldDiffsTest {
     assertThat(diffs.toString()).isEqualTo("severity=BLOCKER|INFO,resolution=OPEN|FIXED");
   }
 
+  @Test
+  public void test_toEncodedString() {
+    diffs.setDiff("assignee", "oldAssignee", NAME_WITH_RESERVED_CHARS);
+    diffs.setDiff("resolution", "OPEN", "FIXED");
+
+    assertThat(diffs.toEncodedString()).isEqualTo("assignee=oldAssignee|" + ENCODED_NAME_WITH_RESERVED_CHARS + ",resolution=OPEN|FIXED");
+  }
+
   @Test
   public void test_toString_with_null_values() {
     diffs.setDiff("severity", null, "INFO");
@@ -121,16 +159,30 @@ public class FieldDiffsTest {
     assertThat(diff.newValue()).isEqualTo("FIXED");
 
     diff = diffs.diffs().get("donut");
-    assertThat(diff.oldValue()).isEqualTo("");
+    assertThat(diff.oldValue()).isEqualTo(null);
     assertThat(diff.newValue()).isEqualTo("new");
 
     diff = diffs.diffs().get("gambas");
-    assertThat(diff.oldValue()).isEqualTo("");
+    assertThat(diff.oldValue()).isEqualTo(null);
     assertThat(diff.newValue()).isEqualTo("miam");
 
     diff = diffs.diffs().get("acme");
     assertThat(diff.oldValue()).isEqualTo("old");
-    assertThat(diff.newValue()).isEqualTo("");
+    assertThat(diff.newValue()).isEqualTo(null);
+  }
+
+  @Test
+  public void test_parse_encoded_assignee() {
+    diffs = FieldDiffs.parse("severity=BLOCKER|INFO,assignee=oldValue|" + ENCODED_NAME_WITH_RESERVED_CHARS);
+    assertThat(diffs.diffs()).hasSize(2);
+
+    FieldDiffs.Diff diff = diffs.diffs().get("severity");
+    assertThat(diff.oldValue()).isEqualTo("BLOCKER");
+    assertThat(diff.newValue()).isEqualTo("INFO");
+
+    diff = diffs.diffs().get("assignee");
+    assertThat(diff.oldValue()).isEqualTo("oldValue");
+    assertThat(diff.newValue()).isEqualTo(NAME_WITH_RESERVED_CHARS);
   }
 
   @Test
@@ -139,12 +191,12 @@ public class FieldDiffsTest {
     assertThat(diffs.diffs()).hasSize(2);
 
     FieldDiffs.Diff diff = diffs.diffs().get("severity");
-    assertThat(diff.oldValue()).isEqualTo("");
+    assertThat(diff.oldValue()).isEqualTo(null);
     assertThat(diff.newValue()).isEqualTo("INFO");
 
     diff = diffs.diffs().get("resolution");
-    assertThat(diff.oldValue()).isEqualTo("");
-    assertThat(diff.newValue()).isEqualTo("");
+    assertThat(diff.oldValue()).isEqualTo(null);
+    assertThat(diff.newValue()).isEqualTo(null);
   }
 
   @Test