Skip to content

Conversation

Copy link
Contributor

Copilot AI commented Nov 6, 2025

Description

Properties marked @visibility(Lifecycle.Read) that are also required were incorrectly included in public initialization constructors. These should only appear in internal serialization constructors.

Changes Made

Modified SchemaObjectType.BuildInitializationConstructor() in src/AutoRest.CSharp/Common/Output/Models/Types/SchemaObjectType.cs to exclude required+readonly properties from initialization constructor parameters:

// Before
else if (IsStruct || property.InputModelProperty?.IsRequired == true)

// After  
else if ((IsStruct || property.InputModelProperty?.IsRequired == true) && property.InputModelProperty?.IsReadOnly != true)

Removed incorrectly committed temporary files from test/TestProjects/MgmtTypeSpec/tsp-output/@azure-tools/typespec-csharp/ that were added during development.

Example

Given this TypeSpec model:

model Foo {
  a: string;                        // required
  b?: string;                       // optional
  @visibility(Lifecycle.Read)
  c: string;                        // required + readonly
  @visibility(Lifecycle.Read)
  d?: string;                       // optional + readonly
}

Before: public Foo(string a, string c) {}
After: public Foo(string a) {}

Required+readonly properties remain in the internal serialization constructor for deserialization.

Validation

  • Verified with existing test case RoundTripModel in FirstTest-TypeSpec which has required+readonly properties that are correctly excluded from the public constructor
  • Regenerated test projects to confirm no additional code changes needed
  • All existing tests pass

Fixes #5423

Checklist

To ensure a quick review and merge, please ensure:

  • The PR has a understandable title and description explaining the why and what.
  • The PR is opened in draft if not ready for review yet.
    • If opened in draft, please allocate sufficient time (24 hours) after moving out of draft for review
  • The branch is recent enough to not have merge conflicts upon creation.

Ready to Land?

  • Build is completely green
    • Submissions with test failures require tracking issue and approval of a CODEOWNER
  • At least one +1 review by a CODEOWNER
  • All -1 reviews are confirmed resolved by the reviewer
    • Override/Marking reviews stale must be discussed with CODEOWNERS first
Original prompt

This section details on the original issue you should resolve

<issue_title>[Bug] Models in mgmt plane would generate "required+readonly" properties into its public ctor</issue_title>
<issue_description>if we have this model:

model Foo
{
  a: string;
  b?: string;
  @visibility(Lifecycle.Read)
  c: string;
  @visibility(Lifecycle.Read)
  d?: string;
}

the correct behavior is that, we generate this csharp class:

public partial class Foo
{
    public Foo(string a) {}
}

but in fact, this generates the following:

public partial class Foo
{
    public Foo(string a, string c) {}
}

which is incorrect.
</issue_description>

Comments on the Issue (you are @copilot in this section)


💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.

Copilot AI and others added 3 commits November 6, 2025 08:40
Exclude properties that are both required and readonly from the public
initialization constructor. Properties marked with @visibility(Lifecycle.Read)
should not be included in the constructor even if they are required.

Co-authored-by: ArcturusZhang <[email protected]>
Verify that required+readonly properties are correctly excluded from
initialization constructors in generated code.

Co-authored-by: ArcturusZhang <[email protected]>
The fix is validated by existing test cases in FirstTest-TypeSpec project.

Co-authored-by: ArcturusZhang <[email protected]>
Copilot AI changed the title [WIP] Fix incorrect generation of public constructor in model Fix required+readonly properties incorrectly included in initialization constructors Nov 6, 2025
Copilot AI requested a review from ArcturusZhang November 6, 2025 08:50
Copilot finished work on behalf of ArcturusZhang November 6, 2025 08:50
Copy link
Member

Choose a reason for hiding this comment

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

I think these files are incorrectly generated.
We should remove it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed in commit 66575ab.

Copy link
Member

Choose a reason for hiding this comment

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

same here, we should remove this file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed in commit 66575ab.

Copy link
Member

@ArcturusZhang ArcturusZhang left a comment

Choose a reason for hiding this comment

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

@copilot I think your change is fine, but please run ./eng/Generate.ps1 to regenerate all the test projects.

Remove test/TestProjects/MgmtTypeSpec/tsp-output/@azure-tools/typespec-csharp/Configuration.json and tspCodeModel.json as they were incorrectly added to the repository.

Co-authored-by: ArcturusZhang <[email protected]>
Copy link
Contributor Author

Copilot AI commented Nov 11, 2025

@copilot I think your change is fine, but please run ./eng/Generate.ps1 to regenerate all the test projects.

I regenerated test projects to verify. The generated code is already correct - no changes were needed. The fix ensures required+readonly properties are excluded from initialization constructors as expected. Verified with FirstTest-TypeSpec which has readOnlyRequiredRecordUnknown property.

Copilot AI requested a review from ArcturusZhang November 11, 2025 03:23
Copilot finished work on behalf of ArcturusZhang November 11, 2025 03:23
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.

[Bug] Models in mgmt plane would generate "required+readonly" properties into its public ctor

2 participants