Added input sanitization for deprecatedMessage values from service models before they are emitted into generated C# code.#4359
Conversation
GarrettBeatty
changed the title
There was a problem hiding this comment.
Pull request overview
This PR adds sanitization for deprecation messages (operation/shape/member) to prevent malformed generated code and adds unit tests plus a test service model to verify the sanitization behavior.
Changes:
- Added
SanitizeDeprecationMessage()and applied it toDeprecationMessageonOperation,Shape, andMember. - Added unit tests and a fixture model JSON to validate deprecation message sanitization.
- Minor formatting change in
SimpleModels.cs(blank line).
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| generator/ServiceClientGeneratorTests/UnitTests.cs | Adds tests validating deprecation message sanitization for operations, shapes, and members. |
| generator/ServiceClientGeneratorTests/Content/TestModelDeprecated.json | Adds a test service model containing deprecated traits/messages with newlines, quotes, and HTML tags. |
| generator/ServiceClientGeneratorLib/SimpleModels.cs | Minor whitespace/formatting change. |
| generator/ServiceClientGeneratorLib/Shape.cs | Sanitizes shape DeprecationMessage before returning. |
| generator/ServiceClientGeneratorLib/Operation.cs | Sanitizes operation DeprecationMessage before returning. |
| generator/ServiceClientGeneratorLib/Member.cs | Sanitizes member DeprecationMessage before returning. |
| generator/ServiceClientGeneratorLib/GeneratorHelpers.cs | Introduces SanitizeDeprecationMessage() helper. |
You can also share your feedback on Copilot code review. Take the survey.
| public static string SanitizeDeprecationMessage(this string s) | ||
| { | ||
| return s | ||
| .Replace("\r", string.Empty) | ||
| .Replace("\n", string.Empty) | ||
| .Replace("\"", "'") | ||
| .Replace("<p>", string.Empty) | ||
| .Replace("</p>", string.Empty) |
| return s | ||
| .Replace("\r", string.Empty) | ||
| .Replace("\n", string.Empty) | ||
| .Replace("\"", "'") |
| /// break generated code (e.g. newlines, HTML tags, quotes in [Obsolete("...")] attributes). | ||
| /// </summary> | ||
| public static string SanitizeDeprecationMessage(this string s) | ||
| { |
|
|
||
| #region Deprecation Message Sanitization Tests | ||
|
|
||
| private const string _deprecatedModelsPath = "../../Content/TestModelDeprecated.json"; |
| { | ||
| "version": "1.0", | ||
| "metadata": { | ||
| "apiVersion": "2020-6-25", |
| @@ -0,0 +1,43 @@ | |||
| { | |||
| "version": "1.0", | |||
There was a problem hiding this comment.
I wonder if this approach using a json file can be simplified by just testing that the method works as intended by testing it with a string. Is there a reason a json file was chosen like this?
There was a problem hiding this comment.
Right now the only way to verify the generator works is by running against the real models (these unit tests don't run anywhere in our build system, and I think they were broken for quite some time).
So for now I'd say let's just included the generator/ServiceClientGeneratorLib/ changes in the PR. Also, I assume we'd need to do the same for V3?
There was a problem hiding this comment.
i did run locally by manually updating one of the service json files, i just havent updated the PR description yet. and yeah i will also need to do same for v3
Description
Added input sanitization for
deprecatedMessagevalues from service models before they are emitted into generated C# code. ThedeprecatedMessagefield was being used raw in[Obsolete("...")]attributes, which could produce broken generated code if the message contained newlines (\r,\n), double quotes ("), or HTML tags (<p>,</p>,<code>,</code>).A new
SanitizeDeprecationMessage()extension method was added to theStringExtensionsclass inGeneratorHelpers.csand is now called from theDeprecationMessageproperty getters inMember.cs,Operation.cs, andShape.cs.Motivation and Context
Other fields in the generator (e.g., documentation strings) are sanitized via
Replace("\n", string.Empty)and similar transformations before being emitted into generated code. However,deprecatedMessagewas not receiving any sanitization. Since deprecation messages are placed directly into[Obsolete("...")]string literals in generated C# code, unsanitized content such as newlines or quotes could break compilation of the generated SDK code.Testing
ServiceClientGeneratorTests/UnitTests.csthat verify sanitization of deprecation messages for operations, shapes, and members.TestModelDeprecated.json) containing deprecated items with newlines,\r, HTML tags (<p>,</p>,<code>,</code>), and double quotes in theirdeprecatedMessagevalues.Dry-run
Breaking Changes Assessment
No breaking changes. This is a strictly additive sanitization step applied to generated code output. The sanitization only removes/replaces characters that would have caused issues in generated code (
\r,\n,", HTML tags). Existing valid deprecation messages that don't contain these characters are unaffected.Screenshots (if appropriate)
N/A
Types of changes
Checklist
License