Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
import org.hisp.dhis.webapi.security.config.WebMvcConfig;
import org.hisp.dhis.webapi.view.SuffixMediaTypeContentNegotiationStrategy;
import org.springframework.http.MediaType;
import org.springframework.web.HttpMediaTypeNotAcceptableException;
import org.springframework.web.bind.annotation.RequestMethod;
import org.springframework.web.method.HandlerMethod;
import org.springframework.web.servlet.HandlerMapping;
Expand All @@ -53,12 +54,14 @@
*
* <p>It also reinstates path-extension and trailing-slash matching without relying on Spring's
* removed/deprecated handler-mapping flags ({@code setUseSuffixPatternMatch} / {@code
* setUseTrailingSlashMatch} / {@code favorPathExtension}). For each request the literal path is
* matched first so controllers that map an extension literally (e.g. OpenAPI's {@code
* /openapi/openapi.json}) keep working. Only if that has no handler do we fall back to matching the
* path with a trailing slash and/or a registered media-type extension removed (e.g. {@code
* /api/dataElements.json} to {@code /api/dataElements}), recording the resolved media type for
* {@link SuffixMediaTypeContentNegotiationStrategy}.
* setUseTrailingSlashMatch} / {@code favorPathExtension}). For each request that ends with a
* registered media-type extension (or trailing slash), the suffix-stripped path is matched first so
* content negotiation works and generic {@code /{property}} handlers do not swallow names like
* {@code metadata.json}. If the stripped lookup finds no handler, or finds one whose {@code
* produces} cannot satisfy the extension-derived media type ({@code 406}), we fall back to the
* original path so controllers that map an extension literally (analytics {@code .xml}/{@code .csv}
* downloads, OpenAPI's {@code /openapi/openapi.json}) keep working. The resolved media type is
* recorded for {@link SuffixMediaTypeContentNegotiationStrategy}.
*
* <p>Forward-compatible on Spring 6.2 (Spring 7 readiness PR-F).
*/
Expand Down Expand Up @@ -90,17 +93,32 @@
@Override
protected HandlerMethod getHandlerInternal(HttpServletRequest request) throws Exception {
// When the path ends with a registered media-type extension (e.g. .json / .json.zip), prefer
// matching the suffix-stripped path so content negotiation works and generic /{property}

Check warning on line 96 in dhis-2/dhis-web-api/src/main/java/org/hisp/dhis/webapi/mvc/CustomRequestMappingHandlerMapping.java

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

This block of commented-out lines of code should be removed.

See more on https://sonarcloud.io/project/issues?id=dhis2_dhis2-core&issues=AZ9sxol5NczkvXLwIAZ1&open=AZ9sxol5NczkvXLwIAZ1&pullRequest=24479
// handlers do not swallow "metadata.json" as a property name. If that fails (literal-extension
// controller mappings such as /openapi/openapi.json), fall back to the original path.
// controller mappings such as /openapi/openapi.json), or the stripped path matches a handler
// whose produces cannot satisfy the extension-derived media type (406 - common for analytics
// download endpoints that map .xml/.csv/... literally next to a JSON produces handler), fall
// back to the original path.
//
// Paths without a registered extension: match as-is, then fall back to trailing-slash strip.
HttpServletRequest normalized = normalize(request);
if (normalized != request) {
clearAndReparsePathCaches(normalized);
HandlerMethod stripped = super.getHandlerInternal(normalized);
if (stripped != null) {
return stripped;
try {
HandlerMethod stripped = super.getHandlerInternal(normalized);
if (stripped != null) {
return stripped;
}
} catch (HttpMediaTypeNotAcceptableException notAcceptable) {
// Stripped path matched a handler, but its produces does not include the media type forced
// by the path extension. Prefer a literal-suffix mapping when one exists; otherwise rethrow
// so the client still gets 406 rather than a misleading 404.
clearAndReparsePathCaches(request);
HandlerMethod literal = super.getHandlerInternal(request);
if (literal != null) {
return literal;
}
throw notAcceptable;
}
// No stripped handler - try the original path (literal extension mappings).
clearAndReparsePathCaches(request);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,8 @@
import static org.junit.jupiter.api.Assertions.assertNotNull;
import static org.junit.jupiter.api.Assertions.assertNull;
import static org.junit.jupiter.api.Assertions.assertSame;
import static org.junit.jupiter.api.Assertions.assertThrows;
import static org.springframework.http.MediaType.APPLICATION_JSON_VALUE;

import jakarta.servlet.http.HttpServletRequest;
import java.lang.reflect.Method;
Expand All @@ -45,6 +47,8 @@
import org.springframework.http.MediaType;
import org.springframework.mock.web.MockHttpServletRequest;
import org.springframework.stereotype.Controller;
import org.springframework.web.HttpMediaTypeNotAcceptableException;
import org.springframework.web.accept.ContentNegotiationManager;
import org.springframework.web.bind.annotation.GetMapping;
import org.springframework.web.bind.annotation.RequestMapping;
import org.springframework.web.method.HandlerMethod;
Expand All @@ -65,6 +69,10 @@ class CustomRequestMappingHandlerMappingNormalizeTest {
void setUp() throws Exception {
mapping = new CustomRequestMappingHandlerMapping();
mapping.setApplicationContext(new StaticApplicationContext());
// Needed so produces conditions evaluate the suffix-forced media type the same way production
// does via SuffixMediaTypeContentNegotiationStrategy (PR-F / #24463).
mapping.setContentNegotiationManager(
new ContentNegotiationManager(new SuffixMediaTypeContentNegotiationStrategy()));
mapping.afterPropertiesSet();

Method method = SampleController.class.getMethod("list");
Expand All @@ -78,6 +86,24 @@ void setUp() throws Exception {
mapping.getMappingForMethod(openapi, SampleController.class),
new SampleController(),
openapi);

Method queryJson = AnalyticsStyleController.class.getMethod("queryJson", String.class);
mapping.registerMapping(
mapping.getMappingForMethod(queryJson, AnalyticsStyleController.class),
new AnalyticsStyleController(),
queryJson);

Method queryXml = AnalyticsStyleController.class.getMethod("queryXml", String.class);
mapping.registerMapping(
mapping.getMappingForMethod(queryXml, AnalyticsStyleController.class),
new AnalyticsStyleController(),
queryXml);

Method onlyJson = AnalyticsStyleController.class.getMethod("onlyJson", String.class);
mapping.registerMapping(
mapping.getMappingForMethod(onlyJson, AnalyticsStyleController.class),
new AnalyticsStyleController(),
onlyJson);
}

@Test
Expand All @@ -93,6 +119,31 @@ void literalExtensionMappingWinsWhenStrippedPathHasNoHandler() throws Exception
SuffixMediaTypeContentNegotiationStrategy.SUFFIX_MEDIA_TYPE_ATTRIBUTE));
}

/**
* Regression for analytics-style dual mappings: JSON produces on {@code /query/{id}} plus a
* literal {@code /query/{id}.xml} download handler. Stripping the suffix must not surface the
* JSON handler's 406 and block the literal download mapping (CI failure after #24463).
*/
@Test
void literalDownloadMappingWinsWhenStrippedProducesIsIncompatible() throws Exception {
MockHttpServletRequest request =
request("/api/analytics/trackedEntities/query/nEenWmSyUEp.xml");
HandlerMethod handler = mapping.getHandlerInternal(request);
assertNotNull(handler);
assertEquals("queryXml", handler.getMethod().getName());
assertEquals(
MediaType.APPLICATION_XML,
request.getAttribute(
SuffixMediaTypeContentNegotiationStrategy.SUFFIX_MEDIA_TYPE_ATTRIBUTE));
}

@Test
void rethrowsNotAcceptableWhenNoLiteralFallbackExists() {
MockHttpServletRequest request = request("/api/analytics/only-json/nEenWmSyUEp.xml");
assertThrows(
HttpMediaTypeNotAcceptableException.class, () -> mapping.getHandlerInternal(request));
}

@ParameterizedTest
@CsvSource({
"/api/dataElements.json, application/json",
Expand Down Expand Up @@ -190,4 +241,31 @@ public String openapi() {
return "openapi";
}
}

/**
* Mirrors the analytics download pattern: a generic path with restricted {@code produces} next to
* a literal-suffix download mapping with no produces restriction.
*/
@Controller
@RequestMapping
static class AnalyticsStyleController {
@GetMapping(
value = "/api/analytics/trackedEntities/query/{trackedEntityType}",
produces = {APPLICATION_JSON_VALUE, "application/javascript"})
public String queryJson(String trackedEntityType) {
return "json";
}

@GetMapping(value = "/api/analytics/trackedEntities/query/{trackedEntityType}.xml")
public String queryXml(String trackedEntityType) {
return "xml";
}

@GetMapping(
value = "/api/analytics/only-json/{id}",
produces = {APPLICATION_JSON_VALUE})
public String onlyJson(String id) {
return "json-only";
}
}
}
Loading