From e5aaebe1745a2477a7f0a295e9a21f0e4a86a4f0 Mon Sep 17 00:00:00 2001 From: Ivan Pizhenko Date: Sun, 21 Feb 2021 15:11:09 +0200 Subject: [PATCH 1/2] issue #91 Improve class UserId --- .../java/org/pgpainless/key/util/UserId.java | 198 ++++++++++++------ .../java/org/pgpainless/key/UserIdTest.java | 161 ++++++++++++-- ...WithGenericCertificationSignatureTest.java | 2 +- .../selection/userid/SelectUserIdTest.java | 3 +- 4 files changed, 280 insertions(+), 84 deletions(-) diff --git a/pgpainless-core/src/main/java/org/pgpainless/key/util/UserId.java b/pgpainless-core/src/main/java/org/pgpainless/key/util/UserId.java index c72ef1f8..ea19941a 100644 --- a/pgpainless-core/src/main/java/org/pgpainless/key/util/UserId.java +++ b/pgpainless-core/src/main/java/org/pgpainless/key/util/UserId.java @@ -1,5 +1,5 @@ /* - * Copyright 2020 Paul Schaub. + * Copyright 2020 Paul Schaub. Copyright 2021 Flowcrypt a.s. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -13,13 +13,67 @@ * See the License for the specific language governing permissions and * limitations under the License. */ + package org.pgpainless.key.util; +// @SuppressWarnings("unused") public final class UserId implements CharSequence { + public static final class Builder { + private String name; + private String comment; + private String email; + + private Builder() { + } + + private Builder(String name, String comment, String email) { + this.name = name; + this.comment = comment; + this.email = email; + } + + public Builder withName(String name) { + checkNotNull("name", name); + this.name = name; + return this; + } + + public Builder withComment(String comment) { + checkNotNull("comment", comment); + this.comment = comment; + return this; + } + + public Builder withEmail(String email) { + checkNotNull("email", email); + this.email = email; + return this; + } + + public Builder noName() { + name = null; + return this; + } + + public Builder noComment() { + comment = null; + return this; + } + + public Builder noEmail() { + email = null; + return this; + } + + public UserId build() { + return new UserId(name, comment, email); + } + } private final String name; private final String comment; private final String email; + private Integer hash; private UserId(String name, String comment, String email) { this.name = name; @@ -27,85 +81,36 @@ public final class UserId implements CharSequence { this.email = email; } - @Override - public String toString() { - StringBuilder sb = new StringBuilder(); - if (name != null) { - sb.append(name); - } - if (comment != null) { - sb.append(" (").append(comment).append(')'); - } - if (email != null) { - sb.append(sb.length() != 0 ? ' ' : "").append(email); - } - return sb.toString(); - } - public static UserId onlyEmail(String email) { - if (email == null) { - throw new IllegalArgumentException("Email must not be null."); - } + checkNotNull("email", email); return new UserId(null, null, email); } public static UserId nameAndEmail(String name, String email) { - return withName(name).noComment().withEmail(email); + checkNotNull("name", name); + checkNotNull("email", email); + return new UserId(name, null, email); } - public static WithComment withName(String name) { - if (name == null) { - throw new IllegalArgumentException("Name must not be null."); - } - return new WithComment(name); + public static Builder newBuilder() { + return new Builder(); } - public static class WithComment { - - private final String name; - - public WithComment(String name) { - this.name = name; - } - - public WithEmail withComment(String comment) { - if (comment == null) { - throw new IllegalArgumentException("Comment must not be null."); - } - return new WithEmail(name, comment); - } - - public WithEmail noComment() { - return new WithEmail(name, null); - } - - public UserId build() { - return new UserId(name, null, null); - } + public Builder toBuilder() { + return new Builder(name, comment, email); } - public static class WithEmail { - - private final String name; - private final String comment; - - public WithEmail(String name, String comment) { - this.name = name; - this.comment = comment; - } - - public UserId withEmail(String email) { - if (email == null) { - throw new IllegalArgumentException("Email must not be null."); - } - return new UserId(name, comment, email.matches("^<.+>$") ? email : '<' + email + '>'); - } - - public UserId noEmail() { - return new UserId(name, comment, null); - } + public String getName() { + return name; } + public String getComment() { + return comment; + } + + public String getEmail() { + return email; + } @Override public int length() { @@ -122,4 +127,63 @@ public final class UserId implements CharSequence { return toString().subSequence(i, i1); } + @Override + public String toString() { + return asString(false); + } + + public String asString(boolean ignoreEmptyValues) { + StringBuilder sb = new StringBuilder(); + if (name != null && (!ignoreEmptyValues || !name.isEmpty())) { + sb.append(name); + } + if (comment != null && (!ignoreEmptyValues || !comment.isEmpty())) { + sb.append(" (").append(comment).append(')'); + } + if (email != null && (!ignoreEmptyValues || !email.isEmpty())) { + final boolean moreThanJustEmail = sb.length() > 0; + if (moreThanJustEmail) sb.append(" <"); + sb.append(email); + if (moreThanJustEmail) sb.append('>'); + } + return sb.toString(); + } + + @Override + public boolean equals(Object o) { + if (o == null) return false; + if (o == this) return true; + if (!(o instanceof UserId)) return false; + final UserId other = (UserId) o; + return isEqualComponent(name, other.name, false) && isEqualComponent(comment, other.comment, false) + && isEqualComponent(email, other.email, true); + } + + @Override + public int hashCode() { + if (hash != null) { + return hash; + } else { + int hash = 7; + hash = 31 * hash + (name == null ? 0 : name.hashCode()); + hash = 31 * hash + (comment == null ? 0 : comment.hashCode()); + hash = 31 * hash + (email == null ? 0 : email.toLowerCase().hashCode()); + this.hash = hash; + return hash; + } + } + + private static boolean isEqualComponent(String value, String otherValue, boolean ignoreCase) { + final boolean valueIsNull = (value == null); + final boolean otherValueIsNull = (otherValue == null); + return (valueIsNull && otherValueIsNull) + || (!valueIsNull && !otherValueIsNull + && (ignoreCase ? value.equalsIgnoreCase(otherValue) : value.equals(otherValue))); + } + + private static void checkNotNull(String paramName, String value) { + if (value == null) { + throw new IllegalArgumentException(paramName + " must be not null"); + } + } } diff --git a/pgpainless-core/src/test/java/org/pgpainless/key/UserIdTest.java b/pgpainless-core/src/test/java/org/pgpainless/key/UserIdTest.java index 2841a8d5..2530222e 100644 --- a/pgpainless-core/src/test/java/org/pgpainless/key/UserIdTest.java +++ b/pgpainless-core/src/test/java/org/pgpainless/key/UserIdTest.java @@ -1,5 +1,5 @@ /* - * Copyright 2020 Paul Schaub. + * Copyright 2020 Paul Schaub. Copyright 2021 Flowcrypt a.s. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -15,37 +15,36 @@ */ package org.pgpainless.key; -import static org.junit.jupiter.api.Assertions.assertEquals; -import static org.junit.jupiter.api.Assertions.assertThrows; - import org.junit.jupiter.api.Test; import org.pgpainless.key.util.UserId; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotEquals; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertThrows; + public class UserIdTest { @Test public void throwForNullName() { - assertThrows(IllegalArgumentException.class, () -> UserId.withName(null)); + assertThrows(IllegalArgumentException.class, () -> UserId.newBuilder().withName(null)); } @Test public void throwForNullComment() { - assertThrows(IllegalArgumentException.class, () -> UserId.withName("foo") - .withComment(null)); + assertThrows(IllegalArgumentException.class, () -> UserId.newBuilder().withComment(null)); } @Test public void throwForNullEmail() { - assertThrows(IllegalArgumentException.class, () -> UserId.withName("foo") - .withComment("bar") - .withEmail(null)); + assertThrows(IllegalArgumentException.class, () -> UserId.newBuilder().withEmail(null)); } @Test public void testFormatOnlyName() { assertEquals( "Juliet Capulet", - UserId.withName("Juliet Capulet") + UserId.newBuilder().withName("Juliet Capulet") .build().toString()); } @@ -53,26 +52,28 @@ public class UserIdTest { public void testFormatNameAndComment() { assertEquals( "Juliet Capulet (from the play)", - UserId.withName("Juliet Capulet") + UserId.newBuilder().withName("Juliet Capulet") .withComment("from the play") - .noEmail().toString()); + .noEmail().build().toString()); } @Test public void testFormatNameCommentAndMail() { assertEquals("Juliet Capulet (from the play) ", - UserId.withName("Juliet Capulet") + UserId.newBuilder().withName("Juliet Capulet") .withComment("from the play") .withEmail("juliet@capulet.lit") + .build() .toString()); } @Test public void testFormatNameAndEmail() { assertEquals("Juliet Capulet ", - UserId.withName("Juliet Capulet") + UserId.newBuilder().withName("Juliet Capulet") .noComment() .withEmail("juliet@capulet.lit") + .build() .toString()); } @@ -86,4 +87,134 @@ public class UserIdTest { UserId userId = UserId.nameAndEmail("Maurice Moss", "moss.m@reynholm.co.uk"); assertEquals("Maurice Moss ", userId.toString()); } + + @Test + void testBuilderWithName() { + final UserId userId = UserId.newBuilder().withName("John Smith").build(); + assertEquals("John Smith", userId.getName()); + assertNull(userId.getComment()); + assertNull(userId.getEmail()); + } + + @Test + void testBuilderWithComment() { + final UserId userId = UserId.newBuilder().withComment("Sales Dept.").build(); + assertNull(userId.getName()); + assertEquals("Sales Dept.", userId.getComment()); + assertNull(userId.getEmail()); + } + + @Test + void testBuilderWithEmail() { + final UserId userId = UserId.newBuilder().withEmail("john.smith@example.com").build(); + assertNull(userId.getName()); + assertNull(userId.getComment()); + assertEquals("john.smith@example.com", userId.getEmail()); + } + + @Test + void testBuilderWithAll() { + final UserId userId = UserId.newBuilder().withEmail("john.smith@example.com") + .withName("John Smith") + .withEmail("john.smith@example.com") + .withComment("Sales Dept.").build(); + assertEquals("John Smith", userId.getName()); + assertEquals("Sales Dept.", userId.getComment()); + assertEquals("john.smith@example.com", userId.getEmail()); + } + + @Test + void testBuilderNoName() { + final UserId.Builder builder = UserId.newBuilder() + .withEmail("john.smith@example.com") + .withName("John Smith") + .withComment("Sales Dept.").build().toBuilder(); + final UserId userId = builder.noName().build(); + assertNull(userId.getName()); + assertEquals("Sales Dept.", userId.getComment()); + assertEquals("john.smith@example.com", userId.getEmail()); + } + + @Test + void testBuilderNoComment() { + final UserId.Builder builder = UserId.newBuilder() + .withEmail("john.smith@example.com") + .withName("John Smith") + .withComment("Sales Dept.").build().toBuilder(); + final UserId userId = builder.noComment().build(); + assertEquals("John Smith", userId.getName()); + assertNull(userId.getComment()); + assertEquals("john.smith@example.com", userId.getEmail()); + } + + @Test + void testBuilderNoEmail() { + final UserId.Builder builder = UserId.newBuilder() + .withEmail("john.smith@example.com") + .withName("John Smith") + .withComment("Sales Dept.").build().toBuilder(); + final UserId userId = builder.noEmail().build(); + assertEquals("John Smith", userId.getName()); + assertEquals("Sales Dept.", userId.getComment()); + assertNull(userId.getEmail()); + } + + @Test + void testEmailOnlyFormatting() { + final UserId userId = UserId.onlyEmail("john.smith@example.com"); + assertEquals("john.smith@example.com", userId.toString()); + } + + @Test + void testEmptyNameAndValidEmailFormatting() { + final UserId userId = UserId.nameAndEmail("", "john.smith@example.com"); + assertEquals("john.smith@example.com", userId.toString()); + assertEquals("john.smith@example.com", userId.asString(false)); + assertEquals("john.smith@example.com", userId.asString(true)); + } + + @Test + void testEmptyNameAndEmptyCommentAndValidEmailFormatting() { + final UserId userId = UserId.newBuilder() + .withComment("") + .withName("") + .withEmail("john.smith@example.com") + .build(); + assertEquals(" () ", userId.toString()); + assertEquals(" () ", userId.asString(false)); + assertEquals("john.smith@example.com", userId.asString(true)); + } + + @Test + void testEqualsWithDifferentCaseEmails() { + final String name = "John Smith"; + final String comment = "Sales Dept."; + final String email = "john.smith@example.com"; + final String upperEmail = email.toUpperCase(); + final UserId userId1 = UserId.newBuilder().withComment(comment).withName(name).withEmail(email).build(); + final UserId userId2 = UserId.newBuilder().withComment(comment).withName(name).withEmail(upperEmail).build(); + assertEquals(userId1, userId2); + } + + @Test + void testNotEqualWithDifferentNames() { + final String name1 = "John Smith"; + final String name2 = "Don Duck"; + final String comment = "Sales Dept."; + final String email = "john.smith@example.com"; + final UserId userId1 = UserId.newBuilder().withComment(comment).withName(name1).withEmail(email).build(); + final UserId userId2 = UserId.newBuilder().withComment(comment).withName(name2).withEmail(email).build(); + assertNotEquals(userId1, userId2); + } + + @Test + void testNotEqualWithDifferentComments() { + final String name = "John Smith"; + final String comment1 = "Sales Dept."; + final String comment2 = "Legal Dept."; + final String email = "john.smith@example.com"; + final UserId userId1 = UserId.newBuilder().withComment(comment1).withName(name).withEmail(email).build(); + final UserId userId2 = UserId.newBuilder().withComment(comment2).withName(name).withEmail(email).build(); + assertNotEquals(userId1, userId2); + } } diff --git a/pgpainless-core/src/test/java/org/pgpainless/key/modification/RevokeKeyWithGenericCertificationSignatureTest.java b/pgpainless-core/src/test/java/org/pgpainless/key/modification/RevokeKeyWithGenericCertificationSignatureTest.java index 850b957a..5ab4ed16 100644 --- a/pgpainless-core/src/test/java/org/pgpainless/key/modification/RevokeKeyWithGenericCertificationSignatureTest.java +++ b/pgpainless-core/src/test/java/org/pgpainless/key/modification/RevokeKeyWithGenericCertificationSignatureTest.java @@ -1,5 +1,5 @@ /* - * Copyright 2021 Ivan Pizhenko, Paul Schaub. + * Copyright 2021 Paul Schaub. Copyright 2021 Flowcrypt a.s. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. diff --git a/pgpainless-core/src/test/java/org/pgpainless/util/selection/userid/SelectUserIdTest.java b/pgpainless-core/src/test/java/org/pgpainless/util/selection/userid/SelectUserIdTest.java index ce5770ea..8abaeceb 100644 --- a/pgpainless-core/src/test/java/org/pgpainless/util/selection/userid/SelectUserIdTest.java +++ b/pgpainless-core/src/test/java/org/pgpainless/util/selection/userid/SelectUserIdTest.java @@ -39,7 +39,8 @@ public class SelectUserIdTest { .simpleEcKeyRing(""); secretKeys = PGPainless.modifyKeyRing(secretKeys) .addUserId( - UserId.withName("Alice Liddell").noComment().withEmail("crazy@the-rabbit.hole"), + UserId.newBuilder().withName("Alice Liddell").noComment() + .withEmail("crazy@the-rabbit.hole").build(), SecretKeyRingProtector.unprotectedKeys()) .done(); From 57f74400392ebf8e88fc813d9d9f781b7cac362a Mon Sep 17 00:00:00 2001 From: Ivan Pizhenko Date: Sun, 21 Feb 2021 16:18:42 +0200 Subject: [PATCH 2/2] Code review --- .../main/java/org/pgpainless/key/util/UserId.java | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/pgpainless-core/src/main/java/org/pgpainless/key/util/UserId.java b/pgpainless-core/src/main/java/org/pgpainless/key/util/UserId.java index ea19941a..dc232b8c 100644 --- a/pgpainless-core/src/main/java/org/pgpainless/key/util/UserId.java +++ b/pgpainless-core/src/main/java/org/pgpainless/key/util/UserId.java @@ -16,7 +16,6 @@ package org.pgpainless.key.util; -// @SuppressWarnings("unused") public final class UserId implements CharSequence { public static final class Builder { private String name; @@ -73,7 +72,7 @@ public final class UserId implements CharSequence { private final String name; private final String comment; private final String email; - private Integer hash; + private long hash = Long.MAX_VALUE; private UserId(String name, String comment, String email) { this.name = name; @@ -132,6 +131,11 @@ public final class UserId implements CharSequence { return asString(false); } + /** + * Returns a string representation of the object. + * @param ignoreEmptyValues Flag which indicates that empty string values should not be outputted. + * @return a string representation of the object. + */ public String asString(boolean ignoreEmptyValues) { StringBuilder sb = new StringBuilder(); if (name != null && (!ignoreEmptyValues || !name.isEmpty())) { @@ -155,14 +159,15 @@ public final class UserId implements CharSequence { if (o == this) return true; if (!(o instanceof UserId)) return false; final UserId other = (UserId) o; - return isEqualComponent(name, other.name, false) && isEqualComponent(comment, other.comment, false) + return isEqualComponent(name, other.name, false) + && isEqualComponent(comment, other.comment, false) && isEqualComponent(email, other.email, true); } @Override public int hashCode() { - if (hash != null) { - return hash; + if (hash != Long.MAX_VALUE) { + return (int) hash; } else { int hash = 7; hash = 31 * hash + (name == null ? 0 : name.hashCode());