Skip to content

GH-3936: Check for trailing comma in JSON arrays for vars and bindings #3937

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

GH-3936: Check for trailing comma in JSON arrays for vars and bindings #3937

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
62 changes: 43 additions & 19 deletions jena-arq/src/main/java/org/apache/jena/riot/rowset/rw/rs_json/RowSetJSONStreaming.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,9 +35,7 @@
import java.util.function.Function;
import java.util.function.Supplier;

import com.google.gson.Gson;
import com.google.gson.JsonElement;
import com.google.gson.JsonObject;
import com.google.gson.*;
import com.google.gson.reflect.TypeToken;
import com.google.gson.stream.JsonReader;

Expand Down Expand Up @@ -82,7 +80,6 @@ public static RowSetJSONStreaming<?> createUnbuffered(InputStream in, LabelToNod

Gson gson = new Gson();
JsonReader reader = gson.newJsonReader(new InputStreamReader(in, StandardCharsets.UTF_8));

// Set up handling of unexpected json elements
UnexpectedJsonEltHandler unexpectedJsonHandler = (_gson, _reader) -> {
ErrorHandlers.relay(errorHandler, validationSettings.unexpectedJsonElementSeverity, () ->
Expand All @@ -94,7 +91,7 @@ public static RowSetJSONStreaming<?> createUnbuffered(InputStream in, LabelToNod
// Experiments with an decoder/encoder that returned Bindings directly without
// wrapping them as RsJsonEltDft did not show a significant performance difference
// that would justify supporting alternative encoder/decoder pairs.
RsJsonEltEncoder<RsJsonEltDft> eltEncoder = new RsJsonEltEncoderDft(labelMap, null, unexpectedJsonHandler);
RsJsonEltEncoder<RsJsonEltDft> eltEncoder = new RsJsonEltEncoderDft(labelMap, null, unexpectedJsonHandler, errorHandler);
RsJsonEltDecoder<RsJsonEltDft> eltDecoder = RsJsonEltDecoderDft.INSTANCE;
IteratorRsJSON<RsJsonEltDft> eltIt = new IteratorRsJSON<>(gson, reader, eltEncoder);

Expand Down Expand Up @@ -335,37 +332,60 @@ public int getUnknownJsonCount() {

/* Parsing (gson-based) ------------------------------------------------ */

private static Type stringListType = new TypeToken<List<String>>() {}.getType();
/** Parse the vars element from head - may return null */
static List<Var> parseHeadVars(Gson gson, JsonReader reader) throws IOException {
static List<Var> parseHeadVars(Gson gson, JsonReader reader, ErrorHandler errorHandler) throws IOException {
List<Var> result = null;
Type stringListType = new TypeToken<List<String>>() {}.getType();
JsonObject headJson = gson.fromJson(reader, JsonObject.class);
JsonObject headJson = fromJsonReader(gson, reader, jsonObjectToken, errorHandler);
JsonElement varsJson = headJson.get(kVars);
if (varsJson != null) {
List<String> varNames = gson.fromJson(varsJson, stringListType);
// A list ending in "," (bad JSON) cause a java null at the end of the array.
List<String> varNames = fromGsonElement(gson, varsJson, stringListType, errorHandler);
// GSON in lenient mode parses trailing comma and returns a java null.
if ( ! varNames.isEmpty() && varNames.getLast() == null ) {
errorHandler.warning("Bad JSON: \"vars\" array ends in a comma", -1, -1);
//throw new ResultSetException("Bad JSON: Array ends in a comma");
varNames.removeLast();
}
result = Var.varList(varNames);
}
return result;
}

static TypeToken<JsonObject> jsonObjectToken = TypeToken.get(JsonObject.class);
static Binding parseBinding(
Gson gson, JsonReader reader, LabelToNode labelMap,
Function<JsonObject, Node> onUnknownRdfTermType) throws IOException {
JsonObject obj = gson.fromJson(reader, JsonObject.class);

Function<JsonObject, Node> onUnknownRdfTermType,
ErrorHandler errorHandler) throws IOException {
JsonObject obj = fromJsonReader(gson, reader, jsonObjectToken, errorHandler);
BindingBuilder bb = BindingFactory.builder();

for (Entry<String, JsonElement> e : obj.entrySet()) {
Var v = Var.alloc(e.getKey());
JsonElement nodeElt = e.getValue();

Node node = parseOneTerm(nodeElt, labelMap, onUnknownRdfTermType);
bb.add(v, node);
}

return bb.build();
}

static JsonObject fromJsonReader(Gson gson, JsonReader reader, TypeToken<JsonObject> typeOfT, ErrorHandler errorHandler) {
try {
return gson.fromJson(reader, typeOfT);
} catch (JsonParseException ex) {
errorHandler.fatal("JSON Syntax error: "+ex.getMessage(), -1, -1);
return null;
}
}

static <T> T fromGsonElement(Gson gson, JsonElement jsonElement, Type type, ErrorHandler errorHandler) {
try {
return gson.fromJson(jsonElement, type);
} catch (JsonParseException ex) {
errorHandler.fatal("JSON Syntax error: "+ex.getMessage(), -1, -1);
return null;
}
}

static Node parseOneTerm(JsonElement jsonElt, LabelToNode labelMap, Function<JsonObject, Node> onUnknownRdfTermType) {

if (jsonElt == null) {
Expand Down Expand Up @@ -557,19 +577,23 @@ private static class RsJsonEltEncoderDft
protected LabelToNode labelMap;
protected Function<JsonObject, Node> unknownRdfTermTypeHandler;
protected UnexpectedJsonEltHandler unexpectedJsonHandler;
protected final ErrorHandler errorHandler;

public RsJsonEltEncoderDft(LabelToNode labelMap,
Function<JsonObject, Node> unknownRdfTermTypeHandler,
UnexpectedJsonEltHandler unexpectedJsonHandler) {
UnexpectedJsonEltHandler unexpectedJsonHandler,
ErrorHandler errorHandler
) {
super();
this.labelMap = labelMap;
this.unknownRdfTermTypeHandler = unknownRdfTermTypeHandler;
this.unexpectedJsonHandler = unexpectedJsonHandler;
this.errorHandler = errorHandler;
}

@Override
public RsJsonEltDft newHeadElt(Gson gson, JsonReader reader) throws IOException {
List<Var> vars = parseHeadVars(gson, reader);
List<Var> vars = parseHeadVars(gson, reader, errorHandler);
return new RsJsonEltDft(vars);
}

Expand All @@ -581,7 +605,7 @@ public RsJsonEltDft newBooleanElt(Gson gson, JsonReader reader) throws IOExcepti

@Override
public RsJsonEltDft newBindingElt(Gson gson, JsonReader reader) throws IOException {
Binding binding = parseBinding(gson, reader, labelMap, unknownRdfTermTypeHandler);
Binding binding = parseBinding(gson, reader, labelMap, unknownRdfTermTypeHandler, errorHandler);
return new RsJsonEltDft(binding);
}

Expand All @@ -592,7 +616,7 @@ public RsJsonEltDft newResultsElt(Gson gson, JsonReader reader) throws IOExcepti

@Override
public RsJsonEltDft newUnknownElt(Gson gson, JsonReader reader) throws IOException {
JsonElement jsonElement = unexpectedJsonHandler == null
JsonElement jsonElement = (unexpectedJsonHandler == null)
? null
: unexpectedJsonHandler.apply(gson, reader);
return new RsJsonEltDft(jsonElement);
Expand Down
57 changes: 54 additions & 3 deletions jena-arq/src/test/java/org/apache/jena/riot/rowset/rw/TestRowSetReader.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,30 +23,38 @@

import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertNotNull;
import static org.junit.jupiter.api.Assertions.assertThrows;
import static org.junit.jupiter.api.Assertions.assertTrue;

import java.io.ByteArrayInputStream;
import java.io.IOException;
import java.io.InputStream;

import org.junit.jupiter.api.Test;

import org.apache.commons.lang3.Strings;
import org.apache.jena.atlas.io.IOX;
import org.apache.jena.atlas.lib.Bytes;
import org.apache.jena.atlas.logging.LogCtl;
import org.apache.jena.query.ARQ;
import org.apache.jena.riot.Lang;
import org.apache.jena.riot.resultset.ResultSetLang;
import org.apache.jena.riot.rowset.RowSetReader;
import org.apache.jena.sparql.exec.RowSet;
import org.apache.jena.sparql.exec.RowSetOps;
import org.apache.jena.sparql.resultset.ResultSetException;
import org.apache.jena.sys.JenaSystem;
import org.junit.jupiter.api.Test;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

/**
* Addition tests for result readers.
* The SPARQl test suite cover most usage. This class adds tests
* The SPARQL test suite cover most usage. This class adds tests.
*/
public class TestRowSetReader {
static { JenaSystem.init(); }

// Check "abc"^^rdf:langString and "abc"^^rdf:dirLangString and "
// Check "abc"^^rdf:langString and "abc"^^rdf:dirLangString - i.e. incomplete forms.
@Test public void resultSet_json_1() {
String r = """
{
Expand All @@ -71,6 +79,49 @@ public class TestRowSetReader {
assertEquals(2, RowSetOps.count(rowset));
}

// Check error handling of bad JSON
@Test public void resultSet_json_bad_vars() {
// Trailing comma in "vars" array. This is only a warning.
String r = """
{ "head": { "vars": [ "x", "y" , ] } ,
"results": {
"bindings": [
{
"x": { "type": "literal" , "value": "A" } ,
"y": { "type": "literal" , "value": "B" }
}
]
}
}
""";
Logger log = LoggerFactory.getLogger(RowSetReaderJSONStreaming.class);
LogCtl.withLevel(log, "ERROR", ()->{
RowSet rowset = read(r, ResultSetLang.RS_JSON);
assertNotNull(rowset);
assertEquals(1, RowSetOps.count(rowset));
});
}

// Check error handling of bad JSON
@Test public void resultSet_json_bad_bindings() {
// Trailing comma in "bindings" array. This is an error. It might be file truncation.
String r = """
{ "head": { "vars": [ "x", "y" ] } ,
"results": {
"bindings": [
{
"x": { "type": "literal" , "value": "A" } ,
"y": { "type": "literal" , "value": "B" } ,
}
]
}
}
""";
ResultSetException ex = assertThrows(ResultSetException.class, ()-> read(r, ResultSetLang.RS_JSON));
String msg = ex.getMessage();
assertTrue(Strings.CI.containsAny(msg, "JSON Syntax error:"));
}

@Test public void resultSet_xml_1() {
String r = """
<?xml version="1.0"?>
Expand Down
10 changes: 8 additions & 2 deletions jena-cmds/src/main/java/arq/rset.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
package arq;

import org.apache.jena.query.ResultSet;
import org.apache.jena.sparql.resultset.ResultSetException;
import org.apache.jena.sparql.util.QueryExecUtils;
import arq.cmdline.CmdARQ;
import arq.cmdline.ModResultsIn;
Expand Down Expand Up @@ -57,8 +58,13 @@ protected String getSummary() {

@Override
protected void exec() {
ResultSet rs = modInput.getResultSet();
QueryExecUtils.outputResultSet(rs, null, modOutput.getResultsFormat(), System.out);
try {
ResultSet rs = modInput.getResultSet();
QueryExecUtils.outputResultSet(rs, null, modOutput.getResultsFormat(), System.out);
} catch (ResultSetException ex) {
System.err.println(ex.getMessage());
}

}

@Override
Expand Down