Skip to content

Conversation

@daniel-jonathan
Copy link
Contributor

@daniel-jonathan daniel-jonathan commented Nov 19, 2025

Optimize Dictionary Lookups in Model Equals Methods

Problem

The generated model Equals methods perform inefficient dictionary comparisons for AdditionalProperties:

input.AdditionalProperties.ContainsKey(kv.Key) && Equals(kv.Value, input.AdditionalProperties[kv.Key])

This performs two dictionary lookups per key:

  1. ContainsKey(kv.Key) - checks if key exists
  2. input.AdditionalProperties[kv.Key] - retrieves the value

CodeQL Warning

Source: CodeQL static analysis on openfga/dotnet-sdk#156

Warning: "Inefficient use of ContainsKey and indexer"

Impact: Performance penalty on every equality check for models with AdditionalProperties

Solution

Use TryGetValue which performs both operations in a single dictionary lookup:

input.AdditionalProperties.TryGetValue(kv.Key, out var inputValue) && Equals(kv.Value, inputValue)

Changes

File: config/clients/dotnet/template/modelGeneric.mustache line 357

Before:

&& (this.AdditionalProperties.Count == input.AdditionalProperties.Count && this.AdditionalProperties.All(kv => input.AdditionalProperties.ContainsKey(kv.Key) && Equals(kv.Value, input.AdditionalProperties[kv.Key])));

After:

&& (this.AdditionalProperties.Count == input.AdditionalProperties.Count && this.AdditionalProperties.All(kv => input.AdditionalProperties.TryGetValue(kv.Key, out var inputValue) && Equals(kv.Value, inputValue)));

Impact

This optimization applies to all generated models with AdditionalProperties, including:

  • All 79+ model files in the dotnet SDK
  • Future model generations
  • Reduces O(2) dictionary operations to O(1)

Testing

Verified that the generated code:

  • Builds successfully with 0 errors
  • Maintains identical equality semantics
  • Resolves CodeQL warning
  • Follows C# best practices

References

Files Modified

  • config/clients/dotnet/template/modelGeneric.mustache

Summary by CodeRabbit

  • Refactor
    • Optimized object comparison logic for improved performance.

✏️ Tip: You can customize this high-level summary in your review settings.

Address CodeQL warning about inefficient ContainsKey usage by replacing
ContainsKey + indexer pattern with TryGetValue.

Changes:
- Update modelGeneric.mustache template line 357
- Replace two dictionary lookups with single TryGetValue call
- Applies to all generated models with AdditionalProperties

Before (inefficient - 2 lookups):
  input.AdditionalProperties.ContainsKey(kv.Key) &&
    Equals(kv.Value, input.AdditionalProperties[kv.Key])

After (efficient - 1 lookup):
  input.AdditionalProperties.TryGetValue(kv.Key, out var inputValue) &&
    Equals(kv.Value, inputValue)

This resolves the CodeQL warning from dotnet-sdk PR #156 and improves
performance for all model equality comparisons across the SDK.
@daniel-jonathan daniel-jonathan requested a review from a team as a code owner November 19, 2025 21:44
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 19, 2025

Important

Review skipped

Auto incremental reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Walkthrough

A .NET template file was modified to refactor the Equals method's AdditionalProperties comparison logic. The implementation changed from using ContainsKey and key lookup to using TryGetValue within a lambda expression for retrieving and comparing property values.

Changes

Cohort / File(s) Summary
Equals method comparison refactoring
config/clients/dotnet/template/modelGeneric.mustache
Modified AdditionalProperties equality check to use TryGetValue instead of ContainsKey with direct key lookup; introduces local variable for retrieved value comparison

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

  • Verify that TryGetValue properly replaces the ContainsKey + lookup pattern without behavioral changes
  • Confirm the null-check logic and comparison semantics remain equivalent to the original implementation
  • Check template syntax validity and proper escaping for Mustache templating

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title directly and accurately summarizes the main change: optimizing dictionary lookups in .NET model Equals methods by replacing ContainsKey+indexer with TryGetValue.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

rhamzeh
rhamzeh previously approved these changes Nov 19, 2025
Address two CodeQL warnings in generated model Equals methods:

1. Inefficient ContainsKey usage
   - Replace ContainsKey + indexer with TryGetValue
   - Reduces dictionary lookups from 2 to 1 per key

2. Equals should not apply "as" cast
   - Add GetType() check before casting
   - Ensures proper equality for subclasses

Changes:
- Line 320-321: Fix Equals(object) to use GetType() check
- Line 357: Optimize AdditionalProperties comparison with TryGetValue

These fixes apply to all generated models with AdditionalProperties.

Resolves CodeQL warnings from dotnet-sdk PR #156.
@daniel-jonathan daniel-jonathan added this pull request to the merge queue Nov 20, 2025
Merged via the queue into main with commit 48ad3c4 Nov 20, 2025
13 of 15 checks passed
@daniel-jonathan daniel-jonathan deleted the fix/contains_key_usage branch November 20, 2025 00:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants