rzo1 opened a new issue, #2084:
URL: https://github.com/apache/stormcrawler/issues/2084
## What happens
`URLFilters.filter()` wraps the whole loop over the configured filters in a
single try/catch. When one filter throws, the catch logs the exception and
execution falls through to `return normalizedUrl`, which still holds the value
produced by the last filter that completed. That value is not null, so callers
such as `StatusEmitterBolt.filterOutlink` treat the URL as accepted. Every
filter positioned after the one that threw is skipped, including the regex
exclusions and `SitemapFilter`, which the archetype places last.
## Where
`core/src/main/java/org/apache/stormcrawler/filtering/URLFilters.java:111-130`.
The same pattern is repeated in the `main()` helper at line 209.
```java
} catch (Exception e) {
LOG.error("URL filtering threw exception", e);
}
return normalizedUrl;
```
## Why it matters
One filter that throws on one URL disables the rest of the chain for that
URL, and the only sign is a log line per URL. The exclusions an operator relies
on to keep the crawler off private ranges and off the `file:` and `ftp:`
schemes sit at the end of the archetype chain, so they are the ones that get
skipped. The shipped chain has no demonstrated thrower today, so this needs a
filter that can throw: `FastURLFilter` is the known one, since `Rule(String)`
at `FastURLFilter.java:337-356` leaves `type` null when the rule line has no
recognised prefix, the file loads without complaint, and
`r.getType().toString()` at line 266 then throws when that rule is evaluated.
## Reproduction
Save as
`core/src/test/java/org/apache/stormcrawler/filtering/URLFiltersExceptionTest.java`.
```java
/*
* Licensed to the Apache Software Foundation (ASF) under one or more
* contributor license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright ownership.
* The ASF licenses this file to you under the Apache License, Version 2.0
* (the "License"); you may not use this file except in compliance with
* the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package org.apache.stormcrawler.filtering;
import java.io.IOException;
import java.net.MalformedURLException;
import java.net.URL;
import java.util.HashMap;
import java.util.Map;
import org.apache.stormcrawler.Metadata;
import org.apache.stormcrawler.util.URLUtil;
import org.jetbrains.annotations.NotNull;
import org.jetbrains.annotations.Nullable;
import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.Test;
/** Behaviour of the filter chain when one of its filters throws. */
class URLFiltersExceptionTest {
/** Stands in for any filter that throws at evaluation time. */
public static class ThrowingURLFilter extends URLFilter {
@Override
public @Nullable String filter(
@Nullable URL sourceUrl,
@Nullable Metadata sourceMetadata,
@NotNull String urlToFilter) {
throw new NullPointerException("filter blew up");
}
}
/** Stands in for an exclusion rule placed after it, such as the
private-range regexes. */
public static class RejectEverythingURLFilter extends URLFilter {
@Override
public @Nullable String filter(
@Nullable URL sourceUrl,
@Nullable Metadata sourceMetadata,
@NotNull String urlToFilter) {
return null;
}
}
@Test
void urlIsRejectedWhenAnEarlierFilterThrows() throws IOException,
MalformedURLException {
Map<String, Object> conf = new HashMap<>();
URLFilters filters = new URLFilters(conf,
"urlfilters-throwing.json");
URL source = URLUtil.toURL("http://www.example.com/index.html";);
Assertions.assertNull(
filters.filter(source, new Metadata(),
"http://www.example.com/outlink.html";));
}
}
```
Save as `core/src/test/resources/urlfilters-throwing.json`.
```json
{
"org.apache.stormcrawler.filtering.URLFilters": [
{
"class":
"org.apache.stormcrawler.filtering.URLFiltersExceptionTest$ThrowingURLFilter",
"name": "ThrowingURLFilter"
},
{
"class":
"org.apache.stormcrawler.filtering.URLFiltersExceptionTest$RejectEverythingURLFilter",
"name": "RejectEverythingURLFilter"
}
]
}
```
Run it:
```
mvn -pl core test -Dtest=URLFiltersExceptionTest
```
The chain is a filter that throws followed by a filter that rejects
everything, so a correct chain returns null. The test fails on main.
```
[ERROR] Tests run: 1, Failures: 1, Errors: 0, Skipped: 0
[ERROR] URLFiltersExceptionTest.urlIsRejectedWhenAnEarlierFilterThrows:62
expected: <null> but was: <http://www.example.com/outlink.html>
```
The exception from the filter appears in the surefire output as the logged
`URL filtering threw exception` line, and the URL is still returned.
## Suggested fix
Move the try/catch inside the loop in `URLFilters.filter()` and treat an
exception from any filter as a rejection: log which filter threw, and return
null. Do the same in `main()` so the command line tool reports the same verdict
as the topology. Add a counter so operators can see how often it happens. This
changes behaviour for topologies that currently have a throwing filter and did
not notice: those URLs start being dropped instead of emitted, which belongs in
the release notes. Separately, make `FastURLFilter.Rule` reject an unrecognised
rule prefix at load time instead of building a rule with a null type.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
