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

Diagnostic for additional unannotated properties that are ignored due to @body #2075

Closed
nguerrera opened this issue Jun 15, 2023 · 2 comments · Fixed by #2945
Closed

Diagnostic for additional unannotated properties that are ignored due to @body #2075

nguerrera opened this issue Jun 15, 2023 · 2 comments · Fixed by #2945
Assignees
Labels
design:needed A design request has been raised that needs a proposal
Milestone

Comments

@nguerrera
Copy link
Contributor

nguerrera commented Jun 15, 2023

Playground Link

Notice how the property a disappears from the output entirely. I believe @body has always had this effect on responses and it can be observed on parameter with nested @body too after #2029. See discussion there.

We have long had a check against having unannotated top-level parameters and @body, but perhaps it should be generalized to find any unannotated property that gets subsumed by a sibling or sibling-descendant @body.

@nguerrera nguerrera changed the title Diagnostic for unannotated properties in response or nested in parameter that are ignored to @body Diagnostic for additional unannotated properties that are ignored due to @body Jun 15, 2023
@markcowl
Copy link
Contributor

markcowl commented Jun 21, 2023

single warning diagnostic for both request and response bodies, emitted from http library

we will need to discuss this with the DPG team before rolling it out

@markcowl markcowl added the design:needed A design request has been raised that needs a proposal label Jun 21, 2023
@markcowl markcowl added this to the [2023] August milestone Jun 21, 2023
@markcowl markcowl modified the milestones: [2023] August, Backlog Aug 22, 2023
@timotheeguerin timotheeguerin self-assigned this Feb 28, 2024
@timotheeguerin timotheeguerin modified the milestones: Backlog, [2024] April Feb 28, 2024
@timotheeguerin
Copy link
Member

timotheeguerin commented Feb 28, 2024

gets fixed as part of #2945

@markcowl markcowl modified the milestones: [2024] April, Backlog Mar 11, 2024
timotheeguerin added a commit that referenced this issue Apr 17, 2024
fix [#2868](#2868)

- Change meaning of `@body` to mean this is the body and nothing will be
excluded(show warning on containing metadata)
- Add new `@bodyRoot` which has the same purpose as the old `@body`(
Allows changing where the body is resolved from but allows mixing with
metadata.
- Add new `@bodyIgnore` which allows a property to be ignored from the
body
- Provide a new body resolution common function for request and response

also fix #2075
## Examples from original issue

1. [Inconsitency between request and
response](https://cadlplayground.z22.web.core.windows.net/prs/2945/?c=aW1wb3J0ICJAdHlwZXNwZWMvaHR0cCI7Cgp1c2luZyBUeXBlU3BlYy5IdHRwOwoKLyoqCiAqIEJhc2VsaW5lIDE6IEhhdsQqYEBoZWFkZXJgIHByb3BlcnR5IG9ubHkgaW4gcmVxdWVzdMYLc3BvbnNlxAl1bMUTbm8gYm9kecRXUscpxBA6IHZvaWTGFnPFM80WLwpAcm91dGUoIi9i5wCNIikKQGdldCBvcCDIEygg5wCWIGN1c3RvbUjFDTogc3RyaW5nKToge90hIH0g6gD0Q2Fz5QDwQWRkxBtgQHZpc2liaWxpdHlgIHRv9AEBYmVoYXZlIGRpZmZlcmVudGx5IGZvcukBEGFuZOkBES7yAQB7ff8A%2FuUA%2FmNhc2UxIikKb3AgxQso6wCYKCJub%2BQBGOkA5XjuAPvfKsUqIH3vAQMy%2BgEDbm9uIGFubm90YXRlZOoBB%2BoB7GVtcOQBF%2FUB7%2FQA78UU7wDtMuoA7TL1AO3%2FAOXMIuUA3Q%3D%3D&e=%40typespec%2Fopenapi3&options=%7B%7D)
2. [Inconsitency between different
ways](https://cadlplayground.z22.web.core.windows.net/prs/2945/?c=aW1wb3J0ICJAdHlwZXNwZWMvaHR0cCI7CtIZdmVyc2lvbmluZyI7Cgp1c2luZyBUeXBlU3BlYy5IdHRwO9AVVskyOwoKQHNlcnZpY2UKQMdJZWQoxyFzKQpuYW1lc3BhY2UgTXlTxik7CmVudW0gyCQgewogIHYxLMQGMiwKfQoKQHJvdXRlKCJ0MSIpIG9wIHQxKCk6IHZvaWQ7IC8vIDIwNMUNyigyxygyxCh7fccmMM8mM8cmM8UmQGhlYWRlciBmb286IHN0cmluZ9g5NMc5NMY5dmlzaWJpbGl0eSgiZ2F0ZXdheSIp30jFSDXHSDXcSP8AmMxQNsdQNsVQ1THGFiJhYmMifco5N8c5N8U5QGFkZOsBtC52MvMAzsR6IGluIHYxykc4x0c430fFRyzpANpvdGhlcthe&e=%40typespec%2Fopenapi3&options=%7B%7D)

## Breaking changes 
Azure spec PR showing scale of breaking changes
Azure/azure-rest-api-specs#27897
### `@body` means this is the body
This change makes it that using `@body` will mean exactly this is the
body and everything underneath will be included, including metadata
properties. It will log a warning explaining that.

```tsp
op a1(): {@Body _: {@Header foo: string, other: string} };
                ^ warning header in a body, it will not be included as a header.
```

Solution use `@bodyRoot` as the goal is only to change where to resolve
the body from.

```tsp
op a1(): {@bodyRoot _: {@Header foo: string, other: string} };
```




### Empty model after removing metadata and visibility property result
in void always
This means the following case have changed from returning `{}` to no
body

```tsp
op b1(): {};
op b2(): {@visibility("none") prop: string};
op b3(): {@added(Versions.v2) prop: string};
```

Workaround: Use explicit `@body`

```tsp
op b1(): {@Body _: {}};
op b2(): {@Body _: {@visibility("none") prop: string}};
op b3(): {@Body _: {@added(Versions.v2) prop: string}};
```

### Status code always 200 except if response is explicitly `void`

```tsp
op c1(): {@Header foo: string}; // status code 200 (used to be 204)
```

Solution: Add explicit `@statusCode`
```tsp
op c1(): {@Header foo: string, @statuscode _: 204};
op c1(): {@Header foo: string, ...NoContent}; // or spread common model
```


### Properties are not automatically omitted if everything was removed
from metadata or visibility

```tsp
op d1(): {headers: {@Header foo: string}}; // body will be {headers: {}}
```

Solution: use `@bodyIgnore`

```tsp
op d1(): {@bodyIgnore headers: {@Header foo: string}}; // body will be {headers: {}}
```

---------

Co-authored-by: Mark Cowlishaw <markcowl@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
design:needed A design request has been raised that needs a proposal
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants