Bidi pass null tests #3083
Bidi pass null tests #3083
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds proper handling for null and undefined values in BiDi realm deserialization. The changes ensure that null/undefined RemoteValue types are correctly distinguished from circular references, which previously both had HasValue=false and were treated the same way.
- Added explicit checks for "null" or "undefined" RemoteValue types across all deserialization paths
- Updated the test expectations to enable the previously failing "should properly serialize null fields" test
- Implemented special handling for Dictionary<string, object> deserialization
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| lib/PuppeteerSharp/Bidi/BidiRealm.cs | Added null/undefined type checks throughout deserialization methods to properly handle null values vs circular references |
| lib/PuppeteerSharp.Nunit/TestExpectations/TestExpectations.local.json | Removed test expectation for "should properly serialize null fields" test, indicating the test now passes |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
lib/PuppeteerSharp/Bidi/BidiRealm.cs
Outdated
| // Null and undefined types have HasValue=false but are not circular references | ||
| if (remoteValue.Type is "null" or "undefined") | ||
| { | ||
| dict[key] = null; | ||
| } | ||
| else if (remoteValue.HasValue) | ||
| { | ||
| dict[key] = DeserializeResult<object>(remoteValue.Value); | ||
| } | ||
| else | ||
| { | ||
| // Circular reference - set to null | ||
| dict[key] = null; | ||
| } |
There was a problem hiding this comment.
The logic for handling null/undefined RemoteValue types is duplicated across five different locations in this file (lines 407-420, 448-452, 515-519, 538-542, and 562-566). Consider extracting this repeated pattern into a private helper method to improve maintainability and reduce code duplication. For example, a method like IsNullOrUndefined(RemoteValue rv) or a method that combines the null check with deserialization logic.
| else | ||
| { | ||
| // Get the value from RemoteValue | ||
| var value = remoteValue.Value; | ||
|
|
||
| // Recursively deserialize the value to the property type | ||
| var deserializedValue = typeof(BidiRealm) | ||
| .GetMethod(nameof(DeserializeResult), BindingFlags.Instance | BindingFlags.NonPublic) | ||
| ?.MakeGenericMethod(property.PropertyType) | ||
| .Invoke(this, [value]); | ||
| // Recursively deserialize the value to the property type | ||
| var deserializedValue = typeof(BidiRealm) | ||
| .GetMethod(nameof(DeserializeResult), BindingFlags.Instance | BindingFlags.NonPublic) | ||
| ?.MakeGenericMethod(property.PropertyType) | ||
| .Invoke(this, [value]); | ||
|
|
||
| // Set the property value on the boxed instance | ||
| property.SetValue(boxedInstance, deserializedValue); | ||
| // Set the property value on the boxed instance | ||
| property.SetValue(boxedInstance, deserializedValue); | ||
| } |
There was a problem hiding this comment.
The property deserialization logic doesn't check for circular references before accessing remoteValue.Value. When remoteValue.Type is not "null" or "undefined" but HasValue is false (indicating a circular reference), this code will attempt to deserialize remoteValue.Value which may lead to unexpected behavior. Consider adding an else-if check for !remoteValue.HasValue to handle circular references, similar to the Dictionary handling code at lines 412-420.
No description provided.