Skip to content

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
"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.

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)

@github-actions
Copy link

There hasn't been any activity on this pull request in the past 4 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days.
If you want this PR to never become stale, please ask a PSC member to apply the "no stale" label.

@github-actions github-actions bot added the stale PR/Issue without recent activity, it'll be soon closed automatically. label Mar 16, 2025
@github-actions github-actions bot closed this Apr 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

stale PR/Issue without recent activity, it'll be soon closed automatically.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants