[Bug #293] Add more general handling of identifier generation errors.

When an invalid unescaped character appears in a generated identifier, throw an application exception that is handled by REST handler and can be displayed to the user.
kbss-cvut · Sep 9, 2024 · e8ed3ac · e8ed3ac
1 parent cc82de9
commit e8ed3ac
Show file tree

Hide file tree

Showing 5 changed files with 47 additions and 12 deletions.
diff --git a/src/main/java/cz/cvut/kbss/termit/exception/InvalidIdentifierException.java b/src/main/java/cz/cvut/kbss/termit/exception/InvalidIdentifierException.java
@@ -2,7 +2,7 @@
 
 public class InvalidIdentifierException extends TermItException {
 
-    public InvalidIdentifierException(String message) {
-        super(message);
+    public InvalidIdentifierException(String message, String messageId) {
+        super(message, messageId);
     }
 }
diff --git a/src/main/java/cz/cvut/kbss/termit/rest/handler/RestExceptionHandler.java b/src/main/java/cz/cvut/kbss/termit/rest/handler/RestExceptionHandler.java
@@ -23,6 +23,7 @@
 import cz.cvut.kbss.termit.exception.AnnotationGenerationException;
 import cz.cvut.kbss.termit.exception.AssetRemovalException;
 import cz.cvut.kbss.termit.exception.AuthorizationException;
+import cz.cvut.kbss.termit.exception.InvalidIdentifierException;
 import cz.cvut.kbss.termit.exception.InvalidLanguageConstantException;
 import cz.cvut.kbss.termit.exception.InvalidParameterException;
 import cz.cvut.kbss.termit.exception.InvalidPasswordChangeRequestException;
@@ -271,4 +272,11 @@ public ResponseEntity<ErrorInfo> invalidPasswordChangeRequestException(HttpServl
                 ErrorInfo.createWithMessageAndMessageId(e.getMessage(), e.getMessageId(), request.getRequestURI()),
                 HttpStatus.CONFLICT);
     }
+
+    @ExceptionHandler
+    public ResponseEntity<ErrorInfo> invalidIdentifierException(HttpServletRequest request,
+                                                                InvalidIdentifierException e) {
+        logException(e, request);
+        return new ResponseEntity<>(errorInfo(request, e), HttpStatus.CONFLICT);
+    }
 }
diff --git a/src/main/java/cz/cvut/kbss/termit/service/IdentifierResolver.java b/src/main/java/cz/cvut/kbss/termit/service/IdentifierResolver.java
@@ -17,6 +17,7 @@
  */
 package cz.cvut.kbss.termit.service;
 
+import cz.cvut.kbss.termit.exception.InvalidIdentifierException;
 import cz.cvut.kbss.termit.exception.TermItException;
 import org.springframework.stereotype.Service;
 
@@ -85,7 +86,7 @@ public class IdentifierResolver {
      * <li>Transforming the value to lower case</li>
      * <li>Trimming the string</li>
      * <li>Replacing white spaces and slashes with dashes</li>
-     * <li>Removing parentheses</li>
+     * <li>Removing invalid characters such as parentheses, square brackets, dollar sign, exclamation mark, etc.</li>
      * </ul>
      * <p>
      * Based on <a href="https://gist.github.com/rponte/893494">https://gist.github.com/rponte/893494</a>
@@ -98,7 +99,7 @@ public static String normalize(String value) {
         return value.toLowerCase()
                     .trim()
                     .replaceAll("[\\s/\\\\]", Character.toString(REPLACEMENT_CHARACTER))
-                    .replaceAll("[(?&$#§),\\[\\]]", "");
+                    .replaceAll("[(?&$#§),\\[\\]@]", "");
     }
 
     /**
@@ -138,7 +139,13 @@ public URI generateIdentifier(String namespace, String... components) {
         if (!namespace.endsWith("/") && !namespace.endsWith("#")) {
             namespace += "/";
         }
-        return URI.create(namespace + normalize(comps));
+        try {
+            return URI.create(namespace + normalize(comps));
+        } catch (IllegalArgumentException e) {
+            throw new InvalidIdentifierException(
+                    "Generated identifier " + namespace + normalize(comps) + " is not a valid URI.",
+                    "error.identifier.invalidCharacters");
+        }
     }
 
     private static boolean isUri(String value) {
@@ -170,7 +177,8 @@ public URI generateDerivedIdentifier(URI baseUri, String namespaceSeparator, Str
     /**
      * Generates a synthetic identifier using the specified base URL.
      * <p>
-     * This particular implementation uses the current system time in millis to generate the locally unique part of the identifier.
+     * This particular implementation uses the current system time in millis to generate the locally unique part of the
+     * identifier.
      *
      * @param base URL base
      * @return Synthetic identifier containing the specified base
@@ -185,8 +193,8 @@ public static URI generateSyntheticIdentifier(String base) {
      * This method assumes that the fragment is a normalized string uniquely identifying a resource in the specified
      * namespace.
      * <p>
-     * Basically, the returned identifier should be the same as would be returned for non-normalized fragments by {@link
-     * #generateIdentifier(String, String...)}.
+     * Basically, the returned identifier should be the same as would be returned for non-normalized fragments by
+     * {@link #generateIdentifier(String, String...)}.
      *
      * @param namespace Identifier namespace
      * @param fragment  Normalized string unique in the specified namespace

diff --git a/src/main/java/cz/cvut/kbss/termit/websocket/handler/WebSocketExceptionHandler.java b/src/main/java/cz/cvut/kbss/termit/websocket/handler/WebSocketExceptionHandler.java
@@ -6,6 +6,7 @@
 import cz.cvut.kbss.termit.exception.AnnotationGenerationException;
 import cz.cvut.kbss.termit.exception.AssetRemovalException;
 import cz.cvut.kbss.termit.exception.AuthorizationException;
+import cz.cvut.kbss.termit.exception.InvalidIdentifierException;
 import cz.cvut.kbss.termit.exception.InvalidLanguageConstantException;
 import cz.cvut.kbss.termit.exception.InvalidParameterException;
 import cz.cvut.kbss.termit.exception.InvalidPasswordChangeRequestException;
@@ -77,6 +78,10 @@ private static ErrorInfo errorInfo(Message<?> message, Throwable e) {
         return ErrorInfo.createWithMessage(e.getMessage(), destination(message));
     }
 
+    private static ErrorInfo errorInfo(Message<?> message, TermItException e) {
+        return ErrorInfo.createWithMessageAndMessageId(e.getMessage(), e.getMessageId(), destination(message));
+    }
+
     @MessageExceptionHandler
     public void messageDeliveryException(Message<?> message, MessageDeliveryException e) {
         // messages without destination will be logged only on trace
@@ -190,7 +195,7 @@ public ErrorInfo unsupportedAssetOperationException(Message<?> message, Unsuppor
     @MessageExceptionHandler(VocabularyImportException.class)
     public ErrorInfo vocabularyImportException(Message<?> message, VocabularyImportException e) {
         logException(e, message);
-        return ErrorInfo.createWithMessageAndMessageId(e.getMessage(), e.getMessageId(), destination(message));
+        return errorInfo(message, e);
     }
 
     @MessageExceptionHandler
@@ -220,7 +225,7 @@ public ErrorInfo maxUploadSizeExceededException(Message<?> message, MaxUploadSiz
     @MessageExceptionHandler
     public ErrorInfo snapshotNotEditableException(Message<?> message, SnapshotNotEditableException e) {
         logException(e, message);
-        return ErrorInfo.createWithMessage(e.getMessage(), destination(message));
+        return errorInfo(message, e);
     }
 
     @MessageExceptionHandler
@@ -238,13 +243,19 @@ public ErrorInfo invalidLanguageConstantException(Message<?> message, InvalidLan
     @MessageExceptionHandler
     public ErrorInfo invalidTermStateException(Message<?> message, InvalidTermStateException e) {
         logException(e, message);
-        return ErrorInfo.createWithMessageAndMessageId(e.getMessage(), e.getMessageId(), destination(message));
+        return errorInfo(message, e);
     }
 
     @MessageExceptionHandler
     public ErrorInfo invalidPasswordChangeRequestException(Message<?> message,
                                                            InvalidPasswordChangeRequestException e) {
         logException(e, message);
-        return ErrorInfo.createWithMessageAndMessageId(e.getMessage(), e.getMessageId(), destination(message));
+        return errorInfo(message, e);
+    }
+
+    @MessageExceptionHandler
+    public ErrorInfo invalidIdentifierException(Message<?> message, InvalidIdentifierException e) {
+        logException(e, message);
+        return errorInfo(message, e);
     }
 }
diff --git a/src/test/java/cz/cvut/kbss/termit/service/IdentifierResolverTest.java b/src/test/java/cz/cvut/kbss/termit/service/IdentifierResolverTest.java
@@ -19,6 +19,7 @@
 
 import cz.cvut.kbss.termit.environment.Environment;
 import cz.cvut.kbss.termit.environment.Generator;
+import cz.cvut.kbss.termit.exception.InvalidIdentifierException;
 import cz.cvut.kbss.termit.util.Vocabulary;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
@@ -327,4 +328,11 @@ void generateIdentifierRemovesSquareBrackets() {
         final URI result = sut.generateIdentifier(namespace, label);
         assertEquals(URI.create(namespace + "/délka-dostřiku-m"), result);
     }
+
+    @Test
+    void generateIdentifierThrowsInvalidIdentifierExceptionWhenComponentsContainsUnforeseenInvalidCharacters() {
+        final String namespace = Vocabulary.s_c_slovnik;
+        final String label = "label with emoji \u3000"; // Ideographic space
+        assertThrows(InvalidIdentifierException.class, () -> sut.generateIdentifier(namespace, label));
+    }
 }