Emit default values in marshalled JSON

[philipithomas]

In order to make APIs more restful, require the marshaler to emit
default values. By default, it omits empty types. This causes
unexpected REST behavior by omitting all false values or zero-valued
integers.

Closes #233

[tmc]

It's unclear what the appropriate default is here. I think being closer to jsonpb defaults is probably for the best and this behavior is configurable via WithMarshalerOption. @yugui - wanna weigh in?

[rio]

I'm pro this change. Ran into this a couple of times now and the default now is just not what I expect it would be.

[tmc]

I've warmed up to this as a change but perhaps should come with a version bump as it changes output behavior. @philipithomas do you want to rebase this and regenerate output?

[MalteJ]

I'd love to see this merged!
With the current settings my API is sort of broken.

[tmc]

@MalteJ to be clear you can control rendering with the WithMarshalerOption option.

[achew22]

@tmc I think I'm supportive of merging this and doing another minor release. What do you think of that?

[jinleileiking]

I'd love to see this merged!

OR YOU PUT THIS INFO IN WIKI OR README!!!!

[achew22]

I rebased this to master and it appears to be failing tests. If someone wanted to fix up the failing tests I would be happy to merge something like this (+ tests)

[ornithocoder]

@philipithomas / @achew22 can you try this?

diff --git a/examples/browser/a_bit_of_everything_service.spec.js b/examples/browser/a_bit_of_everything_service.spec.js
index edcbebe..f99c867 100644
--- a/examples/browser/a_bit_of_everything_service.spec.js
+++ b/examples/browser/a_bit_of_everything_service.spec.js
@@ -34,6 +34,15 @@ describe('ABitOfEverythingService', function() {
       sint32_value: 2147483647,
       sint64_value: "4611686018427387903",
       nonConventionalNameValue: "camelCase",
+      single_nested: null,
+      nested: [  ],
+      enum_value: "ZERO",
+      repeated_string_value: [  ],
+      map_value: Object({  }),
+      mapped_string_value: Object({  }),
+      mapped_nested_value: Object({  }),
+      timestamp_value: null,
+      repeated_enum_value: [  ],
     };
 
     beforeEach(function(done) {
@@ -72,10 +81,9 @@ describe('ABitOfEverythingService', function() {
       sint32_value: 2147483647,
       sint64_value: "4611686018427387903",
       nonConventionalNameValue: "camelCase",
-
       nested: [
-       { name: "bar", amount: 10 },
-       { name: "baz", amount: 20 },
+       { name: "bar", amount: 10, ok: 'FALSE' },
+       { name: "baz", amount: 20, ok: 'FALSE' },
       ],
       repeated_string_value: ["a", "b", "c"],
       oneof_string: "x",
@@ -83,9 +91,13 @@ describe('ABitOfEverythingService', function() {
       map_value: { a: 1, b: 2 },
       mapped_string_value: { a: "x", b: "y" },
       mapped_nested_value: {
-        a: { name: "x", amount: 1 },
-        b: { name: "y", amount: 2 },
+        a: { name: "x", amount: 1, ok: 'FALSE' },
+        b: { name: "y", amount: 2, ok: 'FALSE' },
       },
+      single_nested: null,
+      enum_value: "ZERO",
+      timestamp_value: null,
+      repeated_enum_value: [  ],
     };
 
     beforeEach(function(done) {
diff --git a/examples/browser/echo_service.spec.js b/examples/browser/echo_service.spec.js
index 97888c3..eca49f9 100644
--- a/examples/browser/echo_service.spec.js
+++ b/examples/browser/echo_service.spec.js
@@ -21,7 +21,7 @@ describe('EchoService', function() {
           {id: "foo"},
           {responseContentType: "application/json"}
       ).then(function(resp) {
-        expect(resp.obj).toEqual({id: "foo"});
+        expect(resp.obj).toEqual({id: "foo", num: '0'});
       }).catch(function(err) {
         done.fail(err);
       }).then(done);
@@ -34,7 +34,7 @@ describe('EchoService', function() {
           {body: {id: "foo"}},
           {responseContentType: "application/json"}
       ).then(function(resp) {
-        expect(resp.obj).toEqual({id: "foo"});
+        expect(resp.obj).toEqual({id: "foo", num: '0'});
       }).catch(function(err) {
         done.fail(err);
       }).then(done);

[achew22]

Unfortunately I don't have any way to verify your cla status. Could you send a PR? We have a bot to verify stuff if you send it in that way

[ornithocoder]

Hi @achew22, unfortunately the patch above doesn't fix all the tests. client_test.go and integration_test.go still have to be fixed.

[nycdotnet]

@achew22 We were just surprised by this at Namely and we have a business CLA signed. Would you still be willing to merge a PR that addressed this including all related tests?

[achew22]

@nycdotnet strong yes. #546 is where I started the effort to do this so you can get a sense of the scope. We also got blocked by the fork of swagger-generator=>openapi-generator waiting for their first release (3.0.0 came out on the 1st of the month) so now that they have settled down a bit I think we may be able to continue. How can I help you with that change?

[nycdotnet]

@achew22 Thank you!

The PR you linked is very extensive. How much of that PR would you say is directly related to this issue? I understand that since this qualifies as a breaking change, it's a good opportunity to break all the things, but for a first PR I think it might be tough to jump-in midstream on that. Do you see v2 and #546 as the only way forward?

[achew22]

Sorry for the slow response. I would have sworn I replied already.

The PR you linked is very extensive. How much of that PR would you say is directly related to this issue?

Unfortunately almost all of it. I started that change as "emit default values" but then when it got to be so large, I just said "f-it" I'm going to break a few more things in the PR. Unfortunately I never got around to actually doing it.

I understand that since this qualifies as a breaking change, it's a good opportunity to break all the things, but for a first PR I think it might be tough to jump-in midstream on that. Do you see v2 and #546 as the only way forward?

A major version bump is definitely required. This is a major API breaking change to start emitting default values. I think it's the correct behavior and we should have done it earlier, but it is hard (as you may have noticed).

Are you still interested in helping out given the scope? It might be possible to do it in a few smaller steps if you're willing to take some of those on.

[johanbrandhorst]

Seems like this was superceded by #546