Skip to content

Commit

Permalink
support !(...) in tag filter syntax
Browse files Browse the repository at this point in the history
... to have feature parity with streetcomplete, see streetcomplete/StreetComplete#5107
  • Loading branch information
westnordost committed Aug 15, 2023
1 parent 09d9b8a commit e961455
Show file tree
Hide file tree
Showing 6 changed files with 114 additions and 6 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ internal abstract class Chain<I : Matcher<T>, T> : BooleanExpression<I, T>() {

val children: List<BooleanExpression<I, T>> get() = nodes.toList()

fun addChild(child: BooleanExpression<I, T>) {
open fun addChild(child: BooleanExpression<I, T>) {
child.parent = this
nodes.add(child)
}
Expand Down Expand Up @@ -55,7 +55,7 @@ internal abstract class Chain<I : Matcher<T>, T> : BooleanExpression<I, T>() {
val it = nodes.listIterator()
while (it.hasNext()) {
val child = it.next() as? Chain ?: continue
if (child.nodes.size == 1) {
if (child.nodes.size == 1 && child !is Not<I, T>) {
replaceChildAt(it, child.nodes.first())
it.previous() // = the just replaced node will be checked again
} else {
Expand All @@ -69,6 +69,9 @@ internal abstract class Chain<I : Matcher<T>, T> : BooleanExpression<I, T>() {
val it = nodes.listIterator()
while (it.hasNext()) {
val child = it.next() as? Chain ?: continue
if (child is Not<I, T>) {
continue
}
child.mergeNodesWithSameOperator()

// merge two successive nodes of same type
Expand Down Expand Up @@ -128,6 +131,18 @@ internal class AnyOf<I : Matcher<T>, T> : Chain<I, T>() {
nodes.joinToString(" or ") { "$it" }
}

internal class Not<I : Matcher<T>, T> : Chain<I, T>() {
override fun addChild(child: BooleanExpression<I, T>) {
check(nodes.isEmpty()) { "Adding a second child to '!' (NOT) operator is not allowed" }
super.addChild(child)
}

override fun matches(obj: T, evaluate: (name: String) -> Boolean) =
!nodes.first().matches(obj, evaluate)

override fun toString() = "!(${nodes.firstOrNull() ?: ""})"
}

internal interface Matcher<in T> {
fun matches(obj: T): Boolean
}
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,12 @@ internal class BooleanExpressionBuilder<I : Matcher<T>, T> {
node = node.parent!!
}
node = node.parent!!

if (node is Not)
{
node = node.parent!!
}

node.flatten()
}

Expand Down Expand Up @@ -93,6 +99,12 @@ internal class BooleanExpressionBuilder<I : Matcher<T>, T> {
node = anyOf
}
}

fun addNot() {
val not = Not<I, T>()
node.addChild(not)
node = not
}
}

private fun <I : Matcher<T>, T> Chain<I, T>.ensureNoBracketNodes() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ import kotlin.math.min
* | `shop and name` | has both a tag with key `shop` and one with key `name` |
* | `shop or craft` | has either a tag with key `shop` or one with key `craft` |
* | `shop and (ref or name)` | has a tag with key `shop` and either a tag with key `ref` or `name` |
* | `shop and !(ref or name)` | hat a tag with key `shop` but not either a tag with key `ref` or `name` |
*
* ### Equivalent expressions
* | expression | equivalent expression |
Expand All @@ -41,7 +42,8 @@ import kotlin.math.min
* | `!shop or shop != boat` | `shop != boat` |
* | `shop = car or shop = boat` | `shop ~ car|boat` |
* | `craft or shop and name` | `craft or (shop and name)` (`and` has higher precedence) |
* | `!(amenity and craft)` | **<error>** (negation of expression not supported) |
* | `!(amenity and craft)` | `!amenity or !craft` |
* | `!(amenity or craft)` | `!amenity and !craft` |
* */

private const val OR = "or"
Expand Down Expand Up @@ -81,6 +83,7 @@ private val OPERATORS = linkedSetOf(
private val ESCAPED_QUOTE_REGEX = Regex("\\\\(['\"])")
private val WHITESPACE_REGEX = Regex("\\s")
private val WHITESPACES_REGEX = Regex("\\s*")
private val NOT_WITH_WHITESPACE_AND_OPENING_BRACE = Regex("!\\s*\\(")

internal fun StringWithCursor.parseTags(): BooleanExpression<TagFilter, Map<String, String>> {
val builder = BooleanExpressionBuilder<TagFilter, Map<String, String>>()
Expand All @@ -93,6 +96,13 @@ internal fun StringWithCursor.parseTags(): BooleanExpression<TagFilter, Map<Stri
}
first = false

if (nextMatches(NOT_WITH_WHITESPACE_AND_OPENING_BRACE) != null) {
advanceBy(NOT.length)
builder.addNot()
// continue is required, as !( could be nested
continue
}

if (nextIsAndAdvance(NOT + PLACEHOLDER_START)) {
builder.addNotPlaceholder(parsePlaceholder())
} else if (nextIsAndAdvance(PLACEHOLDER_START)) {
Expand All @@ -105,7 +115,7 @@ internal fun StringWithCursor.parseTags(): BooleanExpression<TagFilter, Map<Stri

if (isAtEnd()) break

// same as with the opening bracket, only that if the string is over, its okay
// same as with the opening bracket, only that if the string is over, it's okay
if (!separated) {
throw ParseException("Expected a whitespace or bracket after the tag", cursorPos)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,6 @@ internal class BooleanExpressionTest {
}

@Test fun list_items() {
assertEquals(listOf("1"), getItems("1"))
assertEquals(listOf("1","2"), getItems("1+2"))
assertEquals(listOf("1","2"), getItems("1*2"))
assertEquals(listOf("1","2","3"), getItems("1*(2+3)"))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -388,6 +388,78 @@ internal class TagFilterExpressionParserTest {
notMatches(mapOfKeys("b", "c"), expr)
}

@Test fun not_with_leaf() {
val expr = "!(a)"
matches(mapOfKeys("b"), expr)
notMatches(mapOfKeys("a"), expr)
notMatches(mapOfKeys("a", "b"), expr)
}

@Test fun not_without_braces() {
val expr = "ways with !highway = residential or access = yes"
shouldFail(expr)
}

@Test fun not_and_with_space() {
val expr = "! (a and b)"
matches(mapOfKeys("a"), expr)
matches(mapOfKeys("b"), expr)
matches(mapOfKeys("b", "c"), expr)
matches(mapOfKeys("c"), expr)
notMatches(mapOfKeys("a", "b", "c"), expr)
}

@Test fun not_and() {
val expr = "!(a and b)"
matches(mapOfKeys("a"), expr)
matches(mapOfKeys("b"), expr)
matches(mapOfKeys("b", "c"), expr)
matches(mapOfKeys("c"), expr)
notMatches(mapOfKeys("a", "b", "c"), expr)
}

@Test fun not_or() {
val expr = "!(a or b)"
matches(mapOfKeys("c"), expr)
matches(mapOfKeys("c", "d", "e"), expr)
notMatches(mapOfKeys("a"), expr)
notMatches(mapOfKeys("b"), expr)
notMatches(mapOfKeys("b", "c"), expr)
notMatches(mapOfKeys("a", "c"), expr)
notMatches(mapOfKeys("a", "b", "c"), expr)
}

@Test fun nested_not() {
val expr = "!(!(a))" // equals the expression "a"
matches(mapOfKeys("a"), expr)
matches(mapOfKeys("a", "b"), expr)
notMatches(mapOfKeys("b"), expr)
}

@Test fun nested_not_with_or() {
val expr = "!(!(a and b) or c)" // equals a and b and !(c)
matches(mapOfKeys("a", "b"), expr)
matches(mapOfKeys("a", "b", "d"), expr)
notMatches(mapOfKeys("a"), expr)
notMatches(mapOfKeys("c"), expr)
notMatches(mapOfKeys("b", "c"), expr)
notMatches(mapOfKeys("a", "b", "c"), expr)
notMatches(mapOfKeys("a", "b", "c", "d"), expr)
}

@Test fun nested_not_with_or_and_switched_operands() {
val expr = "!(c or !(a and b))" // equals a and b and !(c)
matches(mapOfKeys("a", "b"), expr)
matches(mapOfKeys("a", "b", "d"), expr)
notMatches(mapOfKeys("a"), expr)
notMatches(mapOfKeys("c"), expr)
notMatches(mapOfKeys("b", "c"), expr)
notMatches(mapOfKeys("a", "b", "c"), expr)
notMatches(mapOfKeys("a", "b", "c", "d"), expr)
}



@Test fun fail_on_placeholder_not_closed() {
shouldFail("{my placeholder")
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,6 @@ internal class NotHasKeyLikeTest {

@Test fun relevantKey() {
val key = NotHasKeyLike("n.[ms]e")
assertEquals("n.[ms]e", (key.relevantKey.regex as RealRegex).toString())
assertTrue(key.relevantKey.regex is RealRegex)
}
}

3 comments on commit e961455

@riQQ
Copy link

@riQQ riQQ commented on e961455 Jun 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As some of this code here was originally written by me in the context of streetcomplete/StreetComplete#5107, the StreetComplete repository is licensed as GPL v3.0 and this repo (osm-legal-default-speeds) is licensed as BSD 3-Clause, and GPL v3.0 is not compatible to BSD 3-Clause, you should have asked me for releasing my contribution also under the BSD 3-Clause license.

I hereby license the code written by me in streetcomplete/StreetComplete#5107 under the BSD 3-Clause license.

Also, it would have been nice to attribute me in this commit here as a co-author (see https://docs.github.com/en/pull-requests/committing-changes-to-your-project/creating-and-editing-commits/creating-a-commit-with-multiple-authors).

@westnordost
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh sorry, and thanks. Is there a git command I can issue to apply that retroactively?

@riQQ
Copy link

@riQQ riQQ commented on e961455 Jun 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only possible retroactively by rewriting history as the commit message is part of the input for the commit hash. As this commit is already quite old and this is a public repository with some forks it's not worth it in my opinion.

Just keep it mind if you find yourself in a similar situation.

Please sign in to comment.