Skip to content

Align observationProperties to JSON-LD reference objects and fix custom namespace lookups #576

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

Closed
clincoln8 wants to merge 1 commit into from
Closed

Align observationProperties to JSON-LD reference objects and fix custom namespace lookups #576

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
47 changes: 47 additions & 0 deletions pipeline/util/src/test/java/org/datacommons/ingestion/util/GraphReaderTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -604,4 +604,51 @@ public void testExtractTimeSeries_MissingEntity() {
// This should throw RuntimeException because sourceCountry is missing
GraphReader.extractTimeSeries("nodeId", pv, "test_import", true);
}

@Test
public void testExtractTimeSeries_JsonLd_MultiEntity_Integration() throws Exception {
String jsonLd =
"{\n"
+ " \"@context\": {\n"
+ " \"custom\": \"https://customsource.org/schema/\",\n"
+ " \"dcid\": \"https://datacommons.org/browser/\"\n"
+ " },\n"
+ " \"@graph\": [\n"
+ " {\n"
+ " \"@id\": \"dcid:TestObs\",\n"
+ " \"@type\": \"dcid:StatVarObservation\",\n"
+ " \"dcid:variableMeasured\": {\n"
+ " \"@id\": \"custom:FinancialTrade\",\n"
+ " \"dcid:observationProperties\": [\n"
+ " \"custom:destinationCountry\",\n"
+ " \"custom:sourceCountry\"\n"
+ " ]\n"
+ " },\n"
+ " \"dcid:observationDate\": \"2024\",\n"
+ " \"dcid:value\": 100,\n"
+ " \"custom:sourceCountry\": { \"@id\": \"dcid:country/FRA\" },\n"
+ " \"custom:destinationCountry\": { \"@id\": \"dcid:country/USA\" }\n"
+ " }\n"
+ " ]\n"
+ "}";

java.io.InputStream in =
new java.io.ByteArrayInputStream(jsonLd.getBytes(java.nio.charset.StandardCharsets.UTF_8));
McfGraph graph = org.datacommons.util.parser.jsonld.JsonLdParser.parse(in);

PropertyValues pv = graph.getNodesMap().get("TestObs");
org.junit.Assert.assertNotNull(pv);

TimeSeries expected =
TimeSeries.builder()
.entity1("country/USA")
.extraEntities(List.of("country/FRA"))
.variableMeasured("FinancialTrade")
.importName("test_import")
.isBaseDc(true)
.build();

TimeSeries actual = GraphReader.extractTimeSeries("TestObs", pv, "test_import", true);
assertEquals(expected, actual);
}
}
4 changes: 1 addition & 3 deletions simple/stats/jsonld_stream_db.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,9 +91,7 @@ def _write_observation_shard(args):
props_dict = json.loads(props)
if isinstance(props_dict, dict):
prop_keys = [
f"dcid:{k}" if not k.startswith(
("dcid:", "http://", "https://")) else k
for k in props_dict.keys()
_uri_ref(k) for k in props_dict.keys()
]
Comment on lines 93 to 95

@gemini-code-assist gemini-code-assist Bot Jun 23, 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.

medium

Potential None Values in observationProperties

If props_dict contains empty keys or keys that evaluate to None under _uri_ref, the resulting prop_keys list will contain None elements, which produces invalid JSON-LD / RDF.

Filtering out None values using an assignment expression ensures that only valid reference objects are included in dcid:observationProperties.

Suggested change
prop_keys = [
f"dcid:{k}" if not k.startswith(
("dcid:", "http://", "https://")) else k
for k in props_dict.keys()
_uri_ref(k) for k in props_dict.keys()
]
prop_keys = [
ref for k in props_dict.keys() if (ref := _uri_ref(k)) is not None
]

if prop_keys and var_obj:
var_obj["dcid:observationProperties"] = prop_keys
Expand Down
8 changes: 8 additions & 0 deletions simple/tests/stats/jsonld_stream_db_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -287,6 +287,14 @@ def test_observation_parsing_edge_cases(self):
o for o in graph
if o["dcid:observationAbout"]["@id"] == "dcid:country/ALB"
][0]
self.assertEqual(obs1["dcid:variableMeasured"], {
"@id": "dcid:v1",
"dcid:observationProperties": [
{"@id": "dcid:customIntProp"},
{"@id": "dcid:customStrProp"},
{"@id": "http://schema.org/url"}
]
})
self.assertEqual(obs1["dcid:value"], 99)
self.assertEqual(obs1["dcid:observationDate"], 2026)
self.assertEqual(obs1["dcid:scalingFactor"], 100)
Expand Down
19 changes: 15 additions & 4 deletions util/src/main/java/org/datacommons/util/McfUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,10 +162,21 @@ public static Mcf.McfGraph mergeGraphs(List<Mcf.McfGraph> graphs) throws Asserti
}

public static String stripNamespace(String val) {
if (val.startsWith(Vocabulary.DCID_PREFIX)
|| val.startsWith(Vocabulary.SCHEMA_ORG_PREFIX)
|| val.startsWith(Vocabulary.DC_SCHEMA_PREFIX)) {
return val.substring(val.indexOf(Vocabulary.REFERENCE_DELIMITER) + 1);
if (val == null) {
return null;
}
if (val.startsWith("http://") || val.startsWith("https://")) {
int lastSlash = val.lastIndexOf('/');
int lastHash = val.lastIndexOf('#');
int idx = Math.max(lastSlash, lastHash);
if (idx > 0 && idx < val.length() - 1) {
return val.substring(idx + 1);
}
return val;
}
Comment on lines +168 to +176

@gemini-code-assist gemini-code-assist Bot Jun 23, 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.

high

Correctness Issue: Hierarchical ID Corruption with Custom Namespaces

Using val.lastIndexOf('/') to strip the namespace from a full URI will incorrectly strip hierarchical entity IDs (such as country/FRA or geoId/06) down to their last segment (e.g., FRA or 06).

For example, if a custom namespace is https://customsource.org/schema/ and the entity reference is https://customsource.org/schema/country/FRA, this method will return FRA instead of country/FRA. This will break entity resolution, node ID generation, and lookups in the graph database.

Recommended Solution

The root cause is that JsonLdParser does not strip custom namespace prefixes during parsing, leaving full URIs in the McfGraph. Trying to guess the namespace boundary in McfUtil.stripNamespace using lastIndexOf('/') is inherently fragile.

Instead:

  1. Strip Custom Prefixes in JsonLdParser: Extract the namespaces defined in the @context of the JSON-LD document during parsing, and use them to strip prefixes (e.g., converting https://customsource.org/schema/country/FRA to custom:country/FRA or country/FRA) before storing them in the McfGraph.
  2. Fallback in McfUtil: If you must keep a fallback here, avoid stripping past known hierarchical patterns, or ensure that the namespace prefix is explicitly known.

int colonIdx = val.indexOf(Vocabulary.REFERENCE_DELIMITER);
if (colonIdx > 0) {
return val.substring(colonIdx + 1);
}
return val;
}
Expand Down
56 changes: 56 additions & 0 deletions util/src/test/java/org/datacommons/util/JsonLdParserTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,4 +150,60 @@ public void testNestedObservationProperties() throws Exception {
assertTrue(foundDest);
assertTrue(foundSource);
}

@Test
public void testNestedObservationProperties_StringList() throws Exception {
String jsonLd =
"{\n"
+ " \"@context\": {\n"
+ " \"custom\": \"https://customsource.org/schema/\",\n"
+ " \"dcid\": \"https://datacommons.org/browser/\"\n"
+ " },\n"
+ " \"@graph\": [\n"
+ " {\n"
+ " \"@id\": \"dcid:TestObs\",\n"
+ " \"@type\": \"dcid:StatVarObservation\",\n"
+ " \"dcid:variableMeasured\": {\n"
+ " \"@id\": \"custom:FinancialTrade\",\n"
+ " \"dcid:observationProperties\": [\n"
+ " \"custom:destinationCountry\",\n"
+ " \"custom:sourceCountry\"\n"
+ " ]\n"
+ " },\n"
+ " \"dcid:observationDate\": \"2024\",\n"
+ " \"dcid:value\": 100,\n"
+ " \"custom:sourceCountry\": { \"@id\": \"dcid:country/FRA\" },\n"
+ " \"custom:destinationCountry\": { \"@id\": \"dcid:country/USA\" }\n"
+ " }\n"
+ " ]\n"
+ "}";

InputStream in = new java.io.ByteArrayInputStream(jsonLd.getBytes(StandardCharsets.UTF_8));
McfGraph graph = JsonLdParser.parse(in);

assertNotNull(graph);
assertTrue(graph.getNodesMap().containsKey("TestObs"));
McfGraph.PropertyValues node = graph.getNodesMap().get("TestObs");

assertTrue(node.containsPvs("variableMeasured"));
assertTrue(node.containsPvs("observationProperties"));
java.util.List<McfGraph.TypedValue> values =
node.getPvsMap().get("observationProperties").getTypedValuesList();
assertTrue(values.size() == 2);

boolean foundDest = false;
boolean foundSource = false;
for (McfGraph.TypedValue tv : values) {
if ("custom:destinationCountry".equals(tv.getValue())
|| "https://customsource.org/schema/destinationCountry".equals(tv.getValue())) {
foundDest = true;
}
if ("custom:sourceCountry".equals(tv.getValue())
|| "https://customsource.org/schema/sourceCountry".equals(tv.getValue())) {
foundSource = true;
}
}
assertTrue(foundDest);
assertTrue(foundSource);
}
}