Skip to content

Commit abd17fa

Browse files
atoato
authored
Merge pull request #767 from internetarchive/adam/improve_amqp_url_parsing
chore: refactor AMQPUrlReceiver parsing to handle null values coming …
2 parents 0d3582a + d153116 commit abd17fa

2 files changed

Lines changed: 221 additions & 7 deletions

File tree

‎contrib/src/main/java/org/archive/crawler/frontier/AMQPUrlReceiver.java‎

Lines changed: 37 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,8 @@
3232
import java.util.logging.Level;
3333
import java.util.logging.Logger;
3434

35+
import static org.archive.modules.CoreAttributeConstants.A_HERITABLE_KEYS;
36+
3537
import org.archive.url.URIException;
3638
import org.archive.crawler.event.AMQPUrlReceivedEvent;
3739
import org.archive.crawler.event.CrawlStateEvent;
@@ -441,22 +443,50 @@ protected CrawlURI makeCrawlUri(JSONObject jo) throws URIException,
441443
}
442444

443445
// set the heritable data from the parent url, passed back to us via amqp
444-
// XXX brittle, only goes one level deep, and only handles strings and arrays, the latter of which it converts to a Set.
446+
// XXX brittle, only goes one level deep.
445447
// 'heritableData': {'source': 'https://facebook.com/whitehouse/', 'heritable': ['source', 'heritable']}
448+
//
449+
// Values must be strings, apart from the
450+
// A_HERITABLE_KEYS list.
446451
@SuppressWarnings("unchecked")
447452
protected void populateHeritableMetadata(CrawlURI curi, JSONObject parentUrlMetadata) {
448453
JSONObject heritableData = parentUrlMetadata.getJSONObject("heritableData");
449454
for (String key: (Set<String>) heritableData.keySet()) {
450455
Object value = heritableData.get(key);
451-
if (value instanceof JSONArray) {
452-
Set<String> valueSet = new HashSet<String>();
453-
JSONArray arr = ((JSONArray) value);
456+
457+
if (A_HERITABLE_KEYS.equals(key)) {
458+
// the set of key names to pass on to descendants; must stay a
459+
// HashSet<String>, which is what CrawlURI casts it to
460+
if (!(value instanceof JSONArray)) {
461+
logger.warning("ignoring non-array '" + A_HERITABLE_KEYS
462+
+ "' received via AMQP: " + value);
463+
continue;
464+
}
465+
JSONArray arr = (JSONArray) value;
466+
HashSet<String> keyNames = new HashSet<String>();
454467
for (int i = 0; i < arr.length(); i++) {
455-
valueSet.add(arr.getString(i));
468+
Object element = arr.get(i);
469+
if (element instanceof String) {
470+
keyNames.add((String) element);
471+
} else {
472+
logger.fine("skipping non-string element in '"
473+
+ A_HERITABLE_KEYS + "' received via AMQP: "
474+
+ element);
475+
}
476+
}
477+
// Don't store an empty set. CrawlURI.makeHeritable() only
478+
// self-registers A_HERITABLE_KEYS when it creates the set, so
479+
// an empty one left in place would later yield a set that
480+
// doesn't name itself -- inheritance would then stop after a
481+
// single hop.
482+
if (!keyNames.isEmpty()) {
483+
curi.getData().put(key, keyNames);
456484
}
457-
curi.getData().put(key, valueSet);
485+
} else if (value instanceof String) {
486+
curi.getData().put(key, value);
458487
} else {
459-
curi.getData().put(key, heritableData.get(key));
488+
logger.fine("skipping non-string value received via AMQP: "
489+
+ key + "=" + value);
460490
}
461491
}
462492
}

‎contrib/src/test/java/org/archive/crawler/frontier/AMQPUrlReceiverTest.java‎

Lines changed: 184 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,184 @@
1+
/*
2+
* This file is part of the Heritrix web crawler (crawler.archive.org).
3+
*
4+
* Licensed to the Internet Archive (IA) by one or more individual
5+
* contributors.
6+
*
7+
* The IA licenses this file to You under the Apache License, Version 2.0
8+
* (the "License"); you may not use this file except in compliance with
9+
* the License. You may obtain a copy of the License at
10+
*
11+
* http://www.apache.org/licenses/LICENSE-2.0
12+
*
13+
* Unless required by applicable law or agreed to in writing, software
14+
* distributed under the License is distributed on an "AS IS" BASIS,
15+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
16+
* See the License for the specific language governing permissions and
17+
* limitations under the License.
18+
*/
19+
20+
package org.archive.crawler.frontier;
21+
22+
import static org.archive.modules.CoreAttributeConstants.A_HERITABLE_KEYS;
23+
import static org.archive.modules.CoreAttributeConstants.A_SOURCE_TAG;
24+
import static org.junit.jupiter.api.Assertions.assertEquals;
25+
import static org.junit.jupiter.api.Assertions.assertFalse;
26+
import static org.junit.jupiter.api.Assertions.assertNull;
27+
import static org.junit.jupiter.api.Assertions.assertTrue;
28+
29+
import java.util.HashSet;
30+
31+
import org.archive.modules.CrawlURI;
32+
import org.archive.modules.extractor.Hop;
33+
import org.archive.modules.extractor.LinkContext;
34+
import org.json.JSONObject;
35+
import org.junit.jupiter.api.Test;
36+
37+
/**
38+
* Tests {@link AMQPUrlReceiver.UrlConsumer#populateHeritableMetadata}, which
39+
* copies the {@code heritableData} of an AMQP message into a CrawlURI.
40+
*/
41+
public class AMQPUrlReceiverTest {
42+
43+
protected static final String SEED = "https://example.org/seed";
44+
45+
/** Invokes populateHeritableMetadata with the given heritableData json. */
46+
protected CrawlURI populate(String heritableDataJson) throws Exception {
47+
AMQPUrlReceiver receiver = new AMQPUrlReceiver();
48+
AMQPUrlReceiver.UrlConsumer consumer = receiver.new UrlConsumer(null);
49+
50+
JSONObject parentUrlMetadata = new JSONObject(
51+
"{\"pathFromSeed\": \"L\", \"heritableData\": " + heritableDataJson + "}");
52+
53+
CrawlURI curi = CrawlURI.fromHopsViaString("https://example.org/page LL");
54+
consumer.populateHeritableMetadata(curi, parentUrlMetadata);
55+
return curi;
56+
}
57+
58+
@SuppressWarnings("unchecked")
59+
protected HashSet<String> heritableKeysOf(CrawlURI curi) {
60+
return (HashSet<String>) curi.getData().get(A_HERITABLE_KEYS);
61+
}
62+
63+
// ---------------------------------------------------------------
64+
// the normal states, which must not regress
65+
// ---------------------------------------------------------------
66+
67+
/**
68+
* A source-tagged parent: the tag is stored and the heritable key set is
69+
* rebuilt as a HashSet, which is the concrete type CrawlURI casts it to.
70+
*/
71+
@Test
72+
public void testValidSourceIsStored() throws Exception {
73+
CrawlURI curi = populate(
74+
"{\"source\": \"" + SEED + "\", \"heritable\": [\"source\", \"heritable\"]}");
75+
76+
assertTrue(curi.containsDataKey(A_SOURCE_TAG));
77+
assertEquals(SEED, curi.getSourceTag());
78+
79+
HashSet<String> heritable = heritableKeysOf(curi);
80+
assertEquals(new HashSet<>(java.util.Arrays.asList("source", "heritable")), heritable);
81+
}
82+
83+
/**
84+
* The heritable key set must keep naming itself, otherwise inheritance
85+
* stops after a single hop. Checks a grandchild, not just a child.
86+
*/
87+
@Test
88+
public void testSourceTagIsInheritedBeyondOneHop() throws Exception {
89+
CrawlURI curi = populate(
90+
"{\"source\": \"" + SEED + "\", \"heritable\": [\"source\", \"heritable\"]}");
91+
92+
CrawlURI child = curi.createCrawlURI(
93+
"https://example.org/child", LinkContext.NAVLINK_MISC, Hop.NAVLINK);
94+
assertEquals(SEED, child.getSourceTag());
95+
96+
CrawlURI grandchild = child.createCrawlURI(
97+
"https://example.org/grandchild", LinkContext.NAVLINK_MISC, Hop.NAVLINK);
98+
assertEquals(SEED, grandchild.getSourceTag());
99+
}
100+
101+
/** An untagged parent yields neither key -- absence is a supported state. */
102+
@Test
103+
public void testEmptyHeritableDataYieldsNothing() throws Exception {
104+
CrawlURI curi = populate("{}");
105+
106+
assertFalse(curi.containsDataKey(A_SOURCE_TAG));
107+
assertFalse(curi.containsDataKey(A_HERITABLE_KEYS));
108+
}
109+
110+
/**
111+
* Output for a parent with no source tag. The key must be
112+
* absent, not present-and-null: downstream readers such as
113+
* StatisticsTracker guard on containsDataKey(), so a present key with an
114+
* unusable value is what does the damage.
115+
*/
116+
@Test
117+
public void testNullSourceIsSkipped() throws Exception {
118+
CrawlURI curi = populate("{\"source\": null, \"heritable\": []}");
119+
120+
assertFalse(curi.containsDataKey(A_SOURCE_TAG));
121+
assertNull(curi.getSourceTag());
122+
}
123+
124+
/**
125+
* The same, one hop on. CrawlURI.inheritFrom() copies every name in the
126+
* heritable set with a bare get(), so a skipped value paired with a set that
127+
* still named it would reintroduce the key with a java null.
128+
*/
129+
@Test
130+
public void testSkippedSourceIsNotReintroducedByInheritance() throws Exception {
131+
CrawlURI curi = populate("{\"source\": null, \"heritable\": []}");
132+
133+
CrawlURI child = curi.createCrawlURI(
134+
"https://example.org/child", LinkContext.NAVLINK_MISC, Hop.NAVLINK);
135+
136+
assertFalse(child.containsDataKey(A_SOURCE_TAG));
137+
}
138+
139+
@Test
140+
public void testNonStringScalarSourceIsSkipped() throws Exception {
141+
assertFalse(populate("{\"source\": 12345}").containsDataKey(A_SOURCE_TAG));
142+
assertFalse(populate("{\"source\": true}").containsDataKey(A_SOURCE_TAG));
143+
assertFalse(populate("{\"source\": {\"a\": \"b\"}}").containsDataKey(A_SOURCE_TAG));
144+
assertFalse(populate("{\"source\": [\"a\"]}").containsDataKey(A_SOURCE_TAG));
145+
}
146+
147+
/** Non-string elements are dropped individually, not the whole set. */
148+
@Test
149+
public void testNonStringHeritableElementsAreSkipped() throws Exception {
150+
CrawlURI curi = populate(
151+
"{\"heritable\": [\"source\", null, 7, \"heritable\"]}");
152+
153+
assertEquals(new HashSet<>(java.util.Arrays.asList("source", "heritable")),
154+
heritableKeysOf(curi));
155+
}
156+
157+
/**
158+
* An empty set must not be stored: CrawlURI.makeHeritable() only registers
159+
* A_HERITABLE_KEYS in the set it creates, so leaving an empty one in place
160+
* would later produce a set that doesn't name itself.
161+
*/
162+
@Test
163+
public void testEmptyHeritableArrayIsNotStored() throws Exception {
164+
assertFalse(populate("{\"heritable\": []}").containsDataKey(A_HERITABLE_KEYS));
165+
}
166+
167+
/** A heritable value of the wrong shape is ignored rather than cast. */
168+
@Test
169+
public void testNonArrayHeritableIsIgnored() throws Exception {
170+
assertFalse(populate("{\"heritable\": \"source\"}").containsDataKey(A_HERITABLE_KEYS));
171+
assertFalse(populate("{\"heritable\": null}").containsDataKey(A_HERITABLE_KEYS));
172+
}
173+
174+
/** Other string-valued heritable keys still pass through untouched. */
175+
@Test
176+
public void testOtherStringValuesAreStored() throws Exception {
177+
CrawlURI curi = populate(
178+
"{\"source\": \"" + SEED + "\", \"someOtherKey\": \"someValue\","
179+
+ " \"heritable\": [\"source\", \"someOtherKey\", \"heritable\"]}");
180+
181+
assertEquals("someValue", curi.getData().get("someOtherKey"));
182+
assertEquals(SEED, curi.getSourceTag());
183+
}
184+
}

0 commit comments

Comments
 (0)