Skip to content

Fix #2400: AddObjectStatic throwing on null property values #2401

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Bafyn wants to merge 1 commit into
base: dev
Choose a base branch
from
+70 −1
Open

Fix #2400: AddObjectStatic throwing on null property values #2401

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion src/RestSharp/Request/PropertyCache.Populator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,15 @@ internal static Populator From(PropertyInfo property) {

var populate = GetPopulate(getObject, property);

return new(property.Name, populate);
// Skip null property values so a DTO with unset optional properties doesn't throw.
// This matches the reflection-based AddObject.
return new(
property.Name,
(model, parameters) => {
if (getObject(model) is null) return;
populate(model, parameters);
}
Comment on lines +81 to +84

@qodo-free-for-open-source-projects qodo-free-for-open-source-projects Bot Aug 5, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remediation recommended

1. Getter evaluated twice 🐞 Bug ☼ Reliability


Populator.From now calls the property getter once for the null-check and again inside the cached
populate delegate, so non-null properties are evaluated twice. This can double side
effects/expensive getters and can still throw if the value changes to null between the two reads.
Agent Prompt 
### Issue description
`Populator.From(PropertyInfo)` wraps the generated `populate` delegate with a null-check by calling `getObject(model)` and then invoking `populate(model, parameters)`. However, `populate` itself calls `getObject(entity)` again (via `GetPopulate(getObject, property)`), so each non-null property getter is invoked twice.

This changes behavior for stateful/non-idempotent getters and adds avoidable overhead.

### Issue Context
The fix for #2400 is correct (skip null values), but it should not require re-reading the property.

### Fix Focus Areas
- src/RestSharp/Request/PropertyCache.Populator.cs[54-86]
- src/RestSharp/Request/PropertyCache.Populator.cs[122-146]

### Suggested approach
Refactor so the getter is evaluated once per property population:
- Capture `var value = getObject(model);`
- If `value is null`, return.
- Use `value` for the conversion/population path (e.g., introduce a `GetPopulate` variant that accepts the already-fetched `object value`, or build the population logic in `From` based on `property.PropertyType` but operating on the captured `value`).

This preserves the null-skip behavior while avoiding duplicate getter evaluation.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

@Bafyn Bafyn Aug 5, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We technically can do this and pass the captured value to the populator instead of calculating it again from the model, but I'd like to get the confirmation that this is the way we want to go first @alexeyzimarev.

This change is going to affect a few other methods in this class

);
}

static Action<T, ICollection<Parameter>> GetPopulate(Func<T, IFormattable> getFormattable, RequestProperty requestProperty)
Expand Down
61 changes: 61 additions & 0 deletions test/RestSharp.Tests/Parameters/ObjectParameterTests.NullData.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
namespace RestSharp.Tests.Parameters;

public partial class ObjectParameterTests {
[Fact]
public void AddObjectStatic_skips_null_properties() {
var data = new NullableData { Kind = "set" };

var request = new RestRequest().AddObjectStatic(data);

request
.Parameters
.Should()
.ContainSingle()
.Which
.Should()
.BeEquivalentTo(new GetOrPostParameter(nameof(NullableData.Kind), "set"));
}

[Fact]
public void AddObjectStatic_keeps_non_null_properties_and_skips_null_ones() {
var data = new NullableData { Name = "Bob", Age = 30, Link = null, Values = null, Kind = "set" };

var request = new RestRequest().AddObjectStatic(data);

request
.Parameters
.Should()
.BeEquivalentTo(new[] {
new GetOrPostParameter(nameof(NullableData.Name), "Bob"),
new GetOrPostParameter(nameof(NullableData.Age), "30"),
new GetOrPostParameter(nameof(NullableData.Kind), "set")
});
}

[Fact]
public void AddObjectStatic_with_all_null_properties_yields_no_parameters() {
var data = new NullableData { Kind = null };

var request = new RestRequest().AddObjectStatic(data);

request.Parameters.Should().BeEmpty();
}

[Fact]
public void AddObjectStatic_null_property_handling_matches_AddObject() {
var data = new NullableData { Name = null, Age = null, Link = null, Values = null, Kind = "set" };

var objStatic = new RestRequest().AddObjectStatic(data);
var reflection = new RestRequest().AddObject(data);

objStatic.Parameters.Should().BeEquivalentTo(reflection.Parameters);
}

class NullableData {
public string Name { get; set; }
public int? Age { get; set; }
public Uri Link { get; set; }
public List<int> Values { get; set; }
public string Kind { get; set; } = "set";
}
}