From b7734da498c7fcb526013d6dd17b327aa97c097b Mon Sep 17 00:00:00 2001 From: slisson Date: Sun, 8 Dec 2024 13:08:28 +0100 Subject: [PATCH 1/2] fix(modelql)!: disable implicit queries to avoid accidental performance issues You should fix the unintended implicit queries or use .query explicitly. You can also set the environment variable MODELQL_IMPLICIT_QUERIES_ENABLED=true to re-enable this feature. --- .../org/modelix/modelql/client/ModelQLNode.kt | 34 ++++++++++++------- .../modelix/modelql/client/ModelQLNode.js.kt | 20 +++++++++++ .../modelix/modelql/client/ModelQLNode.jvm.kt | 19 +++++++++++ .../modelql/client/ModelQLClientTest.kt | 22 ++++++++++++ 4 files changed, 83 insertions(+), 12 deletions(-) create mode 100644 modelql-client/src/jsMain/kotlin/org/modelix/modelql/client/ModelQLNode.js.kt create mode 100644 modelql-client/src/jvmMain/kotlin/org/modelix/modelql/client/ModelQLNode.jvm.kt diff --git a/modelql-client/src/commonMain/kotlin/org/modelix/modelql/client/ModelQLNode.kt b/modelql-client/src/commonMain/kotlin/org/modelix/modelql/client/ModelQLNode.kt index e44c890aff..87a78487c1 100644 --- a/modelql-client/src/commonMain/kotlin/org/modelix/modelql/client/ModelQLNode.kt +++ b/modelql-client/src/commonMain/kotlin/org/modelix/modelql/client/ModelQLNode.kt @@ -65,6 +65,8 @@ import org.modelix.modelql.untyped.roleInParent import org.modelix.modelql.untyped.setProperty import org.modelix.modelql.untyped.setReference +expect val implicitQueriesEnabled: Boolean + abstract class ModelQLNode(val client: ModelQLClient) : INode, ISupportsModelQL, IQueryExecutor { override fun usesRoleIds(): Boolean = true @@ -93,6 +95,14 @@ abstract class ModelQLNode(val client: ModelQLClient) : INode, ISupportsModelQL, }.flatMapIterable { it } } + private fun implicitQuery(body: (IMonoStep) -> IMonoStep): R { + if (implicitQueriesEnabled) { + return blockingQuery(body) + } else { + throw UnsupportedOperationException("Implicit queries are disabled. Use .query instead.") + } + } + fun blockingQuery(body: (IMonoStep) -> IMonoStep): R { val client = client return client.blockingQuery { root -> @@ -112,13 +122,13 @@ abstract class ModelQLNode(val client: ModelQLClient) : INode, ISupportsModelQL, get() = true override val roleInParent: String? - get() = blockingQuery { it.roleInParent() } + get() = implicitQuery { it.roleInParent() } override val parent: INode? - get() = blockingQuery { it.parent().orNull() } + get() = implicitQuery { it.parent().orNull() } override fun getChildren(role: IChildLink): Iterable { - return blockingQuery { it.children(role.key()).toList() } + return implicitQuery { it.children(role.key()).toList() } } override fun getChildren(role: String?): Iterable { @@ -126,10 +136,10 @@ abstract class ModelQLNode(val client: ModelQLClient) : INode, ISupportsModelQL, } override val allChildren: Iterable - get() = blockingQuery { it.allChildren().toList() } + get() = implicitQuery { it.allChildren().toList() } override fun moveChild(role: IChildLink, index: Int, child: INode) { - blockingQuery { it.moveChild(role, index, child.reference.asMono().resolve()) } + implicitQuery { it.moveChild(role, index, child.reference.asMono().resolve()) } } override fun moveChild(role: String?, index: Int, child: INode) { @@ -137,7 +147,7 @@ abstract class ModelQLNode(val client: ModelQLClient) : INode, ISupportsModelQL, } override fun addNewChild(role: IChildLink, index: Int, concept: IConceptReference?): INode { - return blockingQuery { it.addNewChild(role, index, concept as ConceptReference?).first() } + return implicitQuery { it.addNewChild(role, index, concept as ConceptReference?).first() } } override fun addNewChild(role: IChildLink, index: Int, concept: IConcept?): INode { @@ -153,15 +163,15 @@ abstract class ModelQLNode(val client: ModelQLClient) : INode, ISupportsModelQL, } override fun removeChild(child: INode) { - blockingQuery { child.reference.asMono().resolve().remove() } + implicitQuery { child.reference.asMono().resolve().remove() } } override fun getReferenceTarget(role: String): INode? { - return blockingQuery { it.reference(role).filterNotNull().nodeRefAndConcept().orNull() }?.toNode() + return implicitQuery { it.reference(role).filterNotNull().nodeRefAndConcept().orNull() }?.toNode() } override fun getReferenceTargetRef(role: String): INodeReference? { - return blockingQuery { it.reference(role).nodeReference().orNull() } + return implicitQuery { it.reference(role).nodeReference().orNull() } } override fun setReferenceTarget(link: IReferenceLink, target: INode?) { @@ -169,7 +179,7 @@ abstract class ModelQLNode(val client: ModelQLClient) : INode, ISupportsModelQL, } override fun setReferenceTarget(role: IReferenceLink, target: INodeReference?) { - blockingQuery { it.setReference(role, target.asMono().mapIfNotNull { it.resolve() }) } + implicitQuery { it.setReference(role, target.asMono().mapIfNotNull { it.resolve() }) } } override fun setReferenceTarget(role: String, target: INode?) { @@ -177,11 +187,11 @@ abstract class ModelQLNode(val client: ModelQLClient) : INode, ISupportsModelQL, } override fun getPropertyValue(role: String): String? { - return blockingQuery { it.property(role) } + return implicitQuery { it.property(role) } } override fun setPropertyValue(property: IProperty, value: String?) { - blockingQuery { it.setProperty(property, value.asMono()) } + implicitQuery { it.setProperty(property, value.asMono()) } } override fun setPropertyValue(role: String, value: String?) { diff --git a/modelql-client/src/jsMain/kotlin/org/modelix/modelql/client/ModelQLNode.js.kt b/modelql-client/src/jsMain/kotlin/org/modelix/modelql/client/ModelQLNode.js.kt new file mode 100644 index 0000000000..e0686896d3 --- /dev/null +++ b/modelql-client/src/jsMain/kotlin/org/modelix/modelql/client/ModelQLNode.js.kt @@ -0,0 +1,20 @@ +/* + * Copyright (c) 2024. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.modelix.modelql.client + +actual val implicitQueriesEnabled: Boolean + get() = false \ No newline at end of file diff --git a/modelql-client/src/jvmMain/kotlin/org/modelix/modelql/client/ModelQLNode.jvm.kt b/modelql-client/src/jvmMain/kotlin/org/modelix/modelql/client/ModelQLNode.jvm.kt new file mode 100644 index 0000000000..4c09473ce1 --- /dev/null +++ b/modelql-client/src/jvmMain/kotlin/org/modelix/modelql/client/ModelQLNode.jvm.kt @@ -0,0 +1,19 @@ +/* + * Copyright (c) 2024. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.modelix.modelql.client + +actual val implicitQueriesEnabled: Boolean = System.getenv("MODELQL_IMPLICIT_QUERIES_ENABLED")?.toBooleanStrictOrNull() ?: false 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 index a6e809b415..9f840d9da0 100644 --- a/modelql-client/src/jvmTest/kotlin/org/modelix/modelql/client/ModelQLClientTest.kt +++ b/modelql-client/src/jvmTest/kotlin/org/modelix/modelql/client/ModelQLClientTest.kt @@ -20,8 +20,10 @@ 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.IPropertyReference import org.modelix.model.api.NodeReference import org.modelix.model.api.PBranch import org.modelix.model.api.getRootNode @@ -55,6 +57,7 @@ import org.modelix.modelql.untyped.children import org.modelix.modelql.untyped.descendants import org.modelix.modelql.untyped.nodeReference import org.modelix.modelql.untyped.property +import org.modelix.modelql.untyped.query import org.modelix.modelql.untyped.remove import org.modelix.modelql.untyped.resolve import org.modelix.modelql.untyped.setProperty @@ -294,4 +297,23 @@ class ModelQLClientTest { } assertEquals(HttpStatusCode.NotFound, ex.httpResponse.status) } + + @Test + fun `implicit queries are not allowed anymore`() = runTest { httpClient -> + // it's too easy to use them accidentally inside mapLocal blocks + + val client = ModelQLClient("http://localhost/query", httpClient) + val firstModule = client.query { it.children(IChildLinkReference.fromName("modules")).first() } + assertThrows { + firstModule.getPropertyValue(IPropertyReference.fromName("name").toLegacy()) + } + } + + @Test + fun `explicit queries can be used instead of implicit ones`() = runTest { httpClient -> + val client = ModelQLClient("http://localhost/query", httpClient) + val firstModule = client.query { it.children(IChildLinkReference.fromName("modules")).first() } + val name = firstModule.query { it.property(IPropertyReference.fromName("name")) } + assertEquals("abc", name) + } } From b6cd91a98d045550dddef30a954566b104691c45 Mon Sep 17 00:00:00 2001 From: slisson Date: Sun, 8 Dec 2024 13:13:55 +0100 Subject: [PATCH 2/2] fix(modelql)!: disable implicit queries to avoid accidental performance issues You should fix the unintended implicit queries or use .query explicitly. You can also set the environment variable MODELQL_IMPLICIT_QUERIES_ENABLED=true to re-enable this feature. --- .../kotlin/org/modelix/modelql/typed/TypedModelQLTest.kt | 9 +++++---- .../kotlin/org/modelix/modelql/client/ModelQLNode.js.kt | 2 +- .../org/modelix/modelql/client/ModelQLClientTest.kt | 5 ++++- 3 files changed, 10 insertions(+), 6 deletions(-) diff --git a/model-api-gen-gradle-test/kotlin-generation/src/test/kotlin/org/modelix/modelql/typed/TypedModelQLTest.kt b/model-api-gen-gradle-test/kotlin-generation/src/test/kotlin/org/modelix/modelql/typed/TypedModelQLTest.kt index 194b28a3d7..396d9c4572 100644 --- a/model-api-gen-gradle-test/kotlin-generation/src/test/kotlin/org/modelix/modelql/typed/TypedModelQLTest.kt +++ b/model-api-gen-gradle-test/kotlin-generation/src/test/kotlin/org/modelix/modelql/typed/TypedModelQLTest.kt @@ -227,7 +227,7 @@ class TypedModelQLTest { .untyped() .toList() }.map { it.typed() } - assertEquals("plus", result[0].name) + assertEquals("plus", result[0].query { it.name }) } @Test @@ -239,7 +239,7 @@ class TypedModelQLTest { .filter { it.visibility.instanceOf(C_PublicVisibility) } .toList() } - assertEquals("plus", result[0].name) + assertEquals("plus", result[0].query { it.name }) } @Test @@ -257,8 +257,9 @@ class TypedModelQLTest { .member .ofConcept(C_StaticMethodDeclaration) .first() + .name } - assertEquals(expected, actual.name) + assertEquals(expected, actual) } @Test @@ -297,7 +298,7 @@ class TypedModelQLTest { .first() } assertNotEquals(expected, oldValue) - assertEquals(expected, actual.variableDeclaration.name) + assertEquals(expected, actual.query { it.variableDeclaration.name }) } @Test diff --git a/modelql-client/src/jsMain/kotlin/org/modelix/modelql/client/ModelQLNode.js.kt b/modelql-client/src/jsMain/kotlin/org/modelix/modelql/client/ModelQLNode.js.kt index e0686896d3..5186b0f6aa 100644 --- a/modelql-client/src/jsMain/kotlin/org/modelix/modelql/client/ModelQLNode.js.kt +++ b/modelql-client/src/jsMain/kotlin/org/modelix/modelql/client/ModelQLNode.js.kt @@ -17,4 +17,4 @@ package org.modelix.modelql.client actual val implicitQueriesEnabled: Boolean - get() = false \ No newline at end of file + get() = false 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 index 9f840d9da0..924c195c49 100644 --- a/modelql-client/src/jvmTest/kotlin/org/modelix/modelql/client/ModelQLClientTest.kt +++ b/modelql-client/src/jvmTest/kotlin/org/modelix/modelql/client/ModelQLClientTest.kt @@ -24,6 +24,7 @@ import org.modelix.model.api.IChildLinkReference import org.modelix.model.api.IConceptReference import org.modelix.model.api.INode import org.modelix.model.api.IPropertyReference +import org.modelix.model.api.IReferenceLinkReference import org.modelix.model.api.NodeReference import org.modelix.model.api.PBranch import org.modelix.model.api.getRootNode @@ -43,6 +44,7 @@ import org.modelix.modelql.core.fold import org.modelix.modelql.core.map import org.modelix.modelql.core.memoize import org.modelix.modelql.core.notEqualTo +import org.modelix.modelql.core.orNull import org.modelix.modelql.core.plus import org.modelix.modelql.core.sum import org.modelix.modelql.core.toList @@ -58,6 +60,7 @@ import org.modelix.modelql.untyped.descendants import org.modelix.modelql.untyped.nodeReference import org.modelix.modelql.untyped.property import org.modelix.modelql.untyped.query +import org.modelix.modelql.untyped.reference import org.modelix.modelql.untyped.remove import org.modelix.modelql.untyped.resolve import org.modelix.modelql.untyped.setProperty @@ -258,7 +261,7 @@ class ModelQLClientTest { fun `test IMonoStep nodeRefAndConcept`() = runTest { httpClient -> val client = ModelQLClient("http://localhost/query", httpClient) - val nullNode = client.getRootNode().getReferenceTarget("nonExistentReference") + val nullNode = client.getRootNode().query { it.reference(IReferenceLinkReference.fromName("nonExistentReference")).orNull() } assertEquals(null, nullNode) }