-
-
Notifications
You must be signed in to change notification settings - Fork 305
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
[IMP] fastapi: detail to have ValueError exception message #472
base: 17.0
Are you sure you want to change the base?
[IMP] fastapi: detail to have ValueError exception message #472
Conversation
Hi @lmignon, |
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.
IMO we must be very careful about the provided informations in case of error. We are conscious of the fact that no information is returned in the event of an error that is too generic, in order to prevent it from being used in the development of API attack scenarios by hackers. So I'm not in favor of this change, but let's see what others think.
Realised that handling only |
26de529
to
f4fbf85
Compare
Why not but errors and stacktrace are available into the log file. |
fastapi/error_handlers.py
Outdated
if os.getenv("FASTAPI_DEBUG") == "1": | ||
details = repr(exc) | ||
else: | ||
details = "Internal Server Error" |
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.
if os.getenv("FASTAPI_DEBUG") == "1": | |
details = repr(exc) | |
else: | |
details = "Internal Server Error" | |
details = "Internal Server Error" | |
if config.get_misc("fastapi", "dev_mode"): | |
traceback = "".join(traceback.format_exception(*sys.exc_info())) | |
details = f"{details}\n{traceback}" |
you must also import the right modules...
import sys
import traceback
from odoo.tools.config import config
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.
Thanks for the suggestion. Applied with slight changes.
f4fbf85
to
db16b76
Compare
db16b76
to
d5dd7ac
Compare
@@ -5,7 +5,7 @@ | |||
"name": "Odoo FastAPI", | |||
"summary": """ | |||
Odoo FastAPI endpoint""", | |||
"version": "17.0.3.0.1", | |||
"version": "17.0.4.0.0", |
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 version will be changed at merge time. Can you remove this change plz.
@@ -21,6 +24,9 @@ def convert_exception_to_status_body(exc: Exception) -> tuple[int, dict]: | |||
body = {} | |||
status_code = status.HTTP_500_INTERNAL_SERVER_ERROR | |||
details = "Internal Server Error" | |||
if config.get_misc("fastapi", "dev_mode"): |
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 config should be added into the documentation in a readme/CONFIGURE.rst
(ex https://github.com/OCA/rest-framework/blob/16.0/base_rest/readme/CONFIGURE.rst)
Previously, detail contained only "Internal Server Error" in case of ValueError exception.
Now, it contains an actual exception message