ResourceIdentifier exceptions are SafeLoggable for better, safer obse…

…rvability (#594)

ResourceIdentifier exceptions are SafeLoggable for better, safer observability

build.gradle

@@ -47,10 +47,12 @@ dependencies {
     implementation 'com.fasterxml.jackson.core:jackson-annotations'
     implementation 'com.google.errorprone:error_prone_annotations'
     implementation 'com.palantir.safe-logging:safe-logging'
+    implementation 'com.palantir.safe-logging:preconditions'
 
     testImplementation 'com.fasterxml.jackson.core:jackson-databind'
     testImplementation 'net.jqwik:jqwik'
     testImplementation 'org.junit.jupiter:junit-jupiter'
+    testImplementation 'com.palantir.safe-logging:preconditions-assertj'
 
     jmh 'org.openjdk.jmh:jmh-core'
     jmh 'org.openjdk.jmh:jmh-generator-annprocess'

changelog/@unreleased/pr-594.v2.yml

@@ -0,0 +1,5 @@
+type: improvement
+improvement:
+  description: ResourceIdentifier exceptions are SafeLoggable for better, safer observability
+  links:
+  - https://github.com/palantir/resource-identifier/pull/594

src/main/java/com/palantir/ri/ResourceIdentifier.java

@@ -20,6 +20,10 @@
 import com.fasterxml.jackson.annotation.JsonValue;
 import com.google.errorprone.annotations.Immutable;
 import com.palantir.logsafe.Safe;
+import com.palantir.logsafe.SafeArg;
+import com.palantir.logsafe.Unsafe;
+import com.palantir.logsafe.UnsafeArg;
+import com.palantir.logsafe.exceptions.SafeIllegalArgumentException;
 
 /**
  * Defines a common format for wrapping existing unique identifiers to provide additional context. This class
@@ -328,7 +332,7 @@ public static ResourceIdentifier valueOf(String rid) {
     public static ResourceIdentifier of(String rid) {
         ResourceIdentifier resultRid = tryOf(rid);
         if (resultRid == null) {
-            throw new IllegalArgumentException("Illegal resource identifier format: " + rid);
+            throw new SafeIllegalArgumentException("Illegal resource identifier format", UnsafeArg.of("rid", rid));
         }
 
         return resultRid;
@@ -432,27 +436,27 @@ private static ResourceIdentifier tryOf(String rid) {
         return new ResourceIdentifier(rid, serviceIndex, instanceIndex, typeIndex);
     }
 
-    private static void checkServiceIsValid(CharSequence service) {
+    private static void checkServiceIsValid(@Safe CharSequence service) {
         if (!isValidService(service)) {
-            throw new IllegalArgumentException("Illegal service format: " + service);
+            throw new SafeIllegalArgumentException("Illegal service format", SafeArg.of("service", service));
         }
     }
 
-    private static void checkInstanceIsValid(CharSequence instance) {
+    private static void checkInstanceIsValid(@Safe CharSequence instance) {
         if (!isValidInstance(instance)) {
-            throw new IllegalArgumentException("Illegal instance format: " + instance);
+            throw new SafeIllegalArgumentException("Illegal instance format", SafeArg.of("instance", instance));
         }
     }
 
-    private static void checkTypeIsValid(CharSequence type) {
+    private static void checkTypeIsValid(@Safe CharSequence type) {
         if (!isValidType(type)) {
-            throw new IllegalArgumentException("Illegal type format: " + type);
+            throw new SafeIllegalArgumentException("Illegal type format", SafeArg.of("type", type));
         }
     }
 
-    private static void checkLocatorIsValid(CharSequence locator) {
+    private static void checkLocatorIsValid(@Unsafe CharSequence locator) {
         if (!isValidLocator(locator)) {
-            throw new IllegalArgumentException("Illegal locator format: " + locator);
+            throw new SafeIllegalArgumentException("Illegal locator format", UnsafeArg.of("locator", locator));
         }
     }
 

src/test/java/com/palantir/ri/ResourceIdentifierTest.java

@@ -16,13 +16,15 @@
 
 package com.palantir.ri;
 
+import static com.palantir.logsafe.testing.Assertions.assertThatLoggableExceptionThrownBy;
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertFalse;
 import static org.junit.jupiter.api.Assertions.assertNotEquals;
 import static org.junit.jupiter.api.Assertions.assertTrue;
-import static org.junit.jupiter.api.Assertions.fail;
 
 import com.fasterxml.jackson.databind.ObjectMapper;
+import com.palantir.logsafe.SafeArg;
+import com.palantir.logsafe.UnsafeArg;
 import java.io.IOException;
 import java.util.ArrayList;
 import java.util.List;
@@ -103,54 +105,40 @@ void testIsValidLocator() {
 
     @Test
     void testConstructionErrorMessage() {
-        try {
-            ResourceIdentifier.of(null);
-            fail();
-        } catch (IllegalArgumentException e) {
-            assertEquals("Illegal resource identifier format: null", e.getMessage());
-        }
-        try {
-            ResourceIdentifier.of("ri.bad....dots");
-            fail();
-        } catch (IllegalArgumentException e) {
-            assertEquals("Illegal resource identifier format: ri.bad....dots", e.getMessage());
-        }
-        try {
-            ResourceIdentifier.of("123Service", "", "type", "name");
-            fail();
-        } catch (IllegalArgumentException e) {
-            assertEquals("Illegal service format: 123Service", e.getMessage());
-        }
-        try {
-            ResourceIdentifier.of("service", "Instance", "type", "name");
-            fail();
-        } catch (IllegalArgumentException e) {
-            assertEquals("Illegal instance format: Instance", e.getMessage());
-        }
-        try {
-            ResourceIdentifier.of("service", "i", "type-name", "!@#$");
-            fail();
-        } catch (IllegalArgumentException e) {
-            assertEquals("Illegal locator format: !@#$", e.getMessage());
-        }
-        try {
-            ResourceIdentifier.of(null, null, null, null);
-            fail();
-        } catch (IllegalArgumentException e) {
-            assertEquals("Illegal service format: null", e.getMessage());
-        }
-        try {
-            ResourceIdentifier.of("service", null, null, null);
-            fail();
-        } catch (IllegalArgumentException e) {
-            assertEquals("Illegal instance format: null", e.getMessage());
-        }
-        try {
-            ResourceIdentifier.of("service", "", null, null);
-            fail();
-        } catch (IllegalArgumentException e) {
-            assertEquals("Illegal type format: null", e.getMessage());
-        }
+        assertThatLoggableExceptionThrownBy(() -> ResourceIdentifier.of(null))
+                .isInstanceOf(IllegalArgumentException.class)
+                .hasLogMessage("Illegal resource identifier format")
+                .containsArgs(UnsafeArg.of("rid", null));
+
+        assertThatLoggableExceptionThrownBy(() -> ResourceIdentifier.of("ri.bad....dots"))
+                .isInstanceOf(IllegalArgumentException.class)
+                .hasLogMessage("Illegal resource identifier format")
+                .containsArgs(UnsafeArg.of("rid", "ri.bad....dots"));
+
+        assertThatLoggableExceptionThrownBy(() -> ResourceIdentifier.of("123Service", "", "type", "name"))
+                .isInstanceOf(IllegalArgumentException.class)
+                .hasLogMessage("Illegal service format")
+                .containsArgs(SafeArg.of("service", "123Service"));
+
+        assertThatLoggableExceptionThrownBy(() -> ResourceIdentifier.of("service", "i", "type-name", "!@#$"))
+                .isInstanceOf(IllegalArgumentException.class)
+                .hasLogMessage("Illegal locator format")
+                .containsArgs(UnsafeArg.of("locator", "!@#$"));
+
+        assertThatLoggableExceptionThrownBy(() -> ResourceIdentifier.of(null, null, null, null))
+                .isInstanceOf(IllegalArgumentException.class)
+                .hasLogMessage("Illegal service format")
+                .containsArgs(SafeArg.of("service", null));
+
+        assertThatLoggableExceptionThrownBy(() -> ResourceIdentifier.of("service", null, null, null))
+                .isInstanceOf(IllegalArgumentException.class)
+                .hasLogMessage("Illegal instance format")
+                .containsArgs(SafeArg.of("instance", null));
+
+        assertThatLoggableExceptionThrownBy(() -> ResourceIdentifier.of("service", "", null, null))
+                .isInstanceOf(IllegalArgumentException.class)
+                .hasLogMessage("Illegal type format")
+                .containsArgs(SafeArg.of("type", null));
     }
 
     @Test

versions.lock

@@ -1,11 +1,15 @@
 # Run ./gradlew writeVersionsLocks to regenerate this file
 com.fasterxml.jackson.core:jackson-annotations:2.17.2 (2 constraints: c9173972)
-com.google.errorprone:error_prone_annotations:2.26.1 (1 constraints: 3d05463b)
-com.palantir.safe-logging:safe-logging:3.7.0 (1 constraints: 0c050f36)
+com.google.errorprone:error_prone_annotations:2.26.1 (2 constraints: 6f163e0d)
+com.palantir.safe-logging:preconditions:3.7.0 (2 constraints: 3a19a1b5)
+com.palantir.safe-logging:safe-logging:3.7.0 (3 constraints: 3f2a5a21)
+org.jetbrains:annotations:24.1.0 (1 constraints: 331166d1)
 
 [Test dependencies]
 com.fasterxml.jackson.core:jackson-core:2.17.2 (1 constraints: 8c124321)
 com.fasterxml.jackson.core:jackson-databind:2.17.2 (1 constraints: 3e05463b)
+com.palantir.safe-logging:preconditions-assertj:3.7.0 (1 constraints: 0c050f36)
+net.bytebuddy:byte-buddy:1.14.11 (1 constraints: 7e0bc5ea)
 net.jqwik:jqwik:1.9.1 (1 constraints: 0d050c36)
 net.jqwik:jqwik-api:1.9.1 (4 constraints: 5f24254b)
 net.jqwik:jqwik-engine:1.9.1 (1 constraints: 9e07fe6c)
@@ -14,6 +18,7 @@ net.jqwik:jqwik-web:1.9.1 (1 constraints: 9e07fe6c)
 net.sf.jopt-simple:jopt-simple:5.0.4 (1 constraints: be0ad6cc)
 org.apache.commons:commons-math3:3.6.1 (1 constraints: bf0adbcc)
 org.apiguardian:apiguardian-api:1.1.2 (10 constraints: 4f819858)
+org.assertj:assertj-core:3.25.1 (1 constraints: 60147879)
 org.junit.jupiter:junit-jupiter:5.11.3 (1 constraints: 3c05473b)
 org.junit.jupiter:junit-jupiter-api:5.11.3 (3 constraints: f72f3a52)
 org.junit.jupiter:junit-jupiter-engine:5.11.3 (1 constraints: 370e034a)