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

[UNDERTOW-2119] - add case sensitivity to predicates - path, path-suf… #1416

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

baranowb
Copy link
Contributor

@baranowb baranowb commented Dec 2, 2022

@baranowb
Copy link
Contributor Author

baranowb commented Dec 2, 2022

This is a feature.

@bdw429s
Copy link
Contributor

bdw429s commented Dec 23, 2022

@baranowb @fl4via Please see my question in the comments of my ticket concerning the implementation of this.

@fl4via fl4via added under verification Currently being verified (running tests, reviewing) before posting a review to contributor new feature/API change New feature to be introduced or a change to the API (non suitable to minor releases) labels Jan 13, 2023
@fl4via
Copy link
Member

fl4via commented Jan 13, 2023

@bdw429s I had a look, I'm not 100% sure of which way to go here. On one hand it is true that you have to type less characters... but on the other, it is going to clutter a little bit the list of predicate handlers. But it is also true that, by adding the case-sensitive attribute everywhere we are cluttering the list of attributes a bit.
Any thoughts @baranowb @ropalka @stuartwdouglas ?

@fl4via fl4via added the question Waiting for additional information from contributor label Jan 13, 2023
@baranowb
Copy link
Contributor Author

I was positive I did reply to this. Anyway. Im not keen on introducing another hmm... conceptual/functional types into existing. Currently names are restricted to functional meaning, rather than contextual arguments. This would become confusing, since for path-prefix we would have path-prefix-cocase but regex wouldnt have regex-full-match .
In general Im not against such approach, but I dont like mixing two.

@bdw429s
Copy link
Contributor

bdw429s commented Mar 21, 2023

I certainly wasn't implying that EVERY parameter to predicates would require a permutation of the function. I was suggesting a pragmatic approach that provided convenience functions to reduce boilerplate in common scenarios. As I've integrated Undertow into a tool which uses a custom resource manager to be case insensitive on Windows (to appeals to my IIS/Windows users), wanting your predicates to to not match with case is not only a common need, but a rather important one for security!

I have already added several custom predicates to my tool which largely fills this need
https://ortussolutions.atlassian.net/browse/COMMANDBOX-1549
however, I still can't override the built in predicates to change their behavior to sensible defaults given the context until this ticket is completed https://issues.redhat.com/browse/UNDERTOW-2229

@baranowb
Copy link
Contributor Author

Hey. Not what I meant - such new functional names would be counter intuitive, since other predicates dont have/follow this style. Just my 2c.

@bdw429s
Copy link
Contributor

bdw429s commented Mar 22, 2023

Thanks again for your input. I think they are very intuitive, but I suppose that's possibly because I've been developing in CFML for 20 years and this language uses this exact format for it's built in functions so it's very familiar and makes a great deal of sense to me. And FWIW, you can see in my ticket above (from my tool's ticket tracker) I added -nocase versions to EVERY predicate details with text comparison which makes it 100% consistent and across the board.

  • regex()/regex-nocase()
  • path-suffix()/path-suffix-nocase()
  • path-prefix()/path-prefix-nocase()
  • path()/path-nocase()
  • equals()/equals-nocase()
  • contains()/contains-nocase()

Basically, any time you do text comparison, you have a case sensitive and case insensitive version which is less typing, leaves less ambiguity, and allows positional params to still be used. And this falls right in line with the precedent set by languages like CFML which has matching functions find()/findNoCase(), reFind()/reFindNoCase(), compare()/compareNoCase(), etc. Point being that there is "prior art" in other languages which has been doing this for years so it's definitely a precedent that makes sense to a lot of people, but I understand if you don't personally see the benifit :)

@baranowb
Copy link
Contributor Author

In general I dont disagree, what I mean is scoped solely on predicates and existing features. If it was implemented across the board I wouldnt mind, but that is solely in hands of core devs here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new feature/API change New feature to be introduced or a change to the API (non suitable to minor releases) question Waiting for additional information from contributor under verification Currently being verified (running tests, reviewing) before posting a review to contributor
Projects
None yet
3 participants