You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by "onkar717 (via GitHub)" <gi...@apache.org> on 2026/04/01 20:21:32 UTC
[PR] Add unit tests for FilterValve, ProxyErrorReportValve and SemaphoreValve [tomcat]
onkar717 opened a new pull request, #976:
URL: https://github.com/apache/tomcat/pull/976
## Summary
This PR improves test coverage by adding unit tests for several Valve
implementations that currently lack dedicated tests:
- FilterValve
- ProxyErrorReportValve
- SemaphoreValve
## Motivation
While reviewing Tomcat valve implementations, these components were
identified as having little or no direct test coverage. Adding tests
helps improve reliability and prevents regressions during future
changes.
## Changes
Added new test classes:
- TestFilterValve
- TestProxyErrorReportValve
- TestSemaphoreValve
The tests cover:
- Basic initialization behavior
- Request/response processing
- Error handling paths
- Concurrency behavior (SemaphoreValve)
- Configuration edge cases
## Testing
All tests pass locally using:
ant clean test
No existing tests were modified.
## Impact
No functional changes.
Test coverage improvement only.
## Notes
Further opportunities may exist to improve coverage for other Valve
implementations and configuration parsing logic. I plan to continue
exploring these areas unless there are higher priority suggestions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: [PR] Add unit tests for FilterValve, ProxyErrorReportValve and SemaphoreValve [tomcat]
Posted by "ChristopherSchultz (via GitHub)" <gi...@apache.org>.
ChristopherSchultz commented on code in PR #976:
URL: https://github.com/apache/tomcat/pull/976#discussion_r3057673953
##########
test/org/apache/catalina/valves/TestFilterValve.java:
##########
@@ -0,0 +1,229 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.catalina.valves;
+
+import java.io.IOException;
+import java.util.Collections;
+import java.util.List;
+
+import jakarta.servlet.Filter;
+import jakarta.servlet.FilterChain;
+import jakarta.servlet.ServletException;
+import jakarta.servlet.ServletRequest;
+import jakarta.servlet.ServletResponse;
+import jakarta.servlet.http.HttpServletRequest;
+import jakarta.servlet.http.HttpServletRequestWrapper;
+import jakarta.servlet.http.HttpServletResponse;
+
+import org.junit.Assert;
+import org.junit.Test;
+
+import org.apache.catalina.Context;
+import org.apache.catalina.LifecycleException;
+import org.apache.catalina.startup.Tomcat;
+import org.apache.catalina.startup.TomcatBaseTest;
+import org.apache.tomcat.util.buf.ByteChunk;
+
+public class TestFilterValve extends TomcatBaseTest {
+
+
+ @Test
+ public void testFilterPassthrough() throws Exception {
+ Tomcat tomcat = getTomcatInstance();
+
+ Context ctx = getProgrammaticRootContext();
+
+ Tomcat.addServlet(ctx, "hello", new HelloWorldServlet());
+ ctx.addServletMappingDecoded("/", "hello");
+
+ FilterValve valve = new FilterValve();
+ valve.setFilterClass(PassthroughFilter.class.getName());
+ ctx.getPipeline().addValve(valve);
+
+ tomcat.start();
+
+ ByteChunk res = new ByteChunk();
+ int rc = getUrl("http://localhost:" + getPort(), res, null);
+
+ Assert.assertEquals(HttpServletResponse.SC_OK, rc);
+ Assert.assertEquals(HelloWorldServlet.RESPONSE_TEXT, res.toString());
+ }
+
+
+ @Test
+ public void testFilterBlocks() throws Exception {
+ Tomcat tomcat = getTomcatInstance();
+
+ Context ctx = getProgrammaticRootContext();
+
+ Tomcat.addServlet(ctx, "hello", new HelloWorldServlet());
+ ctx.addServletMappingDecoded("/", "hello");
+
+ FilterValve valve = new FilterValve();
+ valve.setFilterClass(BlockingFilter.class.getName());
+ ctx.getPipeline().addValve(valve);
+
+ tomcat.start();
+
+ ByteChunk res = new ByteChunk();
+ int rc = getUrl("http://localhost:" + getPort(), res, null);
+
+ Assert.assertEquals(HttpServletResponse.SC_FORBIDDEN, rc);
+ }
+
+ @Test
+ public void testFilterWrappingRequestThrows() throws Exception {
+ Tomcat tomcat = getTomcatInstance();
+
+ Context ctx = getProgrammaticRootContext();
+
+ Tomcat.addServlet(ctx, "hello", new HelloWorldServlet());
+ ctx.addServletMappingDecoded("/", "hello");
+
+ FilterValve valve = new FilterValve();
+ valve.setFilterClass(WrappingFilter.class.getName());
+ ctx.getPipeline().addValve(valve);
+
+ tomcat.start();
+
+ int rc = getUrl("http://localhost:" + getPort(), new ByteChunk(), null);
+
+ Assert.assertEquals(HttpServletResponse.SC_INTERNAL_SERVER_ERROR, rc);
+ }
+
+
+ @Test(expected = LifecycleException.class)
+ public void testNullFilterClassThrowsOnStart() throws Exception {
+ Tomcat tomcat = getTomcatInstance();
+
+ Context ctx = getProgrammaticRootContext();
+
+ FilterValve valve = new FilterValve();
+ // Do NOT set filterClassName
+ ctx.getPipeline().addValve(valve);
+
+ tomcat.start();
+ }
+
+
+ @Test(expected = LifecycleException.class)
+ public void testInvalidFilterClassThrowsOnStart() throws Exception {
+ Tomcat tomcat = getTomcatInstance();
+
+ Context ctx = getProgrammaticRootContext();
+
+ FilterValve valve = new FilterValve();
+ valve.setFilterClass("com.nonexistent.FakeFilter");
+ ctx.getPipeline().addValve(valve);
+
+ tomcat.start();
+ }
+
+
+ @Test
+ public void testGetFilterNameReturnsNull() {
+ FilterValve valve = new FilterValve();
+ Assert.assertNull(valve.getFilterName());
+ }
+
+
+ @Test
+ public void testInitParams() {
Review Comment:
Is this (specific) test really necessary?
##########
test/org/apache/catalina/valves/TestFilterValve.java:
##########
@@ -0,0 +1,229 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.catalina.valves;
+
+import java.io.IOException;
+import java.util.Collections;
+import java.util.List;
+
+import jakarta.servlet.Filter;
+import jakarta.servlet.FilterChain;
+import jakarta.servlet.ServletException;
+import jakarta.servlet.ServletRequest;
+import jakarta.servlet.ServletResponse;
+import jakarta.servlet.http.HttpServletRequest;
+import jakarta.servlet.http.HttpServletRequestWrapper;
+import jakarta.servlet.http.HttpServletResponse;
+
+import org.junit.Assert;
+import org.junit.Test;
+
+import org.apache.catalina.Context;
+import org.apache.catalina.LifecycleException;
+import org.apache.catalina.startup.Tomcat;
+import org.apache.catalina.startup.TomcatBaseTest;
+import org.apache.tomcat.util.buf.ByteChunk;
+
+public class TestFilterValve extends TomcatBaseTest {
+
+
+ @Test
+ public void testFilterPassthrough() throws Exception {
+ Tomcat tomcat = getTomcatInstance();
+
+ Context ctx = getProgrammaticRootContext();
+
+ Tomcat.addServlet(ctx, "hello", new HelloWorldServlet());
+ ctx.addServletMappingDecoded("/", "hello");
+
+ FilterValve valve = new FilterValve();
+ valve.setFilterClass(PassthroughFilter.class.getName());
+ ctx.getPipeline().addValve(valve);
+
+ tomcat.start();
+
+ ByteChunk res = new ByteChunk();
+ int rc = getUrl("http://localhost:" + getPort(), res, null);
+
+ Assert.assertEquals(HttpServletResponse.SC_OK, rc);
+ Assert.assertEquals(HelloWorldServlet.RESPONSE_TEXT, res.toString());
+ }
+
+
+ @Test
+ public void testFilterBlocks() throws Exception {
+ Tomcat tomcat = getTomcatInstance();
+
+ Context ctx = getProgrammaticRootContext();
+
+ Tomcat.addServlet(ctx, "hello", new HelloWorldServlet());
+ ctx.addServletMappingDecoded("/", "hello");
+
+ FilterValve valve = new FilterValve();
+ valve.setFilterClass(BlockingFilter.class.getName());
+ ctx.getPipeline().addValve(valve);
+
+ tomcat.start();
+
+ ByteChunk res = new ByteChunk();
+ int rc = getUrl("http://localhost:" + getPort(), res, null);
+
+ Assert.assertEquals(HttpServletResponse.SC_FORBIDDEN, rc);
+ }
+
+ @Test
+ public void testFilterWrappingRequestThrows() throws Exception {
+ Tomcat tomcat = getTomcatInstance();
+
+ Context ctx = getProgrammaticRootContext();
+
+ Tomcat.addServlet(ctx, "hello", new HelloWorldServlet());
+ ctx.addServletMappingDecoded("/", "hello");
+
+ FilterValve valve = new FilterValve();
+ valve.setFilterClass(WrappingFilter.class.getName());
+ ctx.getPipeline().addValve(valve);
+
+ tomcat.start();
+
+ int rc = getUrl("http://localhost:" + getPort(), new ByteChunk(), null);
+
+ Assert.assertEquals(HttpServletResponse.SC_INTERNAL_SERVER_ERROR, rc);
+ }
+
+
+ @Test(expected = LifecycleException.class)
+ public void testNullFilterClassThrowsOnStart() throws Exception {
+ Tomcat tomcat = getTomcatInstance();
+
+ Context ctx = getProgrammaticRootContext();
+
+ FilterValve valve = new FilterValve();
+ // Do NOT set filterClassName
+ ctx.getPipeline().addValve(valve);
+
+ tomcat.start();
+ }
+
+
+ @Test(expected = LifecycleException.class)
+ public void testInvalidFilterClassThrowsOnStart() throws Exception {
+ Tomcat tomcat = getTomcatInstance();
+
+ Context ctx = getProgrammaticRootContext();
+
+ FilterValve valve = new FilterValve();
+ valve.setFilterClass("com.nonexistent.FakeFilter");
+ ctx.getPipeline().addValve(valve);
+
+ tomcat.start();
+ }
+
+
+ @Test
+ public void testGetFilterNameReturnsNull() {
+ FilterValve valve = new FilterValve();
+ Assert.assertNull(valve.getFilterName());
+ }
+
+
+ @Test
+ public void testInitParams() {
+ FilterValve valve = new FilterValve();
+
+ valve.addInitParam("key1", "value1");
+ valve.addInitParam("key2", "value2");
+
+ Assert.assertEquals("value1", valve.getInitParameter("key1"));
+ Assert.assertEquals("value2", valve.getInitParameter("key2"));
+ Assert.assertNull(valve.getInitParameter("nonexistent"));
+
+ List<String> names = Collections.list(valve.getInitParameterNames());
+ Assert.assertEquals(2, names.size());
+ Assert.assertTrue(names.contains("key1"));
+ Assert.assertTrue(names.contains("key2"));
+ }
+
+
+ @Test
+ public void testInitParamsEmpty() {
+ FilterValve valve = new FilterValve();
+
+ Assert.assertNull(valve.getInitParameter("anything"));
+ Assert.assertFalse(valve.getInitParameterNames().hasMoreElements());
+ }
+
+
+ @Test
+ public void testGetSetFilterClassName() {
Review Comment:
Is this (specific) test really necessary?
##########
test/org/apache/catalina/valves/TestFilterValve.java:
##########
@@ -0,0 +1,229 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.catalina.valves;
+
+import java.io.IOException;
+import java.util.Collections;
+import java.util.List;
+
+import jakarta.servlet.Filter;
+import jakarta.servlet.FilterChain;
+import jakarta.servlet.ServletException;
+import jakarta.servlet.ServletRequest;
+import jakarta.servlet.ServletResponse;
+import jakarta.servlet.http.HttpServletRequest;
+import jakarta.servlet.http.HttpServletRequestWrapper;
+import jakarta.servlet.http.HttpServletResponse;
+
+import org.junit.Assert;
+import org.junit.Test;
+
+import org.apache.catalina.Context;
+import org.apache.catalina.LifecycleException;
+import org.apache.catalina.startup.Tomcat;
+import org.apache.catalina.startup.TomcatBaseTest;
+import org.apache.tomcat.util.buf.ByteChunk;
+
+public class TestFilterValve extends TomcatBaseTest {
+
+
+ @Test
+ public void testFilterPassthrough() throws Exception {
+ Tomcat tomcat = getTomcatInstance();
+
+ Context ctx = getProgrammaticRootContext();
+
+ Tomcat.addServlet(ctx, "hello", new HelloWorldServlet());
+ ctx.addServletMappingDecoded("/", "hello");
+
+ FilterValve valve = new FilterValve();
+ valve.setFilterClass(PassthroughFilter.class.getName());
+ ctx.getPipeline().addValve(valve);
+
+ tomcat.start();
+
+ ByteChunk res = new ByteChunk();
+ int rc = getUrl("http://localhost:" + getPort(), res, null);
+
+ Assert.assertEquals(HttpServletResponse.SC_OK, rc);
+ Assert.assertEquals(HelloWorldServlet.RESPONSE_TEXT, res.toString());
+ }
+
+
+ @Test
+ public void testFilterBlocks() throws Exception {
+ Tomcat tomcat = getTomcatInstance();
+
+ Context ctx = getProgrammaticRootContext();
+
+ Tomcat.addServlet(ctx, "hello", new HelloWorldServlet());
+ ctx.addServletMappingDecoded("/", "hello");
+
+ FilterValve valve = new FilterValve();
+ valve.setFilterClass(BlockingFilter.class.getName());
+ ctx.getPipeline().addValve(valve);
+
+ tomcat.start();
+
+ ByteChunk res = new ByteChunk();
+ int rc = getUrl("http://localhost:" + getPort(), res, null);
+
+ Assert.assertEquals(HttpServletResponse.SC_FORBIDDEN, rc);
+ }
+
+ @Test
+ public void testFilterWrappingRequestThrows() throws Exception {
+ Tomcat tomcat = getTomcatInstance();
+
+ Context ctx = getProgrammaticRootContext();
+
+ Tomcat.addServlet(ctx, "hello", new HelloWorldServlet());
+ ctx.addServletMappingDecoded("/", "hello");
+
+ FilterValve valve = new FilterValve();
+ valve.setFilterClass(WrappingFilter.class.getName());
+ ctx.getPipeline().addValve(valve);
+
+ tomcat.start();
+
+ int rc = getUrl("http://localhost:" + getPort(), new ByteChunk(), null);
+
+ Assert.assertEquals(HttpServletResponse.SC_INTERNAL_SERVER_ERROR, rc);
+ }
+
+
+ @Test(expected = LifecycleException.class)
+ public void testNullFilterClassThrowsOnStart() throws Exception {
+ Tomcat tomcat = getTomcatInstance();
+
+ Context ctx = getProgrammaticRootContext();
+
+ FilterValve valve = new FilterValve();
+ // Do NOT set filterClassName
+ ctx.getPipeline().addValve(valve);
+
+ tomcat.start();
+ }
+
+
+ @Test(expected = LifecycleException.class)
+ public void testInvalidFilterClassThrowsOnStart() throws Exception {
+ Tomcat tomcat = getTomcatInstance();
+
+ Context ctx = getProgrammaticRootContext();
+
+ FilterValve valve = new FilterValve();
+ valve.setFilterClass("com.nonexistent.FakeFilter");
+ ctx.getPipeline().addValve(valve);
+
+ tomcat.start();
+ }
+
+
+ @Test
+ public void testGetFilterNameReturnsNull() {
+ FilterValve valve = new FilterValve();
+ Assert.assertNull(valve.getFilterName());
+ }
+
+
+ @Test
+ public void testInitParams() {
+ FilterValve valve = new FilterValve();
+
+ valve.addInitParam("key1", "value1");
+ valve.addInitParam("key2", "value2");
+
+ Assert.assertEquals("value1", valve.getInitParameter("key1"));
+ Assert.assertEquals("value2", valve.getInitParameter("key2"));
+ Assert.assertNull(valve.getInitParameter("nonexistent"));
+
+ List<String> names = Collections.list(valve.getInitParameterNames());
+ Assert.assertEquals(2, names.size());
+ Assert.assertTrue(names.contains("key1"));
+ Assert.assertTrue(names.contains("key2"));
+ }
+
+
+ @Test
+ public void testInitParamsEmpty() {
+ FilterValve valve = new FilterValve();
+
+ Assert.assertNull(valve.getInitParameter("anything"));
+ Assert.assertFalse(valve.getInitParameterNames().hasMoreElements());
+ }
+
+
+ @Test
+ public void testGetSetFilterClassName() {
+ FilterValve valve = new FilterValve();
+
+ Assert.assertNull(valve.getFilterClassName());
+
+ valve.setFilterClassName("com.example.MyFilter");
+ Assert.assertEquals("com.example.MyFilter", valve.getFilterClassName());
+
+ valve.setFilterClass("com.example.OtherFilter");
+ Assert.assertEquals("com.example.OtherFilter", valve.getFilterClassName());
+ }
+
+ @Test(expected = IllegalStateException.class)
+ public void testGetServletContextThrowsBeforeStart() {
Review Comment:
Is this (specific) test really necessary?
##########
test/org/apache/catalina/valves/TestSemaphoreValve.java:
##########
@@ -0,0 +1,440 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.catalina.valves;
+
+import java.io.IOException;
+import java.io.Serial;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.Semaphore;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.concurrent.atomic.AtomicInteger;
+import java.util.concurrent.atomic.AtomicReference;
+
+import jakarta.servlet.http.HttpServlet;
+import jakarta.servlet.http.HttpServletRequest;
+import jakarta.servlet.http.HttpServletResponse;
+
+import org.junit.Assert;
+import org.junit.Test;
+
+import org.apache.catalina.Context;
+import org.apache.catalina.connector.Request;
+import org.apache.catalina.connector.Response;
+import org.apache.catalina.startup.Tomcat;
+import org.apache.catalina.startup.TomcatBaseTest;
+import org.apache.tomcat.util.buf.ByteChunk;
+
+public class TestSemaphoreValve extends TomcatBaseTest {
+
+
+ @Test
+ public void testBasicConcurrency() throws Exception {
+ Tomcat tomcat = getTomcatInstance();
+
+ Context ctx = getProgrammaticRootContext();
+
+ Tomcat.addServlet(ctx, "hello", new HelloWorldServlet());
+ ctx.addServletMappingDecoded("/", "hello");
+
+ SemaphoreValve valve = new SemaphoreValve();
+ valve.setConcurrency(10);
+ ctx.getPipeline().addValve(valve);
+
+ tomcat.start();
+
+ ByteChunk res = new ByteChunk();
+ int rc = getUrl("http://localhost:" + getPort(), res, null);
+
+ Assert.assertEquals(HttpServletResponse.SC_OK, rc);
+ Assert.assertEquals(HelloWorldServlet.RESPONSE_TEXT, res.toString());
+ }
+
+ @Test
+ public void testInterruptedConcurrency() throws Exception {
+ Tomcat tomcat = getTomcatInstance();
+
+ Context ctx = getProgrammaticRootContext();
+
+ Tomcat.addServlet(ctx, "hello", new HelloWorldServlet());
+ ctx.addServletMappingDecoded("/", "hello");
+
+ SemaphoreValve valve = new SemaphoreValve();
+ valve.setConcurrency(10);
+ valve.setInterruptible(true);
+ ctx.getPipeline().addValve(valve);
+
+ tomcat.start();
+
+ ByteChunk res = new ByteChunk();
+ int rc = getUrl("http://localhost:" + getPort(), res, null);
+
+ Assert.assertEquals(HttpServletResponse.SC_OK, rc);
+ Assert.assertEquals(HelloWorldServlet.RESPONSE_TEXT, res.toString());
+ }
+
+
+ @Test
+ public void testNonBlockingDenied() throws Exception {
+ Tomcat tomcat = getTomcatInstance();
+
+ Context ctx = getProgrammaticRootContext();
+
+ CountDownLatch insideServlet = new CountDownLatch(1);
+ CountDownLatch canReturn = new CountDownLatch(1);
+ Tomcat.addServlet(ctx, "slow", new SlowServlet(insideServlet, canReturn));
+ ctx.addServletMappingDecoded("/", "slow");
+
+ SemaphoreValve valve = new SemaphoreValve();
+ valve.setConcurrency(1);
+ valve.setBlock(false);
+ valve.setHighConcurrencyStatus(HttpServletResponse.SC_SERVICE_UNAVAILABLE);
+ ctx.getPipeline().addValve(valve);
+
+ tomcat.start();
+
+ // First request — should acquire the permit and block inside the servlet
+ AtomicInteger firstRc = new AtomicInteger();
+ Thread firstThread = new Thread(() -> {
+ try {
+ firstRc.set(getUrl("http://localhost:" + getPort(), new ByteChunk(), null));
+ } catch (IOException e) {
+ // Ignore
+ }
+ });
+ firstThread.start();
+
+ // Wait until the first request is inside the servlet
+ Assert.assertTrue("First request should reach servlet",
+ insideServlet.await(10, TimeUnit.SECONDS));
+
+ // Second request — should be denied because concurrency=1 and block=false
+ int rc2 = getUrl("http://localhost:" + getPort(), new ByteChunk(), null);
+
+ Assert.assertEquals(HttpServletResponse.SC_SERVICE_UNAVAILABLE, rc2);
+
+ // Release the first request
+ canReturn.countDown();
+ firstThread.join(10000);
+ Assert.assertFalse(firstThread.isAlive());
+ Assert.assertEquals(HttpServletResponse.SC_OK, firstRc.get());
+ }
+
+
+ @Test
+ public void testHighConcurrencyStatusNotSet() throws Exception {
+ Tomcat tomcat = getTomcatInstance();
+
+ Context ctx = getProgrammaticRootContext();
+
+ CountDownLatch insideServlet = new CountDownLatch(1);
+ CountDownLatch canReturn = new CountDownLatch(1);
+ Tomcat.addServlet(ctx, "slow", new SlowServlet(insideServlet, canReturn));
+ ctx.addServletMappingDecoded("/", "slow");
+
+ SemaphoreValve valve = new SemaphoreValve();
+ valve.setConcurrency(1);
+ valve.setBlock(false);
+ // highConcurrencyStatus is -1 by default (no error sent)
+ ctx.getPipeline().addValve(valve);
+
+ tomcat.start();
+
+ // First request holds the permit
+ Thread firstThread = new Thread(() -> {
+ try {
+ getUrl("http://localhost:" + getPort(), new ByteChunk(), null);
+ } catch (IOException e) {
+ // Ignore
+ }
+ });
+ firstThread.start();
+
+ Assert.assertTrue("First request should reach servlet",
+ insideServlet.await(10, TimeUnit.SECONDS));
+
+ // Second request — denied but no error status is sent
+ int rc2 = getUrl("http://localhost:" + getPort(), new ByteChunk(), null);
+
+ // With no highConcurrencyStatus, response is 200 without body
+ Assert.assertEquals(HttpServletResponse.SC_OK, rc2);
+
+ canReturn.countDown();
+ firstThread.join(10000);
+ }
+
+
+ @Test
+ public void testGetSetProperties() {
Review Comment:
Is this (specific) test really necessary?
##########
test/org/apache/catalina/valves/TestFilterValve.java:
##########
@@ -0,0 +1,229 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.catalina.valves;
+
+import java.io.IOException;
+import java.util.Collections;
+import java.util.List;
+
+import jakarta.servlet.Filter;
+import jakarta.servlet.FilterChain;
+import jakarta.servlet.ServletException;
+import jakarta.servlet.ServletRequest;
+import jakarta.servlet.ServletResponse;
+import jakarta.servlet.http.HttpServletRequest;
+import jakarta.servlet.http.HttpServletRequestWrapper;
+import jakarta.servlet.http.HttpServletResponse;
+
+import org.junit.Assert;
+import org.junit.Test;
+
+import org.apache.catalina.Context;
+import org.apache.catalina.LifecycleException;
+import org.apache.catalina.startup.Tomcat;
+import org.apache.catalina.startup.TomcatBaseTest;
+import org.apache.tomcat.util.buf.ByteChunk;
+
+public class TestFilterValve extends TomcatBaseTest {
+
+
+ @Test
+ public void testFilterPassthrough() throws Exception {
+ Tomcat tomcat = getTomcatInstance();
+
+ Context ctx = getProgrammaticRootContext();
+
+ Tomcat.addServlet(ctx, "hello", new HelloWorldServlet());
+ ctx.addServletMappingDecoded("/", "hello");
+
+ FilterValve valve = new FilterValve();
+ valve.setFilterClass(PassthroughFilter.class.getName());
+ ctx.getPipeline().addValve(valve);
+
+ tomcat.start();
+
+ ByteChunk res = new ByteChunk();
+ int rc = getUrl("http://localhost:" + getPort(), res, null);
+
+ Assert.assertEquals(HttpServletResponse.SC_OK, rc);
+ Assert.assertEquals(HelloWorldServlet.RESPONSE_TEXT, res.toString());
+ }
+
+
+ @Test
+ public void testFilterBlocks() throws Exception {
+ Tomcat tomcat = getTomcatInstance();
+
+ Context ctx = getProgrammaticRootContext();
+
+ Tomcat.addServlet(ctx, "hello", new HelloWorldServlet());
+ ctx.addServletMappingDecoded("/", "hello");
+
+ FilterValve valve = new FilterValve();
+ valve.setFilterClass(BlockingFilter.class.getName());
+ ctx.getPipeline().addValve(valve);
+
+ tomcat.start();
+
+ ByteChunk res = new ByteChunk();
+ int rc = getUrl("http://localhost:" + getPort(), res, null);
+
+ Assert.assertEquals(HttpServletResponse.SC_FORBIDDEN, rc);
+ }
+
+ @Test
+ public void testFilterWrappingRequestThrows() throws Exception {
+ Tomcat tomcat = getTomcatInstance();
+
+ Context ctx = getProgrammaticRootContext();
+
+ Tomcat.addServlet(ctx, "hello", new HelloWorldServlet());
+ ctx.addServletMappingDecoded("/", "hello");
+
+ FilterValve valve = new FilterValve();
+ valve.setFilterClass(WrappingFilter.class.getName());
+ ctx.getPipeline().addValve(valve);
+
+ tomcat.start();
+
+ int rc = getUrl("http://localhost:" + getPort(), new ByteChunk(), null);
+
+ Assert.assertEquals(HttpServletResponse.SC_INTERNAL_SERVER_ERROR, rc);
+ }
+
+
+ @Test(expected = LifecycleException.class)
+ public void testNullFilterClassThrowsOnStart() throws Exception {
+ Tomcat tomcat = getTomcatInstance();
+
+ Context ctx = getProgrammaticRootContext();
+
+ FilterValve valve = new FilterValve();
+ // Do NOT set filterClassName
+ ctx.getPipeline().addValve(valve);
+
+ tomcat.start();
+ }
+
+
+ @Test(expected = LifecycleException.class)
+ public void testInvalidFilterClassThrowsOnStart() throws Exception {
Review Comment:
Is this (specific) test really necessary?
##########
test/org/apache/catalina/valves/TestProxyErrorReportValve.java:
##########
@@ -0,0 +1,277 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.catalina.valves;
+
+import java.io.IOException;
+import java.io.Serial;
+
+import jakarta.servlet.http.HttpServlet;
+import jakarta.servlet.http.HttpServletRequest;
+import jakarta.servlet.http.HttpServletResponse;
+
+import org.junit.Assert;
+import org.junit.Test;
+
+import org.apache.catalina.Context;
+import org.apache.catalina.core.StandardHost;
+import org.apache.catalina.startup.Tomcat;
+import org.apache.catalina.startup.TomcatBaseTest;
+import org.apache.tomcat.util.buf.ByteChunk;
+
+public class TestProxyErrorReportValve extends TomcatBaseTest {
+
+ private static final String PROXY_VALVE =
+ "org.apache.catalina.valves.ProxyErrorReportValve";
+
+
+ @Test
+ public void testRedirectMode() throws Exception {
+ Tomcat tomcat = getTomcatInstance();
+ StandardHost host = (StandardHost) tomcat.getHost();
+ host.setErrorReportValveClass(PROXY_VALVE);
+
+ Context ctx = getProgrammaticRootContext();
+
+ Tomcat.addServlet(ctx, "error", new SendErrorServlet(
+ HttpServletResponse.SC_INTERNAL_SERVER_ERROR, "Server broke"));
+ ctx.addServletMappingDecoded("/", "error");
+
+ // Register an error page at the Host's error report valve level
+ // so findErrorPage() returns a URL for the redirect
+ Tomcat.addServlet(ctx, "errorPage", new ErrorPageServlet());
+ ctx.addServletMappingDecoded("/error-page", "errorPage");
+
+ tomcat.start();
+
+ ProxyErrorReportValve valve = (ProxyErrorReportValve) host.getPipeline().getFirst();
+ valve.setProperty("errorCode." + HttpServletResponse.SC_INTERNAL_SERVER_ERROR,
+ "http://localhost:" + getPort() + "/error-page");
+
+ int rc = getUrl("http://localhost:" + getPort(), new ByteChunk(), false);
+
+ Assert.assertEquals(HttpServletResponse.SC_FOUND, rc);
+ }
+
+ @Test
+ public void testProxyMode() throws Exception {
+ Tomcat tomcat = getTomcatInstance();
+ StandardHost host = (StandardHost) tomcat.getHost();
+ host.setErrorReportValveClass(PROXY_VALVE);
+
+ Context ctx = getProgrammaticRootContext();
+
+ Tomcat.addServlet(ctx, "error", new SendErrorServlet(
+ HttpServletResponse.SC_NOT_FOUND, "Not found"));
+ ctx.addServletMappingDecoded("/", "error");
+
+ Tomcat.addServlet(ctx, "errorPage", new ErrorPageServlet());
+ ctx.addServletMappingDecoded("/error-page", "errorPage");
+
+ tomcat.start();
+
+ ProxyErrorReportValve valve = (ProxyErrorReportValve) host.getPipeline().getFirst();
+ valve.setUseRedirect(false);
+ valve.setProperty("errorCode." + HttpServletResponse.SC_NOT_FOUND,
+ "http://localhost:" + getPort() + "/error-page");
+
+ ByteChunk res = new ByteChunk();
+ int rc = getUrl("http://localhost:" + getPort(), res, null);
+
+ Assert.assertEquals(HttpServletResponse.SC_NOT_FOUND, rc);
+ Assert.assertTrue(res.toString().contains("ERROR_PAGE_OK"));
+ }
+
+
+ @Test
+ public void testNoErrorPageFallsBackToSuper() throws Exception {
+ Tomcat tomcat = getTomcatInstance();
+ ((StandardHost) tomcat.getHost()).setErrorReportValveClass(PROXY_VALVE);
+
+ Context ctx = getProgrammaticRootContext();
+
+ Tomcat.addServlet(ctx, "error", new SendErrorServlet(
+ HttpServletResponse.SC_INTERNAL_SERVER_ERROR, "No page configured"));
+ ctx.addServletMappingDecoded("/", "error");
+
+ tomcat.start();
+
+ ByteChunk res = new ByteChunk();
+ int rc = getUrl("http://localhost:" + getPort(), res, null);
+
+ Assert.assertEquals(HttpServletResponse.SC_INTERNAL_SERVER_ERROR, rc);
+
+ String body = res.toString();
+ Assert.assertNotNull(body);
+ Assert.assertTrue("Should contain HTML error report",
+ body.contains("html") &&
+ body.contains(String.valueOf(HttpServletResponse.SC_INTERNAL_SERVER_ERROR)));
+ }
+
+
+ @Test
+ public void testStatusBelow400Ignored() throws Exception {
+ Tomcat tomcat = getTomcatInstance();
+ ((StandardHost) tomcat.getHost()).setErrorReportValveClass(PROXY_VALVE);
+
+ Context ctx = getProgrammaticRootContext();
+
+ Tomcat.addServlet(ctx, "hello", new HelloWorldServlet());
+ ctx.addServletMappingDecoded("/", "hello");
+
+ tomcat.start();
+
+ ByteChunk res = new ByteChunk();
+ int rc = getUrl("http://localhost:" + getPort(), res, null);
+
+ Assert.assertEquals(HttpServletResponse.SC_OK, rc);
+ Assert.assertEquals(HelloWorldServlet.RESPONSE_TEXT, res.toString());
+ }
+
+
+ @Test
+ public void testStatusNotFound() throws Exception {
+ Tomcat tomcat = getTomcatInstance();
+ ((StandardHost) tomcat.getHost()).setErrorReportValveClass(PROXY_VALVE);
+
+ Context ctx = getProgrammaticRootContext();
+
+ Tomcat.addServlet(ctx, "notFound", new SendErrorServlet(
+ HttpServletResponse.SC_NOT_FOUND, "Resource not found"));
+ ctx.addServletMappingDecoded("/", "notFound");
+
+ tomcat.start();
+
+ ByteChunk res = new ByteChunk();
+ int rc = getUrl("http://localhost:" + getPort(), res, null);
+
+ Assert.assertEquals(HttpServletResponse.SC_NOT_FOUND, rc);
+
+ String body = res.toString();
+ Assert.assertNotNull(body);
+ Assert.assertTrue("Should contain error report",
+ body.contains(String.valueOf(HttpServletResponse.SC_NOT_FOUND)));
+ }
+
+
+ @Test
+ public void testGetSetProperties() {
Review Comment:
Is this (specific) test really necessary?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: [PR] Add unit tests for FilterValve, ProxyErrorReportValve and SemaphoreValve [tomcat]
Posted by "dsoumis (via GitHub)" <gi...@apache.org>.
dsoumis merged PR #976:
URL: https://github.com/apache/tomcat/pull/976
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: [PR] Add unit tests for FilterValve, ProxyErrorReportValve and SemaphoreValve [tomcat]
Posted by "ChristopherSchultz (via GitHub)" <gi...@apache.org>.
ChristopherSchultz commented on PR #976:
URL: https://github.com/apache/tomcat/pull/976#issuecomment-4217798231
> @ChristopherSchultz Those keep all coverage branches green without inflicting any issues. They can be used also as a resource for better understanding how the valves work. Since they are not resource heavy, I would keep them.
Yeah, okay, I guess.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: [PR] Add unit tests for FilterValve, ProxyErrorReportValve and SemaphoreValve [tomcat]
Posted by "dsoumis (via GitHub)" <gi...@apache.org>.
dsoumis commented on PR #976:
URL: https://github.com/apache/tomcat/pull/976#issuecomment-4214292894
@ChristopherSchultz Those keep all coverage branches green without inflicting any issues. They can be used also as a resource for better understanding how the valves work. Since they are not resource heavy, I would keep them.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: [PR] Add unit tests for FilterValve, ProxyErrorReportValve and SemaphoreValve [tomcat]
Posted by "markt-asf (via GitHub)" <gi...@apache.org>.
markt-asf commented on PR #976:
URL: https://github.com/apache/tomcat/pull/976#issuecomment-4206127071
Note: the suggested URI is not the canonical URI. This has already been fixed in all branches.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org