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

updated error controller to better handle HTTP400 #647

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

li-sui
Copy link

@li-sui li-sui commented Oct 26, 2021

added IllegalArgumentException in the list, Generally, any error due to inputs should be a 400, HTTP 500 is for logic error.
updated com.salesmanager.shop.store.controller.error.ShopErrorController. now it also handles errors from com.salesmanager.shop.controller.*

Closed #646
related to #638

@JustFlavi
Copy link

I'm afraid that handling any IllegalArgumentException as a HTTP 400 is a little bit too optimistic.
IllegalArgumentExceptions are not exclusively bound to invalid user input. As a general Java standard exception, they are widely used in several APIs.
So it might happen, that while we do some internal server processing (like database access or object conversion), we call an (internal) API incorrectly and run into an IllegalArgumentException. Such a scenario would clearly be an internal issue, where a HTTP 400 would be misleading.

@li-sui
Copy link
Author

li-sui commented Oct 26, 2021

I'm afraid that handling any IllegalArgumentException as a HTTP 400 is a little bit too optimistic. IllegalArgumentExceptions are not exclusively bound to invalid user input. As a general Java standard exception, they are widely used in several APIs. So it might happen, that while we do some internal server processing (like database access or object conversion), we call an (internal) API incorrectly and run into an IllegalArgumentException. Such a scenario would clearly be an internal issue, where a HTTP 400 would be misleading.

Yes, you are right. I will push a new commit to exclude IllegalArgumentException. I think it is still worth merging this as com.salesmanager.shop.store.controller.error.ShopErrorController has not been updated with 400 and errors under com.salesmanager.shop.controller.* has not been handled.

@sonarcloud
Copy link

sonarcloud bot commented Oct 26, 2021

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@JustFlavi
Copy link

Yes, you are right. I will push a new commit to exclude IllegalArgumentException. I think it is still worth merging this as com.salesmanager.shop.store.controller.error.ShopErrorController has not been updated with 400 and errors under com.salesmanager.shop.controller.* has not been handled.

Sounds good, thank you :)

@sonarcloud
Copy link

sonarcloud bot commented Dec 1, 2021

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

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.

java.lang.IllegalArgumentException: Locale part contains invalid characters
2 participants