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

Wrong type ouput for getEffectiveModelType #2991

Open
tadelesh opened this issue Mar 6, 2024 · 11 comments
Open

Wrong type ouput for getEffectiveModelType #2991

tadelesh opened this issue Mar 6, 2024 · 11 comments
Assignees
Milestone

Comments

@tadelesh
Copy link
Member

tadelesh commented Mar 6, 2024

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: Azure/typespec-azure#311.

@tadelesh
Copy link
Member Author

tadelesh commented Mar 6, 2024

cc: @archerzz @MaryGao

@timotheeguerin
Copy link
Member

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

@tadelesh
Copy link
Member Author

tadelesh commented Mar 7, 2024

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.

@MaryGao
Copy link
Member

MaryGao commented Mar 7, 2024

@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
Copy link
Member

MaryGao commented Mar 7, 2024

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
}

@timotheeguerin
Copy link
Member

timotheeguerin commented Mar 7, 2024

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
Copy link
Member

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

@MaryGao
Copy link
Member

MaryGao commented Mar 8, 2024

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;
};

@timotheeguerin
Copy link
Member

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.

@markcowl markcowl added the bug Something isn't working label Mar 11, 2024
@markcowl markcowl added this to the [2024] April milestone Mar 11, 2024
@markcowl
Copy link
Contributor

I believe this will be addressed by this: #3012

@markcowl markcowl removed the bug Something isn't working label Mar 19, 2024
@markcowl
Copy link
Contributor

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 markcowl modified the milestones: [2024] April, [2024] June Apr 6, 2024
@markcowl markcowl modified the milestones: [2024] June, [2024] August Jun 17, 2024
@allenjzhang allenjzhang self-assigned this Jun 26, 2024
@markcowl markcowl modified the milestones: [2024] October, Backlog Oct 10, 2024
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

5 participants