JSON Option for List Command #57

peteboothroyd · 2017-08-04T14:13:18Z

JSON Output

Adding JSON option to list command to allow easy parsing of output.
Altering with_message output to be json which can be copy and pasted straight into a call command.
Now printing response message template as this may be useful for users to determine which service they want to call.
Updating tests to reflect this update.

dinowernli · 2017-08-07T08:15:08Z

src/main/java/me/dinowernli/grpc/polyglot/command/Method.java

@@ -0,0 +1,20 @@
+package me.dinowernli.grpc.polyglot.command;
+
+public class Method {


Do we really need to define these custom types (here and services below)?

Can we not just use the methoddescriptor produced by protobuf?

Sure, we could hard code the logic to print straight to JSON from the protobuf descriptors. I think though that if there is going to be more than one output format option that it will be much easier to use pre-existing tools to process the java object, eg using gson takes one line to go to JSON, I imagine there will be other options to go to other formats just as easily.

dinowernli

General approach looks sound, but I don't think we should be introducing those mutable types for service and method.

dinowernli · 2017-08-07T08:16:05Z

src/main/java/me/dinowernli/grpc/polyglot/command/ServiceList.java

+
+    if (jsonOutput.isPresent() && jsonOutput.get()) {
+      Gson gson = new Gson();
+      output.writeLine(gson.toJson(services));


As discussed above, can we not just json-print the descriptors rather than converting them to our custom data type?

I looked into using the JSON printing but when I tried this it looked like it would take more work. The JSON printed did not resolve all the way down to the primitive level as the as the output currently does. Say a proto message is defined:

"foo": { "name": "STRING", "id": "INT" }

The JSON output would be:

"foo": "Foo"

This doesn't give enough information to people to allow them to construct their own messages.

I think that's what you get when you print a message descriptor. What you actually want is the service descriptor [1]. That should get you the service name, the method name, and the type name for the messages. Service descriptors are already conveniently returns by listServices()

[1] https://github.com/google/protobuf/blob/master/src/google/protobuf/descriptor.proto#L230

dinowernli · 2017-08-07T08:17:22Z

src/main/java/me/dinowernli/grpc/polyglot/config/CommandLineArgs.java


+  //TODO: Move to a "list_services"-specific flag container
+  @Option(
+          name = "--json_output",


Can we have this be a --list_output_format and have "json" be one of the valid flag values?

Sure this should be pretty easy and will be more extensible in the future

dinowernli · 2017-08-07T08:18:45Z

src/main/java/me/dinowernli/grpc/polyglot/Main.java

 import com.google.protobuf.DescriptorProtos.FileDescriptorSet;
 import me.dinowernli.grpc.polyglot.command.ServiceCall;
 import me.dinowernli.grpc.polyglot.command.ServiceList;
+//import me.dinowernli.grpc.polyglot.command.Server;


spurious comment

dinowernli · 2017-08-09T12:39:04Z

src/main/java/me/dinowernli/grpc/polyglot/command/ServiceList.java

+
+    if (jsonOutput.isPresent() && jsonOutput.get()) {
+      Gson gson = new Gson();
+      output.writeLine(gson.toJson(services));


I think that's what you get when you print a message descriptor. What you actually want is the service descriptor [1]. That should get you the service name, the method name, and the type name for the messages. Service descriptors are already conveniently returns by listServices()

[1] https://github.com/google/protobuf/blob/master/src/google/protobuf/descriptor.proto#L230

peteboothroyd · 2017-08-17T17:54:19Z

I've updated with your suggestions. The merging process went a little awry on my end, the main changes are in the ServiceList, Main, and CommandLineArgs.

Changes:

There is now a list_output_format flag with two options, json or the default readable option
Tests updated and added for command line argument parsing
Listing the methods now reports true/false for client & server streaming
Tests updated for ServiceList

googlebot · 2017-08-17T17:54:29Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If your company signed a CLA, they designated a Point of Contact who decides which employees are authorized to participate. You may need to contact the Point of Contact for your company and ask to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the project maintainer to go/cla#troubleshoot.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again.

peteboothroyd · 2017-08-17T19:24:22Z

done

…

On 17 Aug 2017, at 18:54, googlebot ***@***.***> wrote: Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ <https://cla.developers.google.com/> to sign. Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks. If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data <https://cla.developers.google.com/clas> and verify that your email is set on your git commits <https://help.github.com/articles/setting-your-email-in-git/>. If your company signed a CLA, they designated a Point of Contact who decides which employees are authorized to participate. You may need to contact the Point of Contact for your company and ask to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the project maintainer to go/cla#troubleshoot. In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#57 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AM1d-aNMeoRuRPBdpcDbrmHQFvBDnLHrks5sZH5WgaJpZM4OtxP7>.

googlebot · 2017-08-17T19:24:25Z

CLAs look good, thanks!

dinowernli · 2017-08-25T11:29:28Z

src/main/java/me/dinowernli/grpc/polyglot/command/ServiceList.java

          !serviceFilter.isPresent()
          || descriptor.getFullName().toLowerCase().contains(serviceFilter.get().toLowerCase());

      if (matchingDescriptor) {


Almost there :) Any chance we can just print the json of descriptor.toProto()? That should give you the information you need and avoid manually constructing json output.

I could try but will need some help. As I think I mentioned before as far as I can tell using the JsonPrinters do not do what we need. eg.
printing:String jsonFormat = JsonFormat.printer().print(descriptor.toProto().toBuilder());
Returns something similar to:
{ "name": "TestMethod", "inputType": ".polyglot.test.TestRequest", "outputType": ".polyglot.test.TestResponse", "options": {} }
There is not enough information about the types. I imagine you could recursively resolve every type which is more than one level deep to get down to the primitive level of strings, int etc. but this will require a decent change to the code.
I think this approach is the simplest method given what's already written but if you know a better way then let me know.

dinowernli · 2017-08-25T11:30:10Z

src/main/java/me/dinowernli/grpc/polyglot/command/ServiceList.java


    for (MethodDescriptor method : descriptor.getMethods()) {
      if (!methodFilter.isPresent() || method.getName().contains(methodFilter.get())) {



Same here, you should be able to just json-format the method protos.

peteboothroyd · 2017-09-05T14:29:40Z

Changes

JSON output now prints out the service protos, and dependency file protos.
JSON output structure is

{
  serviceProtos: [//list of serviceDescriptorProtos],
  fileProtos: [//list of fileDescriptorProtos]
}

See src/test/java/me/dinowernli/grpc/polyglot/command/jsonOutputGold.txt for more illustrative example

MessageWriter can use custom separator string.
Separator string only used in between messages.

Notes

Services are filtered according to the serviceFilter flag, but the methodFilter flag is ignored for json output

dinowernli · 2017-09-08T10:19:21Z

src/main/java/me/dinowernli/grpc/polyglot/config/ConfigurationLoader.java

-        .orElseThrow(() -> new IllegalArgumentException("Could not find named config: " + name));
+      .filter(config -> config.getName().equals(name))
+      .findAny()
+      .orElseThrow(() -> new IllegalArgumentException("Could not find named config: " + name));


nit: indentation here was correct, please revert

dinowernli · 2017-09-08T10:21:55Z

src/main/java/me/dinowernli/grpc/polyglot/io/MessageWriter.java

    try {
-      output.write(jsonPrinter.print(message) + MESSAGE_SEPARATOR);
+      String outputString = jsonPrinter.print(message);
+      // Only separate messages if there are more than one


I don't actually think this is the right thing to do. To make things easier to parse, I believe we alway want to append a separator, even if there is nothing else coming. Can discuss in person.

dinowernli · 2017-09-08T10:23:01Z

src/main/java/me/dinowernli/grpc/polyglot/io/MessageWriter.java

   */
-  public static <T extends Message> MessageWriter<T> create(Output output) {
-    return new MessageWriter<T>(JsonFormat.printer(), output);
+  public static <T extends Message> MessageWriter<T> create(Output output, String messageSeparator) {


Let's have two factory methods, one called create(), and the other called createWithSeparator().

That way, there is a sensible default and we don't need the ServiceCall class to understand how these separators work.

dinowernli · 2017-09-08T10:23:48Z

src/test/java/me/dinowernli/grpc/polyglot/command/ServiceListTest.java

-      .addFile(TestProto.getDescriptor().toProto())
-      .addFile(FooProto.getDescriptor().toProto())
-      .build();
+    .addFile(TestProto.getDescriptor().toProto())


indentation was correct, please revert (thoughout most of this file)

dinowernli · 2017-09-08T10:24:11Z

src/test/java/me/dinowernli/grpc/polyglot/command/ServiceListTest.java


+//  @Test
+//  public void testServiceListOutputWithMessageDetailJsonFormatted() throws Throwable {
+//    ServiceList.listServices(


please remove this commented-out test

dinowernli · 2017-09-08T10:29:29Z

src/main/java/me/dinowernli/grpc/polyglot/command/ServiceList.java

-}
+
+  private static void printJsonOutput(Output output, ServiceResolver serviceResolver, Optional<String> serviceFilter) {
+    output.writeLine("{ \"serviceProtos\": [");


We should probably not be constructing json ourselves here. If we want to wrap the existing protobuf output, we should define a proto which wraps the messages we need and print that a single time (this would also obviate the need for changing the messageprinter).

peteboothroyd · 2017-09-20T14:14:49Z

Ok the things you mentioned should be addressed now. Formatting is almost definitely not correct, can you advise of an automated way to share whatever settings you have as doing this manually is soul destroying.

dinowernli reviewed Aug 7, 2017

View reviewed changes

dinowernli requested changes Aug 7, 2017

View reviewed changes

dinowernli requested changes Aug 9, 2017

View reviewed changes

dinowernli requested changes Aug 25, 2017

View reviewed changes

Peter Boothroyd added 8 commits August 31, 2017 15:18

adding command line args and tests

e05524d

adding access token test

17991d5

improving logging for case of no stdin

7fa9284

Merge branch 'feature/oauth-config-override'

e757555

formatting

82e649f

renaming to fit convention and fixing test

028ea90

printing json output for file and service protos

e31e5fe

formatting tabs

4414c20

dinowernli requested changes Sep 8, 2017

View reviewed changes

Peter Boothroyd added 4 commits September 14, 2017 15:22

adding output.proto

6933615

adding proto definitions for json output

39e969b

filtering methods in json output

8d82801

reformatting

c33c271

		@@ -0,0 +1,20 @@
		package me.dinowernli.grpc.polyglot.command;

		public class Method {


		for (MethodDescriptor method : descriptor.getMethods()) {
		if (!methodFilter.isPresent() \|\| method.getName().contains(methodFilter.get())) {

JSON Option for List Command #57

Are you sure you want to change the base?

JSON Option for List Command #57

Uh oh!

Conversation

peteboothroyd commented Aug 4, 2017

JSON Output

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dinowernli left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

peteboothroyd Aug 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

peteboothroyd commented Aug 17, 2017

Changes:

Uh oh!

googlebot commented Aug 17, 2017

Uh oh!

peteboothroyd commented Aug 17, 2017 via email

Uh oh!

googlebot commented Aug 17, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

peteboothroyd commented Sep 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Notes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

peteboothroyd commented Sep 20, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

peteboothroyd Aug 7, 2017 •

edited

Loading

peteboothroyd commented Sep 5, 2017 •

edited

Loading