Re: (tomcat) branch main updated: Fix matching of the pattern documented in the javadoc

=D0=BF=D1=82, 31 =D0=B8=D1=8E=D0=BB. 2026=E2=80=AF=D0=B3. =D0=B2 17:18, R=
=C3=A9my Maucherat <[email protected]>:
>
> On Fri, Jul 31, 2026 at 2:49=E2=80=AFPM Konstantin Kolinko
> <[email protected]> wrote:
> >
> > =D0=BF=D1=82, 31 =D0=B8=D1=8E=D0=BB. 2026=E2=80=AF=D0=B3. =D0=B2 12:36,=
 <[email protected]>:
> > >
> > > This is an automated email from the ASF dual-hosted git repository.
> > >
> > > rmaucher pushed a commit to branch main
> > > in repository https://gitbox.apache.org/repos/asf/tomcat.git
> > >
> > >
> > > The following commit(s) were added to refs/heads/main by this push:
> > >      new 1f29baf341 Fix matching of the pattern documented in the jav=
adoc
> > > 1f29baf341 is described below
> > >
> > > commit 1f29baf341220048c66e30aedf43c5b7d0266060
> > > Author: remm <[email protected]>
> > > AuthorDate: Fri Jul 31 11:35:54 2026 +0200
> > >
> > >     Fix matching of the pattern documented in the javadoc
> >
> > 1. The online documentation was correct (before your change), saying "m=
atching",
> > and it was not updated by this commit.
> >
> > https://tomcat.apache.org/tomcat-11.0-doc/config/http.html
> > see "noCompressionUserAgents"
>
> I don't consider matching meant using matches().

?

> The javadoc has always been using the "gorilla|desesplorer|tigrus"
> example pattern, which IMO makes sense.

Yes,
 the sample pattern makes sense, but it is not a real-world example.

(Real names in a User-Agent header start with an Uppercase character.)

Personally, I do not consider JavaDoc to be proper documentation on
how to configure Tomcat.

It is not there by default (only included in the fulldocs bundle), and
we have proper documentation (User Guide and Configuration Reference)
nowadays.

> > 2. The code using "matches()" is there at least from the first
> > revision of CompressionConfig.java (year 2017)
> > when it was moved there out of org/apache/coyote/http11/Http11Processor=
.java
>
> I know. There was no test case for the feature either. There are two
> other occurrences of this usage, all documented with the same kind of
> user-agent pattern matching which won't work. I'd rather fix it.
> In CrawlerSessionManagerValve the default value is that kind of
> pattern. I did not bother adding a test for this one.
>

Patterns used for mathes() vs used for find() can be converted from
one to another by adding either "^","$"  (anchors) or ".*" (any
character pattern) at their ends.

Case-insensitive matching can be turned on with "(?i)".

So once you decide on what API to use, it is easy to tailor your
pattern to that API.

Changing between find() and matches() is not "fixing what does not
work" - either API can be used to get the intended result if you
adjust your pattern.

> >
> > I know that HTTPD uses find rather than matching the whole string, but
> > our code is 9+ old,
> > so I think that it would be better to just align the documentation
> > with the behaviour.
> >
> > https://httpd.apache.org/docs/current/mod/mod_setenvif.html
> > see BrowserMatch, BrowserMatchNoCase
>
> This is a bit different since in that case you can add more
> directives. We don't have a valve equivalent of this one.

That was not the point. The point was the regexp patterns used by
those directives.

Those patterns are for matching using "find()", not "matches()"
Thus they use "^Mozilla" with a "^" anchor character in the example.

(As I have some background with administering Apache HTTPD, I thought
that was your inspiration as well.)

So using a pattern tailored for "find()" makes sense.

Documentation and examples of real-world values:
https://developer.mozilla.org/en-US/docs/Web/HTTP/Reference/Headers/User-Ag=
ent


My problem is that you are changing behaviour in a point release,
and this change is not properly documented.

I ask to either document the change
- be clear that it is a behaviour change in the changelog,
- be ready to mention this in the Migration Guides,
- update the Configuration Reference
or to revert it.

Best regards,
Konstantin Kolinko

> > >     Also add a test case for the noCompressionUserAgents feature, coa=
uthored
> > >     with OpenCode.
> > > ---
> > >  java/org/apache/coyote/CompressionConfig.java      |  2 +-
> > >  .../coyote/TestCompressionConfigUserAgents.java    | 67 ++++++++++++=
++++++++++
> > >  webapps/docs/changelog.xml                         |  6 ++
> > >  3 files changed, 74 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/java/org/apache/coyote/CompressionConfig.java b/java/org=
/apache/coyote/CompressionConfig.java
> > > index 7c5efcfec9..b07b8c10b5 100644
> > > --- a/java/org/apache/coyote/CompressionConfig.java
> > > +++ b/java/org/apache/coyote/CompressionConfig.java
> > > @@ -375,7 +375,7 @@ public class CompressionConfig {
> > >                  MessageBytes userAgentValueMB =3D request.getMimeHea=
ders().getValue("user-agent");
> > >                  if (userAgentValueMB !=3D null) {
> > >                      String userAgentValue =3D userAgentValueMB.toStr=
ing();
> > > -                    if (noCompressionUserAgents.matcher(userAgentVal=
ue).matches()) {
> > > +                    if (noCompressionUserAgents.matcher(userAgentVal=
ue).find()) {
> > >                          return false;
> > >                      }
> > >                  }