If ByteArrayDeserializer gets an empty array, return null instead of calling Activator.CreateInstance#70
Conversation
…calling Activator.CreateInstance TODO: Maybe make this behavior configurable before merge? Resolves #22
There was a problem hiding this comment.
Pull Request Overview
This PR updates ByteArrayDeserializer to handle empty byte arrays differently by returning null instead of calling Activator.CreateInstance for non-byte[] types. Key changes include adding an early return for empty arrays in the deserializer and introducing new test cases to validate the behavior for both byte arrays and objects.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| Activout.RestClient/Serialization/Implementation/ByteArrayDeserializer.cs | Added logic to return a null object when an empty byte array is encountered for types other than byte[]. |
| Activout.RestClient.Newtonsoft.Json.Test/RestClientTests.cs | Added tests to cover scenarios for empty byte arrays for both byte[] and object deserialization. |
Activout.RestClient/Serialization/Implementation/ByteArrayDeserializer.cs
Outdated
Show resolved
Hide resolved
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds explicit handling in the byte-array deserializer: return raw bytes for Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant ByteArrayDeserializer as Deserializer
participant Activator
Caller->>Deserializer: Deserialize(bytes, targetType)
alt targetType == byte[]
Deserializer-->>Caller: return bytes
else targetType is value type
Deserializer-->>Caller: throw InvalidOperationException
else bytes.length == 0
Deserializer-->>Caller: return null
else
Deserializer->>Activator: CreateInstance(targetType, bytes)
Activator-->>Deserializer: instance
Deserializer-->>Caller: return instance
end
note right of Deserializer #f0f4c3: New branches for empty bytes and byte[] target
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
Activout.RestClient/Serialization/Implementation/ByteArrayDeserializer.cs (1)
22-25: Document the rationale (and configurability) in code.Add a short comment explaining why empty payloads map to null (except byte[]), and reference the “consider making configurable” TODO from the PR description for future maintainers.
if (bytes.Length == 0) { - return type == typeof(byte[]) ? bytes : null; + // Empty payload: for raw byte[] return the empty array; for other targets, treat as "no value". + // TODO: consider making this behavior configurable (see PR #70 / Issue #22). + return type == typeof(byte[]) ? bytes : null; }
🧹 Nitpick comments (3)
Activout.RestClient/Serialization/Implementation/ByteArrayDeserializer.cs (1)
22-25: Guard against value-type targets on empty payloads.If a consumer ever requests a value type (struct) here, returning null will fail downstream. Consider throwing a clearer exception instead.
if (bytes.Length == 0) { - return type == typeof(byte[]) ? bytes : null; + if (type == typeof(byte[])) return bytes; + if (type.IsValueType) + throw new InvalidOperationException($"Cannot deserialize empty application/octet-stream to value type '{type.FullName}'."); + return null; }Activout.RestClient.Newtonsoft.Json.Test/RestClientTests.cs (2)
484-491: Prefer Array.Empty() and set Content-Type explicitly.Minor: avoid allocating new arrays and ensure the byte deserializer is chosen deterministically.
- _mockHttp - .When($"{BaseUri}/movies/bytes") - .Respond(new ByteArrayContent(new byte[0])); + _mockHttp + .When($"{BaseUri}/movies/bytes") + .Respond(_ => + { + var content = new ByteArrayContent(Array.Empty<byte>()); + content.Headers.ContentType = new MediaTypeHeaderValue("application/octet-stream"); + return content; + });
523-526: Apply the same header and allocation tweaks here.Mirror the improvements from the other test for consistency.
- _mockHttp - .When($"{BaseUri}/movies/byte-object") - .Respond(new ByteArrayContent(new byte[0])); + _mockHttp + .When($"{BaseUri}/movies/byte-object") + .Respond(_ => + { + var content = new ByteArrayContent(Array.Empty<byte>()); + content.Headers.ContentType = new MediaTypeHeaderValue("application/octet-stream"); + return content; + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Activout.RestClient.Newtonsoft.Json.Test/RestClientTests.cs(2 hunks)Activout.RestClient/Serialization/Implementation/ByteArrayDeserializer.cs(1 hunks)
🔇 Additional comments (3)
Activout.RestClient/Serialization/Implementation/ByteArrayDeserializer.cs (1)
22-25: Behavior change aligns with Issue #22—good early return.Returning null for non-byte[] targets on empty payloads matches the stated objective; byte[] continues to return the empty array. Looks good.
Please confirm there are no call sites that relied on Activator.CreateInstance for empty payloads (potential behavioral break).
Activout.RestClient.Newtonsoft.Json.Test/RestClientTests.cs (2)
484-501: Nice coverage for empty byte[] case.Assertions are precise: non-null and empty for byte[]. LGTM.
519-535: Good null-path assertion for object target.Validates the new empty-payload behavior for non-byte[] types. LGTM.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
Activout.RestClient/Serialization/Implementation/ByteArrayDeserializer.cs (1)
11-14: Use collection expression syntax per coding guidelines.The coding guidelines specify using
[]list syntax for collections instead ofnew[].Apply this diff:
- public IReadOnlyCollection<MediaType> SupportedMediaTypes => new[] - { - MediaType.ValueOf("application/octet-stream") - }; + public IReadOnlyCollection<MediaType> SupportedMediaTypes => + [ + MediaType.ValueOf("application/octet-stream") + ];As per coding guidelines
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Activout.RestClient.Test/NonJsonRestClientTests.cs(2 hunks)Activout.RestClient/Serialization/Implementation/ByteArrayDeserializer.cs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.cs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.cs: Use [] list syntax for collections
Use file-scoped namespaces
Use primary constructors wherever possible
Use records for data transfer objects (DTOs) and immutable data structures
Use var for local variable declarations when possible
Use expression-bodied members for simple methods and properties
Use pattern matching for type checks and deconstruction
Use using statements for resource management
Use async and await for asynchronous programming
Never include "Async" in method names
Only include comments that explain why something is done, not what is done
Files:
Activout.RestClient.Test/NonJsonRestClientTests.csActivout.RestClient/Serialization/Implementation/ByteArrayDeserializer.cs
🧬 Code graph analysis (1)
Activout.RestClient.Test/NonJsonRestClientTests.cs (2)
Activout.RestClient.Newtonsoft.Json.Test/RestClientTests.cs (11)
Fact(52-76)Fact(78-97)Fact(114-148)Fact(150-166)Fact(168-188)Fact(190-214)Fact(216-243)Fact(245-272)Fact(274-290)Fact(292-308)Activout(16-310)Activout.RestClient.Test.Json/RestClientTests.cs (1)
RestClientTests(29-348)
🔇 Additional comments (4)
Activout.RestClient/Serialization/Implementation/ByteArrayDeserializer.cs (2)
21-24: LGTM! Early return for byte[] target type is correct.This appropriately returns the raw byte array immediately when the target type is
byte[], avoiding unnecessary object instantiation and correctly handling both empty and non-empty arrays.
32-33: Empty array handling correctly implements the PR objective.The null return for empty byte arrays resolves issue #22 as intended. The comment appropriately explains the rationale.
Note: A past review suggested adding a comment explaining why empty arrays return the bytes for
byte[]types (line 23) but null for object types (line 33). While the current implementation is clear, you may consider adding a brief comment at line 21-24 if you want to address that feedback.Based on learnings
Activout.RestClient.Test/NonJsonRestClientTests.cs (2)
232-248: LGTM! Test correctly validates byte[] return behavior for empty arrays.This test properly verifies that when the target type is
byte[], the deserializer returns an empty byte array rather than null, which aligns with the early return logic in ByteArrayDeserializer (lines 21-24).
267-282: LGTM! Test correctly validates null return behavior for empty arrays with object types.This test properly verifies that when the target type is an object (not
byte[]), the deserializer returns null for empty byte arrays, which aligns with the new behavior in ByteArrayDeserializer (line 33). Together withTestGetEmptyByteArray, these tests provide comprehensive coverage of the PR objective.
Activout.RestClient/Serialization/Implementation/ByteArrayDeserializer.cs
Show resolved
Hide resolved
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: David Eriksson <david@activout.se>
TODO: Maybe make this behavior configurable before merge?
Resolves #22
Summary by CodeRabbit
Bug Fixes
Tests