-
Notifications
You must be signed in to change notification settings - Fork 12
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 clientHandler for 501 response and httpParser #224
base: main
Are you sure you want to change the base?
Conversation
Qodana Community for JVMIt seems all right 👌 No new problems were found according to the checks applied 💡 Qodana analysis was run in the pull request mode: only the changed files were checked View the detailed Qodana reportTo be able to view the detailed Qodana report, you can either:
To get - name: 'Qodana Scan'
uses: JetBrains/qodana-action@v2023.3.1
with:
upload-result: true Contact Qodana teamContact us at qodana-support@jetbrains.com
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems to be missing method id HttpParser.java other than that looks fine to me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems to be missing a getRequestMethod that is being called at ClientHandler.java Line 64
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have added a getRequestMethod and it seems to be working now, thanks for the feedback!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Method looks to be called correctly.
|
||
private void handleIOException(IOException e) { | ||
if (e.getMessage().contains("500")) | ||
LOGGER.error(ResponseCode.HTTP_RESPONSE_STATUS_CODES); | ||
else | ||
LOGGER.error(e.getMessage()); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method is never used and can be removed?
String method = HttpParser.getRequestMethod(input); | ||
if (!HttpParser.isMethodSupported(method)) { | ||
return get501Response(); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this code is added in the getResponse method it will not be used for requests handled before this point, i.e. isCoffeeRequest. But maybe it is the check for coffeerequest and return of 418 response that should be moved into the getresponse method instead to be a part of this check.
FileInfo fileInfo = new FileInfo(Path.of("/"), description.getBytes()); | ||
return fileRequestHandler.writeResponse(fileInfo, description); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we really try to return root path that will be translated into index.html? Check Teapot.write418Response() for how to make a response in code without returning a file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem with returning a file as a response is that it can include sensitive information.
included HEAD in list of supported methods, added new method isMethodSupported.
updated the getResponse method, and added new method get501Response