Repository navigation
fix: Relax detection of resumable upload methods. - #922
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the resumable upload allowlist mechanism to support wildcard-like matching using prefixes and suffixes, avoiding the need to restrict resumable uploads to specific Google Ads API versions. The allowlist is changed from a HashSet to a Dictionary, and the lookup logic is updated to use StartsWith and EndsWith. Feedback suggests optimizing this implementation by using an array of named tuples instead of a Dictionary to avoid lookup overhead, and explicitly specifying StringComparison.Ordinal in the string comparison methods to prevent culture-sensitive comparison issues.
b5f0758 to
8dafbe8
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the resumable upload allowlist in RestMethod.cs from a strict HashSet<string> to an array of prefix/suffix tuples, allowing matching across different versions of the Google Ads API without explicit allowlist entries. Corresponding unit tests were added to verify this behavior. The feedback suggests using StringComparison.Ordinal instead of StringComparison.OrdinalIgnoreCase for case-sensitive protobuf names, adding a length check to prevent overlapping prefix/suffix matches, and updating the XML documentation to correctly refer to the new tuple array structure instead of a dictionary.
See b/573617047.
8dafbe8 to
9cd0483
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the resumable upload method allowlist to support prefix and suffix matching, which avoids having to manually add new Google Ads API versions. It also adds corresponding unit tests. The review feedback suggests replacing the LINQ Any call with a foreach loop to avoid closure allocations on every request, which would also allow removing the unused using System.Linq; directive.
See b/573617047.