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 DefaultValueHandling to JsonSerializerSettings #306

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

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