Azure / typespec-azure

About TypeSpec Azure Libraries
https://azure.github.io/typespec-azure/
MIT License
15 stars 41 forks source link

Issues of getArmResources #1282

Open pshao25 opened 3 months ago

pshao25 commented 3 months ago

I'm looking at getArmResources. There are several problems:

1. Incorrect assumption of "a model can only belong to one resource" From the implementation of @armResourceRead and others, we could see that the model passed in to the template of ArmResourceRead<Resource> will be taken as a resource model, and all the operations using the model this template way will be taken as the corresponding operations to this resource model. In real services, we have many cases that a model can belong to several resources. For example, this client and this client share the same resource model Site.

Actual: the key of resource is model. Expected: I think the key of resource should be request path, or at least resource type.

2. A resource can only have one operation for each lifecycle operation For example,

model Parameter {
  @path
  @segment("slots")
  slot: string;
}

@armResourceOperations
interface WebApps {
  get is ArmResourceRead<Site>;
  getSlot is ArmResourceRead<Site, Parameters = Parameter>;
}

The second getSlot wins. But I assume if we fix the first problem, this is not a problem any more.

3. Whether an arbitrary model can be the resource model of a resource? For the current implementation, as long as the model is used in a template, it could be the resource model of a resource. However, in .net generator implementation, there needs some conditions to make a model a resource model. For example, the request path should have even number of segments after the providers segment, there should be a get operation, etc. I believe it follows some standard. This leads to the below scenario:

interface PrivateLinks {
  listByMongoCluster is ArmResourceListByParent<PrivateLinkResource>;
}

For this case, only list exists and that's it.

Actual: the output contains one resource with PrivateLinkResource as resource model, and a list operation belongs to it. Expected: we should come to an agreement whether there should be a resource.

4. Resource Type This is how we calculate resource type. Is that correct? I think it should be Microsoft.Contoso/Databases for the case in comment.

lirenhe commented 3 months ago

4. Resource Type This is how we calculate resource type. Is that correct? I think it should be Microsoft.Contoso/Databases for the case in comment.

We use the below logic to calculate resource type when building Swagger KPI and I believe this matches the ARM definition of resource type.

https://devdiv.visualstudio.com/DevDiv/_git/openapi-swaggerkpi?path=/azure-functions/DurableScanSwagger/scanner.ts&version=GBmain&line=53&lineEnd=54&lineStartColumn=1&lineEndColumn=1&lineStyle=plain&_a=contents

m-nash commented 3 months ago

@pshao25 it looks like 1 and 2 are the same thing? Do you have a concrete example of when we would want multiple life cycle operations of the same type for the same resource? I assume by life cycle operation you mean CRUD only since you can have multiple POST operations?

For 3 @markcowl typically if you cannot act on something by an id / name we don't consider it a resource. If something only can be listed should we be treating it as a resource here?

For 4 @pshao25 can you run some unit tests to see if we get the right resource type out given different urls? It does appear there might be an issue but its hard to say for sure exactly which scenario comes out incorrect.

pshao25 commented 3 months ago

@m-nash

it looks like 1 and 2 are the same thing? Do you have a concrete example of when we would want multiple life cycle operations of the same type for the same resource?

Yes, I pointed out as long as we correctly handled 1, 2 is on longer a problem. And as long as TypeSpec correctly define resource, I agree only one operation is of same type.

I assume by life cycle operation you mean CRUD only since you can have multiple POST operations?

Correct. TypeSpec uses this terminology, so I use it too.

can you run some unit tests to see if we get the right resource type out given different urls? It does appear there might be an issue but its hard to say for sure exactly which scenario comes out incorrect.

I noticed this problem right because it gives me wrong output. For example. below case gives me "/subscriptions/{subscriptionId}/providers/MgmtTypeSpec/privateLinkResources", which I think should be MgmtTypeSpec/privateLinkResources.

interface PrivateLinks {
  listByMongoCluster is ArmResourceListByParent<PrivateLinkResource>;
  get is ArmResourceRead<PrivateLinkResource>;
}
markcowl commented 3 months ago

(1) Here Resource means ArmResource. It is not possible, AFAIK for an ARM resource to be registered at different paths: different registrations == different resources, and the way resources / resource-types are defined is in the resource model itself. There may be common schema between resources at different paths, which is different than these being the same resources. I think the issue here is how to represent those cases where two resources share the same envelope and rp-specific properties.

(2) I think this goes back to point (1), as Michael indicated. You should not use ArmResourceRead for non-resource models. Either these are two resources that share a schema, or one is a resource and one is not.

(3) We should use the arm definition: anything registered as a resource is a resource. Anything not registered as a resource is not. It is possible to have non-resource operations outside the RPC, for example, 'get' on a non-resource path, and provider actions. Currently, these would need to be modeled as low-level operations. We should determine if any additional low-level building blocks would help.

(4) No, this is not the same as resource Type. We likely need to include resource type in what is here (it would just concatenate the segment values from the parent resource chain and prepend the provider name. Here is a sample of how this is done in the service generator, this should just be returned as part of the resource: https://github.com/Azure/typespec-azure-pr/blob/providerhub/packages/typespec-providerhub-controller/src/generate.ts#L206-L215

I think there is also a gap in how we would use resource types to describe ARM built-in resources for the Azure Resource Manager RP: some parts of this RP are conventional resource that could use these patterns, but subscription, resourceGroup, and other built-ins would need to get special treatment