You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
[.NET] Fix issues around handling errors during certificate signing operations - #390
Please add unit coverage for each supported error payload shape and assert that both Accepted and Completed settle. The existing certificate integration test covers only success, while the unit-test MQTT mock can inject responses; without these regression cases, omitted fields, incompatible field types, malformed payloads, and callback failures can reintroduce the indefinite wait this change targets.
A 400 payload of JSON null deserializes without throwing, so this constructs a CertificateSigningRequestFailedException whose required, non-null Error property is actually null. The new test only checks the exception type, but callers such as the integration path dereference e.Error.Message and will then get a NullReferenceException. Treat a null deserialization result as an unreadable response and synthesize the fallback error.
Reject null deserialization results before accepting responses
Valid JSON null does not throw JsonException. For a 202 response, deserialization therefore passes null to SetAccepted, so Accepted succeeds with a null result while Completed remains pending indefinitely—the hang this change is intended to prevent. Explicitly reject null results from each response deserialization before settling the operation.
This statement conflicts with the operation's actual task semantics and the new CertificateSigningErrorAfterAcceptanceFailsOnlyCompletedTask test: once Accepted has succeeded, a later issuance failure cannot make it throw, and a completion callback exception is propagated as its original type. Distinguish failures before acceptance from failures during completion so consumers do not expect both tasks to fault in all cases.
System.Text.Json can assign null when the service sends "info": null, overwriting this initializer while the public property still promises a non-null value. Since the new remarks state every field is optional, make this property nullable (or normalize null in a setter) so its annotation matches the values callers can actually receive.
Reject null error payloads before creating failure exceptions
For an error payload containing JSON null, deserialization succeeds with a null result and creates a CertificateSigningRequestFailedException whose required Error property is null. Callers such as the sample and integration tests dereference ex.Error.Message, so handling this failure causes another NullReferenceException. Route null through the malformed-response handling instead.
This issue also appears on line 202 of the same file.
Document behavior before and after acceptance completes
This still overstates the task behavior: once Accepted has completed successfully, TrySetException cannot rewrite it, and the new post-acceptance error test explicitly verifies that only Completed faults. Describe the before- versus after-acceptance behavior so consumers do not wait for an exception from an already-completed task.
Test null payloads for successful response statuses
The new malformed-response matrix covers JSON null only for an error status, but the 202 branch is the case where null currently completes Accepted with null and leaves Completed hanging. Add null cases for the successful response statuses so this regression is exercised.
JsonSerializer.Deserialize returns null for the valid JSON token null; it does not throw JsonException. Consequently a 202 payload of null calls SetAccepted(null) and leaves Completed pending, a 200 payload can complete with a null result, and an error payload produces an exception whose required Error is null. Explicitly reject null after all three deserializations (for example, ?? throw new JsonException(...)) so these malformed responses take this failure path.
This issue also appears on line 197 of the same file.
This test misses the pending-operation leak: SendCertificateSigningRequestAsync inserts the operation before subscribing/publishing and never removes it when either await throws. Thus each failed send leaves an operation that the caller cannot access in _pendingCertificateSigningOperations. Assert that the count returns to zero here and clean up the dictionary on send failure.
This contract says both tasks throw whenever the operation fails, but a failure after acceptance leaves Accepted successful (as the new test verifies), and a completion callback can propagate an exception other than CertificateSigningRequestFailedException. Distinguish failures before and after acceptance and callback failures so consumers can rely on the public documentation.
Throw JsonException if response is null after deserialization.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
This public contract still says both tasks fail for an error at any point, but the new callback-failure behavior (and the added test) leaves Accepted successfully completed and faults only Completed. The same is true for any hub failure after a successful 202. Document the phase-dependent behavior so callers do not incorrectly expect Accepted to be rewritten.
/// <returns>A set of tasks. One that completes when IoT hub accepts the request (and starts signing), one that completes when IoT hub completes the signing, and one that completes if any step in the process fails.</returns>
public async Task<CertificateSigningOperation> SendCertificateSigningRequestAsync(IotHubCertificateSigningRequest request, CancellationToken cancellationToken = default)
{
//TODO how does hub respond if device loses connection at any point during this process?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Some error payload shapes weren't deserialized correctly so the certificate operation call would appear to hang