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

[http] Add Parameter Decorator @cookie to Specify Cookie Parameters #4761

Merged
merged 28 commits into from
Nov 1, 2024

Conversation

su8ru
Copy link
Contributor

@su8ru su8ru commented Oct 16, 2024

close #4739

  • @typespec/http
    • add @cookie decorator to decorate parameters as cookie parameters
    • add response-cookie-not-supported diagnostics
      • to be emitted when response cookies are specified
    • add tests for cookie params
  • @typespec/http-server-javascript
    • throw UnimplementedError when cookie params came
  • @typespec/openapi3
    • support cookie param case
    • add tests for cookie params

response cookie design

in explicit body implicit body
diagnostics metadata-ignored response-cookie-not-supported
result included (unified with other metadata) NOT included (prevent breaking change for set-cookie)

based on:

@su8ru
Copy link
Contributor Author

su8ru commented Oct 16, 2024

I think that @cookie in the return type should be an implicit body (like @path and @query), but I haven't been able to find where this is implemented.
Added a check for isCookieParam() to isMetadata(), but properties with @cookie in the return type are ignored from the content of the response.

@timotheeguerin
Copy link
Member

aren't cookies allowed in response(server asking client to set the following cookies) we might need to design and decide exactly what we want in those cases though

@su8ru
Copy link
Contributor Author

su8ru commented Oct 16, 2024

aren't cookies allowed in response

I noticed that cookies can also be allowed or required in responses (e.g. in communication between servers).
This could cause problems if cookies cannot be set, so it might be better to be able to specify cookies in responses.

Copy link
Member

@timotheeguerin timotheeguerin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution this is awesome! I think there is a little design around the response cookies we'll want to sort out before we merge this but this looks great overall!

packages/http/src/decorators.ts Outdated Show resolved Hide resolved
packages/http/src/decorators.ts Show resolved Hide resolved
packages/http/src/decorators.ts Outdated Show resolved Hide resolved
@timotheeguerin
Copy link
Member

/azp run typespec - pr tools

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@su8ru
Copy link
Contributor Author

su8ru commented Oct 21, 2024

I ran pnpm change add and added the changelog... is this correct? This is my first time 😟

@su8ru

This comment was marked as resolved.

@su8ru
Copy link
Contributor Author

su8ru commented Oct 25, 2024

@timotheeguerin Hi, CI issues solved. Could you take a look? Thanks!

Copy link
Member

@timotheeguerin timotheeguerin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @su8ru the PR looks great, we just need to confirm thats exactly what we want to do before merging. I believe this should make it for next release(early november).

@timotheeguerin timotheeguerin added this pull request to the merge queue Oct 31, 2024
@timotheeguerin
Copy link
Member

Thanks again @su8ru for the contribution!!

@timotheeguerin timotheeguerin removed this pull request from the merge queue due to a manual request Oct 31, 2024
@su8ru
Copy link
Contributor Author

su8ru commented Nov 1, 2024

(why conflicted...? and does it resolved...?)

@timotheeguerin
Copy link
Member

(why conflicted...? and does it resolved...?)

yeah we have been reorganizating the docs, sorry I meant to go fix the conflict in your branch after I tried to merge it and forgot.

@timotheeguerin timotheeguerin added this pull request to the merge queue Nov 1, 2024
Merged via the queue into microsoft:main with commit 220769e Nov 1, 2024
18 checks passed
swatkatz pushed a commit to swatkatz/typespec that referenced this pull request Nov 5, 2024
…microsoft#4761)

close microsoft#4739 

- `@typespec/http`
  - add `@cookie` decorator to decorate parameters as cookie parameters
  - add `response-cookie-not-supported` diagnostics
    - to be emitted when response cookies are specified
  - add tests for cookie params
- `@typespec/http-server-javascript`
  - throw UnimplementedError when cookie params came
- `@typespec/openapi3`
  - support cookie param case
  - add tests for cookie params
  
### response cookie design

| in | explicit body | implicit body |
| - | - | - |
| diagnostics | `metadata-ignored` | `response-cookie-not-supported` |
| result | included (unified with other metadata) | NOT included
(prevent breaking change for `set-cookie`) |

---

based on:
- microsoft#4721

---------

Co-authored-by: Timothee Guerin <timothee.guerin@outlook.com>
Co-authored-by: Timothee Guerin <tiguerin@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature Request: Add parameter decorator @cookie to specify that is in: cookie
3 participants