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

ErrorAttributeOptions.excluding throws IllegalArgumentException if includes is empty #24230

Closed
wants to merge 1 commit into from

Conversation

wanderleisouza
Copy link
Contributor

No description provided.

@spencergibb
Copy link
Member

Did you target the wrong branch?

@snicoll snicoll changed the base branch from 2.3.x to master November 23, 2020 07:43
@snicoll
Copy link
Member

snicoll commented Nov 23, 2020

@wanderleisouza thanks for the PR.

Thanks @spencergibb, wrong branch indeed. I've fixed that and we can decide where to target this when we review/polish the PR.

@wanderleisouza
Copy link
Contributor Author

oh, thank you @snicoll and @spencergibb. I think it should be in 2.3.x, no? Let me know how to proceed (notice concourse-ci/status is broken due to boot:checkstyleTest). Should I submit a new PR?

@snicoll
Copy link
Member

snicoll commented Nov 23, 2020

I think it should be in 2.3.x, no?

As I've already indicated we can merge the PR in the appropriate branches as part of reviewing the PR.

notice concourse-ci/status is broken due to boot:checkstyleTest). Should I submit a new PR?

Ideally you should have built the project locally before submitting the PR, which would have revealed the build failure. We can also take care of that as part of reviewing.

Please don't open a new PR as it'd create unnecessary noise. FYI, pushing additional changes to your existing branch will update this PR.

@snicoll snicoll changed the title Fixes ErrorAttributeOptions.excluding throwing IllegalArgumentException when includes is empty. Fixes gh-24153 ErrorAttributeOptions.excluding throws IllegalArgumentException if includes is empty Nov 23, 2020
@snicoll snicoll added type: bug A general bug and removed status: waiting-for-triage An issue we've not yet triaged labels Nov 23, 2020
@snicoll snicoll added this to the 2.3.x milestone Nov 23, 2020
@snicoll snicoll self-assigned this Nov 23, 2020
@snicoll snicoll modified the milestones: 2.3.x, 2.3.7 Nov 23, 2020
snicoll pushed a commit that referenced this pull request Nov 23, 2020
@snicoll snicoll closed this in c383ab7 Nov 23, 2020
@snicoll
Copy link
Member

snicoll commented Nov 23, 2020

@wanderleisouza thank you for making your first contribution to Spring Boot. I've polished it and applied to both 2.3.x and master.

I've reworked the tests in particular as the assertions were not running properly. It can be tricky with assertJ but the assertion should run "outside" of the main thing to assert. So assertThat(errorAttributeOptions.getIncludes().isEmpty()); should have been assertThat(errorAttributeOptions.getIncludes()).isEmpty();. This assertion was wrong for the include use case so I've fixed that too.

humaolin pushed a commit to humaolin/spring-boot that referenced this pull request May 7, 2022
Also sync up to master and 5.1.x on refactorings in ContentDisposition
and ContentDispositionTests.

Closes spring-projectsgh-24230
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug A general bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants