Valid json when using jsonproto output in queries #18701

chiragramani · 2023-06-15T22:44:22Z

When using bazel query ... --output=jsonproto , the output JSON is not valid as

elements are not comma separated.
the outer containers don't have "[", "]".

This PR fixes the above issue.

chiragramani · 2023-06-15T22:54:55Z

meisterT · 2023-06-26T06:12:41Z

The current output follows the NDJSON (see http://ndjson.org/) format where each line a separate JSON value. The advantage of that is that you can start parsing each line individually

zhengwei143 · 2023-06-26T10:12:48Z

Rather, the current output is concatenated json instead of ndjson - there isn't any newline separating each target proto.

I also just realized a slight discrepancy: currently --output=proto outputs a QueryResult proto, but --output=jsonproto doesn't include the QueryResult wrapper (making it more like --output=streamed_proto).

EDIT: I would suggest renaming it to --output=streamed_jsonproto to be consistent with --output=streamed_proto. That is because cquery --output=jsonproto outputs a single CqueryResult and aquery --output=jsonproto outputs a single ActionGraphContainer.

Back to query, given the current implementation, we can't nicely wrap the Target(s) in a QueryResult without a larger refactor or introducing more codepaths in QueryCommand#doQuery (which I'd rather not) because query --order_output=no runs the output formatter callback in parallel (e.g. for multiple top level targets):

bazel/src/main/java/com/google/devtools/build/lib/query2/query/output/JSONProtoOutputFormatter.java

Lines 39 to 49 in 0efaf54

    
             return new OutputFormatterCallback<Target>() { 
        
               @Override 
        
               public void processOutput(Iterable<Target> partialResult) 
        
                   throws IOException, InterruptedException { 
        
                 for (Target target : partialResult) { 
        
                   out.write( 
        
                       jsonPrinter.print(toTargetProtoBuffer(target)).getBytes(StandardCharsets.UTF_8)); 
        
                 } 
        
               } 
        
             }; 
        
           }

It is for this same reason that we can't wrap the Target(s) in square brackets [ ... ] as implemented here. I would suggest going with the ndjson format, but trivially fix this by adding a newline after printing the Target message in json.

zhengwei143 · 2023-06-26T10:14:25Z

src/main/java/com/google/devtools/build/lib/query2/query/output/JSONProtoOutputFormatter.java

        }
+        out.write("[".getBytes(StandardCharsets.UTF_8));


As mentioned in #18701 (comment), instead of wrapping it in square brackets, follow the ndjson format and append a newline after the json message instead.

zhengwei143

As an add on, I would suggest renaming this to --output=streamed_jsonproto if you follow my other comment to output it in ndjson format.

cquery --output=jsonproto actually outputs only a single CqueryResult (the behavior was wrongly changed recently in 607d0f7 and reverted in dba9e43). So renaming it as mentioned above would ensure that it isn't inconsistent --output=jsonproto and consistent with --output=streamed_proto of the other cquery/aquery output formats.

Otherwise, if you wish to stick with --output=jsonproto and output a single QueryResult proto, you'll need to find a workaround to first aggregate the targets and wrapping them in the QueryResult before writing (especially in the case of --order_output=no).

chiragramani · 2023-06-29T20:34:35Z

Thank you @zhengwei143 , @meisterT - updated the diff to update jsonproto -> streamed_proto and parse as per the NDJSON specs, please let me know if there is anything else needed from my end.

zhengwei143 · 2023-06-30T09:44:12Z

The changes look good, thanks! Just a few minor comments and we should be good to go.

chiragramani · 2023-06-30T16:23:17Z

The changes look good, thanks! Just a few minor comments and we should be good to go.

let me know once you submit the comments and I will be happy to look into them, thanks.

zhengwei143 · 2023-06-30T09:36:16Z

...java/com/google/devtools/build/lib/query2/query/output/StreamedJSONProtoOutputFormatter.java

@@ -20,15 +20,17 @@
 import java.io.IOException;
 import java.io.OutputStream;
 import java.nio.charset.StandardCharsets;
+import java.util.ArrayList;
+import java.util.List;

 /**
 * An output formatter that outputs a protocol buffer json representation of a query result and


This comment is not accurate anymore since we aren't outputting a query result, but a list of targets in ndjson format. Could you update the comment here w.r.t the new output format?

zhengwei143 · 2023-06-30T09:40:47Z

src/test/shell/integration/bazel_query_test.sh

@@ -1109,15 +1109,15 @@ genrule(
    cmd = "echo unused > $(OUTS)",
 )
 EOF
-  bazel query --output=jsonproto --noimplicit_deps "//$pkg:bar" > output 2> "$TEST_log" \
+  bazel query --output=streamed_jsonproto --noimplicit_deps "//$pkg:bar" > output 2> "$TEST_log" \


Could you add a few more targets to the $pkg/BUILD file, query here for //$pkg:* so that we can test that the overall output format is in ndjson, instead of a QueryResult.

zhengwei143 · 2023-06-30T16:39:02Z

let me know once you submit the comments and I will be happy to look into them, thanks.

Of course, I forgot to submit the comments :)

chiragramani · 2023-07-11T00:53:10Z

Hey @zhengwei143 , I have updated the PR however, some of the CI jobs failed and I think jq is not available on all platforms - what are your suggestions to go about it - any existing validation utilities that I can make use of which work on all the platforms?

zhengwei143 · 2023-07-14T07:53:59Z

Thanks for the update! Let me check to see whether it is possible to use jq.

remove jq

chiragramani · 2023-08-09T02:41:22Z

Thanks for the update! Let me check to see whether it is possible to use jq.

I removed jq and tests are all successful. I would like your help with this PR as the changes that went live in 6.3 have a bug where the output isn't in NDJSON format and isn't valid JSON either. I plan to make this change available in 6.4, please let me know if you have any other suggestions for me.

zhengwei143

Thanks for the changes, just some minor changes requested and it should be good to go (sorry for not catching that one earlier).

Internally, it seems that there have been some usages of --output=jsonproto that I'll need to migrate. That might take a while but I'll get started while this review is ongoing.

src/test/shell/integration/bazel_query_test.sh

...java/com/google/devtools/build/lib/query2/query/output/StreamedJSONProtoOutputFormatter.java

brentleyjones · 2023-08-10T12:02:46Z

@bazel-io flag

iancha1992 · 2023-08-10T17:59:33Z

@bazel-io fork 6.4.0

…put=streamed_jsonproto` implementation. Closes bazelbuild#18701. PiperOrigin-RevId: 555417403 Change-Id: I30eb06f734188f8511884954f43c5f5b3c0091a3

…w `--ouput=streamed_jsonproto` implementation. (#19226) Fix valid json when using jsonproto output in queries with new `--ouput=streamed_jsonproto` implementation. Closes #18701. Commit 2cd583a PiperOrigin-RevId: 555417403 Change-Id: I30eb06f734188f8511884954f43c5f5b3c0091a3 Co-authored-by: Chirag Ramani <[email protected]>

iancha1992 · 2023-09-21T20:36:41Z

The changes in this PR have been included in Bazel 6.4.0 RC1. Please test out the release candidate and report any issues as soon as possible. If you're using Bazelisk, you can point to the latest RC by setting USE_BAZEL_VERSION=last_rc.
Thanks!

github-actions bot added awaiting-review PR is awaiting review from an assigned reviewer team-Performance Issues for Performance teams labels Jun 15, 2023

chiragramani force-pushed the valid_json_proto_query branch from 86478e4 to 6eb7a29 Compare June 15, 2023 22:53

zhengwei143 self-requested a review June 16, 2023 07:45

zhengwei143 requested changes Jun 26, 2023

View reviewed changes

zhengwei143 requested changes Jun 29, 2023

View reviewed changes

zhengwei143 requested changes Jun 30, 2023

View reviewed changes

chiragramani force-pushed the valid_json_proto_query branch from b6923cc to a62d39e Compare July 10, 2023 21:06

chiragramani force-pushed the valid_json_proto_query branch from a62d39e to 2697336 Compare August 9, 2023 01:04

chiragramani added 3 commits August 8, 2023 18:06

Valid json when using jsonproto output in queries

419d91d

Using NDJSON spec

29540aa

Updating test

73bce66

remove jq

chiragramani force-pushed the valid_json_proto_query branch from 2697336 to 73bce66 Compare August 9, 2023 01:06

zhengwei143 reviewed Aug 9, 2023

View reviewed changes

src/test/shell/integration/bazel_query_test.sh Outdated Show resolved Hide resolved

...java/com/google/devtools/build/lib/query2/query/output/StreamedJSONProtoOutputFormatter.java Show resolved Hide resolved

zhengwei143 approved these changes Aug 9, 2023

View reviewed changes

copybara-service bot closed this in 2cd583a Aug 10, 2023

github-actions bot removed the awaiting-review PR is awaiting review from an assigned reviewer label Aug 10, 2023

bazel-io added the potential release blocker Flagged by community members using "@bazel-io flag". Should be added to a release blocker milestone label Aug 10, 2023

bazel-io removed the potential release blocker Flagged by community members using "@bazel-io flag". Should be added to a release blocker milestone label Aug 10, 2023

bazel-io mentioned this pull request Aug 10, 2023

[6.4.0] Valid json when using jsonproto output in queries #19223

Closed

iancha1992 mentioned this pull request Aug 10, 2023

[Duplicate] Fix valid json when using jsonproto output in queries with new --output=streamed_jsonproto implementation. #19224

Closed

iancha1992 mentioned this pull request Aug 10, 2023

[6.4.0] Fix valid json when using jsonproto output in queries with new --ouput=streamed_jsonproto implementation. #19226

Merged

brentleyjones mentioned this pull request Sep 21, 2023

query --output jsonproto produces invalid json when multiple objects are returned #19583

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Valid json when using jsonproto output in queries #18701

Valid json when using jsonproto output in queries #18701

chiragramani commented Jun 15, 2023 •

edited

Loading

chiragramani commented Jun 15, 2023

meisterT commented Jun 26, 2023

zhengwei143 commented Jun 26, 2023 •

edited

Loading

zhengwei143 Jun 26, 2023

zhengwei143 left a comment

chiragramani commented Jun 29, 2023

zhengwei143 commented Jun 30, 2023

chiragramani commented Jun 30, 2023

zhengwei143 Jun 30, 2023

zhengwei143 Jun 30, 2023

zhengwei143 commented Jun 30, 2023

chiragramani commented Jul 11, 2023 •

edited

Loading

zhengwei143 commented Jul 14, 2023

chiragramani commented Aug 9, 2023

zhengwei143 left a comment

brentleyjones commented Aug 10, 2023

iancha1992 commented Aug 10, 2023

iancha1992 commented Sep 21, 2023

Valid json when using jsonproto output in queries #18701

Valid json when using jsonproto output in queries #18701

Conversation

chiragramani commented Jun 15, 2023 • edited Loading

chiragramani commented Jun 15, 2023

meisterT commented Jun 26, 2023

zhengwei143 commented Jun 26, 2023 • edited Loading

zhengwei143 Jun 26, 2023

Choose a reason for hiding this comment

zhengwei143 left a comment

Choose a reason for hiding this comment

chiragramani commented Jun 29, 2023

zhengwei143 commented Jun 30, 2023

chiragramani commented Jun 30, 2023

zhengwei143 Jun 30, 2023

Choose a reason for hiding this comment

zhengwei143 Jun 30, 2023

Choose a reason for hiding this comment

zhengwei143 commented Jun 30, 2023

chiragramani commented Jul 11, 2023 • edited Loading

zhengwei143 commented Jul 14, 2023

chiragramani commented Aug 9, 2023

zhengwei143 left a comment

Choose a reason for hiding this comment

brentleyjones commented Aug 10, 2023

iancha1992 commented Aug 10, 2023

iancha1992 commented Sep 21, 2023

chiragramani commented Jun 15, 2023 •

edited

Loading

zhengwei143 commented Jun 26, 2023 •

edited

Loading

chiragramani commented Jul 11, 2023 •

edited

Loading