JsonNet: Throw if custom quantity instantiation fails, not null

[andrewmeservy]

Why

UnitsNetBaseJsonConverter instantiates registered custom quantity types with Activator.CreateInstance(). If that reflection call fails, or if it returns null, the converter should not return null from its non-null IQuantity conversion path or surface an unclear reflection exception.

What

• Throw a UnitsNetException when a registered custom quantity cannot be instantiated.
• Include the quantity type, unit type, unit value, and a stable error code in Exception.Data.
• Use UnitsNetException.ErrorCodeDataKey as the shared Exception.Data key for error codes.
• Preserve the original reflection/constructor failure as the inner exception.
• Add regression tests for failed registered quantity instantiation and constructor failures.

Validation

• dotnet test UnitsNet.Serialization.JsonNet.Tests -f net8.0 --filter "FullyQualifiedName~UnitsNetBaseJsonConverterTest"
• dotnet test UnitsNet.Serialization.JsonNet.Tests -f net9.0 --filter "FullyQualifiedName~UnitsNetBaseJsonConverterTest"
• dotnet test UnitsNet.Serialization.JsonNet.Tests -f net10.0 --filter "FullyQualifiedName~UnitsNetBaseJsonConverterTest"

[github-actions]

This PR is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 7 days.

[andrewmeservy]

I can't remove the stale label, but this pr is still needed

[angularsen]

@claude review this

[claude]

Claude finished @angularsen's task in 1m 51s —— View job

---

Review of PR #1664 — UnitsNetBaseJsonConverter compatibility patch

• Gather context (PR diff, changed files)
• Review UnitsNetBaseJsonConverter.cs changes
• Review test changes
• Post review feedback

Summary

The core bug fix here is correct and well tested. The original problem: ConvertValueUnit did

return (IQuantity)Activator.CreateInstance(registeredQuantity, valueUnit.Value, unit)!;

If Activator.CreateInstance returned null (e.g., interface/abstract registered type, or another edge case), the ! null-forgiving operator would let a null silently flow out as if it were a valid non-nullable IQuantity, causing a confusing NullReferenceException downstream instead of a clear error at the point of failure.

Current state of the code

Note that d746457 (the original commit from this PR) has since been refined by a follow-up commit (7ef4e15, "Use specific exception for custom quantity instantiation failures") already on this branch, which:

• Wraps Activator.CreateInstance in a try/catch so constructor-argument mismatches (MissingMethodException), constructor exceptions (TargetInvocationException), etc. are also caught, not just a null return.
• Replaces the generic throw new Exception(...) with a dedicated InvalidOperationException, enriched with Data["quantityType"], Data["unitType"], Data["unit"], and the original exception as InnerException.
• The catch (Exception ex) when (ex is not InvalidOperationException) filter correctly avoids double-wrapping the manually-thrown InvalidOperationException from the ?? throw expression.

This is a solid improvement over the original patch — richer diagnostics and a single, specific, catchable exception type instead of a bare Exception.

Minor observations (non-blocking)

1. UnitsNetBaseJsonConverter.cs:114-121 — registeredType.Unit ?? unit.GetType() fallback: since _registeredTypes always stores (quantity, unit) as a pair in RegisterCustomType, if registeredQuantity is non-null then registeredType.Unit is guaranteed non-null too. The ?? fallback is unreachable dead code — harmless, but could be simplified to registeredType.Unit! with a comment, or left as defensive code if you'd prefer to guard against future refactors.
2. XML doc — ConvertValueUnit's doc comment (line ~85) only documents <exception cref="UnitsNetException">. Consider adding <exception cref="InvalidOperationException">Thrown when a registered custom quantity type cannot be instantiated</exception> since this is now part of the public contract for subclasses.
3. Test coverage in UnitsNetBaseJsonConverterTest.cs (..._throws_InvalidOperationException_when_registered_quantity_cannot_be_instantiated) is good — it verifies the message, Data entries, and InnerException are all populated correctly.

Note on stale label

@andrewmeservy is right that this is still needed — the underlying null-cast bug is real and the fix (as refined) looks correct and safe to merge. I don't have permission to remove labels, but the fix itself looks ready from a code standpoint.

I wasn't able to run dotnet test in this sandboxed environment (Bash execution requires manual approval here), so please make sure CI passes, but the logic and existing test additions look correct on inspection.

[angularsen]

Release UnitsNet/6.0.0-pre020 · angularsen/UnitsNet

Release JsonNet/6.0.0-pre020 · angularsen/UnitsNet