[Bug]: Weird behavior for spread model with additional properties

tadelesh commented 1 month ago

Describe the bug

For this spec which is simplified from a service definition here, there is a usage of spread model with additional properties (though i think it should be banned). Also, the http body is composed from both the spread thing and azure core traits thing, then the weird thing happened, the http body parameter type will have a query parameter from the trait, which will not happen is the spread model has no additional properties. I'm wondering if it is a bug for body resolving. But I think a good solution is not allow to spread model with additional properties.

Reproduction

Compile with the playground or the spec provided.

Checklist

[x] Follow our Code of Conduct
[x] Check that there isn't already an issue that request the same bug to avoid creating a duplicate.
[x] Check that this is a concrete bug. For Q&A open a GitHub Discussion.
[x] The provided reproduction is a minimal reproducible example of the bug.

timotheeguerin commented 1 month ago

But I think a good solution is not allow to spread model with additional properties.

This is allowed by design, something we explicitly designed a few month ago, i am not sure I see the issue in teh simplified spec, everthing looks correct no?

tadelesh commented 1 month ago

But I think a good solution is not allow to spread model with additional properties.

This is allowed by design, something we explicitly designed a few month ago, i am not sure I see the issue in teh simplified spec, everthing looks correct no?

if i spread a model A with additional properties to model B, then the model B will have additional properties? what if the spread happen in operation parameter?

the problem is showed in the screenshot, the body type of http operation will have a property of api version query parameter. if remove the ...Record<unknown>, the query will not in the body type.

timotheeguerin commented 1 month ago

oh I see, yeah seems like we do have a special case that bypass the filterModelProperties part if it has an indexer in the body resolution.

So 2 things:

we probably have a fix to do in http library
We are not filtering properties for any nested types today. This is something the http typekits would I think improve on. This means if you are having an issue with this pattern you would also not exclude nested query params most likely
```
op getEmbeddings(@bodyIgnore nested: {@query foo: string}, name: string): void;
```
playground

microsoft / typespec