Skip to content
New issue

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

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

Already on GitHub? # to your account

Add new report to track properties that get overwritten during flattening #3013

Merged
merged 3 commits into from
Jan 12, 2024

Conversation

thomas11
Copy link
Contributor

@thomas11 thomas11 commented Jan 11, 2024

The Azure spec has an annotation x-ms-client-flatten which instructs the schema generator to flatten the property into the parent object. However, there are cases where this leads to overwriting a property, creating incorrect schema and SDKs, when inner and outer property have the same name. I discovered this problem accidentally. First step is to quantify, which this report is for.

@thomas11 thomas11 requested review from danielrbradley and a team January 11, 2024 22:34
Copy link

Does the PR have any schema changes?

Looking good! No breaking changes found.
No new resources/functions.

Copy link

codecov bot commented Jan 11, 2024

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (be38591) 60.37% compared to head (e535245) 60.41%.

Files Patch % Lines
provider/pkg/versioning/build_schema.go 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3013      +/-   ##
==========================================
+ Coverage   60.37%   60.41%   +0.03%     
==========================================
  Files          65       65              
  Lines       10897    10912      +15     
==========================================
+ Hits         6579     6592      +13     
- Misses       3775     3777       +2     
  Partials      543      543              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@danielrbradley danielrbradley left a comment

Choose a reason for hiding this comment

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

Core change looks good. A couple of minor nits inline.

Also, can we add a brief explanation of the report to reports/README.md?

@thomas11 thomas11 force-pushed the tkappler/report-flattening-conflicts branch from 134949e to e535245 Compare January 12, 2024 17:57
@thomas11 thomas11 enabled auto-merge (squash) January 12, 2024 17:58
@thomas11 thomas11 merged commit 507e107 into master Jan 12, 2024
15 checks passed
@thomas11 thomas11 deleted the tkappler/report-flattening-conflicts branch January 12, 2024 18:26
@mjeffryes mjeffryes added this to the 0.99 milestone Jan 26, 2024
thomas11 added a commit that referenced this pull request Jan 8, 2025
Skip the flattening of nested properties indicated by
[x-ms-client-flatten](https://github.com/Azure/autorest/blob/main/docs/extensions/readme.md#x-ms-client-flatten)
if it would lead to overwriting a property, creating incorrect schema
and SDKs. This case happens when inner and outer property have the same
name. For a report on all occurrences see #3013.

This change is breaking and could therefore only be applied to v3 of the
provider.

The PR is written to be reviewed commit by commit. It supersedes the
previous #3801. **I recommend hiding whitespace when reviewing since the
level of indentation of otherwise unaffected code changed.**

Resolves #3195
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants