Skip to content

#2130 MetadataTransfer: single pass over the metadata for wildcard keys #2133

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
GGraziadei wants to merge 1 commit into
base: main
Choose a base branch
from
+191 −14
Open

#2130 MetadataTransfer: single pass over the metadata for wildcard keys #2133

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
104 changes: 90 additions & 14 deletions core/src/main/java/org/apache/stormcrawler/util/MetadataTransfer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,11 @@

package org.apache.stormcrawler.util;

import java.util.ArrayList;
import java.util.HashMap;
import java.util.HashSet;
import java.util.List;
import java.util.Locale;
import java.util.Map;
import java.util.Set;
import org.apache.commons.lang3.StringUtils;
Expand Down Expand Up @@ -123,7 +127,7 @@ protected void configure(Map<String, Object> conf) {
* the URL path.
*/
public Metadata getMetaForOutlink(String targetUrl, String sourceUrl, Metadata parentMetadata) {
Metadata md = filter(parentMetadata, mdToTransfer);
Metadata md = filter(parentMetadata, transferFilter());

// keep the path?
if (trackPath) {
Expand All @@ -150,11 +154,11 @@ public Metadata getMetaForOutlink(String targetUrl, String sourceUrl, Metadata p
* not necessarily transferred to the outlinks.
*/
public Metadata filter(Metadata metadata) {
Metadata filteredMetadata = filter(metadata, mdToTransfer);
Metadata filteredMetadata = filter(metadata, transferFilter());

// add the features that are only persisted but
// not transferred like __redirTo_
filteredMetadata.putAll(filter(metadata, mdToPersistOnly));
filteredMetadata.putAll(filter(metadata, persistOnlyFilter()));

return filteredMetadata;
}
Expand All @@ -163,20 +167,92 @@ public Metadata filter(Metadata metadata) {
* Filter the metadata based on a set of keys. If a key ends with a * then all the keys starting
* with the prefix will be added.
*/
private Metadata filter(Metadata metadata, Set<String> filter) {
Metadata filteredMetadata = new Metadata();

for (String key : filter) {
if (key.endsWith("*")) {
String prefix = key.substring(0, key.length() - 1);
for (String k : metadata.keySet(prefix)) {
metadata.copy(filteredMetadata, k);
private static Metadata filter(Metadata metadata, CompiledFilter compiled) {
final Map<String, String[]> source = metadata.asMap();
final Map<String, String[]> target = new HashMap<>();

// exact keys: direct lookups
for (String key : compiled.exactKeys) {
final String[] values = source.get(key);
if (values != null && values.length > 0) {
target.put(key, values);
}
}

// wildcards: a single pass over the metadata for all the prefixes,
// without allocating an intermediate key set per prefix
if (compiled.prefixes.length > 0) {
for (Map.Entry<String, String[]> entry : source.entrySet()) {
final String key = entry.getKey();
final String[] values = entry.getValue();
if (values == null || values.length == 0 || target.containsKey(key)) {
continue;
}
for (String prefix : compiled.prefixes) {
if (key.startsWith(prefix)) {
target.put(key, values);
break;
}
}
} else {
metadata.copy(filteredMetadata, key);
}
}

return filteredMetadata;
return new Metadata(target);
}

/**
* Pre-computed, normalised form of a set of keys to transfer: exact keys and wildcard prefixes.
* Keeps a snapshot of the keys it was built from so that a stale instance can be detected.
*/
private static final class CompiledFilter {
private final Set<String> snapshot;
private final Set<String> exactKeys;
private final String[] prefixes;

private CompiledFilter(Set<String> filter) {
this.snapshot = new HashSet<>(filter);
final Set<String> exact = new HashSet<>();
final List<String> prefixList = new ArrayList<>();
for (String key : snapshot) {
final String normalised = key.toLowerCase(Locale.ROOT);
if (normalised.endsWith("*")) {
prefixList.add(normalised.substring(0, normalised.length() - 1));
} else {
exact.add(normalised);
}
}
this.exactKeys = exact;
this.prefixes = prefixList.toArray(new String[0]);
}

/** True if this instance was built from exactly the given keys. */
private boolean isFor(Set<String> filter) {

@rzo1 rzo1 Sep 6, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

return sourceSet == filter && sourceSize == filter.size();

This detects a size change but not a content change of the same size. mdToTransfer is protected final Set<String> with mutable contents, so a subclass doing

mdToTransfer.remove("depth");
mdToTransfer.add("mycustom");

after the first filter() call keeps the size and leaves the compiled filter stale. Every outlink from then on carries the wrong metadata, silently.

The javadoc on line 205 says the cache is "rebuilt if the set has been modified since (e.g. by a subclass)", which is a stronger claim than the code makes good on.

@GGraziadei GGraziadei Sep 6, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Fixed in a9578fd. CompiledFilter now keeps a HashSet snapshot of the keys it was built from and isFor is snapshot.equals(filter), so a same-size content change (remove("depth"); add("mycustom")) rebuilds the compiled form. Javadoc adjusted to describe what the code does. Test testSameSizeMutationOfTransferSetIsHonoured reproduces the exact scenario and failed on the previous revision.

return snapshot.equals(filter);
}
}

// Built lazily on first use and rebuilt whenever the underlying set no longer matches the
// snapshot, so subclasses may keep editing mdToTransfer / mdToPersistOnly at any time. The
// equality check is a handful of hash lookups with no allocation; the compile itself only
// runs when the keys actually changed.
private CompiledFilter compiledTransfer;
private CompiledFilter compiledPersistOnly;

private CompiledFilter transferFilter() {
CompiledFilter compiled = compiledTransfer;
if (compiled == null || !compiled.isFor(mdToTransfer)) {
compiled = new CompiledFilter(mdToTransfer);
compiledTransfer = compiled;
}
return compiled;
}

private CompiledFilter persistOnlyFilter() {
CompiledFilter compiled = compiledPersistOnly;
if (compiled == null || !compiled.isFor(mdToPersistOnly)) {
compiled = new CompiledFilter(mdToPersistOnly);
compiledPersistOnly = compiled;
}
return compiled;
}
}
101 changes: 101 additions & 0 deletions core/src/test/java/org/apache/stormcrawler/util/MetadataTransferTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,4 +152,105 @@ void testFilterWithAsterisk() {
}

static class MyCustomTransferClass extends MetadataTransfer {}

@Test
void testWildcardPrefixIsCaseInsensitiveAndSelective() throws MalformedURLException {

@rzo1 rzo1 Sep 6, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Good addition, and it pins the case-insensitive prefix behaviour.

Nothing covers the caching, which is the part carrying the risk. A test that calls getMetaForOutlink, then mutates mdToTransfer through a subclass without changing its size, then calls it again, would fail today.

@GGraziadei GGraziadei Sep 6, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Added three tests: testSameSizeMutationOfTransferSetIsHonoured (remove/add keeping the size, second call must see the new keys; failed before), testSubclassCanExtendTransferSetInConfigure (subclass adds a wildcard after super.configure()), and testNullValueArrayIsSkipped. Full core verify: 447 tests, 0 failures.

Map<String, Object> conf = new HashMap<>();
conf.put(MetadataTransfer.trackPathParamName, false);
conf.put(MetadataTransfer.trackDepthParamName, false);
conf.put(MetadataTransfer.metadataTransferParamName, List.of("Cookie.*", "exact"));
Metadata parentMD = new Metadata();
parentMD.addValue("cookie.id", "42");
parentMD.addValue("cookies", "not a prefix match");
parentMD.addValue("cook", "no");
parentMD.addValue("exact", "yes");
parentMD.addValue("exactly", "no");
Metadata outlinkMD =
MetadataTransfer.getInstance(conf)
.getMetaForOutlink(
"http://www.example.com/outlink.html",
"http://www.example.com",
parentMD);
Assertions.assertEquals(Set.of("cookie.id", "exact"), outlinkMD.keySet());
Assertions.assertEquals("42", outlinkMD.getFirstValue("cookie.id"));
}

/** Subclass that extends the transfer set after the base configuration, a supported pattern. */
static class ExtendingTransferClass extends MetadataTransfer {
@Override
protected void configure(Map<String, Object> conf) {
super.configure(conf);
mdToTransfer.add("added.*");
}
}

@Test
void testSubclassCanExtendTransferSetInConfigure() throws MalformedURLException {
Map<String, Object> conf = new HashMap<>();
conf.put(MetadataTransfer.trackPathParamName, false);
conf.put(MetadataTransfer.trackDepthParamName, false);
conf.put(
MetadataTransfer.metadataTransferClassParamName,
ExtendingTransferClass.class.getName());
conf.put(MetadataTransfer.metadataTransferParamName, List.of("cookie.*"));
Metadata parentMD = new Metadata();
parentMD.addValue("cookie.id", "42");
parentMD.addValue("added.key", "yes");
parentMD.addValue("other", "no");
Metadata outlinkMD =
MetadataTransfer.getInstance(conf)
.getMetaForOutlink(
"http://www.example.com/outlink.html",
"http://www.example.com",
parentMD);
Assertions.assertEquals(Set.of("cookie.id", "added.key"), outlinkMD.keySet());
}

@Test
void testSameSizeMutationOfTransferSetIsHonoured() throws MalformedURLException {
Map<String, Object> conf = new HashMap<>();
conf.put(MetadataTransfer.trackPathParamName, false);
conf.put(MetadataTransfer.trackDepthParamName, false);
conf.put(MetadataTransfer.metadataTransferParamName, List.of("cookie.*"));
MetadataTransfer mdt = MetadataTransfer.getInstance(conf);
Metadata parentMD = new Metadata();
parentMD.addValue("cookie.id", "42");
parentMD.addValue("other", "yes");
Assertions.assertEquals(
Set.of("cookie.id"),
mdt.getMetaForOutlink(
"http://www.example.com/outlink.html",
"http://www.example.com",
parentMD)
.keySet());
// same size, different content: the compiled filter must not be stale
mdt.mdToTransfer.remove("cookie.*");
mdt.mdToTransfer.add("other");
Assertions.assertEquals(
Set.of("other"),
mdt.getMetaForOutlink(
"http://www.example.com/outlink.html",
"http://www.example.com",
parentMD)
.keySet());
}

@Test
void testNullValueArrayIsSkipped() throws MalformedURLException {
Map<String, Object> conf = new HashMap<>();
conf.put(MetadataTransfer.trackPathParamName, false);
conf.put(MetadataTransfer.trackDepthParamName, false);
conf.put(MetadataTransfer.metadataTransferParamName, List.of("cookie.*", "exact"));
Map<String, String[]> backing = new HashMap<>();
backing.put("cookie.id", new String[] {"42"});
backing.put("cookie.broken", null);
backing.put("exact", null);
Metadata outlinkMD =
MetadataTransfer.getInstance(conf)
.getMetaForOutlink(
"http://www.example.com/outlink.html",
"http://www.example.com",
new Metadata(backing));
Assertions.assertEquals(Set.of("cookie.id"), outlinkMD.keySet());
}
}