Change swagger generator to produce lowerCamelCase field names, change default marshaler to do the same, and also emit default values

[alexleigh]

In the protobuf canonical JSON representation, field names are in lowerCamelCase. This change causes the swagger generator to produce schema files that correspond to the canonical JSON representation, using lowerCamelCase field names. To produce output that conforms to the generated schema, the server should marshal using jsonpb with OrigName: false. The test server has been changed accordingly to succeed against the changed schema.

This fixes #375.

[achew22]

@alexleigh. can you help me understand something about the output? Do you know if this will change any of the generated code in substantive ways? I know this is kind of a vast question, but is there reason to expect that in python the generated output code for the field my_field would go from get_my_field to getMyField?

@tmc I would appreciate if you could also weigh in here. To me this seems reasonable but I will admit that it is breaking. Should we couple this with changing the default and mark this as a 2.0 release?

[alexleigh]

For the code generated by swagger-codegen using the schema files generated by this project, the impact of this change should be relatively minimal. Much like how protoc converts snake_case protobuf field names to lowerCamelCase field names for JSON marshaling, swagger-codegen takes care to convert the lowerCamelCase field names in the JSON schema to the preferred field names in the target language. For example, in the generated Python code, there is this map:

self.attribute_map = {
            'single_nested': 'singleNested',
            'uuid': 'uuid',
            'nested': 'nested',
            'float_value': 'floatValue',
            'double_value': 'doubleValue',
            'int64_value': 'int64Value',
            'uint64_value': 'uint64Value',
            'int32_value': 'int32Value',
            'fixed64_value': 'fixed64Value',
            'fixed32_value': 'fixed32Value',
            'bool_value': 'boolValue',
            'string_value': 'stringValue',
            'bytes_value': 'bytesValue',
            'uint32_value': 'uint32Value',
            'enum_value': 'enumValue',
            'sfixed32_value': 'sfixed32Value',
            'sfixed64_value': 'sfixed64Value',
            'sint32_value': 'sint32Value',
            'sint64_value': 'sint64Value',
            'repeated_string_value': 'repeatedStringValue',
            'oneof_empty': 'oneofEmpty',
            'oneof_string': 'oneofString',
            'map_value': 'mapValue',
            'mapped_string_value': 'mappedStringValue',
            'mapped_nested_value': 'mappedNestedValue',
            'non_conventional_name_value': 'nonConventionalNameValue',
            'timestamp_value': 'timestampValue',
            'repeated_enum_value': 'repeatedEnumValue'
}

This is a mapping from the lowerCamelCase names in JSON to the snake_case field names in Python. The situation is similar for Ruby and Go, and I imagine for other languages swagger-codegen should pick the appropriate field naming as well.

You are right that this change is breaking, insofar as the generated Swagger schemas will change. I actually think another change makes sense when combined with this change: the default jsonpb marshaling in grpc-gateway should have OrigName: false. This will cause grpc-gateway servers to emit JSON output in line with protobuf canonical JSON representations. However this would be even more of a breaking change, causing not only the swagger output to change, but also the output of all servers with default marshaling configurations. Thus for this change I left the default marshaling alone, and only modified the marshaling of the test server. Unfortunately this means the default server marshaling output and the swagger schema generated have diverged. It's up to the user to ensure the two are consistent by customizing the jsonpb marshaling options.

[cyriltovena]

@alexleigh can you do the requested change ? or do you want me to submit another PR ? I'm also in need of this.

just change this

grpc-gateway/runtime/marshaler_registry.go

Line 16 in acebe0f

defaultMarshaler = &JSONPb{OrigName: true}

to false.

[achew22]

@Kuqd there have been other attempts to do this (#508 comes to mind). It might be useful to start with their work.

[cyriltovena]

Is it related ? Not sure I follow, the point is to not output snake case as it's the standard for protobuf but camel case which is the json standard.

[achew22]

Sorry, I did a poor job of communicating what my intent was. We have for a while wanted to turn things from snake_case to camelCase and turn on default value emission in the same change. They are not inherently linked but they are both breaking changes. Since they are breaking, it's my preference to do both of them in rapid succession (or at the same time) as to minimize impact on people's production systems and keep the migrations down to a single event.

[alexleigh]

@achew22 if you prefer, I can amend this PR to change the default marshaling to use camelCase and emit default values. Currently I accomplish both of those by customizing the marshaling options. But it would be good to change the default marshaling options to follow suit.

[achew22]

TBH I would love it if anyone would do it. $DAYJOB has been keeping me extremely busy of late so I haven't had time to do it. I will take care of the releasing a 2.0 if you want to go through and enable default values and camelCase

[cyriltovena]

@alexleigh you got it. Thanks a lot

[alexleigh]

I changed the default marshaling to use camelCase and emit default fields and fixed the tests. Unfortunately, swagger-codegen 2.2.2 does not support enum fields for generated Go code (2.3.0 purports to support it but in my tests it seems broken, it generates incorrect type names that don't compile). So when default values for enum fields are emitted, swagger-codegen generated Go client code can't unmarshal the object due to the missing enums. To keep the existing tests I had to make a copy of the ABE message that has all the same fields, except for all of the enums, and adjust the client test code to use these messages instead. On the plus side this let me enable one of the previously disabled tests.

[cyriltovena]

LGTM

[achew22]

Could you file a bug against swagger codegen and @mention me in it? I'd prefer to not have a duplicate ABE proto if it's at all possible.

[achew22]

swagger-api/swagger-codegen#7636

[cyriltovena]

Do you guys have an ETA ?

[achew22]

I just replied to the comment over there. That PR has all the info I've got.

[achew22]

To update ppl here, the aforementioned PR is now in swagger-codegen. I'm waiting on the 2.4.0 release to go in before I pick up the work in #546 to upgrade to 2.4.0. Once that happens, we can resume work here with a relatively surgical change to the default marshaller/unmarshaller and the swagger name selector. Then once that lands I will update docs and tag a 2.0 release to indicate that one of our longest standing issues is now fixed. 🎉

[johanbrandhorst]

This seems to be superceded by #546

[johanbrandhorst]

We'd be happy to reconsider this for v2.