Merge pull request #1228 from modelix/bugfix/no-500-on-modelql-error

feat(model-server)!: use a consistent status code for query failures

Author: languitar

model-server-openapi/specifications/model-server-v2.yaml

@@ -324,8 +324,8 @@ paths:
       responses:
         "200":
           $ref: '#/components/responses/200json'
-        "404":
-          $ref: '#/components/responses/modelql-404'
+        "422":
+          $ref: '#/components/responses/ModelQlQueryExecutionFailed'
         default:
           $ref: '#/components/responses/GeneralError'
   /repositories/{repository}/init:
@@ -433,8 +433,8 @@ paths:
       responses:
         "200":
           $ref: '#/components/responses/200json'
-        "404":
-          $ref: '#/components/responses/modelql-404'
+        "422":
+          $ref: '#/components/responses/ModelQlQueryExecutionFailed'
         default:
           $ref: '#/components/responses/GeneralError'
 
@@ -507,8 +507,16 @@ components:
         application/problem+json:
           schema:
             $ref: 'problem.yaml#/Problem'
-    "modelql-404":
-      description: "A IProducingStep<INodeReference>.resolve() call failed to find the node."
+    "ModelQlQueryExecutionFailed":
+      description: "A syntactically valid query failed to provide a result due to a mismatch of the query and the underlying data."
+      content:
+        text/plain:
+          schema:
+            type: string
+            description: "Contains a technical error description why query execution failed."
+            example: |-
+              server version: 0.0.0
+              Exception in thread "main" java.lang.IllegalArgumentException: At least one element was expected. it<1004>.typed().flatMap { ...
     GeneralError:
       description: Unexpected error
       content:

modelql-client/src/jvmTest/kotlin/org/modelix/modelql/client/ModelQLClientTest.kt

@@ -7,6 +7,7 @@ import io.ktor.server.testing.testApplication
 import kotlinx.coroutines.withTimeout
 import org.junit.jupiter.api.assertThrows
 import org.modelix.model.api.ConceptReference
+import org.modelix.model.api.IChildLinkReference
 import org.modelix.model.api.IConceptReference
 import org.modelix.model.api.INode
 import org.modelix.model.api.NodeReference
@@ -17,6 +18,7 @@ import org.modelix.model.lazy.CLTree
 import org.modelix.model.lazy.ObjectStoreCache
 import org.modelix.model.persistent.MapBaseStore
 import org.modelix.modelql.core.IFluxUnboundQuery
+import org.modelix.modelql.core.assertNotEmpty
 import org.modelix.modelql.core.buildFluxQuery
 import org.modelix.modelql.core.contains
 import org.modelix.modelql.core.count
@@ -262,8 +264,8 @@ class ModelQLClientTest {
             it.memoize { n ->
                 n.count()
                     .sum(
-                        n.allChildren().fold(0) { acc, it ->
-                            acc.sum(it.mapRecursive())
+                        n.allChildren().fold(0) { acc, child ->
+                            acc.sum(child.mapRecursive())
                         },
                     )
             }
@@ -272,13 +274,35 @@ class ModelQLClientTest {
     }
 
     @Test
-    fun `resolving a non-existing node reference returns 404`() = runTest { httpClient ->
+    fun `resolving a non-existing node reference returns 422`() = runTest { httpClient ->
         val client = ModelQLClient("http://localhost/query", httpClient)
         val ex = assertThrows<ModelQueryRequestException> {
             client.query<INode> {
                 NodeReference("doesnotexist").asMono().resolve()
             }
         }
-        assertEquals(HttpStatusCode.NotFound, ex.httpResponse.status)
+        assertEquals(HttpStatusCode.UnprocessableEntity, ex.httpResponse.status)
+    }
+
+    @Test
+    fun `failed assertions in queries return 422`() = runTest { httpClient ->
+        val client = ModelQLClient("http://localhost/query", httpClient)
+        val ex = assertThrows<ModelQueryRequestException> {
+            client.query<INode> {
+                it.children(IChildLinkReference.fromName("test")).first().assertNotEmpty()
+            }
+        }
+        assertEquals(HttpStatusCode.UnprocessableEntity, ex.httpResponse.status)
+    }
+
+    @Test
+    fun `empty query results return 422`() = runTest { httpClient ->
+        val client = ModelQLClient("http://localhost/query", httpClient)
+        val ex = assertThrows<ModelQueryRequestException> {
+            client.query<INode> {
+                it.children(IChildLinkReference.fromName("test")).first()
+            }
+        }
+        assertEquals(HttpStatusCode.UnprocessableEntity, ex.httpResponse.status)
     }
 }

modelql-core/src/commonMain/kotlin/org/modelix/modelql/core/Query.kt

@@ -62,13 +62,15 @@ private abstract class BoundQuery<In, out AggregationOut, out ElementOut>(val ex
     }
 }
 
+class EmptyQueryResultException(message: String, cause: Throwable? = null) : RuntimeException(message, cause)
+
 private class MonoBoundQuery<In, Out>(executor: IQueryExecutor<In>, override val query: MonoUnboundQuery<In, Out>) : BoundQuery<In, Out, Out>(executor), IMonoQuery<Out> {
 
     override suspend fun execute(): IStepOutput<Out> {
         try {
             return executor.createStream(query).exactlyOne().getSuspending()
         } catch (ex: NoSuchElementException) {
-            throw RuntimeException("Empty query result: " + this, ex)
+            throw EmptyQueryResultException("Empty query result: $this", ex)
         }
     }
 
@@ -170,7 +172,7 @@ class MonoUnboundQuery<In, ElementOut>(
         try {
             return asStream(evaluationContext, input.asObservable()).exactlyOne().getSynchronous()
         } catch (ex: NoSuchElementException) {
-            throw RuntimeException("Empty query result: " + this, ex)
+            throw EmptyQueryResultException("Empty query result: $this", ex)
         }
     }
 

modelql-server/src/main/kotlin/org/modelix/modelql/server/ModelQLServer.kt

@@ -13,6 +13,7 @@ import kotlinx.serialization.KSerializer
 import org.modelix.model.api.INode
 import org.modelix.model.api.UnresolvableNodeReferenceException
 import org.modelix.model.area.IArea
+import org.modelix.modelql.core.EmptyQueryResultException
 import org.modelix.modelql.core.IMonoUnboundQuery
 import org.modelix.modelql.core.IStepOutput
 import org.modelix.modelql.core.MODELIX_VERSION
@@ -22,6 +23,7 @@ import org.modelix.modelql.core.VersionAndData
 import org.modelix.modelql.core.upcast
 import org.modelix.modelql.untyped.UntypedModelQL
 import org.modelix.modelql.untyped.createQueryExecutor
+import org.modelix.streams.StreamAssertionError
 import kotlin.system.measureTimeMillis
 
 class ModelQLServer private constructor(val rootNodeProvider: () -> INode?, val area: IArea? = null) {
@@ -101,17 +103,19 @@ class ModelQLServer private constructor(val rootNodeProvider: () -> INode?, val
                 val serializedResult = json.encodeToString(VersionAndData.serializer(serializer), versionAndResult)
                 afterQueryExecution()
                 call.respondText(text = serializedResult, contentType = ContentType.Application.Json)
-            } catch (ex: UnresolvableNodeReferenceException) {
-                afterQueryExecution()
-                call.respondText(
-                    text = "server version: $MODELIX_VERSION\n" + ex.stackTraceToString(),
-                    status = HttpStatusCode.NotFound,
-                )
-            } catch (ex: Throwable) {
+            } catch (@Suppress("TooGenericExceptionCaught") ex: Exception) {
                 afterQueryExecution()
+                val statusCode = when (ex) {
+                    is UnresolvableNodeReferenceException,
+                    is StreamAssertionError,
+                    is EmptyQueryResultException,
+                    -> HttpStatusCode.UnprocessableEntity
+
+                    else -> HttpStatusCode.InternalServerError
+                }
                 call.respondText(
                     text = "server version: $MODELIX_VERSION\n" + ex.stackTraceToString(),
-                    status = HttpStatusCode.InternalServerError,
+                    status = statusCode,
                 )
             }
         }

streams/src/commonMain/kotlin/org/modelix/streams/StreamExtensions.kt

@@ -34,6 +34,8 @@ import com.badoo.reaktive.single.map
 import com.badoo.reaktive.single.subscribe
 import kotlinx.coroutines.flow.single
 
+class StreamAssertionError(message: String) : IllegalArgumentException(message)
+
 fun Observable<*>.count(): Single<Int> = collect({ arrayOf(0) }) { acc, it -> acc[0]++ }.map { it[0] }
 fun <T, R> Observable<T>.fold(initial: R, operation: (R, T) -> R): Single<R> {
     return collect({ mutableListOf(initial) }) { acc, it -> acc[0] = operation(acc[0], it) }.map { it[0] }
@@ -51,13 +53,13 @@ fun <T> Observable<T>.withIndex(): Observable<IndexedValue<T>> {
     return map { IndexedValue(index++, it) }
 }
 fun <T> Observable<T>.assertNotEmpty(message: () -> String): Observable<T> {
-    return this.switchIfEmpty { throw IllegalArgumentException("At least one element was expected. " + message()) }
+    return this.switchIfEmpty { throw StreamAssertionError("At least one element was expected. xxx " + message()) }
 }
 fun <T> Maybe<T>.assertEmpty(message: (T) -> String): Completable {
-    return map { throw IllegalArgumentException(message(it)) }.asCompletable()
+    return map { throw StreamAssertionError(message(it)) }.asCompletable()
 }
 fun <T> Observable<T>.assertEmpty(message: (T) -> String): Completable {
-    return map { throw IllegalArgumentException(message(it)) }.asCompletable()
+    return map { throw StreamAssertionError(message(it)) }.asCompletable()
 }
 fun <T> Single<T>.cached(): Single<T> {
     return this.asObservable().replay(1).autoConnect()