search

oss-review-toolkit / ort

A suite of tools to automate software compliance checks.
https://oss-review-toolkit.org
Apache License 2.0
1.61k stars 309 forks source link

Wrong matching with [AntPathMatcher] for path exclude #5901

Closed nnobelis closed 2 years ago

nnobelis commented 2 years ago

We have a project with the following path exclude

paths:
    - pattern: "{gradlew,gradlew.bat}"
      reason: "BUILD_TOOL_OF"
      comment: "Gradle build files"

However it seems this pattern excludes the build.gradle in the project.

After debugging, I came to evaluate this bit of code in ORT Reporter:

AntPathMatcher().apply { setCaseSensitive(true) }.match("{gradlew,gradlew.bat}", "build.gradle")

and to my surprise it evaluates to true.

Has anyone an idea what happens here ? It does not seem to conform to what's in Spring Javadoc 1.

As a sidenote, should ORT not use 2 instead for glob matching ?

nnobelis commented 2 years ago

Thinking about it maybe this is not a bug. Maybe this expression is just not supported. Please advise.

sschuberth commented 2 years ago

As a sidenote, should ORT not use 2 instead for glob matching ?

Unfortunately, that standard implementation behaves oddly in some edge cases, i.e. it does not match files that we expect to be matched for certain patterns.

sschuberth commented 2 years ago

As anyone an idea what happens here ?

Probably @fviernau can advise.

fviernau commented 2 years ago

@nnobelis @sschuberth , I wonder if the AntPathMatcher syntax is actually the best choice. When I have some time I'd make some research what other implementations are available, so that we could switch to something else. Do you agree at least rearching it makes sense?

Meanwhile, we could merge my above PR to have things aligned.

nnobelis commented 2 years ago

Do you agree at least searching it makes sense?

Definitively. Spring also as a replacement for AntPathMatcher if I am not mistaken: https://www.baeldung.com/spring-5-mvc-url-matching

sschuberth commented 2 years ago

When I have some time I'd make some research what other implementations are available, so that we could switch to something else. Do you agree at least rearching it makes sense?

Yes, doing some research does not hurt, and I did that myself some time ago as I was considering to write a Kotlin-only (and multi-platform) glob matcher because the Kotlin stdlib unfortunately does not provide one (nor could I find a decent one, except for this outdated library). An article I found helpful was https://research.swtch.com/glob as it gives some nice overview of the approaches used in different programming languages and their performance.

sschuberth commented 2 years ago

JGit also has an (internal) class to convert .gitignore-style globs to Java Regexes that we maybe could use.

fviernau commented 2 years ago

I've extracted improving the externally facing glob syntax to a separate ticket: see #5908.