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.

Sign up for GitHub

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

Jump to bottom

Fix BasicErrorController to use binding error cfg #21702

Closed
wants to merge 1 commit into from
Closed

Fix BasicErrorController to use binding error cfg #21702

wants to merge 1 commit into from

Conversation

Aurdo
Copy link
Contributor

@Aurdo Aurdo commented Jun 4, 2020

server.error.include-binding-errors had no effect on BasicErrorController.isIncludeBindingErrors(), due to wrong property being used

@Aurdo
Fix BasicErrorController to use binding error cfg
89c54f3 
`server.error.include-binding-errors` had no effect on BasicErrorController.isIncludeBindingErrors(), due to wrong property being used
@pivotal-issuemaster
Copy link

@Aurdo Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

@pivotal-issuemaster
Copy link

@Aurdo Thank you for signing the Contributor License Agreement!

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jun 4, 2020
@scottfrederick scottfrederick self-assigned this Jun 4, 2020
@scottfrederick scottfrederick added type: bug A general bug and removed status: waiting-for-triage An issue we've not yet triaged labels Jun 4, 2020
@scottfrederick
Copy link
Contributor

Good catch @Aurdo, thanks. This was my copy-and-paste mistake that happened when the control of the message and errors fields were split from one property into two.

The reason this wasn't caught earlier is that the tests to verify the behavior of BasicErrorController always test the server.error.include-message and server.error.include-binding-errors fields together. Those tests should be split up so that include-message and include-binding-errors are tested separately.

If you'd like, you can also modify BasicErrorControllerIntegrationTests to split up those tests and add that change in another commit to this PR. If you'd prefer not to do that, I can modify the tests when the PR is merged.

@scottfrederick scottfrederick added this to the 2.3.1 milestone Jun 4, 2020
@Aurdo
Copy link
Contributor Author

Aurdo commented Jun 4, 2020

Good catch @Aurdo, thanks. This was my copy-and-paste mistake that happened when the control of the message and errors fields were split from one property into two.

The reason this wasn't caught earlier is that the tests to verify the behavior of BasicErrorController always test the server.error.include-message and server.error.include-binding-errors fields together. Those tests should be split up so that include-message and include-binding-errors are tested separately.

If you'd like, you can also modify BasicErrorControllerIntegrationTests to split up those tests and add that change in another commit to this PR. If you'd prefer not to do that, I can modify the tests when the PR is merged.

@scottfrederick whatever works for you is fine with me. Just not sure how long would it take to clone and import project locally

@scottfrederick
Copy link
Contributor

@Aurdo If you haven't cloned, imported, and built the project already don't worry about it. I'll fix up the tests.

scottfrederick pushed a commit that referenced this pull request Jun 8, 2020
@Aurdo @scottfrederick
Fix BasicErrorController include parameter parsing
e2e52a1 
This commit fixes an error in BasicErrorController where the wrong
property was referenced for binding error inclusion.

See gh-21702
scottfrederick added a commit that referenced this pull request Jun 8, 2020
@scottfrederick
Improve tests for BasicErrorController
73aff08 
This commit improves the tests for BasicErrorController by decoupling
coverage for the include-message and include-binding-errors
parameters to ensure the options operate properly independent of
each other.

See gh-21702
@scottfrederick scottfrederick mentioned this pull request Jun 8, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@scottfrederick scottfrederick

Labels
type: bug A general bug
Projects
None yet
Milestone
2.3.1
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@Aurdo @pivotal-issuemaster @scottfrederick @spring-projects-issues