From: Benoit <43733395+benoit-sns@users.noreply.github.com> Date: Fri, 9 Nov 2018 15:54:30 +0000 (+0000) Subject: SONAR-10361 Handle assigning issue to a user with commas in name (#903) X-Git-Tag: 7.5~158 X-Git-Url: https://source.dussan.org/?a=commitdiff_plain;h=bdd538eb0c2b5937c565480ecab58908d095aad2;p=sonarqube.git SONAR-10361 Handle assigning issue to a user with commas in name (#903) --- diff --git a/server/sonar-db-dao/src/main/java/org/sonar/db/issue/IssueChangeDto.java b/server/sonar-db-dao/src/main/java/org/sonar/db/issue/IssueChangeDto.java index 5c8dd1aa94f..5b97fe7478d 100644 --- a/server/sonar-db-dao/src/main/java/org/sonar/db/issue/IssueChangeDto.java +++ b/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()); diff --git a/server/sonar-server/src/main/java/org/sonar/server/issue/ws/ChangelogAction.java b/server/sonar-server/src/main/java/org/sonar/server/issue/ws/ChangelogAction.java index 15098897b0c..c31d7d46e72 100644 --- a/server/sonar-server/src/main/java/org/sonar/server/issue/ws/ChangelogAction.java +++ b/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(); }; diff --git a/sonar-core/src/main/java/org/sonar/core/issue/FieldDiffs.java b/sonar-core/src/main/java/org/sonar/core/issue/FieldDiffs.java index 34e764aa298..23d568ef4cf 100644 --- a/sonar-core/src/main/java/org/sonar/core/issue/FieldDiffs.java +++ b/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 diffs = Maps.newLinkedHashMap(); public Map diffs() { + if (diffs.containsKey(ASSIGNEE)) { + Map 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 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 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 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(); } diff --git a/sonar-core/src/test/java/org/sonar/core/issue/FieldDiffsTest.java b/sonar-core/src/test/java/org/sonar/core/issue/FieldDiffsTest.java index b4bab8d0491..da6cdea3f5e 100644 --- a/sonar-core/src/test/java/org/sonar/core/issue/FieldDiffsTest.java +++ b/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