opendev/gerrit
Code Issues Proposed changes

ChangeField: Include change ID into error logs when parsing reviewer fields

Browse Source 
Without the change ID one doesn't know for which change the reviewer
fields have unexpected values.

Change-Id: I3003acc4cd0f54c13fce4be67d6e092c1dd67412
Signed-off-by: Edwin Kempin <ekempin@google.com>
This commit is contained in:
Edwin Kempin
2018-02-12 09:22:57 +01:00
parent 6414a9d934
commit 655b00855d
4 changed files with 39 additions and 13 deletions
Download Patch File Download Diff File Expand all files Collapse all files

4
java/com/google/gerrit/elasticsearch/ElasticChangeIndex.java
View File

@@ -290,6 +290,7 @@ class ElasticChangeIndex extends AbstractElasticIndex<Change.Id, ChangeData>
if (source.get(ChangeField.REVIEWER.getName()) != null) { if (source.get(ChangeField.REVIEWER.getName()) != null) {
cd.setReviewers( cd.setReviewers(
ChangeField.parseReviewerFieldValues( ChangeField.parseReviewerFieldValues(
cd.getId(),
FluentIterable.from(source.get(ChangeField.REVIEWER.getName()).getAsJsonArray()) FluentIterable.from(source.get(ChangeField.REVIEWER.getName()).getAsJsonArray())
.transform(JsonElement::getAsString))); .transform(JsonElement::getAsString)));
} else if (fields.contains(ChangeField.REVIEWER.getName())) { } else if (fields.contains(ChangeField.REVIEWER.getName())) {
@@ -300,6 +301,7 @@ class ElasticChangeIndex extends AbstractElasticIndex<Change.Id, ChangeData>
if (source.get(ChangeField.REVIEWER_BY_EMAIL.getName()) != null) { if (source.get(ChangeField.REVIEWER_BY_EMAIL.getName()) != null) {
cd.setReviewersByEmail( cd.setReviewersByEmail(
ChangeField.parseReviewerByEmailFieldValues( ChangeField.parseReviewerByEmailFieldValues(
cd.getId(),
FluentIterable.from( FluentIterable.from(
source.get(ChangeField.REVIEWER_BY_EMAIL.getName()).getAsJsonArray()) source.get(ChangeField.REVIEWER_BY_EMAIL.getName()).getAsJsonArray())
.transform(JsonElement::getAsString))); .transform(JsonElement::getAsString)));
@@ -311,6 +313,7 @@ class ElasticChangeIndex extends AbstractElasticIndex<Change.Id, ChangeData>
if (source.get(ChangeField.PENDING_REVIEWER.getName()) != null) { if (source.get(ChangeField.PENDING_REVIEWER.getName()) != null) {
cd.setPendingReviewers( cd.setPendingReviewers(
ChangeField.parseReviewerFieldValues( ChangeField.parseReviewerFieldValues(
cd.getId(),
FluentIterable.from( FluentIterable.from(
source.get(ChangeField.PENDING_REVIEWER.getName()).getAsJsonArray()) source.get(ChangeField.PENDING_REVIEWER.getName()).getAsJsonArray())
.transform(JsonElement::getAsString))); .transform(JsonElement::getAsString)));
@@ -322,6 +325,7 @@ class ElasticChangeIndex extends AbstractElasticIndex<Change.Id, ChangeData>
if (source.get(ChangeField.PENDING_REVIEWER_BY_EMAIL.getName()) != null) { if (source.get(ChangeField.PENDING_REVIEWER_BY_EMAIL.getName()) != null) {
cd.setPendingReviewersByEmail( cd.setPendingReviewersByEmail(
ChangeField.parseReviewerByEmailFieldValues( ChangeField.parseReviewerByEmailFieldValues(
cd.getId(),
FluentIterable.from( FluentIterable.from(
source.get(ChangeField.PENDING_REVIEWER_BY_EMAIL.getName()).getAsJsonArray()) source.get(ChangeField.PENDING_REVIEWER_BY_EMAIL.getName()).getAsJsonArray())
.transform(JsonElement::getAsString))); .transform(JsonElement::getAsString)));

4
java/com/google/gerrit/lucene/LuceneChangeIndex.java
View File

@@ -586,12 +586,14 @@ public class LuceneChangeIndex implements ChangeIndex {
private void decodeReviewers(ListMultimap<String, IndexableField> doc, ChangeData cd) { private void decodeReviewers(ListMultimap<String, IndexableField> doc, ChangeData cd) {
cd.setReviewers( cd.setReviewers(
ChangeField.parseReviewerFieldValues( ChangeField.parseReviewerFieldValues(
cd.getId(),
FluentIterable.from(doc.get(REVIEWER_FIELD)).transform(IndexableField::stringValue))); FluentIterable.from(doc.get(REVIEWER_FIELD)).transform(IndexableField::stringValue)));
} }
private void decodeReviewersByEmail(ListMultimap<String, IndexableField> doc, ChangeData cd) { private void decodeReviewersByEmail(ListMultimap<String, IndexableField> doc, ChangeData cd) {
cd.setReviewersByEmail( cd.setReviewersByEmail(
ChangeField.parseReviewerByEmailFieldValues( ChangeField.parseReviewerByEmailFieldValues(
cd.getId(),
FluentIterable.from(doc.get(REVIEWER_BY_EMAIL_FIELD)) FluentIterable.from(doc.get(REVIEWER_BY_EMAIL_FIELD))
.transform(IndexableField::stringValue))); .transform(IndexableField::stringValue)));
} }
@@ -599,6 +601,7 @@ public class LuceneChangeIndex implements ChangeIndex {
private void decodePendingReviewers(ListMultimap<String, IndexableField> doc, ChangeData cd) { private void decodePendingReviewers(ListMultimap<String, IndexableField> doc, ChangeData cd) {
cd.setPendingReviewers( cd.setPendingReviewers(
ChangeField.parseReviewerFieldValues( ChangeField.parseReviewerFieldValues(
cd.getId(),
FluentIterable.from(doc.get(PENDING_REVIEWER_FIELD)) FluentIterable.from(doc.get(PENDING_REVIEWER_FIELD))
.transform(IndexableField::stringValue))); .transform(IndexableField::stringValue)));
} }
@@ -607,6 +610,7 @@ public class LuceneChangeIndex implements ChangeIndex {
ListMultimap<String, IndexableField> doc, ChangeData cd) { ListMultimap<String, IndexableField> doc, ChangeData cd) {
cd.setPendingReviewersByEmail( cd.setPendingReviewersByEmail(
ChangeField.parseReviewerByEmailFieldValues( ChangeField.parseReviewerByEmailFieldValues(
cd.getId(),
FluentIterable.from(doc.get(PENDING_REVIEWER_BY_EMAIL_FIELD)) FluentIterable.from(doc.get(PENDING_REVIEWER_BY_EMAIL_FIELD))
.transform(IndexableField::stringValue))); .transform(IndexableField::stringValue)));
} }

41
java/com/google/gerrit/server/index/change/ChangeField.java
View File

@@ -256,39 +256,44 @@ public class ChangeField {
return state.toString() + ',' + adr; return state.toString() + ',' + adr;
} }
public static ReviewerSet parseReviewerFieldValues(Iterable<String> values) { public static ReviewerSet parseReviewerFieldValues(Change.Id changeId, Iterable<String> values) {
ImmutableTable.Builder<ReviewerStateInternal, Account.Id, Timestamp> b = ImmutableTable.Builder<ReviewerStateInternal, Account.Id, Timestamp> b =
ImmutableTable.builder(); ImmutableTable.builder();
for (String v : values) { for (String v : values) {
int i = v.indexOf(','); int i = v.indexOf(',');
if (i < 0) { if (i < 0) {
log.error("Invalid value for reviewer field: {}", v); log.error("Invalid value for reviewer field from change {}: {}", changeId.get(), v);
continue; continue;
} }
int i2 = v.lastIndexOf(','); int i2 = v.lastIndexOf(',');
if (i2 == i) { if (i2 == i) {
log.error("Invalid value for reviewer field: {}", v); log.error("Invalid value for reviewer field from change {}: {}", changeId.get(), v);
continue; continue;
} }
com.google.common.base.Optional<ReviewerStateInternal> reviewerState = com.google.common.base.Optional<ReviewerStateInternal> reviewerState =
Enums.getIfPresent(ReviewerStateInternal.class, v.substring(0, i)); Enums.getIfPresent(ReviewerStateInternal.class, v.substring(0, i));
if (!reviewerState.isPresent()) { if (!reviewerState.isPresent()) {
log.error("Failed to parse reviewer state from reviewer field: {}", v); log.error(
"Failed to parse reviewer state of reviewer field from change {}: {}",
changeId.get(),
v);
continue; continue;
} }
Optional<Account.Id> accountId = Account.Id.tryParse(v.substring(i + 1, i2)); Optional<Account.Id> accountId = Account.Id.tryParse(v.substring(i + 1, i2));
if (!accountId.isPresent()) { if (!accountId.isPresent()) {
log.error("Failed to parse account ID from reviewer field: {}", v); log.error(
"Failed to parse account ID of reviewer field from change {}: {}", changeId.get(), v);
continue; continue;
} }
Long l = Longs.tryParse(v.substring(i2 + 1, v.length())); Long l = Longs.tryParse(v.substring(i2 + 1, v.length()));
if (l == null) { if (l == null) {
log.error("Failed to parse timestamp from reviewer field: {}", v); log.error(
"Failed to parse timestamp of reviewer field from change {}: {}", changeId.get(), v);
continue; continue;
} }
Timestamp timestamp = new Timestamp(l); Timestamp timestamp = new Timestamp(l);
@@ -298,37 +303,49 @@ public class ChangeField {
return ReviewerSet.fromTable(b.build()); return ReviewerSet.fromTable(b.build());
} }
public static ReviewerByEmailSet parseReviewerByEmailFieldValues(Iterable<String> values) { public static ReviewerByEmailSet parseReviewerByEmailFieldValues(
Change.Id changeId, Iterable<String> values) {
ImmutableTable.Builder<ReviewerStateInternal, Address, Timestamp> b = ImmutableTable.builder(); ImmutableTable.Builder<ReviewerStateInternal, Address, Timestamp> b = ImmutableTable.builder();
for (String v : values) { for (String v : values) {
int i = v.indexOf(','); int i = v.indexOf(',');
if (i < 0) { if (i < 0) {
log.error("Invalid value for reviewer by email field: {}", v); log.error(
"Invalid value for reviewer by email field from change {}: {}", changeId.get(), v);
continue; continue;
} }
int i2 = v.lastIndexOf(','); int i2 = v.lastIndexOf(',');
if (i2 == i) { if (i2 == i) {
log.error("Invalid value for reviewer by email field: {}", v); log.error(
"Invalid value for reviewer by email field from change {}: {}", changeId.get(), v);
continue; continue;
} }
com.google.common.base.Optional<ReviewerStateInternal> reviewerState = com.google.common.base.Optional<ReviewerStateInternal> reviewerState =
Enums.getIfPresent(ReviewerStateInternal.class, v.substring(0, i)); Enums.getIfPresent(ReviewerStateInternal.class, v.substring(0, i));
if (!reviewerState.isPresent()) { if (!reviewerState.isPresent()) {
log.error("Failed to parse reviewer state from reviewer by email field: {}", v); log.error(
"Failed to parse reviewer state of reviewer by email field from change {}: {}",
changeId.get(),
v);
continue; continue;
} }
Address address = Address.tryParse(v.substring(i + 1, i2)); Address address = Address.tryParse(v.substring(i + 1, i2));
if (address == null) { if (address == null) {
log.error("Failed to parse address from reviewer by email field: {}", v); log.error(
"Failed to parse address of reviewer by email field from change {}: {}",
changeId.get(),
v);
continue; continue;
} }
Long l = Longs.tryParse(v.substring(i2 + 1, v.length())); Long l = Longs.tryParse(v.substring(i2 + 1, v.length()));
if (l == null) { if (l == null) {
log.error("Failed to parse timestamp from reviewer by email field: {}", v); log.error(
"Failed to parse timestamp of reviewer by email field from change {}: {}",
changeId.get(),
v);
continue; continue;
} }
Timestamp timestamp = new Timestamp(l); Timestamp timestamp = new Timestamp(l);

3
javatests/com/google/gerrit/server/index/change/ChangeFieldTest.java
View File

@@ -24,6 +24,7 @@ import com.google.common.collect.Table;
import com.google.gerrit.common.TimeUtil; import com.google.gerrit.common.TimeUtil;
import com.google.gerrit.common.data.SubmitRecord; import com.google.gerrit.common.data.SubmitRecord;
import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.server.ReviewerSet; import com.google.gerrit.server.ReviewerSet;
import com.google.gerrit.server.notedb.ReviewerStateInternal; import com.google.gerrit.server.notedb.ReviewerStateInternal;
import com.google.gerrit.testing.GerritBaseTests; import com.google.gerrit.testing.GerritBaseTests;
@@ -60,7 +61,7 @@ public class ChangeFieldTest extends GerritBaseTests {
.containsExactly( .containsExactly(
"REVIEWER,1", "REVIEWER,1," + t1.getTime(), "CC,2", "CC,2," + t2.getTime()); "REVIEWER,1", "REVIEWER,1," + t1.getTime(), "CC,2", "CC,2," + t2.getTime());
assertThat(ChangeField.parseReviewerFieldValues(values)).isEqualTo(reviewers); assertThat(ChangeField.parseReviewerFieldValues(new Change.Id(1), values)).isEqualTo(reviewers);
} }
@Test @Test