Fix nullable handling for value-type data properties

[desjoerd]

Only apply non-nullable reference type metadata to reference-type members so custom data contract resolvers can mark struct wrapper properties as nullable. Add regression coverage for resolver-provided nullable value-type members.

Fixes partly #3977 and would fix some issues around nullable in #2359.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 95.04%. Comparing base (c3071ad) to head (45b9799).

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #3998   +/-   ##
=======================================
  Coverage   95.04%   95.04%           
=======================================
  Files         111      111           
  Lines        3958     3959    +1     
  Branches      801      801           
=======================================
+ Hits         3762     3763    +1     
  Misses        196      196           

Flag	Coverage Δ
Linux	95.04% <100.00%> (+<0.01%)	⬆️
Windows	95.04% <100.00%> (+<0.01%)	⬆️
macOS	95.04% <100.00%> (+<0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[martincostello]

Could you add/update at least one snapshot test for this change please? It's easier to visualise the changes and protect against regressions with those.

[desjoerd]

Will do, it requires a custom resolver. Shall I do it with an Optional mock?

[martincostello]

Shall I do it with an Optional mock?

Whatever you think best validates the scenario 👍

[desjoerd]

Ok, because I make it completely opague and make the underlying type the member type, this patch does not work.

I think NRT needs to be handled in the JsonSerializerDataContractResolver and the NewtonsoftDataContractResolver to make them authoritive around the nullability of properties.

Working on it, but will take more time!

[desjoerd]

I've moved the logic, a few open questions:

Seems that references do not map nullable to an oneOf ref, but maybe this is another issue.

The changes give a change in the public api, I've made it backwards compatible, but having the SchemaGeneratorOptions as a argument gives more than the DataContractResolvers requires as we only use one setting. But this should not directly cause a maintainability issue I think.

The nullability for the JsonSerializer has the moved logic which is custom logic, the JsonPropertyInfo could also be used for the nullability as it handles NRT as well. Maybe a future optional improvement.

(sorry for brevity and not adding backlinks from my phone 🙂)