Skip to content

Add content disposition setter to ResponseEntity #33343

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

Conversation

rfigueroa
Copy link
Contributor

see #33342

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Aug 8, 2024
@rfigueroa rfigueroa force-pushed the reponse-entity-content-disposition branch from 6c7aa99 to e6c76e7 Compare August 8, 2024 02:05
@snicoll snicoll added the in: web Issues in web modules (web, webmvc, webflux, websocket) label Aug 8, 2024
@rfigueroa rfigueroa force-pushed the reponse-entity-content-disposition branch from e6c76e7 to 9d6e720 Compare August 9, 2024 02:13
@@ -567,6 +577,12 @@ public BodyBuilder contentType(MediaType contentType) {
return this;
}

@Override
public BodyBuilder contentDisposition(ContentDisposition contentDisposition) {
this.headers.setContentDisposition(contentDisposition);

Choose a reason for hiding this comment

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

This looks well written, Just a small comment
Consider adding a null check for the contentDisposition parameter. If null values are not allowed, you could throw an IllegalArgumentException

@sdeleuze
Copy link
Contributor

I understand the ask and thanks for providing a PR, but it is hard to argue why we should add this one and many others, + there is also setContentDispositionFormData(), so I think I would recommend to just use headers(Consumer<HttpHeaders> headersConsumer) for that need. As a consequence, I am going to decline this PR.

@sdeleuze sdeleuze closed this Aug 20, 2024
@sdeleuze sdeleuze added status: declined A suggestion or change that we don't feel we should currently apply and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels Aug 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) status: declined A suggestion or change that we don't feel we should currently apply
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants