Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(modelql)!: disable implicit queries to avoid accidental performan… #1234

Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -227,7 +227,7 @@ class TypedModelQLTest {
.untyped()
.toList()
}.map { it.typed<StaticMethodDeclaration>() }
assertEquals("plus", result[0].name)
assertEquals("plus", result[0].query { it.name })
}

@Test
Expand All @@ -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
Expand All @@ -257,8 +257,9 @@ class TypedModelQLTest {
.member
.ofConcept(C_StaticMethodDeclaration)
.first()
.name
}
assertEquals(expected, actual.name)
assertEquals(expected, actual)
}

@Test
Expand Down Expand Up @@ -297,7 +298,7 @@ class TypedModelQLTest {
.first()
}
assertNotEquals(expected, oldValue)
assertEquals(expected, actual.variableDeclaration.name)
assertEquals(expected, actual.query { it.variableDeclaration.name })
}

@Test
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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<INode> {
override fun usesRoleIds(): Boolean = true

Expand Down Expand Up @@ -93,6 +95,14 @@ abstract class ModelQLNode(val client: ModelQLClient) : INode, ISupportsModelQL,
}.flatMapIterable { it }
}

private fun <R> implicitQuery(body: (IMonoStep<INode>) -> IMonoStep<R>): R {
if (implicitQueriesEnabled) {
return blockingQuery(body)
} else {
throw UnsupportedOperationException("Implicit queries are disabled. Use .query instead.")
}
}

fun <R> blockingQuery(body: (IMonoStep<INode>) -> IMonoStep<R>): R {
val client = client
return client.blockingQuery { root ->
Expand All @@ -112,32 +122,32 @@ 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<INode> {
return blockingQuery { it.children(role.key()).toList() }
return implicitQuery { it.children(role.key()).toList() }
}

override fun getChildren(role: String?): Iterable<INode> {
return getChildren(resolveChildLinkOrFallback(role))
}

override val allChildren: Iterable<INode>
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) {
moveChild(resolveChildLinkOrFallback(role), index, child)
}

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 {
Expand All @@ -153,35 +163,35 @@ 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?) {
setReferenceTarget(link, target?.reference)
}

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?) {
setReferenceTarget(resolveReferenceLinkOrFallback(role), target)
}

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?) {
Expand Down
Original file line number Diff line number Diff line change
@@ -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
Original file line number Diff line number Diff line change
@@ -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

Check warning

Code scanning / detekt

The property implicitQueriesEnabled is missing documentation. Warning

The property implicitQueriesEnabled is missing documentation.
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,11 @@ 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.IReferenceLinkReference
import org.modelix.model.api.NodeReference
import org.modelix.model.api.PBranch
import org.modelix.model.api.getRootNode
Expand All @@ -41,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
Expand All @@ -55,6 +59,8 @@ 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.reference
import org.modelix.modelql.untyped.remove
import org.modelix.modelql.untyped.resolve
import org.modelix.modelql.untyped.setProperty
Expand Down Expand Up @@ -255,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)
}
Expand Down Expand Up @@ -294,4 +300,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<INode> { it.children(IChildLinkReference.fromName("modules")).first() }
assertThrows<UnsupportedOperationException> {
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<INode> { it.children(IChildLinkReference.fromName("modules")).first() }
val name = firstModule.query { it.property(IPropertyReference.fromName("name")) }
assertEquals("abc", name)
}
}
Loading