Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add support for newtonsoft.json #48

Closed
devlux opened this issue Oct 6, 2020 · 4 comments
Closed

add support for newtonsoft.json #48

devlux opened this issue Oct 6, 2020 · 4 comments
Assignees
Labels
💡 enhancement New feature or request

Comments

@devlux
Copy link
Contributor

devlux commented Oct 6, 2020

add support for Newtonsoft.Json by providing a new newtonsoft specific saunter package.

@m-wild
Copy link
Collaborator

m-wild commented Oct 7, 2020

While this reflects the behaviour of Newtonsoft.Json, it does not reflect the behaviour of System.Text.Json.

void Main()
{
	Newtonsoft.Json.JsonConvert.SerializeObject(new Example(), new JsonSerializerSettings
		{
			Formatting = Newtonsoft.Json.Formatting.Indented,
			ContractResolver = new CamelCasePropertyNamesContractResolver()
		})
		.Dump();
	// {
	//    "someOtherName": null
	// }
	
	System.Text.Json.JsonSerializer.Serialize(new Example(), new JsonSerializerOptions
		{
			WriteIndented = true,
			PropertyNamingPolicy = JsonNamingPolicy.CamelCase,
		})
		.Dump();
	// {
	//    "SomeOtherName": null
	// }
}

class Example
{
	[JsonProperty("SomeOtherName")]
	[JsonPropertyName("SomeOtherName")]
	public string FooBar { get; set; }
}

Since we are now using System.Text.Json as our serializer (as is the default for new dotnet projects), I feel it makes sense to keep this behaviour as-is.

Additionally, we should remove the current DataMember check as this does not match System.Text.Json behaviour either.

Someone using Newtonsoft.Json will need to provide a custom PropertyNameSelector which matches their Newtonsoft.Json settings.

options.PropertyNameSelector = prop => 
{
    var name = prop.GetCustomAttribute<JsonPropertyAttribute>()?.PropertyName
        ?? prop.GetCustomAttribute<DataMemberAttribute>()?.Name
        ?? prop.Name;
    return new CamelCaseNamingStrategy().GetPropertyName(name, false);
}

I think the better option here would be to provide a Newtonsoft.Json compatibility library.
This would be a 2nd, optional, Nuget package, which provides an extension method for enabling Newtonsoft.Json support.

$ dotnet add package Saunter
$ dotnet add package Saunter.NewtonsoftJson

Startup.cs

services.AddAsyncApiSchemaGeneration(options =>
{
    options.UseNewtonsoftJson();
});

This UseNewtonsoftJson() method could set a default PropertyNameSelector which mimics Newtonsoft.Json behaviour (checking [Newtonsoft.Json.JsonPropertyAttribute] and [DataMemberAttribute])

Though, given this entire library would be just the ~4 lines of code, maybe just providing documentation would be better!

@devlux
Copy link
Contributor Author

devlux commented Oct 7, 2020

Good point, I didn't know this difference. If this is the case, then your comment makes absolutely sense. Even though I find the system.text.json strategy to be quite strange as you could end up with an inconsistent naming. But as long as they stick with this weird solution, it makes sense to be inline. ;-)

@devlux devlux changed the title propertyselector should also be applied on overrides add support for newtonsoft.json Oct 9, 2020
@m-wild m-wild added the 💡 enhancement New feature or request label Oct 10, 2020
@neoaisac
Copy link

I've proposed in #89 that the serialization logic is extracted into a separate type. Developers can then use this to provide their own serializing logic, and it could potentially be used to create different adapters for this, including Newtonsoft.Json or Yaml.

@m-wild
Copy link
Collaborator

m-wild commented Jul 19, 2021

Closing in favor of #60

@m-wild m-wild closed this as completed Jul 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
💡 enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants