Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes intermittent issues in model renaming. #3187

Merged
merged 2 commits into from
Aug 23, 2023

Conversation

andrueastman
Copy link
Member

This PR fixes microsoftgraph/msgraph-sdk-dotnet#2084

After changes made in #3107 involved the CodeType sharing the name with the TypeDefinition property to limit unnecessary string copies. This however led to indeterministic renaming of model types as renaming a TypeDefinition instance through the parameter type or discriminator mapping would lead to indeterministic results depending on which was processed first and sometimes fail to check for conflicts after renaming.
An example include renaming microsoft.graph.directory to microsoft.graph.directoryObject (which already exists rather than resolving the renaming conflict) when the TypeDefinition when encountered through the discriminator mapping of the base rather than as child element of the microsoft.graph namespace.

This PR therefore eliminates other points of TypeDefinition/CodeType renaming(as multiple string copies are no longer held) by dropping functions that looked up the TypeDefinition/CodeType name indirectly through the

  • Discriminator mappings
  • Code using declarations
  • Parameter types

@andrueastman andrueastman marked this pull request as ready for review August 23, 2023 09:17
@andrueastman andrueastman requested a review from a team as a code owner August 23, 2023 09:17
@andrueastman andrueastman enabled auto-merge August 23, 2023 09:17
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

90.0% 90.0% Coverage
0.0% 0.0% Duplication

@andrueastman andrueastman self-assigned this Aug 23, 2023
Copy link
Member

@baywet baywet left a comment

Choose a reason for hiding this comment

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

Thanks for fixing that. Even less work to do since we're already renaming the definitions (class/interface/enum)

@andrueastman andrueastman merged commit e8a26a9 into main Aug 23, 2023
@andrueastman andrueastman deleted the andrueastman/fixRenaming branch August 23, 2023 12:17
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.

Latest release returns incorrect models for groups
2 participants