Skip to content

Fix #1390: Suppress stack traces for incomplete OSM relations #1431

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
zstadler wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Fix #1390: Suppress stack traces for incomplete OSM relations #1431

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
407e6ba
Fix #1390: Suppress stack traces for incomplete OSM relations
zstadler Jan 5, 2026
c0d08bf
Fix SonarCloud issues and improve test coverage
zstadler Jan 5, 2026
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
50 changes: 48 additions & 2 deletions planetiler-core/src/main/java/com/onthegomap/planetiler/reader/osm/OsmReader.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -233,6 +233,43 @@ record BlockWithResult(OsmBlockSource.Block block, WeightedHandoffQueue<OsmEleme
timer.stop();
}

/**
* Checks if an exception is related to an incomplete OSM relation (missing ways or nodes).
* These are expected in regional extracts and should be logged as warnings without stack traces.
*
* @param e the exception to check
* @return true if the exception is related to incomplete relations
*/
private static boolean isIncompleteRelationException(Throwable e) {
if (e == null) {
return false;
}
String message = e.getMessage();
if (message != null) {
// Check for missing node exceptions
if (message.contains("Missing location for node")) {
return true;
}
// Check for incomplete multipolygon exceptions
if (message.contains("error building multipolygon") ||
message.contains("no rings to process") ||
message.contains("multipolygon not closed") ||
message.contains("missing_way") ||
message.contains("missing node")) {
return true;
}
// Check for GeometryException related to incomplete relations
if (e instanceof GeometryException geometryException) {
String stat = geometryException.stat();
if (stat != null && (stat.contains("invalid_multipolygon") || stat.contains("missing"))) {
return true;
}
}
}
// Check cause chain
return isIncompleteRelationException(e.getCause());
}

void processPass1Blocks(Iterable<? extends Iterable<? extends OsmElement>> blocks) {
// may be called by multiple threads so need to synchronize access to any shared data structures
try (
Expand Down Expand Up @@ -284,7 +321,11 @@ void processPass1Blocks(Iterable<? extends Iterable<? extends OsmElement>> block
}
}
} catch (Exception e) {
LOGGER.error("Error preprocessing OSM relation " + relation.id(), e);
if (isIncompleteRelationException(e)) {
LOGGER.warn("Incomplete OSM relation {}: {}", relation.id(), e.getMessage());
} else {
LOGGER.error("Error preprocessing OSM relation " + relation.id(), e);
}
Comment on lines +324 to +328
Copy link
Contributor

@msbarry msbarry Jan 15, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding this! I already have some similar handling along these lines where these "verbose" exceptions are instances of "GeometryException.Verbose", and GeometryException has a built in geometryException.log("Error processing relation") or geometryException.log("Error processing relation", stats, "osm_relation_error") if we want to include these in the summary of geometry errors that gets printed at the end.

It seems like we might be able to replace this whole PR with just changing these 2 "catch (Exception e)" blocks to something like:

} catch (GeometryException e) {
  e.log("Error processing relation " + relation.id());
} catch (Exception e) {
  LOGGER.error("Error preprocessing OSM relation {}", relation.id(), e);
}

Copy link
Contributor Author

@zstadler zstadler Jan 15, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Frankly, I don't have a strong opinion about the appearance in the log, other than avoiding a scary ERROR and a useless stack trace.

Please feel free to change the PR code in any way you like.

Copy link
Contributor

@msbarry msbarry Jan 20, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually could you share one or a few full stack traces that you're seeing? I tried adding that line but found that GeometryException is actually caught before that point and logged using that method that suppresses verbose logs already.

Copy link
Contributor Author

@zstadler zstadler Jan 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a stack trace in #1390 (comment)

Copy link
Contributor

@msbarry msbarry Jan 25, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh I see, these geometry exceptions are normally caught and logged quietly (or not logged at all) but your profile asks for the area of a feature, and Unit.java calls applyAndWrapException which propagates any geometry exception that would have been suppressed very loudly.

What do you think the fallback should be when area/length fail to compute? Should it return 0? null? or omit the entire feature

Or more broadly, if any of the properties in a yaml file fail with an exception do you think we should just omit that property? or the entire feature?

Copy link
Contributor Author

@zstadler zstadler Jan 25, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are actually two questions here:

  1. Should an incomplete polygon be processed at all? For example, should its attributes be computed?
    IMO, if a potential feature does not have a valid geometry it should be dropped as soon as can be.

  2. How to handle (other) exceptions while computing attribute values?
    IMO, it would be best if there is a clear error message with the OSM feature id and type, the layer.feature.name and the attribute name.
    That should allow users to fix their definition/code.
    Silent drop is generally not a good option. On the other hand, keeping the current stack trace is not very helpful for the user, and an unnecessary support burden for you.

Copy link
Contributor

@msbarry msbarry Jan 30, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK got it, what about this approach: #1454 - it avoids calling attribute processing functions if we already failed to construct the geometry, and also logs any exceptions less verbosely if they do occur?

Copy link
Contributor Author

@zstadler zstadler Jan 30, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand correctly, #1454 should address question 1 in a more general way than this PR.
You may want to use the tests added in this PR.

Question 2 is not addressed by this PR #1431 or the issue it would resolve - #1390.

}
// TODO allow limiting multipolygon storage to only ones that profile cares about
if (isMultipolygon(relation)) {
Expand Down Expand Up @@ -482,7 +523,12 @@ private void render(FeatureCollector.Factory featureCollectors, FeatureRenderer
}
} catch (Exception e) {
String type = element.getClass().getSimpleName();
LOGGER.error("Error processing OSM " + type + " " + element.id(), e);
// For relations, check if it's an incomplete relation exception
if (element instanceof OsmElement.Relation && isIncompleteRelationException(e)) {
LOGGER.warn("Incomplete OSM relation {}: {}", element.id(), e.getMessage());
} else {
LOGGER.error("Error processing OSM " + type + " " + element.id(), e);
}
}
}

Expand Down
215 changes: 215 additions & 0 deletions planetiler-core/src/test/java/com/onthegomap/planetiler/reader/osm/OsmReaderTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,14 @@
import java.util.Map;
import java.util.function.Function;
import java.util.stream.Stream;
import org.apache.logging.log4j.Level;
import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.core.LogEvent;
import org.apache.logging.log4j.core.LoggerContext;
import org.apache.logging.log4j.core.appender.AbstractAppender;
import org.apache.logging.log4j.core.config.Configuration;
import org.apache.logging.log4j.core.config.LoggerConfig;
import java.util.ArrayList;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.ValueSource;
Expand Down Expand Up @@ -649,6 +657,213 @@ void testMultiPolygonRefersToNonexistentWay() {
assertThrows(GeometryException.class, feature::validatedPolygon);
}

/**
* Custom appender to capture log events for testing.
*/
private static class TestAppender extends AbstractAppender implements AutoCloseable {
private final List<LogEvent> logEvents = new ArrayList<>();

protected TestAppender(String name) {
super(name, null, null, false, null);
}

@Override
public void append(LogEvent event) {
logEvents.add(event.toImmutable());
}

public List<LogEvent> getLogEvents() {
return new ArrayList<>(logEvents);
}

@Override
public void close() {
stop();
}
}

@Test
void testIncompleteRelationLoggedAsWarningWithoutStackTrace() {
// Set up TestAppender to capture log events
LoggerContext context = (LoggerContext) LogManager.getContext(false);
Configuration config = context.getConfiguration();
LoggerConfig loggerConfig = config.getLoggerConfig("com.onthegomap.planetiler.reader.osm.OsmReader");
TestAppender testAppender = new TestAppender("TestAppender");
testAppender.start();
loggerConfig.addAppender(testAppender, Level.ALL, null);
context.updateLoggers();

try (testAppender) {
// Create a profile that throws an incomplete relation exception during preprocessing
Profile testProfile = new Profile.NullProfile() {
@Override
public List<OsmRelationInfo> preprocessOsmRelation(OsmElement.Relation relation) {
// Throw an exception that matches incomplete relation pattern
throw new IllegalArgumentException("Missing location for node: 123");
}
};

OsmReader reader = new OsmReader("osm", () -> osmSource, nodeMap, multipolygons, testProfile, stats);
var relation = new OsmElement.Relation(6);
relation.setTag("type", "multipolygon");

// Process the relation - this should trigger the exception and log it as a warning
processPass1Block(reader, List.of(relation));

// Verify that incomplete relation exceptions are logged as WARN without stack trace
List<LogEvent> events = testAppender.getLogEvents();
boolean foundIncompleteWarning = false;
for (LogEvent event : events) {
String message = event.getMessage().getFormattedMessage();
if (message.contains("Incomplete OSM relation")) {
// Should be WARN level, not ERROR
assertEquals(Level.WARN, event.getLevel(),
"Incomplete relation exception should be logged as WARN, not " + event.getLevel());
// Should not have a stack trace (throwable should be null)
assertNull(event.getThrown(),
"Incomplete relation exception should not include stack trace, but got: " + event.getThrown());
foundIncompleteWarning = true;
}
}
assertTrue(foundIncompleteWarning, "Should have logged a warning for incomplete relation. Events: " +
events.stream().map(e -> e.getLevel() + ": " + e.getMessage().getFormattedMessage()).toList());
} finally {
// Clean up
loggerConfig.removeAppender("TestAppender");
context.updateLoggers();
}
}

@Test
void testIncompleteRelationExceptionDetection() {
// Test various incomplete relation exception patterns
assertTrue(isIncompleteRelationException(new IllegalArgumentException("Missing location for node: 123")));
assertTrue(isIncompleteRelationException(new RuntimeException("error building multipolygon 123")));
assertTrue(isIncompleteRelationException(new Exception("no rings to process")));
assertTrue(isIncompleteRelationException(new Exception("multipolygon not closed")));
assertTrue(isIncompleteRelationException(new Exception("missing_way")));
assertTrue(isIncompleteRelationException(new Exception("missing node")));
assertTrue(isIncompleteRelationException(
new GeometryException("osm_invalid_multipolygon", "test")));
assertTrue(isIncompleteRelationException(
new GeometryException("osm_missing_way", "test")));
assertTrue(isIncompleteRelationException(
new RuntimeException("test", new IllegalArgumentException("Missing location for node: 123"))));

// Test non-incomplete exceptions
assertFalse(isIncompleteRelationException(new RuntimeException("Some other error")));
assertFalse(isIncompleteRelationException(new GeometryException("other_error", "test")));
assertFalse(isIncompleteRelationException(null));
assertFalse(isIncompleteRelationException(new Exception())); // null message
}

@Test
void testNonIncompleteRelationExceptionLoggedAsError() {
// Set up TestAppender to capture log events
LoggerContext context = (LoggerContext) LogManager.getContext(false);
Configuration config = context.getConfiguration();
LoggerConfig loggerConfig = config.getLoggerConfig("com.onthegomap.planetiler.reader.osm.OsmReader");
TestAppender testAppender = new TestAppender("TestAppender");
testAppender.start();
loggerConfig.addAppender(testAppender, Level.ALL, null);
context.updateLoggers();

try (testAppender) {
// Create a profile that throws a non-incomplete exception
Profile testProfile = new Profile.NullProfile() {
@Override
public List<OsmRelationInfo> preprocessOsmRelation(OsmElement.Relation relation) {
throw new RuntimeException("Unexpected error");
}
};

OsmReader reader = new OsmReader("osm", () -> osmSource, nodeMap, multipolygons, testProfile, stats);
var relation = new OsmElement.Relation(6);
relation.setTag("type", "multipolygon");

// Process the relation - should log as ERROR with stack trace
processPass1Block(reader, List.of(relation));

// Verify that non-incomplete exceptions are logged as ERROR
List<LogEvent> events = testAppender.getLogEvents();
boolean foundError = false;
for (LogEvent event : events) {
String message = event.getMessage().getFormattedMessage();
if (message.contains("Error preprocessing OSM relation")) {
assertEquals(Level.ERROR, event.getLevel(),
"Non-incomplete exception should be logged as ERROR");
assertNotNull(event.getThrown(),
"Non-incomplete exception should include stack trace");
foundError = true;
}
}
assertTrue(foundError, "Should have logged an error for non-incomplete exception");
} finally {
// Clean up
loggerConfig.removeAppender("TestAppender");
context.updateLoggers();
}
}

@Test
void testIncompleteRelationExceptionWithVariousMessages() {
// Test all the different message patterns that indicate incomplete relations
assertTrue(isIncompleteRelationException(new Exception("error building multipolygon 123")));
assertTrue(isIncompleteRelationException(new Exception("no rings to process")));
assertTrue(isIncompleteRelationException(new Exception("multipolygon not closed")));
assertTrue(isIncompleteRelationException(new Exception("missing_way")));
assertTrue(isIncompleteRelationException(new Exception("missing node")));
}

@Test
void testIncompleteRelationExceptionWithGeometryExceptionStats() {
// Test GeometryException with different stat values
assertTrue(isIncompleteRelationException(
new GeometryException("osm_invalid_multipolygon", "test")));
assertTrue(isIncompleteRelationException(
new GeometryException("osm_missing_way", "test")));
assertTrue(isIncompleteRelationException(
new GeometryException("osm_missing_node", "test")));
// Note: "other_invalid_multipolygon" contains "invalid_multipolygon" so it matches
assertTrue(isIncompleteRelationException(
new GeometryException("other_invalid_multipolygon", "test")));
assertFalse(isIncompleteRelationException(
new GeometryException("osm_other_error", "test")));
}

@Test
void testIncompleteRelationExceptionWithCauseChain() {
// Test exception with cause chain
assertTrue(isIncompleteRelationException(
new RuntimeException("outer", new IllegalArgumentException("Missing location for node: 123"))));
assertTrue(isIncompleteRelationException(
new RuntimeException("outer", new Exception("error building multipolygon"))));
assertFalse(isIncompleteRelationException(
new RuntimeException("outer", new Exception("other error"))));
}

@Test
void testIncompleteRelationExceptionEdgeCases() {
// Test edge cases
assertFalse(isIncompleteRelationException(null));
assertFalse(isIncompleteRelationException(new Exception())); // null message
assertFalse(isIncompleteRelationException(new Exception(""))); // empty message
assertFalse(isIncompleteRelationException(new RuntimeException("Some other error")));
}


// Helper method to access private method for testing
private boolean isIncompleteRelationException(Throwable e) {
// Use reflection to test the private method
try {
var method = OsmReader.class.getDeclaredMethod("isIncompleteRelationException", Throwable.class);
method.setAccessible(true);
return (Boolean) method.invoke(null, e);
} catch (Exception ex) {
throw new RuntimeException(ex);
}
}

@Test
void testWayInRelation() {
record OtherRelInfo(long id) implements OsmRelationInfo {}
Expand Down
Loading