Fix #2400: AddObjectStatic throwing on null property values

[Bafyn]

Description

Closes #2400

Problem

AddObjectStatic picks a conversion path from each property's static type but dereferences the runtime value without a null check. A DTO with an unset optional field therefore crashes while the request is being built.

Purpose

This pull request is a:

• Bugfix (non-breaking change which fixes an issue)

Checklist

• I have added tests that prove my fix is effective

[qodo-free-for-open-source-projects]

PR Summary by Qodo

Fix AddObjectStatic null property handling to skip unset optional values

🐞 Bug fix 🧪 Tests 🕐 10-20 Minutes

AI Description

• Prevent AddObjectStatic from throwing when DTO properties are null.
• Skip null-valued properties to match reflection-based AddObject behavior.
• Add regression tests covering mixed null/non-null and all-null DTOs.

Diagram

graph TD
  A["DTO instance"] --> B["RestRequest.AddObjectStatic"] --> C["PropertyCache.Populator.From"] --> D["getObject(model)"] --> E{"value is null?"}
  E -->|"yes"| F["skip property"]
  E -->|"no"| G["populate(model, parameters)"] --> H["Request.Parameters"]

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Add null checks inside each typed conversion path

• ➕ Keeps null-handling co-located with type-specific conversion logic
• ➕ Potentially allows type-specific null semantics (e.g., empty vs omitted)
• ➖ More code churn across multiple conversion paths
• ➖ Higher risk of missing a path and regressing behavior

2. Pre-filter properties with null values before building populators

• ➕ Avoids wrapping delegates per property
• ➕ Centralizes filtering logic in one place
• ➖ May require extra reflection/value reads up front
• ➖ Harder to keep parity with existing caching/compiled delegate strategy

Recommendation: The chosen wrapper delegate approach in Populator.From is a good fit: it’s minimal, consistently prevents dereferencing null runtime values regardless of static type, and explicitly aligns AddObjectStatic behavior with AddObject. Alternatives add broader churn or complicate caching without clear benefit.

Files changed (2) +70 / -1

Bug fix (1) +9 / -1

PropertyCache.Populator.csGuard AddObjectStatic populator against null runtime property values +9/-1

Guard AddObjectStatic populator against null runtime property values

• Wraps the generated per-property populate delegate with a runtime null check on the property getter. If the property value is null, the populator returns without adding parameters, matching reflection-based AddObject behavior.

src/RestSharp/Request/PropertyCache.Populator.cs

Tests (1) +61 / -0

ObjectParameterTests.NullData.csAdd AddObjectStatic null-property regression coverage +61/-0

Add AddObjectStatic null-property regression coverage

• Introduces tests asserting AddObjectStatic skips null properties, retains non-null properties, yields no parameters when all are null, and matches AddObject behavior for equivalent inputs.

test/RestSharp.Tests/Parameters/ObjectParameterTests.NullData.cs

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[qodo-free-for-open-source-projects]

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (0) 📎 Requirement gaps (0) 📜 Skill insights (0)

1. Getter evaluated twice 🐞 Bug ☼ Reliability

Description

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.

Code

src/RestSharp/Request/PropertyCache.Populator.cs[R81-84]

+                (model, parameters) => {
+                    if (getObject(model) is null) return;
+                    populate(model, parameters);
+                }

Evidence

The new wrapper performs a null-check by calling getObject(model), but the populate delegate
produced by GetPopulate(getObject, property) reads the property again via getObject(entity)
during conversion/population, causing duplicate evaluations.

src/RestSharp/Request/PropertyCache.Populator.cs[54-86]
src/RestSharp/Request/PropertyCache.Populator.cs[122-146]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### 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

---

Context used

✅ Tickets: 🎫 AddObjectStatic throws on any null property value (inconsistent with AddObject)

_{To customize comments, go to the Qodo configuration screen, or learn more in the docs.}

[qodo-free-for-open-source-projects]

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]

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