[Entitlements] Fix: consider case sensitiveness differences

[ldematte]

Our path comparison for file access is string based, due to the fact that we need to support Paths created for different file systems/platforms.
However, Windows files and paths are (sort of) case insensitive - see https://discuss.elastic.co/t/not-entitled-component-repository-s3-after-update-from-8-17-4-to-8-18-0/377166 for a real issue where we hit this problem.
This PR fixes the problem by abstracting String comparison operations (moving them to FileUtils) and making them case sensitive or not based on the host OS.

Closes #127047

[elasticsearchmachine]

Pinging @elastic/es-core-infra (Team:Core/Infra)

[elasticsearchmachine]

Hi @ldematte, I've created a changelog YAML for you.

[prdoyle]

I wonder if we'd be better of solving this via normalization rather than changing the comparator? If we normalize to lowercase on Windows, then the comparator will just work.

[ldematte]

If we normalize to lowercase on Windows, then the comparator will just work.

I'm not sure this is the right way to do it; probably in the case of a Windows FS it does not matter (the char comparison in WindowsPath do lower case after all), but usually "case insensitive comparison" and "normalize to lower case" are different. Since we already have a comparator, I thought it was a comparable amount of work/comparable change.

[prdoyle]

I just worry that our comparator is going to become rather incomprehensible. Adding toLowerCase in the normalization on Windows would be a one-liner.

Do you have an example where normalize-then-compare is different from a case-insensitive compare?

[ldematte]

CaseInsensitiveComparator -- see https://stackoverflow.com/questions/15518731/understanding-logic-in-caseinsensitivecomparator/15518878#15518878

[prdoyle]

Do I understand correctly that the concern would be that a user would supply two different path strings that compare equal under String.toLowercase(), but are actually distinct? I don't think the comparator logic in this PR solves that problem any better, because it's also doing toLowercase.

[prdoyle]

I notice there's a reply to the above post that mentions toLowercase(toUppercase(c)) as the solution. Perhaps we ought to do that either way, whether we use normalization or custom comparator logic?

[ldematte]

I don't think the comparator logic in this PR solves that problem any better, because it's also doing toLowercase.

Yeah

in the case of a Windows FS it does not matter (the char comparison in WindowsPath do lower case after all)

the startsWith thing for isParent is OK though.I wonder if we should "do better", but ... better than WindowsPath? Not sure is worth it at all. Still, I like the approach of fixing the comparator better; I don't like the idea of having a "broken" comparator around, that happens to work because we normalized strings beforehand.

[ldematte]

On the other hand it's true that normalization happens once, so there is a little performance benefit there... IDK.

[prdoyle]

the startsWith thing for isParent is OK though.I wonder if we should "do better", but ... better than WindowsPath?

Agreed, the idea of overachieving relative to WindowsPath seems very much like a goal not worth pursuing.

Not sure is worth it at all. Still, I like the approach of fixing the comparator better; I don't like the idea of having a "broken" comparator around, that happens to work because we normalized strings beforehand.

But this is the heart of why normalization is beneficial: it reduces the space of cases to consider, and then subsequent processing is greatly simplified. The comparator isn't "broken" if the cases it doesn't handle are ones that never occur thanks to normalization.

(In fact, the only reason I didn't propose normalizing the path separators is that there exists no reasonable separator character that would solve our sorting problem.)

[prdoyle]

A note: at the team meeting, we chose to proceed with improved comparator instead of normalization, primarily based on the performance impact of doing a toLowercase() on every string we look up every time.

[rjernst]

LGTM

[prdoyle]

Is there a significance to the fact that we switched from uppercase to lowercase?

[ldematte]

Nope, just a suggestion from Ryan to make it shorter and more readable

[prdoyle]

toUpperCase is equally short and readable, no?

I just think if we use one that's different from the actual filesystem code you referenced, we're asking for unexpected corner cases.

[elasticsearchmachine]

💚 Backport successful

Status	Branch	Result
✅	8.18
✅	8.x
✅	9.0