Skip to content

Add DefaultValueHandling to JsonSerializerSettings#306

Open
swiftMessenger wants to merge 1 commit intofszlin:mainfrom
swiftMessenger:patch-1
Open

Add DefaultValueHandling to JsonSerializerSettings#306
swiftMessenger wants to merge 1 commit intofszlin:mainfrom
swiftMessenger:patch-1

Conversation

@swiftMessenger
Copy link
Contributor

Description

When the default JsonSerializerSettings are set to DefaultValueHandling = DefaultValueHandling.Ignore, certes is unable to generate SSL certificates. The error message is: "NewOrder request included invalid non-DNS type identifier: type "", value "DomainName"". The best workarounds I could think of to make certes work in a solution that needs to keep the DefaultValueHandling.Ignore as the default were both lengthly and ugly. However, preventing this from becoming an issue in the first place is a one-line fix.

New Test

I also started writing a unit test to verify that Certes continues to work despite the default JSON settings, but am not familiar enough with how the tests work in your solution to make much progress. Here is what I had previously if you are interested and willing to add it to the solution:

        [Fact]
        public async Task DoesNotRelyOnDefaultJsonSettings()
        {
            //Intentionally setting default settings that might cause problems when used with the AcmeHttpClient
            Newtonsoft.Json.JsonConvert.DefaultSettings = () => new JsonSerializerSettings
            {
                DateFormatHandling = DateFormatHandling.MicrosoftDateFormat,
                DateTimeZoneHandling = DateTimeZoneHandling.RoundtripKind,
                NullValueHandling = NullValueHandling.Ignore,
                DefaultValueHandling = DefaultValueHandling.Ignore,
                Formatting = Formatting.None,
                ObjectCreationHandling = ObjectCreationHandling.Reuse
            };

            //Now confirm that we can place the order despite our bad default settings
            var ctx = new AcmeContext(WellKnownServers.LetsEncryptStagingV2);
            var order = await ctx.NewOrder(new[] { "a.com" });
            var auths = await order.Authorizations();
            Assert.True(auths.GetEnumerator().MoveNext());
        }

@swiftMessenger
Copy link
Contributor Author

Any update on accepting this pull request? The failed checks were due to environmental factors rather than code and I have so far been unable to find a way to re-trigger them.

This PR is a fix for Issue #309

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant