rest: Report warnings when parsing invalid queries

arteymix · arteymix · commit f3aafa77a61f · 2024-09-20T14:51:05.000-07:00
Fix the structure of error in a WellComposedErrorBody to comply with the
Google JSON style-guide.

Add location and locationType to help identify what the error (or
warning) is relevant to.
diff --git a/gemma-core/src/main/java/ubic/gemma/core/search/lucene/LuceneQueryUtils.java b/gemma-core/src/main/java/ubic/gemma/core/search/lucene/LuceneQueryUtils.java
@@ -24,6 +24,7 @@
 import java.util.HashSet;
 import java.util.LinkedHashSet;
 import java.util.Set;
+import java.util.function.Consumer;
 import java.util.regex.Pattern;
 
 import static java.util.Objects.requireNonNull;
@@ -68,19 +69,27 @@ private static QueryParser createQueryParser() {
      * escaped if necessary.
      */
     public static Query parseSafely( SearchSettings settings, QueryParser queryParser ) throws SearchException {
-        return parseSafely( settings.getQuery(), queryParser );
+        return parseSafely( settings.getQuery(), queryParser, settings.getIssueReporter() );
     }
 
     /**
      * Safely parse the given search query into a Lucene query, falling back on a query with special characters
      * escaped if necessary.
+     * @param report a consumer for potential {@link ParseException} when attempting to parse the query, ignored if null
      */
-    public static Query parseSafely( String query, QueryParser queryParser ) throws SearchException {
+    public static Query parseSafely( String query, QueryParser queryParser, @Nullable Consumer<? super ParseException> report ) throws SearchException {
         try {
             return queryParser.parse( query );
         } catch ( ParseException e ) {
             String strippedQuery = escape( query );
-            log.debug( String.format( "Failed to parse '%s': %s.", query, ExceptionUtils.getRootCauseMessage( e ) ), e );
+            String m = String.format( "Failed to parse '%s': %s, it will be reattempted stripped from Lucene special characters as '%s'.",
+                    query, ExceptionUtils.getRootCauseMessage( e ), strippedQuery );
+            if ( report != null ) {
+                log.debug( m, e ); // no need to warn if it is consumed
+                report.accept( e );
+            } else {
+                log.warn( m, e );
+            }
             try {
                 return queryParser.parse( strippedQuery );
             } catch ( ParseException e2 ) {
@@ -241,7 +250,7 @@ public static String prepareDatabaseQuery( SearchSettings settings, boolean allo
 
     @Nullable
     public static String prepareDatabaseQuery( String query, boolean allowWildcards ) throws SearchException {
-        return prepareDatabaseQueryInternal( parseSafely( query, createQueryParser() ), allowWildcards );
+        return prepareDatabaseQueryInternal( parseSafely( query, createQueryParser(), null ), allowWildcards );
     }
 
     @Nullable
@@ -274,7 +283,7 @@ private static String prepareDatabaseQueryInternal( Query query, boolean allowWi
 
     @Nullable
     public static URI prepareTermUriQuery( String s ) throws SearchException {
-        Query query = parseSafely( s, createQueryParser() );
+        Query query = parseSafely( s, createQueryParser(), null );
         if ( query instanceof TermQuery ) {
             Term term = ( ( TermQuery ) query ).getTerm();
             return tryParseUri( term );
diff --git a/gemma-core/src/main/java/ubic/gemma/model/common/search/SearchSettings.java b/gemma-core/src/main/java/ubic/gemma/model/common/search/SearchSettings.java
@@ -39,6 +39,7 @@
 import java.io.Serializable;
 import java.util.Map;
 import java.util.Set;
+import java.util.function.Consumer;
 import java.util.stream.Collectors;
 
 /**
@@ -207,6 +208,14 @@ public static SearchSettings geneSearch( String query, @Nullable Taxon taxon ) {
     @Nullable
     private transient Highlighter highlighter;
 
+    /**
+     * A consumer that accepts various query issues.
+     * <p>
+     * This is meant to report non-critical issues.
+     */
+    @Nullable
+    private transient Consumer<Throwable> issueReporter;
+
     /**
      * Check if this is configured to search a given result type.
      */
diff --git a/gemma-rest/src/main/java/ubic/gemma/rest/DatasetsWebService.java b/gemma-rest/src/main/java/ubic/gemma/rest/DatasetsWebService.java
diff --git a/gemma-rest/src/main/java/ubic/gemma/rest/providers/AbstractExceptionMapper.java b/gemma-rest/src/main/java/ubic/gemma/rest/providers/AbstractExceptionMapper.java
@@ -8,13 +8,15 @@
 import ubic.gemma.core.util.BuildInfo;
 import ubic.gemma.rest.util.BuildInfoValueObject;
 import ubic.gemma.rest.util.ResponseErrorObject;
+import ubic.gemma.rest.util.WellComposedError;
 import ubic.gemma.rest.util.WellComposedErrorBody;
 
 import javax.ws.rs.container.ResourceContext;
 import javax.ws.rs.core.Context;
 import javax.ws.rs.core.MediaType;
 import javax.ws.rs.core.Response;
 import javax.ws.rs.ext.ExceptionMapper;
+import java.util.List;
 
 public abstract class AbstractExceptionMapper<E extends Throwable> implements ExceptionMapper<E> {
 
@@ -43,7 +45,7 @@ protected AbstractExceptionMapper( String hostUrl, OpenAPI spec, BuildInfo build
      */
     protected WellComposedErrorBody getWellComposedErrorBody( E exception ) {
         // for security reasons, we don't include the error object in the response entity
-        return new WellComposedErrorBody( getStatus( exception ), exception.getMessage() );
+        return new WellComposedErrorBody( getStatus( exception ).getStatusCode(), exception.getMessage() );
     }
 
     /**
@@ -91,23 +93,28 @@ public final Response toResponse( E exception ) {
             version = null;
         }
         Response.ResponseBuilder responseBuilder = getResponseBuilder( request, exception );
+        WellComposedErrorBody body = getWellComposedErrorBody( exception );
         if ( request == null || acceptsJson( request ) ) {
             return responseBuilder
                     .type( MediaType.APPLICATION_JSON_TYPE )
-                    .entity( new ResponseErrorObject( getWellComposedErrorBody( exception ), version, new BuildInfoValueObject( buildInfo ) ) )
+                    .entity( new ResponseErrorObject( version, new BuildInfoValueObject( buildInfo ), body ) )
                     .build();
         } else {
-            WellComposedErrorBody body = getWellComposedErrorBody( exception );
             StringBuilder builder = new StringBuilder();
             builder.append( "Request method: " ).append( requestMethod != null ? requestMethod : "?" ).append( '\n' );
             builder.append( "Request URI: " ).append( requestUri != null ? requestUri : "?" ).append( '\n' );
             builder.append( "Version: " ).append( version != null ? version : "?" ).append( '\n' );
             builder.append( "Build info: " ).append( buildInfo ).append( '\n' );
             builder.append( "Message: " ).append( body.getMessage() );
             if ( body.getErrors() != null ) {
-                body.getErrors().forEach( ( k, v ) -> {
-                    builder.append( '\n' ).append( k ).append( ": " ).append( v );
-                } );
+                List<WellComposedError> errors = body.getErrors();
+                for ( int i = 0; i < errors.size(); i++ ) {
+                    WellComposedError e = errors.get( i );
+                    builder.append( '\n' ).append( '\n' )
+                            .append( "Error #" ).append( i + 1 ).append( '\n' )
+                            .append( "Reason: " ).append( e.getReason() ).append( '\n' )
+                            .append( "Message: " ).append( e.getMessage() );
+                }
             }
             return responseBuilder
                     .type( MediaType.TEXT_PLAIN_TYPE )
diff --git a/gemma-rest/src/main/java/ubic/gemma/rest/providers/MalformedArgExceptionMapper.java b/gemma-rest/src/main/java/ubic/gemma/rest/providers/MalformedArgExceptionMapper.java
@@ -1,10 +1,13 @@
 package ubic.gemma.rest.providers;
 
 import io.swagger.v3.oas.models.OpenAPI;
+import org.apache.commons.lang3.exception.ExceptionUtils;
 import org.springframework.beans.factory.annotation.Autowired;
 import org.springframework.beans.factory.annotation.Value;
 import org.springframework.stereotype.Component;
 import ubic.gemma.core.util.BuildInfo;
+import ubic.gemma.persistence.util.EntityUtils;
+import ubic.gemma.rest.util.LocationType;
 import ubic.gemma.rest.util.MalformedArgException;
 import ubic.gemma.rest.util.WellComposedErrorBody;
 
@@ -27,9 +30,10 @@ protected Response.Status getStatus( MalformedArgException exception ) {
 
     @Override
     protected WellComposedErrorBody getWellComposedErrorBody( MalformedArgException exception ) {
-        WellComposedErrorBody body = new WellComposedErrorBody( Response.Status.BAD_REQUEST, exception.getMessage() );
+        WellComposedErrorBody body = new WellComposedErrorBody( Response.Status.BAD_REQUEST.getStatusCode(), exception.getMessage() );
         if ( exception.getCause() != null ) {
-            WellComposedErrorBody.addExceptionFields( body, exception.getCause() );
+            // TODO: report the exact request parameter that caused this
+            body.addError( ExceptionUtils.getRootCause( exception ), null, null );
         }
         return body;
     }
diff --git a/gemma-rest/src/main/java/ubic/gemma/rest/providers/NotFoundExceptionMapper.java b/gemma-rest/src/main/java/ubic/gemma/rest/providers/NotFoundExceptionMapper.java
@@ -1,6 +1,7 @@
 package ubic.gemma.rest.providers;
 
 import io.swagger.v3.oas.models.OpenAPI;
+import org.apache.commons.lang3.exception.ExceptionUtils;
 import org.springframework.beans.factory.annotation.Autowired;
 import org.springframework.beans.factory.annotation.Value;
 import org.springframework.stereotype.Component;
@@ -37,9 +38,9 @@ protected Response.Status getStatus( NotFoundException exception ) {
 
     @Override
     protected WellComposedErrorBody getWellComposedErrorBody( NotFoundException exception ) {
-        WellComposedErrorBody errorBody = new WellComposedErrorBody( Response.Status.NOT_FOUND, exception.getMessage() );
+        WellComposedErrorBody errorBody = new WellComposedErrorBody( Response.Status.NOT_FOUND.getStatusCode(), exception.getMessage() );
         if ( exception.getCause() != null ) {
-            WellComposedErrorBody.addExceptionFields( errorBody, exception.getCause() );
+            errorBody.addError( ExceptionUtils.getRootCause( exception ), null, null );
         }
         return errorBody;
     }
diff --git a/gemma-rest/src/main/java/ubic/gemma/rest/security/RestAuthEntryPoint.java b/gemma-rest/src/main/java/ubic/gemma/rest/security/RestAuthEntryPoint.java
@@ -66,8 +66,8 @@ public void commence( final HttpServletRequest request, final HttpServletRespons
             realm = "Gemma RESTful API";
             version = null;
         }
-        WellComposedErrorBody errorBody = new WellComposedErrorBody( Response.Status.UNAUTHORIZED, MESSAGE_401 );
-        ResponseErrorObject errorObject = new ResponseErrorObject( errorBody, version, new BuildInfoValueObject( buildInfo ) );
+        WellComposedErrorBody errorBody = new WellComposedErrorBody( Response.Status.UNAUTHORIZED.getStatusCode(), MESSAGE_401 );
+        ResponseErrorObject errorObject = new ResponseErrorObject( version, new BuildInfoValueObject( buildInfo ), errorBody );
         response.setContentType( MediaType.APPLICATION_JSON );
         // using 'xBasic' instead of 'basic' to prevent default browser login popup
         response.addHeader( "WWW-Authenticate", "xBasic realm=" + realm );
diff --git a/gemma-rest/src/main/java/ubic/gemma/rest/util/LocationType.java b/gemma-rest/src/main/java/ubic/gemma/rest/util/LocationType.java
@@ -0,0 +1,7 @@
+package ubic.gemma.rest.util;
+
+public enum LocationType {
+    HEADERS,
+    QUERY,
+    BODY
+}
diff --git a/gemma-rest/src/main/java/ubic/gemma/rest/util/ResponseDataObject.java b/gemma-rest/src/main/java/ubic/gemma/rest/util/ResponseDataObject.java
@@ -14,11 +14,16 @@
  */
 package ubic.gemma.rest.util;
 
+import com.fasterxml.jackson.annotation.JsonInclude;
 import lombok.Getter;
 
+import java.util.ArrayList;
+import java.util.LinkedHashSet;
+import java.util.List;
+
 /**
  * Wrapper for a non-error response payload compliant with the
- * <a href="https://google.github.io/styleguide/jsoncstyleguide.xml?showone=error#error">Google JSON style-guide</a>
+ * <a href="https://google.github.io/styleguide/jsoncstyleguide.xml?#data">Google JSON style-guide</a>
  *
  * @author tesarst
  */
@@ -27,10 +32,31 @@ public class ResponseDataObject<T> {
 
     private final T data;
 
+    /**
+     * A list of warnings applicable to the request.
+     * <p>
+     * This is an extension to the Google JSON style-guide.
+     */
+    @JsonInclude(JsonInclude.Include.NON_EMPTY)
+    private final List<WellComposedWarning> warnings = new ArrayList<>();
+
     /**
      * @param payload the data to be serialised and returned as the response payload.
      */
     public ResponseDataObject( T payload ) {
         this.data = payload;
     }
+
+    /**
+     * Add a bunch of warnings to {@link #getWarnings()}.
+     */
+    public <S extends ResponseDataObject<T>> S addWarnings( Iterable<Throwable> throwables, String location, LocationType locationType ) {
+        LinkedHashSet<WellComposedWarning> distinctWarnings = new LinkedHashSet<>();
+        for ( Throwable throwable : throwables ) {
+            distinctWarnings.add( new WellComposedWarning( throwable.getClass().getName(), throwable.getMessage(), location, locationType ) );
+        }
+        warnings.addAll( distinctWarnings );
+        //noinspection unchecked
+        return ( S ) this;
+    }
 }
diff --git a/gemma-rest/src/main/java/ubic/gemma/rest/util/ResponseErrorObject.java b/gemma-rest/src/main/java/ubic/gemma/rest/util/ResponseErrorObject.java
@@ -17,15 +17,27 @@
 import lombok.Value;
 
 /**
- * Wrapper for an error response payload compliant with the
- * <a href="https://google.github.io/styleguide/jsoncstyleguide.xml?showone=error#error">Google JSON style-guide</a>
+ * Wrapper for an error response payload compliant with the <a href="https://google.github.io/styleguide/jsoncstyleguide.xml#error">Google JSON style-guide</a>.
  *
  * @author tesarst
  */
 @Value
 public class ResponseErrorObject {
 
-    WellComposedErrorBody error;
+    /**
+     * API version.
+     */
     String apiVersion;
+
+    /**
+     * Build information.
+     * <p>
+     * This is an extension to the Google JSON style-guide.
+     */
     BuildInfoValueObject buildInfo;
+
+    /**
+     * Error beign reported.
+     */
+    WellComposedErrorBody error;
 }
diff --git a/gemma-rest/src/main/java/ubic/gemma/rest/util/WellComposedError.java b/gemma-rest/src/main/java/ubic/gemma/rest/util/WellComposedError.java
@@ -0,0 +1,22 @@
+package ubic.gemma.rest.util;
+
+import com.fasterxml.jackson.annotation.JsonInclude;
+import lombok.Value;
+
+import javax.annotation.Nullable;
+
+@Value
+public class WellComposedError {
+
+    String reason;
+
+    String message;
+
+    @Nullable
+    @JsonInclude(JsonInclude.Include.NON_NULL)
+    String location;
+
+    @Nullable
+    @JsonInclude(JsonInclude.Include.NON_NULL)
+    LocationType locationType;
+}
diff --git a/gemma-rest/src/main/java/ubic/gemma/rest/util/WellComposedErrorBody.java b/gemma-rest/src/main/java/ubic/gemma/rest/util/WellComposedErrorBody.java
diff --git a/gemma-rest/src/main/java/ubic/gemma/rest/util/WellComposedWarning.java b/gemma-rest/src/main/java/ubic/gemma/rest/util/WellComposedWarning.java
diff --git a/gemma-rest/src/main/java/ubic/gemma/rest/util/args/DatasetArgService.java b/gemma-rest/src/main/java/ubic/gemma/rest/util/args/DatasetArgService.java
diff --git a/gemma-rest/src/main/resources/restapidocs/CHANGELOG.md b/gemma-rest/src/main/resources/restapidocs/CHANGELOG.md
