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

Discrepancy of generation from tsp and swagger for resource name property #378

Closed
tadelesh opened this issue Mar 7, 2024 · 7 comments
Closed
Assignees
Milestone

Comments

@tadelesh
Copy link
Member

tadelesh commented Mar 7, 2024

Current tsp, we define a resource name property in the final resource model, not inherit it from the base resource model. This will cause the name property in the generated code for JS and Python to be breaking.

For example, a FooResource defined as:

@doc("Foo resource")
model FooResource is TrackedResource<FooResourceProperties> {
@doc("Foo name")
@key("fooName")
@segment("foos")
@path
name: string;
}

It is sourced from TrackedResource template:
@doc("Concrete tracked resource types can be created by aliasing this type using a specific property type.")
@armResourceInternal(Properties)
@includeInapplicableMetadataInPayload(false)
model TrackedResource<Properties extends {}> extends TrackedResourceBase {
@doc("The resource-specific properties for this resource.")
@visibility("read", "create")
@extension("x-ms-client-flatten", true)
properties?: Properties;
}

Which is derived from TrackedResourceBase model:
model TrackedResourceBase extends ArmResource {
@doc("The geo-location where the resource lives")
@visibility("read", "create")
location: string;
...ArmTagsProperty;
}

Which is derived from ArmResource model:
@doc("Common properties for all Azure Resource Manager resources.")
model ArmResource extends ArmResourceBase {
@doc("Fully qualified resource ID for the resource. Ex - /subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/providers/{resourceProviderNamespace}/{resourceType}/{resourceName}")
@visibility("read")
id: string;
// The name property must be included by the resource type author!
@doc("The type of the resource. E.g. \"Microsoft.Compute/virtualMachines\" or \"Microsoft.Storage/storageAccounts\"")
@visibility("read")
type: string;
@doc("Azure Resource Manager metadata containing createdBy and modifiedBy information.")
@visibility("read")
systemData?: SystemData;
}

You could see name property is defined in FooResource, not in ArmResource.

But with swagger common-types, the name is defined in the root Resource model:
https://github.com/Azure/azure-rest-api-specs/blob/84849e5293de3a8e01cbdde13b2d7086d3fb34d6/specification/common-types/resource-management/v5/types.json#L9-L37

@tadelesh
Copy link
Member Author

One real issue from service team: Azure/azure-rest-api-specs#27565 (comment)

@markcowl
Copy link
Member

markcowl commented Mar 11, 2024

@tadelesh I don't think the referenced spec is actually blocked by this, because it cannot use the resource templates in any case, due to the resource type inheriance form TrackedResource or ProxyResource. The only way to represent this API without breaks is to use a custom-defined resource type, as shown in this playground

Note that this will be a problem for new specs that were originally introduced in TypeSpec, or any specs that use resource types that directly inherit from ProxyResource of TrackedResource (a small list). This problem should be addressed by this issue: https://github.com/Azure/typespec-azure-pr/issues/3873

@tadelesh
Copy link
Member Author

@tadelesh I don't think the referenced spec is actually blocked by this, because it cannot use the resource templates in any case, due to the resource type inheriance form TrackedResource or ProxyResource. The only way to represent this API without breaks is to use a custom-defined resource type, as shown in this playground

Note that this will be a problem for new specs that were originally introduced in TypeSpec, or any specs that use resource types that directly inherit from ProxyResource of TrackedResource (a small list). This problem should be addressed by this issue: Azure/typespec-azure-pr#3873

Then how should emitter deal with the ArmResource model in different namespace? In swagger, we will ask service team to choose either use common types or use self-defined base resource models to prevent duplicate schema. I'm afraid it will happen again in TypeSpec?

@allenjzhang
Copy link
Member

@allenjzhang allenjzhang self-assigned this Apr 9, 2024
@markcowl markcowl added this to the [2024] May milestone Apr 23, 2024
@allenjzhang
Copy link
Member

#661 Merged

@tadelesh
Copy link
Member Author

cc: @pshao25 pay attention to this change. convertor need to adopt the change.

@pshao25
Copy link
Member

pshao25 commented May 6, 2024

cc: @pshao25 pay attention to this change. convertor need to adopt the change.

Will do after it releases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants