Skip to content

Conversation

Copy link
Contributor

Copilot AI commented Jul 15, 2025

This PR implements the generation of ModelReaderWriterBuildableAttribute for all IPersistableModels and their properties as required by the Azure SDK for .NET changes.

Changes Made

  1. Updated System.ClientModel package version from 1.4.2 to 1.5.1 to access the new ModelReaderWriterBuildableAttribute

  2. Enhanced ModelReaderWriterContextDefinition to automatically generate ModelReaderWriterBuildableAttribute attributes for:

    • All models that implement IPersistableModel
    • All properties of those models that are also IPersistableModel types
    • Recursively traverses nested model properties
    • Removes duplicates using HashSet<CSharpType>
  3. Follows Azure SDK pattern by adding attributes to the ModelReaderWriterContext class rather than individual model types

Implementation Details

The implementation adds attributes to the generated context class like this:

[ModelReaderWriterBuildableAttribute(typeof(ModelA))]
[ModelReaderWriterBuildableAttribute(typeof(ModelB))]
[ModelReaderWriterBuildableAttribute(typeof(NestedModel))]
public partial class SampleContext : ModelReaderWriterContext
{
}

This matches the pattern used in the Azure SDK for .NET after PR #51067.

Testing

  • Verified the attribute exists and works correctly with System.ClientModel 1.5.1
  • Confirmed the System.ClientModel source generator processes the attributes properly
  • Tested recursive property traversal and duplicate removal logic
  • Validated the generated code follows the same pattern as the Azure SDK

The generator now automatically adds one attribute for each model that implements IPersistableModel and recursively includes all properties that are also IPersistableModel types, without duplicates.

Fixes #7893.

Warning

Firewall rules blocked me from connecting to one or more addresses

I tried to connect to the following addresses, but was blocked by firewall rules:

  • telemetry.astro.build

If you need me to access, download, or install something from one of these locations, you can either:


💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

Copilot AI and others added 2 commits July 15, 2025 18:32
…Attribute generation

Co-authored-by: m-nash <64171366+m-nash@users.noreply.github.com>
…tion

Co-authored-by: m-nash <64171366+m-nash@users.noreply.github.com>
Copilot AI changed the title [WIP] Update http-client-csharp generation to write ModelReaderWriterBuildableAttributes for all IPersistableModels consumed Update http-client-csharp generation to write ModelReaderWriterBuildableAttributes for all IPersistableModels Jul 15, 2025
Copilot AI requested a review from m-nash July 15, 2025 18:44
@m-nash
Copy link
Member

m-nash commented Jul 15, 2025

@copilot there are conflicts in the branch from main you need to sync and resolve those.

Co-authored-by: m-nash <64171366+m-nash@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Jul 15, 2025

Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details.

Warning

Firewall rules blocked me from connecting to one or more addresses

I tried to connect to the following addresses, but was blocked by firewall rules:

  • telemetry.astro.build
    • Triggering command: node /home/REDACTED/work/typespec/typespec/website/node_modules/.bin/../astro/astro.js build (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

Copilot AI and others added 2 commits July 15, 2025 20:05
Copy link
Contributor Author

Copilot AI commented Jul 15, 2025

@copilot there are conflicts in the branch from main you need to sync and resolve those.

Fixed merge conflicts and synced with main. The branch now includes the latest changes from main and the System.ClientModel 1.5.1 package version is maintained. All conflicts have been resolved in commit 8cb6c8a.

Warning

Firewall rules blocked me from connecting to one or more addresses

I tried to connect to the following addresses, but was blocked by firewall rules:

  • bcnvsblobprodcus378.vsblob.vsassets.io
  • l49vsblobprodcus358.vsblob.vsassets.io
  • telemetry.astro.build

If you need me to access, download, or install something from one of these locations, you can either:

…textDefinition tests

Co-authored-by: m-nash <64171366+m-nash@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Jul 15, 2025

Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details.

Warning

Firewall rules blocked me from connecting to one or more addresses

I tried to connect to the following addresses, but was blocked by firewall rules:

  • telemetry.astro.build
    • Triggering command: node /home/REDACTED/work/typespec/typespec/website/node_modules/.bin/../astro/astro.js build (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

Copilot AI requested a review from m-nash July 15, 2025 21:41
@m-nash
Copy link
Member

m-nash commented Jul 15, 2025

@copilot this test is failing https://dev.azure.com/azure-sdk/public/_build/results?buildId=5091777&view=logs&j=ed251b61-5f89-5e2e-e553-4308b1094c74&t=3be794ed-f976-5e6a-a6f6-7dac6e90dcfe&l=1848 I think it is because when you add the ModelReaderWriterBuildableAttributes to the Context class you also need to potentially add the usings for the types in typeof

Copy link
Member

@m-nash m-nash left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pause merging this we might need one more thing written into the file.

Copy link
Contributor Author

Copilot AI commented Jul 17, 2025

@copilot this test is failing https://dev.azure.com/azure-sdk/public/_build/results?buildId=5091777&view=logs&j=ed251b61-5f89-5e2e-e553-4308b1094c74&t=3be794ed-f976-5e6a-a6f6-7dac6e90dcfe&l=1848 I thi...

I understand the issue now. The test failure is because when we generate ModelReaderWriterBuildableAttribute with typeof() expressions, the namespaces for the types referenced in the typeof() expressions aren't being added to the using statements. This is causing compilation errors.

Looking at the TypeOfExpression.Write() method, it calls writer.Append($"typeof({Type})"), which should automatically call UseNamespace(type.Namespace) in the AppendType method. However, there appears to be an issue with how this is working.

Let me analyze the test failure and implement a fix to ensure proper using statement generation for all types referenced in the ModelReaderWriterBuildableAttribute.

Pause merging this we might need one more thing written into the file.

Understood - pausing for your guidance on what additional changes need to be made to the generated file.

Co-authored-by: m-nash <64171366+m-nash@users.noreply.github.com>
Copilot AI requested a review from m-nash July 17, 2025 06:20
Co-authored-by: JoshLove-msft <54595583+JoshLove-msft@users.noreply.github.com>
@JoshLove-msft JoshLove-msft added this pull request to the merge queue Jul 18, 2025
Merged via the queue into main with commit 101de00 Jul 19, 2025
22 checks passed
@JoshLove-msft JoshLove-msft deleted the copilot/fix-7893 branch July 19, 2025 00:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

emitter:client:csharp Issue for the C# client emitter: @typespec/http-client-csharp

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Update http-client-csharp generation to write ModelReaderWriterBuildableAttributes for all IPersistableModels consumed

5 participants