Fix ParamName in guard of Base64.GetMaxEncodedToUtf8Length - #131085
Fix ParamName in guard of Base64.GetMaxEncodedToUtf8Length#131085lilinus wants to merge 1 commit into
Base64.GetMaxEncodedToUtf8Length#131085Conversation
|
Azure Pipelines: Successfully started running 3 pipeline(s). 13 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
|
Tagging subscribers to this area: @dotnet/area-system-memory |
|
Workflow state for the Holistic Review Orchestrator. {
"version": 5,
"last_dispatched_commit": "3499bfadbec56338cc12deccfd58740adc237a4c",
"last_dispatched_base_ref": "main",
"last_dispatched_base_sha": "ea3f7f141e0596cab37785d305910e64d031ab29",
"last_reviewed_commit": "3499bfadbec56338cc12deccfd58740adc237a4c",
"last_reviewed_base_ref": "main",
"last_reviewed_base_sha": "ea3f7f141e0596cab37785d305910e64d031ab29",
"last_recorded_worker_run_id": "29755369720",
"review_attempt_commit": "",
"review_attempt_base_ref": "",
"review_attempt_count": 0,
"max_review_attempts": 5,
"review_history_format": "holistic-review-disclosure-v1",
"review_history": [
{
"commit": "3499bfadbec56338cc12deccfd58740adc237a4c",
"review_id": 4736503634
}
]
} |
There was a problem hiding this comment.
Holistic Review
Motivation: Base64.GetEncodedLength (surfaced through GetMaxEncodedToUtf8Length) and Base64Url.GetEncodedLength validate their input with ArgumentOutOfRangeException.ThrowIfGreaterThan<uint>((uint)bytesLength, MaximumEncodeLength). Because that overload derives paramName from CallerArgumentExpression, the thrown exception's ParamName was the literal expression "(uint)bytesLength" rather than the public parameter name bytesLength. That is confusing to callers and inconsistent with the non-NET fallback path, which already threw with nameof(bytesLength).
Approach: The fix passes nameof(bytesLength) explicitly to ThrowIfGreaterThan in both the Base64 and Base64Url encoders, so ParamName is now the correct bytesLength. The tests in Base64EncoderUnitTests and Base64UrlEncoderUnitTests are updated to assert the expected ParamName (Assert.Throws<ArgumentOutOfRangeException>("bytesLength", ...)) for both the overflow and negative-input cases, locking in the corrected behavior.
Summary: This is a minimal, correct, and well-targeted bug fix. The change aligns the #if NET validation path with the existing non-NET fallback (throw new ArgumentOutOfRangeException(nameof(bytesLength))), so both paths now report the same ParamName. The updated assertions provide regression coverage. The ParamName value is a technically observable change, but moving from an internal expression string to the documented parameter name is a strict improvement and not a meaningful compatibility concern. No functional behavior, thrown exception type, or valid-input results change. I found no correctness, performance, or convention issues. LGTM.
Note
This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.
Generated by Holistic Review · 39.6 AIC · ⌖ 9.7 AIC · ⊞ 10K
| Assert.Throws<ArgumentOutOfRangeException>(() => Base64.GetMaxEncodedToUtf8Length(-1)); | ||
| Assert.Throws<ArgumentOutOfRangeException>(() => Base64.GetMaxEncodedToUtf8Length(int.MinValue)); | ||
| Assert.Throws<ArgumentOutOfRangeException>("bytesLength", () => Base64.GetMaxEncodedToUtf8Length(-1)); | ||
| Assert.Throws<ArgumentOutOfRangeException>("bytesLength", () => Base64.GetMaxEncodedToUtf8Length(int.MinValue)); |
There was a problem hiding this comment.
Looks like Base64.GetMaxEncodedToUtf8Length's parameter is called just length
There was a problem hiding this comment.
I missed that detail
Without fix
ParamNameof the exception was(uint)bytesLengthAlso fix same issue for
Base64Url.GetEncodedLength