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

issue 260: add parsing exception handling #266

Merged
merged 1 commit into from
Mar 1, 2023
Merged

issue 260: add parsing exception handling #266

merged 1 commit into from
Mar 1, 2023

Conversation

al-niessner
Copy link
Contributor

🗒️ Summary

Another simple one line change to add more exception handling back to HTTP error codes and messaging.

⚙️ Test Data and/or Report

$ curl --request GET 'http://localhost:8080//classes/collections?q=%22%22'
{"request":"//classes/collections","message":"The given search string '""' cannot be parsed."}

HOWEVER

$ curl --request GET 'http://localhost:8080//classes/collections?q=""'
<!doctype html><html lang="en"><head><title>HTTP Status 400 – Bad Request</title><style type="text/css">body {font-family:Tahoma,Arial,sans-serif;} h1, h2, h3, b {color:white;background-color:#525D76;} h1 {font-size:22px;} h2 {font-size:16px;} h3 {font-size:14px;} p {font-size:12px;} a {color:black;} .line {height:1px;background-color:#525D76;border:none;}</style></head><body><h1>HTTP Status 400 – Bad Request</h1></body></html>n

BECAUSE the error is prior to registry-api in the apache code as the log shows which is a completely different problem:

java.lang.IllegalArgumentException: Invalid character found in the request target [//classes/collections?q="" ]. The valid characters are defined in RFC 7230 and RFC 3986
	at org.apache.coyote.http11.Http11InputBuffer.parseRequestLine(Http11InputBuffer.java:494) ~[tomcat-embed-core-9.0.63.jar:9.0.63]
	at org.apache.coyote.http11.Http11Processor.service(Http11Processor.java:271) ~[tomcat-embed-core-9.0.63.jar:9.0.63]
	at org.apache.coyote.AbstractProcessorLight.process(AbstractProcessorLight.java:65) ~[tomcat-embed-core-9.0.63.jar:9.0.63]
	at org.apache.coyote.AbstractProtocol$ConnectionHandler.process(AbstractProtocol.java:890) ~[tomcat-embed-core-9.0.63.jar:9.0.63]
	at org.apache.tomcat.util.net.NioEndpoint$SocketProcessor.doRun(NioEndpoint.java:1743) ~[tomcat-embed-core-9.0.63.jar:9.0.63]
	at org.apache.tomcat.util.net.SocketProcessorBase.run(SocketProcessorBase.java:49) ~[tomcat-embed-core-9.0.63.jar:9.0.63]
	at org.apache.tomcat.util.threads.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1191) ~[tomcat-embed-core-9.0.63.jar:9.0.63]
	at org.apache.tomcat.util.threads.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:659) ~[tomcat-embed-core-9.0.63.jar:9.0.63]
	at org.apache.tomcat.util.threads.TaskThread$WrappingRunnable.run(TaskThread.java:61) ~[tomcat-embed-core-9.0.63.jar:9.0.63]
	at java.base/java.lang.Thread.run(Thread.java:829) ~[na:na]

♻️ Related Issues

closes #260
closes #255
closes #252
closes #206

@al-niessner al-niessner self-assigned this Feb 28, 2023
@al-niessner
Copy link
Contributor Author

@alexdunnjpl @jordanpadams @nutjob4life @tloubrieu-jpl

This is how you solve exception problems. The whole point of reordering the code was to make a single place where exceptions are handled and turned back into http errors. It should become distributed again and certainly not while UserParameters are being instantiated.

The better flow and one that currently exists is that you take what is as is and load it into a Java object for simpler processing. Once you have the object and the connection context from spring-something, then you have all the bits you need to start error processing. If you try to do it too early, then you have 10000 different blocks all doing exactly the same exception handling - okay, 4 or so blocks without using hyperbole. The way the code is currently structured, all endpoints have the same error handling because there is this one and only one block that handles exceptions. Not sure why that is so terrible or hard. The advantage is that no more cut-n-paste of code which is the quintessential anti-pattern.

So, I see lots of questioning on breaking this all apart and making it more confusing with even more nested exceptions when simply adding to JavaSwaggerTransmutter.process() all that needs to be done because no exception can be thrown outside of that method and that method does all of the exception handling. Miraculous on stop shop.

Ready for review.

@alexdunnjpl
Copy link
Contributor

alexdunnjpl commented Feb 28, 2023 •
edited
Loading

@al-niessner is there some context from in-person discussion I've missed, or is this all regarding the in-issue discussion in #252?

I'm having some trouble parsing your comment if it's the latter - is the assertion that

Add internal IllegalArgumentException to invalid start and limit setter calls

is a bad approach, or is there some relation to

Remove summaryOnly-related internal code and rework Singular/Plural API route behaviour to be more explicit

? My fault for opening a PR with two unrelated changes I suppose lol.

EDIT Oh nevermind - I see the engineering details in #260 which I presume is what you're talking about. Let me know if there's anything in #256 I need to rethink.

alexdunnjpl
alexdunnjpl reviewed Mar 1, 2023
View reviewed changes
service/src/main/java/gov/nasa/pds/api/registry/controller/SwaggerJavaTransmuter.java
@@ -114,6 +116,10 @@ protected ResponseEntity<Object> processs(EndpointHandler handler, URIParameters
} catch (NothingFoundException e) {
log.warn("Could not find any matching reference(s) in database.");
return new ResponseEntity<Object>(ErrorFactory.build(e, this.request), HttpStatus.NOT_FOUND);
} catch (NoViableAltException | ParseCancellationException e) {
log.warn("The given search string '" + parameters.getQuery() + "' cannot be parsed.");
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe log.info is more appropriate here, given that it's just bad user input?

alexdunnjpl
alexdunnjpl approved these changes Mar 1, 2023
View reviewed changes
Copy link
Contributor

@alexdunnjpl alexdunnjpl left a comment

Choose a reason for hiding this comment

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

LGTM

tloubrieu-jpl
tloubrieu-jpl approved these changes Mar 1, 2023
View reviewed changes
Copy link
Member

@tloubrieu-jpl tloubrieu-jpl left a comment

Choose a reason for hiding this comment

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

Yes, that is much simpler that what I proposed indeed, thanks Al

@tloubrieu-jpl tloubrieu-jpl merged commit 4cf874f into main Mar 1, 2023
@tloubrieu-jpl tloubrieu-jpl deleted the issue_260 branch March 1, 2023 01:30
@tloubrieu-jpl
Copy link
Member

@al-niessner @alexdunnjpl I m just realizing that fix does not help yet to fix the ticket #252 but we'll cross that bridge when we will be there.

@al-niessner
Copy link
Contributor Author

@alexdunnjpl @tloubrieu-jpl

Agreed. I opened #252 again with an explanation.

@tloubrieu-jpl tloubrieu-jpl mentioned this pull request Mar 3, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexdunnjpl alexdunnjpl alexdunnjpl approved these changes

@tloubrieu-jpl tloubrieu-jpl tloubrieu-jpl approved these changes

Assignees

@al-niessner al-niessner

Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@al-niessner @alexdunnjpl @tloubrieu-jpl