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

languitar · languitar · commit ff653fd4dc18 · 2024-12-09T11:00:21.000+01:00
When a modelql query fails due to a mismatch between what's expected by
the query and the underlying data, always returns the same status code.
Before, most cases resulted in a 500 internal server, which in turn
triggered the default retry mechanism of the modelql client. This leads
to unnecessary amounts of queries on the server and slows down error
reports on the client side.

I have chosen 422 here. 5xx status codes are reserved for actual
application failures instead of business logic errors. 404 would be too
specific. 422 looks like it covers all kinds of errors related to being
unable to "process" an otherwise valid query successfully.

BREAKING CHANGE: resolving a non-existing node via ModelQL now returns
    status code 422 instead of 404.
diff --git a/model-server-openapi/specifications/model-server-v2.yaml b/model-server-openapi/specifications/model-server-v2.yaml
@@ -322,8 +322,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:
@@ -429,8 +429,8 @@ paths:
       responses:
         "200":
           $ref: '#/components/responses/200json'
-        "404":
-          $ref: '#/components/responses/modelql-404'
+        "422":
+          $ref: '#/components/responses/ModelQlQueryExecutionFailed'
         default:
           $ref: '#/components/responses/GeneralError'
 
@@ -503,8 +503,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:
diff --git a/modelql-client/src/jvmTest/kotlin/org/modelix/modelql/client/ModelQLClientTest.kt b/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)
     }
 }
diff --git a/modelql-core/src/commonMain/kotlin/org/modelix/modelql/core/Query.kt b/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)
         }
     }
 
diff --git a/modelql-server/src/main/kotlin/org/modelix/modelql/server/ModelQLServer.kt b/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,
                 )
             }
         }
diff --git a/streams/src/commonMain/kotlin/org/modelix/streams/StreamExtensions.kt b/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()

Original file line number	Diff line number	Diff line change
`@@ -62,13 +62,15 @@ private abstract class BoundQuery<In, out AggregationOut, out ElementOut>(val ex`
`62`	`62`	`}`
`63`	`63`	`}`
`64`	`64`
	`65`	`+class EmptyQueryResultException(message: String, cause: Throwable? = null) : RuntimeException(message, cause)`
	`66`	`+`
`65`	`67`	`private class MonoBoundQuery<In, Out>(executor: IQueryExecutor<In>, override val query: MonoUnboundQuery<In, Out>) : BoundQuery<In, Out, Out>(executor), IMonoQuery<Out> {`
`66`	`68`
`67`	`69`	`override suspend fun execute(): IStepOutput<Out> {`
`68`	`70`	`try {`
`69`	`71`	`return executor.createStream(query).exactlyOne().getSuspending()`
`70`	`72`	`} catch (ex: NoSuchElementException) {`
`71`		`- throw RuntimeException("Empty query result: " + this, ex)`
	`73`	`+ throw EmptyQueryResultException("Empty query result: $this", ex)`
`72`	`74`	`}`
`73`	`75`	`}`
`74`	`76`
`@@ -170,7 +172,7 @@ class MonoUnboundQuery<In, ElementOut>(`
`170`	`172`	`try {`
`171`	`173`	`return asStream(evaluationContext, input.asObservable()).exactlyOne().getSynchronous()`
`172`	`174`	`} catch (ex: NoSuchElementException) {`
`173`		`- throw RuntimeException("Empty query result: " + this, ex)`
	`175`	`+ throw EmptyQueryResultException("Empty query result: $this", ex)`
`174`	`176`	`}`
`175`	`177`	`}`
`176`	`178`
Original file line number	Diff line number	Diff line change
`@@ -34,6 +34,8 @@ import com.badoo.reaktive.single.map`
`34`	`34`	`import com.badoo.reaktive.single.subscribe`
`35`	`35`	`import kotlinx.coroutines.flow.single`
`36`	`36`
	`37`	`+class StreamAssertionError(message: String) : IllegalArgumentException(message)`
	`38`	`+`
`37`	`39`	`fun Observable<*>.count(): Single<Int> = collect({ arrayOf(0) }) { acc, it -> acc[0]++ }.map { it[0] }`
`38`	`40`	`fun <T, R> Observable<T>.fold(initial: R, operation: (R, T) -> R): Single<R> {`
`39`	`41`	`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>> {`
`51`	`53`	`return map { IndexedValue(index++, it) }`
`52`	`54`	`}`
`53`	`55`	`fun <T> Observable<T>.assertNotEmpty(message: () -> String): Observable<T> {`
`54`		`- return this.switchIfEmpty { throw IllegalArgumentException("At least one element was expected. " + message()) }`
	`56`	`+ return this.switchIfEmpty { throw StreamAssertionError("At least one element was expected. xxx " + message()) }`
`55`	`57`	`}`
`56`	`58`	`fun <T> Maybe<T>.assertEmpty(message: (T) -> String): Completable {`
`57`		`- return map { throw IllegalArgumentException(message(it)) }.asCompletable()`
	`59`	`+ return map { throw StreamAssertionError(message(it)) }.asCompletable()`
`58`	`60`	`}`
`59`	`61`	`fun <T> Observable<T>.assertEmpty(message: (T) -> String): Completable {`
`60`		`- return map { throw IllegalArgumentException(message(it)) }.asCompletable()`
	`62`	`+ return map { throw StreamAssertionError(message(it)) }.asCompletable()`
`61`	`63`	`}`
`62`	`64`	`fun <T> Single<T>.cached(): Single<T> {`
`63`	`65`	`return this.asObservable().replay(1).autoConnect()`