[M0nd0R]

Fixes WW-5821

An unknown HTTP method reaches HttpMethodInterceptor and produces a 500.

HttpMethod.parse() is valueOf(method.toUpperCase()) with no fallback, and the interceptor calls it with the raw request method for any HttpMethodAware action — before the allow-checks, and with no try/catch:

HttpMethod method = HttpMethod.parse(request.getMethod());

The enum has nine constants, so PROPFIND, MKCOL, REPORT or any invented token raises IllegalArgumentException instead of being treated as "not allowed". When http-method is configured the code already knows how to answer that case — 405 with an Allow header — it just never gets there.

HttpMethod.parseOrNull() returns null for an unrecognised method (case-insensitive lookup, no exception). The interceptor always calls HttpMethodAware#setMethod, passing null for an unknown method, and then falls through to the existing isAllowed() check. XmlDocConfigurationProvider.buildHttpMethods now uses HttpMethod.parse, so there is a single lookup rule.

Repro: any action with http-method set, request PROPFIND /that-action → 500 before, 405 + Allow after.

Tests: HttpMethodTest and HttpMethodInterceptorTest (including testHttpMethodAwareActionWithUnknownRequestMethod, which drives the interceptor path) pass.

[lukaszlenart]

Thanks for the fix and the clear write-up. I reproduced the bug with an HttpMethodAware action and a PROPFIND request: IllegalArgumentException on main, 405 with Allow: POST on this branch. This is an ordinary bug and the public PR is the right place for it.

Before we go further, please create a Jira ticket at https://issues.apache.org/jira/projects/WW (type Bug, component Core Interceptors) and retitle the PR and the commit as WW-XXXX Handle an unknown HTTP method instead of raising IllegalArgumentException. If you would rather not create an account, say so and I will file it and give you the key.

The inline comments cover the rest. The one I would not merge without is the interceptor-level regression test.

[lukaszlenart]

The four new tests exercise the enum directly, so none of them reach this line. I reverted this hunk locally and HttpMethodTest + HttpMethodInterceptorTest still pass. The existing testConfigRejectsUnknownRequestMethod uses TestAction, which is not HttpMethodAware, so it never hit the old parse() call.

Please add a test to HttpMethodInterceptorTest that drives the fixed path, along the lines of:

public void testHttpMethodAwareActionWithUnknownRequestMethod() throws Exception {
    HttpMethodsTestAction action = new HttpMethodsTestAction();
    prepareActionInvocation(action, actionConfig());
    MockHttpServletResponse response = prepareRequest("PROPFIND");

    String resultName = interceptor.intercept(invocation);

    assertEquals(Action.NONE, resultName);
    assertEquals(HttpServletResponse.SC_METHOD_NOT_ALLOWED, response.getStatus());
    assertEquals("POST", response.getHeader("Allow"));
    assertNull(action.getHttpMethod());
}

[lukaszlenart]

Skipping the call changes the HttpMethodAware contract: its Javadoc says the action is notified about the request method, and an action instance that is reused (a Spring singleton, a custom ObjectFactory) would keep the previous request's method. I would call setMethod(requestedHttpMethod) unconditionally and add a line to HttpMethodAware#setMethod saying null means the request method is not one of the enum constants.

The debug line just above also still says "setting request method: PROPFIND" when nothing is set; worth adjusting once the null is passed through.

[lukaszlenart]

After this change parse() has no caller in src/main: XmlDocConfigurationProvider.buildHttpMethods calls valueOf(value.toUpperCase(Locale.ROOT)) directly. The new shouldStillRejectUnknownMethodInConfiguration test describes parse() as the configuration path, which it currently is not.

Simplest fix: have buildHttpMethods call HttpMethod.parse(value) so there is one lookup and one locale rule, and the test's claim becomes true.

[lukaszlenart]

Minor: catching IllegalArgumentException on a client-driven path uses the exception as control flow. The interceptor already matches by name with equalsIgnoreCase in isAllowed, so a lookup in the same style keeps the two consistent and drops the locale question:

return Arrays.stream(values())
    .filter(method -> method.name().equalsIgnoreCase(httpRequestMethod))
    .findFirst()
    .orElse(null);

[lukaszlenart]

Minor: the second paragraph is the rationale from the PR description, which is where it is best kept. A one-liner is enough here: @return the matching method, or null when the request method is not one of the constants.

[lukaszlenart]

Minor: JUnit 4.13 has assertThrows, which reads as the intent and fails with a clear message:

assertThrows(IllegalArgumentException.class, () -> HttpMethod.parse("PROPFIND"));

[M0nd0R]

Thanks for the review. Pushed an update addressing the inline comments:

• Added testHttpMethodAwareActionWithUnknownRequestMethod to HttpMethodInterceptorTest (405 + Allow: POST, action's method stays null).
• The interceptor now always calls setMethod(...), passing null for an unknown method; HttpMethodAware#setMethod Javadoc documents this and the debug line shows the parsed value.
• parseOrNull uses a case-insensitive lookup instead of catching IllegalArgumentException; Javadoc trimmed to a @return line.
• XmlDocConfigurationProvider.buildHttpMethods now calls HttpMethod.parse; the config test uses assertThrows.

HttpMethodTest and HttpMethodInterceptorTest pass locally (6 and 30 tests).

On the Jira ticket / WW-XXXX title: I don't have a Jira account, so if you're happy to file it, please share the key and I'll retitle the PR and commit.

[lukaszlenart]

Thanks, the update covers everything. I ran HttpMethodTest, HttpMethodInterceptorTest and XmlConfigurationProviderActionsTest against 0af7770 and they pass.

I filed WW-5821. Please retitle the PR as WW-5821 Handle an unknown HTTP method instead of raising IllegalArgumentException, prefix the commit messages with WW-5821, and add Fixes [WW-5821](https://issues.apache.org/jira/browse/WW-5821) to the description.

[M0nd0R]

Thanks for filing WW-5821 and for running the tests. Done: the PR is retitled, both commit messages are prefixed with WW-5821 (branch force-pushed; the code is unchanged), and the description now has the Fixes line and describes the final behaviour.