Modify BuildFileToJsonParser to use Gson's streaming parser. Summary: Instead of using our hand-tuned streaming parser, use the default one in Gson. This also enables us to parse arbitrary JSON structures instead of only arrays of arrays of primitives. This changes `BuildFileToJsonParser` to take `isServerMode` as a constructor param so that we are explicit. Despite what the comment on `BuildFileToJsonParser#isServerMode` says, I don't believe that we can use "regular mode" going forward. That is, because the output from `buck.py` includes an `__includes` entry, we cannot read a single JSON object in isolation because there may be context from the `__includes` entry that it needs. Test Plan: Sandcastle builds.
diff --git a/src/com/facebook/buck/json/BUCK b/src/com/facebook/buck/json/BUCK index dac4faf..052934e 100644 --- a/src/com/facebook/buck/json/BUCK +++ b/src/com/facebook/buck/json/BUCK
@@ -3,12 +3,12 @@ srcs = glob(['*.java']), deps = [ '//lib:guava', - '//lib:jackson-core', '//src/com/facebook/buck/util/environment:environment', '//src/com/facebook/buck/util:constants', '//src/com/facebook/buck/util:exceptions', '//src/com/facebook/buck/util:io', '//src/com/facebook/buck/util:util', + '//third-party/java/gson:gson', ], visibility = [ 'PUBLIC',
diff --git a/src/com/facebook/buck/json/BuildFileToJsonParser.java b/src/com/facebook/buck/json/BuildFileToJsonParser.java index 2a8e8eb..7121a25 100644 --- a/src/com/facebook/buck/json/BuildFileToJsonParser.java +++ b/src/com/facebook/buck/json/BuildFileToJsonParser.java
@@ -16,18 +16,20 @@ package com.facebook.buck.json; -import com.fasterxml.jackson.core.JsonFactory; -import com.fasterxml.jackson.core.JsonParseException; -import com.fasterxml.jackson.core.JsonParser; -import com.fasterxml.jackson.core.JsonToken; -import com.google.common.base.Preconditions; +import com.google.common.annotations.VisibleForTesting; import com.google.common.collect.Lists; import com.google.common.collect.Maps; +import com.google.gson.Gson; +import com.google.gson.JsonArray; +import com.google.gson.JsonElement; +import com.google.gson.JsonObject; +import com.google.gson.JsonPrimitive; +import com.google.gson.stream.JsonReader; +import com.google.gson.stream.JsonToken; -import java.io.EOFException; import java.io.IOException; -import java.io.InputStream; import java.io.Reader; +import java.io.StringReader; import java.util.List; import java.util.Map; @@ -37,142 +39,123 @@ * sort of nested arrays or objects are allowed in the output as Parser is implemented * today. This simplification makes it easier to leverage Jackson's streaming JSON API. */ -public class BuildFileToJsonParser { +public class BuildFileToJsonParser implements AutoCloseable { - private final JsonParser parser; + private final Gson gson; + private final JsonReader reader; - public BuildFileToJsonParser(String json) throws IOException { - JsonFactory jsonFactory = new JsonFactory(); - this.parser = jsonFactory.createJsonParser(json); + /** + * The parser below uses these objects for stateful purposes with the ultimate goal + * of populating the parsed rules into `currentObjects`. + * + * The parser is expecting two different styles of output: + * 1. Server mode: [{"key": "value"}, {"key": "value"}, ...] + * 2. Regular mode: {"key": "value"}, {"key": "value"}, ... + * + * Server mode output is a necessary short-term step to keep logic in the main Parser + * consistent (expecting to be able to correlate a set of rules with the specific BUCK file + * that generated them). This requirement creates an unnecessary performance weakness + * in this design where we cannot parallelize buck.py's parsing of BUCK files with buck's + * processing of the result into a DAG. Once this can be addressed, server mode should be + * eliminated. + */ + private final boolean isServerMode; + + /** + * @param jsonReader That contains the JSON data. + */ + public BuildFileToJsonParser(Reader jsonReader, boolean isServerMode) { + this.gson = new Gson(); + this.reader = new JsonReader(jsonReader); + this.isServerMode = isServerMode; + + // This is used to read one line at a time. + reader.setLenient(true); } - public BuildFileToJsonParser(InputStream json) throws IOException { - JsonFactory jsonFactory = new JsonFactory(); - this.parser = jsonFactory.createJsonParser(json); - } - - public BuildFileToJsonParser(Reader json) throws IOException { - JsonFactory jsonFactory = new JsonFactory(); - this.parser = jsonFactory.createJsonParser(json); + @VisibleForTesting + public BuildFileToJsonParser(String json, boolean isServerMode) { + this(new StringReader(json), isServerMode); } /** * Access the next set of rules from the build file processor. Note that for non-server * invocations, this will collect all of the rules into one enormous list. * - * @return List of rules expressed as a <em>very</em> simple mapping of JSON field names - * to Java primitives. + * @return The parsed JSON, represented as Java collections. Ideally, we would use Gson's object + * model directly to avoid the overhead of converting between object models. That would + * require updating all code that depends on this method, which may be a lot of work. Also, + * bear in mind that using the Java collections decouples clients of this method from the JSON + * parser that we use. */ - public List<Map<String, Object>> nextRules() throws IOException { - // The parser below uses these objects for stateful purposes with the ultimate goal - // of populating the parsed rules into `currentObjects`. - // - // The parser is expecting two different styles of output: - // 1. Server mode: [{"key": "value"}, {"key": "value"}, ...] - // 2. Regular mode: {"key": "value"}, {"key": "value"}, ... - // - // Server mode output is a necessary short-term step to keep logic in the main Parser - // consistent (expecting to be able to correlate a set of rules with the specific BUCK file - // that generated them). This requirement creates an unnecessary performance weakness - // in this design where we cannot parallelize buck.py's parsing of BUCK files with buck's - // processing of the result into a DAG. Once this can be addressed, server mode should be - // eliminated. - List<Map<String, Object>> currentObjects = null; - String currentFieldName = null; - List<String> currentArray = null; - Map<String, Object> currentObject = null; + @SuppressWarnings("unchecked") + List<Map<String, Object>> nextRules() throws IOException { + List<Map<String, Object>> items = Lists.newArrayList(); - while (true) { - JsonToken token = parser.nextToken(); - if (token == null) { - if (currentObject != null) { - throw new EOFException("unexpected end-of-stream"); - } else if (currentObjects == null) { - // This happens when buck.py failed to produce any output for this build rule (python - // parse error or raised exception, I bet). - throw new EOFException("missing build rules"); - } else { - return currentObjects; - } + if (isServerMode) { + reader.beginArray(); + + while (reader.hasNext()) { + JsonObject json = gson.fromJson(reader, JsonObject.class); + items.add((Map<String, Object>) toRawTypes(json)); } - switch (token) { - case START_OBJECT: - // The syntax differs very slightly between server mode and not. If we encounter - // an object not inside of an array, we aren't in server mode and so we can just read - // all the objects until exhaustion. - if (currentObjects == null) { - currentObjects = Lists.newArrayList(); - } - currentObject = Maps.newHashMap(); - break; - - case END_OBJECT: - currentObjects.add(currentObject); - currentObject = null; - break; - - case START_ARRAY: - // Heuristic to detect whether we are in the above server mode or regular mode. If we - // encounter START_ARRAY before START_OBJECT, it must be server mode so we should build - // `currentObjects` now. Otherwise, this is the start of a new sub-array within - // an object. - if (currentObjects == null) { - currentObjects = Lists.newArrayList(); - } else { - currentArray = Lists.newArrayList(); - } - break; - - case END_ARRAY: - if (currentArray != null) { - currentObject.put(currentFieldName, currentArray); - currentArray = null; - currentFieldName = null; - break; - } else { - // Must be in server mode and we finished parsing a single BUCK file. - return currentObjects; - } - - case FIELD_NAME: - currentFieldName = parser.getText(); - break; - - case VALUE_STRING: - if (currentArray == null) { - currentObject.put(currentFieldName, parser.getText()); - currentFieldName = null; - } else { - currentArray.add(parser.getText()); - } - break; - - case VALUE_TRUE: - case VALUE_FALSE: - Preconditions.checkState(currentArray == null, "Unexpected boolean in JSON array"); - currentObject.put(currentFieldName, token == JsonToken.VALUE_TRUE); - currentFieldName = null; - break; - - case VALUE_NUMBER_INT: - Preconditions.checkState(currentArray == null, "Unexpected int in JSON array"); - currentObject.put(currentFieldName, parser.getLongValue()); - currentFieldName = null; - break; - - case VALUE_NULL: - if (currentArray == null) { - currentObject.put(currentFieldName, null); - currentFieldName = null; - } else { - currentArray.add(null); - } - break; - - default: - throw new JsonParseException("Unexpected token: " + token, parser.getCurrentLocation()); + reader.endArray(); + } else { + while (reader.peek() != JsonToken.END_DOCUMENT) { + JsonObject json = gson.fromJson(reader, JsonObject.class); + items.add((Map<String, Object>) toRawTypes(json)); } } + + return items; + } + + /** + * @return One of: String, Boolean, Long, Number, List<Object>, Map<String, Object>. + */ + @VisibleForTesting + static Object toRawTypes(JsonElement json) { + // Cases are ordered from most common to least common. + if (json.isJsonPrimitive()) { + JsonPrimitive primitive = json.getAsJsonPrimitive(); + if (primitive.isString()) { + return primitive.getAsString(); + } else if (primitive.isBoolean()) { + return primitive.getAsBoolean(); + } else if (primitive.isNumber()) { + Number number = primitive.getAsNumber(); + // Number is likely an instance of class com.google.gson.internal.LazilyParsedNumber. + if (number.longValue() == number.doubleValue()) { + return number.longValue(); + } else { + return number; + } + } else { + throw new IllegalStateException("Unknown primitive type: " + primitive); + } + } else if (json.isJsonArray()) { + JsonArray array = json.getAsJsonArray(); + List<Object> out = Lists.newArrayListWithCapacity(array.size()); + for (JsonElement item : array) { + out.add(toRawTypes(item)); + } + return out; + } else if (json.isJsonObject()) { + Map<String, Object> out = Maps.newHashMap(); + for (Map.Entry<String, JsonElement> entry : json.getAsJsonObject().entrySet()) { + out.put(entry.getKey(), toRawTypes(entry.getValue())); + } + return out; + } else if (json.isJsonNull()) { + return null; + } else { + throw new IllegalStateException("Unknown type: " + json); + } + } + + @Override + public void close() throws IOException { + reader.close(); } }
diff --git a/src/com/facebook/buck/json/ProjectBuildFileParser.java b/src/com/facebook/buck/json/ProjectBuildFileParser.java index ab1eb70..f87b5bb 100644 --- a/src/com/facebook/buck/json/ProjectBuildFileParser.java +++ b/src/com/facebook/buck/json/ProjectBuildFileParser.java
@@ -21,6 +21,7 @@ import com.facebook.buck.util.ProjectFilesystem; import com.facebook.buck.util.environment.Platform; import com.google.common.annotations.VisibleForTesting; +import com.google.common.base.Charsets; import com.google.common.base.Optional; import com.google.common.base.Preconditions; import com.google.common.base.Throwables; @@ -31,8 +32,10 @@ import java.io.File; import java.io.IOException; import java.io.InputStream; +import java.io.InputStreamReader; import java.io.OutputStream; import java.io.OutputStreamWriter; +import java.io.Reader; import java.util.Collections; import java.util.List; import java.util.Map; @@ -127,6 +130,10 @@ stderrConsumer.start(); buckPyStdinWriter = new BufferedWriter(new OutputStreamWriter(stdin)); + + // TODO(mbolin): Ensure that the Reader gets closed. + Reader reader = new InputStreamReader(buckPyProcess.getInputStream(), Charsets.UTF_8); + buckPyStdoutParser = new BuildFileToJsonParser(reader, isServerMode); } private ImmutableList<String> buildArgs() { @@ -223,21 +230,6 @@ buckPyStdinWriter.flush(); } - // Construct the parser lazily because Jackson expects that when the parser is made it is - // safe to immediately begin reading from the underlying stream to detect the encoding. - // For our server use case, the server will produce no output until directed to by a - // request for a particular build file. - // - // TODO: Jackson has a severe bug which assumes that it is safe to require at least 4 bytes - // of input due to JSON's BOM concept (see detectEncoding). buck.py, and many other - // facilities, do not write a BOM header and therefore may end up producing insufficient - // bytes to unwedge the parser's construction. For example, if the first BUCK file - // submitted outputted no rules (that is, "[]"), then this line would hang waiting for - // more input! - if (buckPyStdoutParser == null) { - buckPyStdoutParser = new BuildFileToJsonParser(buckPyProcess.getInputStream()); - } - return buckPyStdoutParser.nextRules(); }
diff --git a/test/com/facebook/buck/json/BUCK b/test/com/facebook/buck/json/BUCK index 18ac6fe..b56c69a 100644 --- a/test/com/facebook/buck/json/BUCK +++ b/test/com/facebook/buck/json/BUCK
@@ -6,8 +6,8 @@ ], deps = [ '//lib:guava', - '//lib:jackson-core', '//lib:junit', '//src/com/facebook/buck/json:json', + '//third-party/java/gson:gson', ], )
diff --git a/test/com/facebook/buck/json/BuildFileToJsonParserTest.java b/test/com/facebook/buck/json/BuildFileToJsonParserTest.java index dbe0068..19535db 100644 --- a/test/com/facebook/buck/json/BuildFileToJsonParserTest.java +++ b/test/com/facebook/buck/json/BuildFileToJsonParserTest.java
@@ -18,11 +18,13 @@ import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNotEquals; -import static org.junit.Assert.assertTrue; -import com.fasterxml.jackson.core.JsonParseException; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; +import com.google.common.collect.Maps; +import com.google.gson.JsonArray; +import com.google.gson.JsonObject; +import com.google.gson.JsonPrimitive; import org.junit.Test; @@ -36,54 +38,99 @@ public class BuildFileToJsonParserTest { @Test - public void testSimpleParse() throws JsonParseException, IOException { + public void testSimpleParse() throws IOException { String json = "{" + "\"srcs\": [\"src/com/facebook/buck/Bar.java\", \"src/com/facebook/buck/Foo.java\"]" + "}"; - BuildFileToJsonParser parser = new BuildFileToJsonParser(json); - List<Map<String, Object>> tokens = parser.nextRules(); - assertEquals( - ImmutableList.of( - ImmutableMap.of("srcs", - ImmutableList.of( - "src/com/facebook/buck/Bar.java", - "src/com/facebook/buck/Foo.java"))), - tokens); + + try (BuildFileToJsonParser parser = new BuildFileToJsonParser(json, false /* isServerMode */)) { + assertEquals( + ImmutableList.of( + ImmutableMap.of("srcs", + ImmutableList.of( + "src/com/facebook/buck/Bar.java", + "src/com/facebook/buck/Foo.java"))), + parser.nextRules()); + } } @Test public void testParseLong() throws IOException { String json = "{\"thing\": 27}"; - BuildFileToJsonParser parser = new BuildFileToJsonParser(json); - List<Map<String, Object>> rules = parser.nextRules(); + List<Map<String, Object>> tokens; + try (BuildFileToJsonParser parser = new BuildFileToJsonParser(json, false /* isServerMode */)) { + tokens = parser.nextRules(); + } - assertEquals(1, rules.size()); - Map<String, Object> rule = rules.get(0); + assertEquals(1, tokens.size()); + Map<String, Object> rule = tokens.get(0); Object value = rule.get("thing"); - assertTrue(value instanceof Long); + assertEquals(Long.class, value.getClass()); assertEquals(27L, rule.get("thing")); assertNotEquals(27, rule.get("thing")); } @Test - public void testServerModeParse() throws JsonParseException, IOException { + public void testServerModeParse() throws IOException { String json = "[{\"foo\": \"a:1\"}, {\"foo\": \"a:2\"}]\n" + "[{\"bar\": \"b:1\"}]"; - BuildFileToJsonParser parser = new BuildFileToJsonParser(json); - List<Map<String, Object>> a = parser.nextRules(); - assertEquals( - ImmutableList.of( - ImmutableMap.of("foo", "a:1"), - ImmutableMap.of("foo", "a:2")), - a); + try (BuildFileToJsonParser parser = new BuildFileToJsonParser(json, true /* isServerMode */)) { + assertEquals( + ImmutableList.of( + ImmutableMap.of("foo", "a:1"), + ImmutableMap.of("foo", "a:2")), + parser.nextRules()); - List<Map<String, Object>> b = parser.nextRules(); - assertEquals( - ImmutableList.of( - ImmutableMap.of("bar", "b:1")), - b); + assertEquals( + ImmutableList.of( + ImmutableMap.of("bar", "b:1")), + parser.nextRules()); + } + } + + @Test + public void testParseNestedStructures() throws IOException { + String json = + "{" + + "\"foo\": [\"a\", \"b\"]," + + "\"bar\": {\"baz\": \"quox\"}" + + "}"; + + try (BuildFileToJsonParser parser = new BuildFileToJsonParser(json, false /* isServerMode */)) { + assertEquals( + ImmutableList.of(ImmutableMap.of( + "foo", ImmutableList.of("a", "b"), + "bar", ImmutableMap.of("baz", "quox"))), + parser.nextRules()); + } + } + + @Test + public void testToRawTypes() { + JsonObject ruleJson = new JsonObject(); + + ruleJson.addProperty("name", "foo"); + ruleJson.addProperty("export_deps", false); + ruleJson.addProperty("proguard_config", (String)null); + ruleJson.addProperty("source", 6); + + JsonArray deps = new JsonArray(); + deps.add(new JsonPrimitive("//foo:bar")); + deps.add(new JsonPrimitive("//foo:baz")); + deps.add(new JsonPrimitive("//foo:biz")); + ruleJson.add("deps", deps); + + // Note that an ImmutableMap does not allow null values, but BuildFileToJsonParser.toRawTypes() + // can return a Map with null values, so we use an ordinary HashMap for the expected value. + Map<String, Object> expected = Maps.newHashMap(); + expected.put("name", "foo"); + expected.put("export_deps", false); + expected.put("proguard_config", null); + expected.put("source", 6L); + expected.put("deps", ImmutableList.of("//foo:bar", "//foo:baz", "//foo:biz")); + assertEquals(expected, BuildFileToJsonParser.toRawTypes(ruleJson)); } }