Skip to content

Issue #3262 [ASPNET5] Server Bug - doesn't properly serialize JSON pr…#3263

Closed
daviddodsworth wants to merge 1 commit intoswagger-api:masterfrom
daviddodsworth:master
Closed

Issue #3262 [ASPNET5] Server Bug - doesn't properly serialize JSON pr…#3263
daviddodsworth wants to merge 1 commit intoswagger-api:masterfrom
daviddodsworth:master

Conversation

@daviddodsworth
Copy link
Copy Markdown

…operties in camelCase

Adding suggested fix of [JsonProperty (PropertyName = "{{baseName}}")]

…ize JSON properties in camelCase

Adding suggested fix of [JsonProperty (PropertyName = "{{baseName}}")]
/// {{^description}}Gets or Sets {{{name}}}{{/description}}{{#description}}{{{description}}}{{/description}}
/// </summary>{{#description}}
/// <value>{{{description}}}</value>{{/description}}
[JsonProperty(PropertyName = "{{baseName}}")]
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the indentation, can we use 4-space instead of tab?

@wing328
Copy link
Copy Markdown
Contributor

wing328 commented Jul 1, 2016

@daviddodsworth thanks for the PR. I've left you a comment about the indentation style. Please take a look.

@wing328
Copy link
Copy Markdown
Contributor

wing328 commented Jul 1, 2016

cc @jimschubert

@jimschubert
Copy link
Copy Markdown
Contributor

#3262

@jimschubert
Copy link
Copy Markdown
Contributor

I'm not sure this is the proper solution. Many APIs prefer snake case to differentiate between server side data and client data. This forces all APIs to use camel case.

I feel like this should actually be a NewtonSoft JSON concern, and a generator option. But I'd have to look into it.

@jimschubert
Copy link
Copy Markdown
Contributor

Oh, I see from the linked issue your comment relates to matching the case of the swagger definition. This may be a partial solution to the exposed API. However, the aspnet5 generator also includes generated Swagger API documentation. This should match the swagger document you provided, but if I remember correctly Ahoy (the .NET Core version of Swashbuckle) works with [DataMember]... I'm not sure it works with [JsonProperty]. I'll need to take a look either tonight or later this weekend.

@wing328
Copy link
Copy Markdown
Contributor

wing328 commented Jul 1, 2016

@jimschubert just want to share that for PHP, we plan to reuse the same set of model-related mustache templates for PHP API clients and server stubs (Slim, Silex, Lumen) so as to make the code for PHP model more consistent across PHP API client and server stub and more easily to maintain.

Not sure if we can do the same for C# API client and server stub.

@jimschubert
Copy link
Copy Markdown
Contributor

@wing328 If we stick with [DataMember], I think that would be fine for C# because it's part of the core framework. If we tightly couple everything to NewtonSoft JSON, I don't think it would be as beneficial for maintenance. I haven't looked at the PHP implementation, but I assume they're just dumb data models which don't need any knowledge of how they're transferred on the wire. For C#, for example, [JsonProperty] would be fine for JSON APIs only, but plenty of APIs support JSON and XML both of which should be handled correctly by DataMember.

@daviddodsworth
Copy link
Copy Markdown
Author

@jimschubert I would vote for the [DataMember] solution, and I would agree that it's more portable, I just didn't have cycles to fully vet it. With that said, we're doing a contract first design with Swagger and not code first (with Swashbuckle). That's how we noticed the issue in the first place, since we need the Swagger definition of the JSON objects to be properly generated for server and several client APIs from a "on the wire" perspective.

I'll take a look at flipping over to the [DataMember] solution later this week.

@jimschubert
Copy link
Copy Markdown
Contributor

I figured that's what it was. I had a similar issue when doing contract first in node.js before I knew of swagger-codegen. I defined custom jsdoc comments as the contract and parsed those out on build to generate the swagger definition used by swagger UI.

I was going to take a look this weekend at the DataMember implementation, which should be pretty simple. But, I've had problems with my Windows VM (left the employer to whom it was licensed). I had to buy Windows 10, then set it up and realized I installed 32 bit... so I had to start over. :/

@wing328 wing328 modified the milestones: v2.3.0, v2.2.0 Jul 7, 2016
@jimschubert
Copy link
Copy Markdown
Contributor

@daviddodsworth did you make progress on the [DataMember] stuff? I can take a look at it if you haven't.

@jimschubert
Copy link
Copy Markdown
Contributor

@daviddodsworth @wing328 since there wasn't a response for a little over a month, I went ahead and opened a PR with the [DataMember] implementation. The referencing PR would replace this one.

@wing328 wing328 modified the milestones: v2.2.1, v2.3.0 Aug 8, 2016
@wing328 wing328 closed this Sep 26, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants