[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

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

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. Getter evaluated twice ✓ Resolved 🐞 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

---

Tip of the day

💡 Did you know, you can turn on the rule miner and Qodo learns your standards from review history

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

[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