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 @@ -4,6 +4,7 @@
import au.org.aodn.ogcapi.server.core.exception.UnauthorizedServerException;
import au.org.aodn.ogcapi.server.core.model.enumeration.SseEventName;
import lombok.extern.slf4j.Slf4j;
import org.springframework.web.client.HttpStatusCodeException;
import org.springframework.web.servlet.mvc.method.annotation.SseEmitter;

import java.io.IOException;
Expand All @@ -17,6 +18,7 @@ public class WfsErrorHandler {
private static final Set<SseEmitter> handledEmitters = Collections.newSetFromMap(new ConcurrentHashMap<>());

private enum ErrorType {
WFS_SERVER_ERROR,
NETWORK_ERROR,
VALIDATION_ERROR,
UNAUTHORIZED_SERVER_ERROR,
Expand All @@ -43,6 +45,19 @@ public static void handleError(Exception e, String uuid, SseEmitter emitter, Run
}

switch (errorType) {
case WFS_SERVER_ERROR -> {
// ERROR level so New Relic can alert on failed downloads and identify
// the problematic upstream server (the URL is in the exception message).
log.error("WFS server error during download for UUID {}", uuid, e);
emitter.send(SseEmitter.event()
.name(SseEventName.ERROR.getValue())
.data(Map.of(
"message", "WFS server error",
"timestamp", System.currentTimeMillis()
)));
emitter.completeWithError(e);
}

case NETWORK_ERROR -> {
log.warn("Client disconnected for UUID: {}", uuid);
emitter.completeWithError(e);
Expand Down Expand Up @@ -82,7 +97,7 @@ public static void handleError(Exception e, String uuid, SseEmitter emitter, Run
}

case UNKNOWN_ERROR -> {
log.warn("Unknown error for UUID {}", uuid, e);
log.error("Unknown error during WFS download for UUID {}", uuid, e);
emitter.send(SseEmitter.event()
.name(SseEventName.ERROR.getValue())
.data(Map.of(
Expand All @@ -103,6 +118,13 @@ public static void handleError(Exception e, String uuid, SseEmitter emitter, Run
}

private static ErrorType categorizeError(Exception e) {
// Upstream WFS server rejected the request (4xx/5xx). Must be checked before
// the network heuristics below so a failing server is never mistaken for a
// client disconnect.
if (e instanceof HttpStatusCodeException) {
return ErrorType.WFS_SERVER_ERROR;
}

if (e instanceof IOException || e instanceof IllegalStateException || e.getMessage() != null && (
e.getMessage().contains("Broken pipe") ||
e.getMessage().contains("Connection reset") ||
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,9 @@
import org.springframework.cache.annotation.Cacheable;
import org.springframework.context.annotation.Lazy;
import org.springframework.http.*;
import org.springframework.web.client.HttpStatusCodeException;
import org.springframework.web.client.ResourceAccessException;
import org.springframework.web.client.RestClientException;
import org.springframework.web.client.RestTemplate;
import org.springframework.web.servlet.mvc.method.annotation.SseEmitter;

Expand Down Expand Up @@ -104,7 +107,13 @@
uuid, null, null, null, null, layerName, "application/json", 1L, false
);

ResponseEntity<String> response = restTemplate.exchange(countUrl, HttpMethod.GET, pretendUserEntity, String.class);
ResponseEntity<String> response;
try {
response = restTemplate.exchange(countUrl, HttpMethod.GET, pretendUserEntity, String.class);

Check failure

Code scanning / CodeQL

Server-side request forgery Critical

Potential server-side request forgery due to a
user-provided value
.
Potential server-side request forgery due to a
user-provided value
.
Comment thread
NekoLyn marked this conversation as resolved.
Dismissed
} catch (RestClientException e) {
log.error("WFS record count request failed for {}/{} against server url {}", uuid, layerName, countUrl, e);
throw e;
}
if (response.getStatusCode().is2xxSuccessful() && response.getBody() != null) {
try {
JsonNode root = objectMapper.readTree(response.getBody());
Expand Down Expand Up @@ -137,7 +146,13 @@
uuid, null, null, null, null, layerName, outputFormat, sampleSize, false
);

ResponseEntity<byte[]> bytes = restTemplate.exchange(sampleUrl, HttpMethod.GET, pretendUserEntity, byte[].class);
ResponseEntity<byte[]> bytes;
try {
bytes = restTemplate.exchange(sampleUrl, HttpMethod.GET, pretendUserEntity, byte[].class);

Check failure

Code scanning / CodeQL

Server-side request forgery Critical

Potential server-side request forgery due to a
user-provided value
.
Potential server-side request forgery due to a
user-provided value
.
Comment thread
NekoLyn marked this conversation as resolved.
Dismissed
} catch (RestClientException e) {
log.error("WFS sample download failed for {}/{} against server url {}", uuid, layerName, sampleUrl, e);
throw e;
}
if (bytes.getStatusCode().is2xxSuccessful() && bytes.getBody() != null) {
return BigInteger.valueOf(bytes.getBody().length).divide(BigInteger.valueOf(sampleSize));
}
Expand All @@ -164,7 +179,13 @@
uuid, startDate, endDate, multiPolygon, fields, layerName, "application/json", 1L, false
);

ResponseEntity<String> countResponse = restTemplate.exchange(countUrl, HttpMethod.GET, pretendUserEntity, String.class);
ResponseEntity<String> countResponse;
try {
countResponse = restTemplate.exchange(countUrl, HttpMethod.GET, pretendUserEntity, String.class);

Check failure

Code scanning / CodeQL

Server-side request forgery Critical

Potential server-side request forgery due to a
user-provided value
.
Potential server-side request forgery due to a
user-provided value
.
Comment thread
NekoLyn marked this conversation as resolved.
Dismissed
} catch (RestClientException e) {
log.error("WFS estimate count request failed for {}/{} against server url {}", uuid, layerName, countUrl, e);
throw e;
}

if (countResponse.getStatusCode().is2xxSuccessful() && countResponse.getBody() != null) {
try {
Expand Down Expand Up @@ -201,6 +222,31 @@
String outputFormat,
SseEmitter emitter,
AtomicBoolean wfsServerResponded) {
try {
doStreamWfsDataWithSse(wfsRequestUrl, uuid, layerName, outputFormat, emitter, wfsServerResponded);
} catch (HttpStatusCodeException e) {
log.error("WFS download failed for UUID {} layer {}: server url {} returned status {}",
uuid, layerName, wfsRequestUrl, e.getStatusCode(), e);
throw e;
} catch (ResourceAccessException e) {
// A ResourceAccessException raised before the WFS server responded means the
// server itself is unreachable; after it responded, the wrapped IOException
// almost always comes from emitter.send() to a disconnected client.
if (!wfsServerResponded.get()) {
log.error("WFS download failed for UUID {} layer {}: cannot reach server url {}",
uuid, layerName, wfsRequestUrl, e);
}
throw e;
}
}

private void doStreamWfsDataWithSse(
String wfsRequestUrl,
String uuid,
String layerName,
String outputFormat,
SseEmitter emitter,
AtomicBoolean wfsServerResponded) {
restTemplate.execute(
wfsRequestUrl,
HttpMethod.GET,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -377,7 +377,9 @@ public <T> T getFieldValues(String collectionId, WfsFeatureRequest request, Para
}
}
} catch (Exception e) {
log.debug("Ignore error for {}, will try another url", uri);
// WARN so New Relic can identify a problematic WFS server even when a
// later url succeeds and the download recovers.
log.warn("WFS field value query failed for {}, will try another url. Cause: {}", uri, e.toString());
}
}
}
Expand Down Expand Up @@ -419,7 +421,9 @@ public WfsFields getDownloadableFields(String collectionId, WfsFeatureRequest re
}
}
} catch (URISyntaxException | JsonProcessingException | RestClientException e) {
log.debug("Ignore error for {}, will try another url", uri);
// WARN so New Relic can identify a problematic WFS server even when a
// later url succeeds and the download recovers.
log.warn("WFS DescribeFeatureType failed for {}, will try another url. Cause: {}", uri, e.toString());
}
}
} else {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -283,7 +283,7 @@ public SseEmitter estimateWfsDownloadWithSse(String uuid,
"timestamp", System.currentTimeMillis()
));
} catch (Exception e) {
log.warn("Unexpected error during size estimation for UUID {}: {}", uuid, e.getMessage());
log.error("WFS size estimation failed for UUID {} layer {}", uuid, layerName, e);
session.send(SseEventName.ESTIMATE_FAILED, Map.of(
"message", "Size estimation failed: " + e.getMessage(),
"timestamp", System.currentTimeMillis()
Expand Down Expand Up @@ -344,7 +344,7 @@ public SseEmitter estimateCloudOptimisedDownloadWithSse(String uuid,
}

/**
* Shared input validation for the two WFS SSE flows.
* Shared input validation for the two SSE flows.
*/
private void validateWfsSseInputs(String uuid, String layerName, String outputFormat) {
if (uuid == null || layerName == null || layerName.trim().isEmpty()) {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,131 @@
package au.org.aodn.ogcapi.server.core.exception.wfs;

import au.org.aodn.ogcapi.server.core.exception.GeoserverFieldsNotFoundException;
import au.org.aodn.ogcapi.server.core.exception.UnauthorizedServerException;
import au.org.aodn.ogcapi.server.core.util.TestLogAppender;
import org.apache.logging.log4j.Level;
import org.apache.logging.log4j.core.LogEvent;
import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
import org.springframework.http.HttpStatus;
import org.springframework.web.client.HttpServerErrorException;
import org.springframework.web.servlet.mvc.method.annotation.SseEmitter;

import java.io.IOException;
import java.util.List;

import static org.junit.jupiter.api.Assertions.*;
import static org.mockito.Mockito.*;

public class WfsErrorHandlerTest {

protected TestLogAppender logAppender;

@BeforeEach
void attachLogAppender() {
logAppender = TestLogAppender.attachTo(WfsErrorHandler.class);
}

@AfterEach
void detachLogAppender() {
logAppender.detachFrom(WfsErrorHandler.class);
}

/**
* An upstream WFS server error status must be logged at ERROR level (so New
* Relic can pick it up), sent to the client as an SSE error event, and must
* not be mistaken for a client disconnect.
*/
@Test
void verifyUpstreamServerErrorLoggedAtErrorLevel() throws IOException {
SseEmitter emitter = mock(SseEmitter.class);

WfsErrorHandler.handleError(
new HttpServerErrorException(HttpStatus.INTERNAL_SERVER_ERROR),
"uuid-123", emitter, null);

List<LogEvent> errors = logAppender.eventsAtLevel(Level.ERROR);
assertEquals(1, errors.size());
assertTrue(errors.get(0).getMessage().getFormattedMessage().contains("uuid-123"));
assertNotNull(errors.get(0).getThrown(), "stack trace expected in error log");

verify(emitter, times(1)).send(any(SseEmitter.SseEventBuilder.class));
verify(emitter, times(1)).completeWithError(any());
}

/**
* An unexpected exception must be logged at ERROR level too.
*/
@Test
void verifyUnknownErrorLoggedAtErrorLevel() throws IOException {
SseEmitter emitter = mock(SseEmitter.class);

WfsErrorHandler.handleError(new RuntimeException("boom"), "uuid-123", emitter, null);

assertEquals(1, logAppender.eventsAtLevel(Level.ERROR).size());
verify(emitter, times(1)).send(any(SseEmitter.SseEventBuilder.class));
verify(emitter, times(1)).completeWithError(any());
}

/**
* A WFS server that is not authorized must be logged at WARN with the
* exception attached (its message names the server) and reported to the
* client, without raising an ERROR alert.
*/
@Test
void verifyUnauthorizedServerLoggedAtWarnLevel() throws IOException {
SseEmitter emitter = mock(SseEmitter.class);

WfsErrorHandler.handleError(
new UnauthorizedServerException("Server http://not-allowed/wfs is not authorized"),
"uuid-123", emitter, null);

assertTrue(logAppender.eventsAtLevel(Level.ERROR).isEmpty());
List<LogEvent> warns = logAppender.eventsAtLevel(Level.WARN);
assertEquals(1, warns.size());
assertTrue(warns.get(0).getMessage().getFormattedMessage().contains("uuid-123"));
assertNotNull(warns.get(0).getThrown(), "exception expected in log for server identification");

verify(emitter, times(1)).send(any(SseEmitter.SseEventBuilder.class));
verify(emitter, times(1)).completeWithError(any());
}

/**
* No downloadable fields found must be logged at WARN with the exception
* attached and reported to the client, without raising an ERROR alert; the
* per-server failures behind it are logged separately by WfsServer.
*/
@Test
void verifyDownloadableFieldsNotFoundLoggedAtWarnLevel() throws IOException {
SseEmitter emitter = mock(SseEmitter.class);

WfsErrorHandler.handleError(
new GeoserverFieldsNotFoundException("No downloadable fields found for all url"),
"uuid-123", emitter, null);

assertTrue(logAppender.eventsAtLevel(Level.ERROR).isEmpty());
List<LogEvent> warns = logAppender.eventsAtLevel(Level.WARN);
assertEquals(1, warns.size());
assertTrue(warns.get(0).getMessage().getFormattedMessage().contains("uuid-123"));
assertNotNull(warns.get(0).getThrown(), "exception expected in log");

verify(emitter, times(1)).send(any(SseEmitter.SseEventBuilder.class));
verify(emitter, times(1)).completeWithError(any());
}

/**
* A client disconnect stays at WARN level so New Relic error alerting is not
* flooded with failures that are not server problems.
*/
@Test
void verifyClientDisconnectStaysAtWarnLevel() {
SseEmitter emitter = mock(SseEmitter.class);

WfsErrorHandler.handleError(new IOException("Broken pipe"), "uuid-123", emitter, null);

assertTrue(logAppender.eventsAtLevel(Level.ERROR).isEmpty());
assertEquals(1, logAppender.eventsAtLevel(Level.WARN).size());
verify(emitter, times(1)).completeWithError(any());
}
}
Original file line number Diff line number Diff line change
@@ -1,6 +1,10 @@
package au.org.aodn.ogcapi.server.core.service.geoserver.wfs;

import au.org.aodn.ogcapi.server.core.exception.GeoserverFieldsNotFoundException;
import au.org.aodn.ogcapi.server.core.util.TestLogAppender;
import org.apache.logging.log4j.Level;
import org.apache.logging.log4j.core.LogEvent;
import org.springframework.web.client.ResourceAccessException;
import au.org.aodn.stac.model.StacCollectionModel;
import au.org.aodn.stac.model.LinkModel;
import au.org.aodn.ogcapi.server.core.model.ogc.FeatureRequest;
Expand Down Expand Up @@ -311,6 +315,55 @@ public void testGetDownloadableFieldsNetworkError() {
assertTrue(exception.getMessage().contains("Connection timeout"));
}

/**
* An unreachable WFS server during DescribeFeatureType must leave a WARN log
* entry naming the failing url, so New Relic can identify the problematic
* server even though the failure surfaces as "no downloadable fields".
*/
@Test
public void testGetDownloadableFieldsUnreachableServerIsLoggedWithUrl() {
WfsServer.WfsFeatureRequest request = WfsServer.WfsFeatureRequest.builder().layerName("test:layer").build();

ElasticSearchBase.SearchResult<StacCollectionModel> stac = new ElasticSearchBase.SearchResult<>();
stac.setCollections(List.of(
StacCollectionModel.builder()
.links(List.of(
LinkModel.builder()
.href("http://external-geoserver.example.org/geoserver/ows")
.title(request.getLayerName())
.aiGroup(WFS_LINK_MARKER)
.build())
)
.build()
));

String id = "id6";

when(mockSearch.searchCollections(eq(id)))
.thenReturn(stac);

when(restTemplate.exchange(any(String.class), eq(HttpMethod.GET), eq(entity), eq(String.class)))
.thenThrow(new ResourceAccessException("I/O error on GET request: Connection refused"));

WfsServer server = new WfsServer(mockSearch, restTemplate, new RestTemplateUtils(restTemplate), entity, wfsDefaultParam);

TestLogAppender logAppender = TestLogAppender.attachTo(WfsServer.class);
try {
assertThrows(
GeoserverFieldsNotFoundException.class,
() -> server.getDownloadableFields(id, request)
);

List<LogEvent> warns = logAppender.eventsAtLevel(Level.WARN);
assertEquals(1, warns.size(), "Failing server should be logged at WARN");
String message = warns.get(0).getMessage().getFormattedMessage();
assertTrue(message.contains("external-geoserver.example.org"), "Log should name the failing server");
assertTrue(message.contains("Connection refused"), "Log should include the failure cause");
} finally {
logAppender.detachFrom(WfsServer.class);
}
}

@Test
public void testGetDownloadableFieldsNoCollection() {
WfsServer.WfsFeatureRequest request = WfsServer.WfsFeatureRequest.builder().layerName("test:layer").build();
Expand Down
Loading
Loading