typespec icon indicating copy to clipboard operation
typespec copied to clipboard

Wrong type ouput for `getEffectiveModelType`

Open tadelesh opened this issue 1 year ago • 11 comments

For the following TypeSpec, when we call getEffectiveModelType for type Product, we will get a type named ProxyResource with the same properties of the original type Product. I suppose it should return the original type of Product.

model Product is ProxyResource<ProductProperties> {
  @doc("Name of product.")
  @pattern("^[\\w][\\w\\s]{1,48}[\\w]$|^\\.default$|^\\.unassigned$")
  @key("productName")
  @path
  @segment("products")
  name: string;
}

This output may impact many things, include the output of getLroMetadata for Azure: https://github.com/Azure/typespec-azure/issues/311.

tadelesh avatar Mar 06 '24 03:03 tadelesh

cc: @archerzz @marygao

tadelesh avatar Mar 06 '24 03:03 tadelesh

What options to you pass to getEffectiveModelType? I fixed a bug in getEffectiveModelType when dealling with read visibility here https://github.com/microsoft/typespec/pull/2945 maybe thats the same problem

timotheeguerin avatar Mar 06 '24 17:03 timotheeguerin

What options to you pass to getEffectiveModelType? I fixed a bug in getEffectiveModelType when dealling with read visibility here #2945 maybe thats the same problem

isMetadata. I don't think the issue is from visibility. It should be some reason from the candidate finding logic of getEffectiveModelType.

tadelesh avatar Mar 07 '24 05:03 tadelesh

@timotheeguerin JS has two cases failed and our current workaround is to get rid of getEffectiveModelType if the model has a name.

Case 1: A is TemplateModel

The getEffectiveModelType(a) would return Template name as model not A, this introduced a lot of duplicated names because they would always return the template name(see js issue).

model B<Parameter> {
  prop1: string;
  prop2: Parameter;
}
model A is B<string> {
  @query
  name: string;
};

Case 2: A extends B

The getEffectiveModelType(a) would return B and also this caused duplicated names and also caused hierarchy information lost.

model B {
  prop1: string;
  prop2: string;
}
model A extends B {
  @query
  name: string;
};

image

MaryGao avatar Mar 07 '24 14:03 MaryGao

JS has two cases failed and our current workaround is to get rid of getEffectiveModelType if the model has a name.

Case 1: A is TemplateModel

...

One more case is for spread usage, i just guess maybe all of them are the same root cause.

model ResponseError {
  @header
  @doc("Error code.")
  "x-ms-error-code"?: string,
  ...ErrorResponse
}

MaryGao avatar Mar 07 '24 14:03 MaryGao

So I think its maybe just how you are using it that might be wrong. Are you using the same call in the request and response with isMetadata as the value?

The problem I think if you look at all the repros is the model that doesn't get returned contains only a single readonly metadata property(It is in the response body but it is a metadata property)

What I think you might want to do instead is call with isApplicableMetadata and pass the correct visibility.

If that's not the case are those cases reproducing with openapi3 or autorest emitter? can you share a whole repro in there? (For example @MaryGao your example works fine for me at picking up ResponseError playground)

timotheeguerin avatar Mar 07 '24 15:03 timotheeguerin

Ok actually if I update the previous playground with an intersection then it is getting into that problem. However I have this exact issue fixed in the body consistency pr Playground from that pr as example

timotheeguerin avatar Mar 07 '24 15:03 timotheeguerin

So I think its maybe just how you are using it that might be wrong. Are you using the same call in the request and response with isMetadata as the value?

Yeah in our code we use isMetadata and i notice that isApplicableMetadata would require a visibility param, i may not understand why visibility could fix that, any simple explaination?

Could you explian with me what is the usage assumption for getEffectiveModelType?

Imagine following A and C types we would use filter to filter out name property(and then they have the exact same properites like B string) so the effective model for them are both B<string>, if then the emitter should not call this function for named model because the name is not expected.

model B<Parameter> {
  prop1: string;
  prop2: Parameter;
}
model A is B<string> {
  @query
  name: string;
};
model C is B<string> {
  @query
  name: string;
};

MaryGao avatar Mar 08 '24 03:03 MaryGao

Yeah sorry I actually got mistaken from my previous playground it is not working there too.

Yeah I do feel like getEffectiveModel type in those cases should return A and C, there might be a bug in the logic. I might need to dig more into it if it was done on purpose.

For why you need visibility, it depends on the usage but for the use case of openapi we need to create a definition for the body exactly(so extract the metadata property out) but we need to get a name (if possible) for that body. The body might not be the same in the request or response depending on the visibility so it could alter what is the effective model type.

timotheeguerin avatar Mar 08 '24 03:03 timotheeguerin

I believe this will be addressed by this: https://github.com/microsoft/typespec/issues/3012

markcowl avatar Mar 19 '24 20:03 markcowl

In ARM operation templates (as in rest operation templates), the logical request and response type for a model is given by the @body decorator. The linked issue will provide a single field that any emitter can use to map to a logical response type.

meanwhile MPG emitters should use isBody to determine if there is a body type.

markcowl avatar Mar 19 '24 20:03 markcowl