fix get map feature res handler

server/src/main/java/au/org/aodn/ogcapi/server/core/model/ogc/wms/Feature.java

@@ -0,0 +1,23 @@
+package au.org.aodn.ogcapi.server.core.model.ogc.wms;
+
+import com.fasterxml.jackson.dataformat.xml.annotation.JacksonXmlElementWrapper;
+import com.fasterxml.jackson.dataformat.xml.annotation.JacksonXmlProperty;
+import java.util.List;
+import lombok.*;
+
+/**
+ * ncWMS wrap the FeatureInfo inside a Feature element, one per layer.
+ */
+@Getter
+@Setter
+@Builder
+@AllArgsConstructor
+@NoArgsConstructor
+public class Feature {
+    @JacksonXmlProperty(localName = "layer")
+    protected String layer;
+
+    @JacksonXmlProperty(localName = "FeatureInfo")
+    @JacksonXmlElementWrapper(useWrapping = false)
+    protected List<FeatureInfo> featureInfo;
+}

server/src/main/java/au/org/aodn/ogcapi/server/core/model/ogc/wms/FeatureInfo.java

@@ -10,6 +10,9 @@
 @Getter
 @Setter
 public class FeatureInfo {
+    @JacksonXmlProperty(localName = "id")
+    protected String id;
+
     @JacksonXmlProperty(localName = "time")
     protected ZonedDateTime time;
 

server/src/main/java/au/org/aodn/ogcapi/server/core/model/ogc/wms/FeatureInfoResponse.java

@@ -37,4 +37,9 @@ public class FeatureInfoResponse {
     @JacksonXmlProperty(localName = "FeatureInfo")
     @JacksonXmlElementWrapper(useWrapping = false)
     protected List<FeatureInfo> featureInfo;
+
+    // ncWMS put the FeatureInfo one level down, inside a Feature element
+    @JacksonXmlProperty(localName = "Feature")
+    @JacksonXmlElementWrapper(useWrapping = false)
+    protected List<Feature> feature;
 }

server/src/main/java/au/org/aodn/ogcapi/server/core/service/geoserver/wms/WmsServer.java

@@ -42,6 +42,9 @@
 
 @Slf4j
 public class WmsServer {
+    // Max chars of a geoserver response body we put in the log
+    protected static final int LOG_BODY_LIMIT = 2000;
+
     protected final XmlMapper xmlMapper;
 
     @Autowired
@@ -430,36 +433,117 @@
 
         Optional<String> mapServerUrl = getMapServerUrl(collectionId, request);
 
-        if (mapServerUrl.isPresent()) {
-            List<String> urls = createMapFeatureQueryUrl(mapServerUrl.get(), collectionId, request);
-            // Try one by one, we exit when any works
-            for (String url : urls) {
-                ResponseEntity<String> response = restTemplateUtils.handleRedirect(url, restTemplate.exchange(url, HttpMethod.GET, pretendUserEntity, String.class), String.class, pretendUserEntity);
-                if (response.getStatusCode().is2xxSuccessful()) {
-                    // Now try to unify the return
-                    if (MediaType.TEXT_HTML.isCompatibleWith(response.getHeaders().getContentType())) {
-                        String html = response.getBody();
-                        // This is a simple trick to check if the html is in fact empty body, if empty
-                        // try another url
-                        if (html != null && (html.contains("class=\"feature\"") || html.contains("class=\"featureInfo\""))) {
-                            // Some source strangely encode the html tags
-                            return FeatureInfoResponse.builder()
-                                    .html(HtmlUtils.htmlUnescape(html))
-                                    .build();
-                        }
-                    } else if (MediaType.APPLICATION_XML.isCompatibleWith(response.getHeaders().getContentType())) {
-                        FeatureInfoResponse r = xmlMapper.readValue(response.getBody(), FeatureInfoResponse.class);
-                        //  give another url a chance
-                        if (!r.getFeatureInfo().isEmpty()) {
-                            return r;
-                        }
+        if (mapServerUrl.isEmpty()) {
+            log.warn("GetFeatureInfo no wms server url for uuid {} layer {}", collectionId, request.getLayerName());
+            return null;
+        }
+
+        List<String> urls = createMapFeatureQueryUrl(mapServerUrl.get(), collectionId, request);
+
+        if (urls == null || urls.isEmpty()) {
+            log.warn("GetFeatureInfo cannot build query url from {} for uuid {} layer {}", mapServerUrl.get(), collectionId, request.getLayerName());
+            return null;
+        }
+
+        log.debug("GetFeatureInfo request uuid {} layer {} x {} y {} width {} height {} bbox {}",
+                collectionId, request.getLayerName(), request.getX(), request.getY(),
+                request.getWidth(), request.getHeight(), request.getBbox());
+
+        // Try one by one, we exit when any works
+        for (String url : urls) {
+            log.debug("GetFeatureInfo call geoserver {}", url);
+            ResponseEntity<String> response = restTemplateUtils.handleRedirect(url, restTemplate.exchange(url, HttpMethod.GET, pretendUserEntity, String.class), String.class, pretendUserEntity);
+            String body = response.getBody();
+
+            log.debug("GetFeatureInfo response status {} content-type {} body length {}",
+                    response.getStatusCode(), response.getHeaders().getContentType(),
+                    body == null ? 0 : body.length());
+            log.debug("GetFeatureInfo response body {}", truncateForLog(body));
+
+            if (response.getStatusCode().is2xxSuccessful()) {
+                // Now try to unify the return
+                if (MediaType.TEXT_HTML.isCompatibleWith(response.getHeaders().getContentType())) {
+                    // This is a simple trick to check if the html is in fact empty body, if empty
+                    // try another url
+                    if (body != null && (body.contains("class=\"feature\"") || body.contains("class=\"featureInfo\""))) {
+                        // Some source strangely encode the html tags
+                        log.debug("GetFeatureInfo html has feature marker, return it to caller");
+                        return FeatureInfoResponse.builder()
+                                .html(HtmlUtils.htmlUnescape(body))
+                                .build();
+                    }
+                    log.debug("GetFeatureInfo html has no feature marker, skip url {}", url);
+                } else if (isXml(response.getHeaders().getContentType())) {
+                    FeatureInfoResponse r = xmlMapper.readValue(body, FeatureInfoResponse.class);
+                    flattenFeatures(r);
+                    //  give another url a chance
+                    if (r.getFeatureInfo() != null && !r.getFeatureInfo().isEmpty()) {
+                        log.debug("GetFeatureInfo xml parsed {} feature(s), return it to caller", r.getFeatureInfo().size());
+                        return r;
                     }
+                    log.debug("GetFeatureInfo xml has no FeatureInfo element, skip url {}", url);
+                } else {
+                    log.warn("GetFeatureInfo unexpected content-type {} from {}", response.getHeaders().getContentType(), url);
                 }
+            } else {
+                log.warn("GetFeatureInfo failed with status {} from {}", response.getStatusCode(), url);
             }
         }
+
+        log.warn("GetFeatureInfo no usable response for uuid {} layer {}, caller gets empty body", collectionId, request.getLayerName());
         return null;
     }
 
+    /**
+     * Check if the content type is some flavour of xml. Geoserver wms answer text/html, but ncwms answer
+     * text/xml, and some server answer application/xml, so we need to accept all of them.
+     *
+     * @param contentType - The content type from the response header, can be null
+     * @return - True if we can parse the body as xml
+     */
+    protected static boolean isXml(MediaType contentType) {
+        if (contentType == null) {
+            return false;
+        }
+        return MediaType.APPLICATION_XML.isCompatibleWith(contentType)
+                || MediaType.TEXT_XML.isCompatibleWith(contentType)
+                || contentType.getSubtype().endsWith("+xml");
+    }
+
+    /**
+     * ncWMS nest the FeatureInfo inside a Feature element, one Feature per layer. Move them up to the top
+     * level list so the response looks the same no matter which server answered.
+     *
+     * @param response - The parsed response, changed in place
+     */
+    protected static void flattenFeatures(FeatureInfoResponse response) {
+        if (response.getFeatureInfo() != null && !response.getFeatureInfo().isEmpty()) {
+            return;
+        }
+        if (response.getFeature() == null) {
+            return;
+        }
+        response.setFeatureInfo(
+                response.getFeature()
+                        .stream()
+                        .filter(f -> f.getFeatureInfo() != null)
+                        .flatMap(f -> f.getFeatureInfo().stream())
+                        .toList());
+    }
+
+    /**
+     * Cut a response body down to a size that is safe to put in the log.
+     *
+     * @param body - The raw response body, can be null
+     * @return - The body, cut at 2000 chars
+     */
+    protected static String truncateForLog(String body) {
+        if (body == null) {
+            return null;
+        }
+        return body.length() <= LOG_BODY_LIMIT ? body : body.substring(0, LOG_BODY_LIMIT) + "... [truncated]";
+    }
+
     /**
      * Get the wms image/png tile
      *

server/src/main/java/au/org/aodn/ogcapi/server/features/RestServices.java

@@ -93,8 +93,17 @@ public ResponseEntity<DatasetMetadata> getDatasetMetadata(String id) {
 
     public ResponseEntity<FeatureInfoResponse> getWmsMapFeature(String collectionId, FeatureRequest request) {
         try {
-            return ResponseEntity.ok()
-                    .body(wmsServer.getMapFeatures(collectionId, request));
+            FeatureInfoResponse response = wmsServer.getMapFeatures(collectionId, request);
+
+            if (response == null) {
+                log.warn("GetFeatureInfo returns null for uuid {} layer {}, popup will render nothing", collectionId, request.getLayerName());
+            } else {
+                log.debug("GetFeatureInfo returns uuid {} layer {} html length {} featureInfo count {}",
+                        collectionId, request.getLayerName(),
+                        response.getHtml() == null ? 0 : response.getHtml().length(),
+                        response.getFeatureInfo() == null ? 0 : response.getFeatureInfo().size());
+            }
+            return ResponseEntity.ok().body(response);
         } catch (JsonProcessingException | URISyntaxException e) {
             throw new RuntimeException(e);
         }

server/src/test/java/au/org/aodn/ogcapi/server/core/service/geoserver/wms/WmsServerTest.java

@@ -9,6 +9,7 @@
 import au.org.aodn.stac.model.LinkModel;
 import au.org.aodn.ogcapi.server.core.model.ogc.FeatureRequest;
 import au.org.aodn.ogcapi.server.core.model.ogc.wms.DescribeLayerResponse;
+import au.org.aodn.ogcapi.server.core.model.ogc.wms.FeatureInfoResponse;
 import au.org.aodn.ogcapi.server.core.service.ElasticSearchBase;
 import au.org.aodn.ogcapi.server.core.service.Search;
 import au.org.aodn.ogcapi.server.core.service.geoserver.wfs.WfsDefaultParam;
@@ -308,6 +309,48 @@ public void verifyParseCorrect() throws JsonProcessingException {
         assertEquals("imos:srs_ghrsst_l4_gamssa_url", value.getLayerDescription().getQuery().getTypeName());
     }
 
+    /**
+     * ncWMS answer text/xml and nest the FeatureInfo inside a Feature element, make sure we still pick it up.
+     */
+    @Test
+    public void verifyNcwmsFeatureInfoParseCorrect() throws JsonProcessingException {
+        FeatureInfoResponse value = wmsServer.xmlMapper.readValue(
+                """
+                        <FeatureInfoResponse>
+                            <longitude>95.33578364084919</longitude>
+                            <latitude>-20.364301475359852</latitude>
+                            <Feature>
+                                <layer>sea_surface_temperature</layer>
+                                <FeatureInfo>
+                                    <id>sea_surface_temperature</id>
+                                    <value>297.5120817180723</value>
+                                </FeatureInfo>
+                            </Feature>
+                        </FeatureInfoResponse>""", FeatureInfoResponse.class);
+
+        assertEquals(95.33578364084919, value.getLongitude());
+        assertEquals(1, value.getFeature().size());
+        assertEquals("sea_surface_temperature", value.getFeature().get(0).getLayer());
+
+        // Before flatten the top level list is empty, that is why the popup was empty
+        assertNull(value.getFeatureInfo());
+
+        WmsServer.flattenFeatures(value);
+
+        assertEquals(1, value.getFeatureInfo().size());
+        assertEquals("sea_surface_temperature", value.getFeatureInfo().get(0).getId());
+        assertEquals("297.5120817180723", value.getFeatureInfo().get(0).getValue());
+    }
+
+    @Test
+    public void verifyXmlContentTypeAccepted() {
+        assertTrue(WmsServer.isXml(MediaType.parseMediaType("text/xml;charset=ISO-8859-1")));
+        assertTrue(WmsServer.isXml(MediaType.parseMediaType("application/xml")));
+        assertTrue(WmsServer.isXml(MediaType.parseMediaType("application/vnd.ogc.gml+xml")));
+        assertFalse(WmsServer.isXml(MediaType.TEXT_HTML));
+        assertFalse(WmsServer.isXml(null));
+    }
+
     /**
      * Test with only one dateTime field in the describe layer
      */