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

Make handling of empty object consistent in request/response body #2868

Closed
3 of 7 tasks
timotheeguerin opened this issue Jan 31, 2024 · 1 comment · Fixed by #2945
Closed
3 of 7 tasks

Make handling of empty object consistent in request/response body #2868

timotheeguerin opened this issue Jan 31, 2024 · 1 comment · Fixed by #2945
Assignees
Labels
breaking-change A change that might cause specs or code to break triaged:core
Milestone

Comments

@timotheeguerin
Copy link
Member

timotheeguerin commented Jan 31, 2024

Design #2513

  1. Introduce new decorators and repurpose some
  • @body => @bodyRoot in its current meaning
  • @body => now means actual body. Nothing inside of this can be metadata(header, query, etc.)
  • @bodyIgnore => provide a way to ignore a property from the body
  1. If the resulting body is {} after removing all ignored properties then the body is void
  2. Status code is always 200 unless the body is explicitly void then it is 204 (Or do we want to always make it 200)

Tasks

@markcowl
Copy link
Contributor

markcowl commented Feb 5, 2024

est: 13

@timotheeguerin timotheeguerin self-assigned this Feb 20, 2024
@markcowl markcowl modified the milestones: [2024] March, [2024] April Mar 25, 2024
@markcowl markcowl modified the milestones: [2024] April, [2024] May Apr 6, 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
breaking-change A change that might cause specs or code to break triaged:core
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants