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

[IMP] fastapi: detail to have ValueError exception message #472

Open
wants to merge 1 commit into
base: 17.0
Choose a base branch
from

Conversation

anikeenko-viktor
Copy link

Previously, detail contained only "Internal Server Error" in case of ValueError exception.
Now, it contains an actual exception message

@OCA-git-bot
Copy link
Contributor

Hi @lmignon,
some modules you are maintaining are being modified, check this out!

Copy link
Contributor

@lmignon lmignon left a 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.

@anikeenko-viktor
Copy link
Author

Realised that handling only ValueError is not enough in my case. I think we should have ability to see detailed error info somehow. Probably need to add some kind of DEBUG flag and show detailed error info only if it's provided (in staging, local servers for example)

@anikeenko-viktor anikeenko-viktor marked this pull request as draft November 12, 2024 09:17
@anikeenko-viktor anikeenko-viktor force-pushed the 17.0-fastapi-value-error-handling branch 3 times, most recently from 26de529 to f4fbf85 Compare November 12, 2024 09:28
@lmignon
Copy link
Contributor

lmignon commented Nov 12, 2024

Realised that handling only ValueError is not enough in my case. I think we should have ability to see detailed error info somehow. Probably need to add some kind of DEBUG flag and show detailed error info only if it's provided (in staging, local servers for example)

Why not but errors and stacktrace are available into the log file.

Comment on lines 25 to 28
if os.getenv("FASTAPI_DEBUG") == "1":
details = repr(exc)
else:
details = "Internal Server Error"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
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

Copy link
Author

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.

@anikeenko-viktor anikeenko-viktor force-pushed the 17.0-fastapi-value-error-handling branch from f4fbf85 to db16b76 Compare November 12, 2024 10:19
@anikeenko-viktor anikeenko-viktor force-pushed the 17.0-fastapi-value-error-handling branch from db16b76 to d5dd7ac Compare November 12, 2024 10:21
@anikeenko-viktor anikeenko-viktor marked this pull request as ready for review November 12, 2024 10:21
@@ -5,7 +5,7 @@
"name": "Odoo FastAPI",
"summary": """
Odoo FastAPI endpoint""",
"version": "17.0.3.0.1",
"version": "17.0.4.0.0",
Copy link
Contributor

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"):
Copy link
Contributor

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)

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.

4 participants