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

fix, mgmt shadow properties from Resource/ProxyResource #2905

Merged
merged 3 commits into from
Aug 9, 2024

Conversation

XiaofeiCao
Copy link
Contributor

@XiaofeiCao XiaofeiCao commented Aug 9, 2024

The shadow property is in the middle of the inheritance hierarchy. In this case, "ResourceWithWritableName", since it has parent class ProxyResource.

Previously, we only check if property is shadowed in the leaf subclass.
Now, we check if property is shadowed in the inheritance hierarchy, and use the one that's closest to the leaf.

code: 9149f75

generated code diffs are reordering of the shadowed properties(a side effect of the fix logic). Now properties from subclass will come before the ones from parent class.

"ResourceWithWritableName": {
  "description": "ARM resource.",
  "type": "object",
  "properties": {
    "id": {
      "description": "Resource ID.",
      "type": "string",
      "readOnly": true
    },
    "name": {
      "description": "Resource name.",
      "type": "string"
    },
    "type": {
      "description": "Resource type.",
      "type": "string",
      "readOnly": true
    }
  },
  "x-ms-azure-resource": true
},
"ProxyResourceWithWritableName": {
  "description": "ARM proxy resource.",
  "type": "object",
  "allOf": [
    {
      "$ref": "#/definitions/ResourceWithWritableName"
    }
  ],
  "properties": {}
},
"FirewallRule": {
  "description": "A server firewall rule.",
  "type": "object",
  "allOf": [
    {
      "$ref": "#/definitions/ProxyResourceWithWritableName"
    }
  ],
  "properties": {
    "properties": {
      "$ref": "#/definitions/ServerFirewallRuleProperties",
      "description": "Resource properties.",
      "x-ms-client-flatten": true
    }
  }
}

@@ -170,4 +171,8 @@ public void testPolymophicSubClass() {
public void testSchemaCleanup() {
UserAssignedIdentity userAssignedIdentity = new UserAssignedIdentity();
}

public void testResourceWithWritableName() {
FirewallRuleInner firewallRuleInner = new FirewallRuleInner();
Copy link
Member

Choose a reason for hiding this comment

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

What would happen of the class, if we did not have this fix?

Copy link
Contributor Author

@XiaofeiCao XiaofeiCao Aug 9, 2024

Choose a reason for hiding this comment

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

diff after fix here: Azure/azure-sdk-for-java@806b4e0#diff-a6e36e0731c287838e46e86be1e06840866bec2766bbe888a0de3deac129ee80

  1. name will be improperly shadowed in FirewallRuleInner, also getter improperly generated for it.
    This is due to in current getFieldPropeties, we use name from ProxyResource, not the shadowed one in ResourceWithWritableName. Now we start search from subclass to parent, to choose the one closest to the leaf subclass.
    codegen fix here.
  2. setter won't be generated for the writable name.
    Cause is the same.
  3. fromJson would set name twice, once for name from ProxyResource, once for name from ResourceWithWritableName.
    codegen fix here

This is a special case for mgmt, as we have our own Resource base class. Swagger and TypeSpec doesn't allow property shadowing, from my test.
I didn't touch validate() here, since in this particular case, customer unlikely to define location or tags to override parent ones... If they were to, duplicate validation seems not a big issue..

@XiaofeiCao XiaofeiCao marked this pull request as ready for review August 9, 2024 06:50
@XiaofeiCao XiaofeiCao merged commit ab74362 into Azure:main Aug 9, 2024
5 checks passed
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.

3 participants