[16.0][ADD] fastapi_log #554

SirPyTech · 2025-08-04T10:10:51Z

In the past months we have been proposing many improvements to #501 with akretion#8.

From more than a month ago, the branch is outdated (akretion#8 (comment)) and it is causing conflicts in akretion#8.

Here we are proposing all those improvements to OCA directly.

SirPyTech · 2025-08-04T10:14:00Z

@simahawk here I addressed your latest comments akretion#8 (comment) and akretion#8 (comment), please have a look

simahawk · 2025-08-12T07:53:07Z

api_log/models/api_log.py

+    collection_model = fields.Char(
+        compute="_compute_collection",
+        store=True,
+        index=True,


I'm not sure we need all these indexes. Looks like you search only using collection_ref.
IMO you can drop the collection_ref index and search via model+id

simahawk · 2025-08-12T07:55:00Z

api_log/models/api_log.py

+    time = fields.Float(compute="_compute_time", store=True)
+    request_preview = fields.Text(compute="_compute_request_preview")
+    response_preview = fields.Text(compute="_compute_response_preview")
+    request_b64 = fields.Binary(


what is the reason for these b64 fields?

I thought they would be useful to store the binary value of the request/response, but now I see request/response body is already Binary.
Removed

They were used to dowload the payload directly from the log form in case of a file upload/download.

They were used to dowload the payload directly from the log form in case of a file upload/download.

Thanks for letting us know, I added them back.

simahawk · 2025-08-12T07:58:03Z

api_log/models/api_log.py

+    @api.depends(
+        "collection_ref",
+    )
+    def _compute_collection(self):


shouldn't be the other way around? You compute collection_ref based on model + id

Sure, you can compute collection_ref based on model + id; or you can compute model + id from collection_ref.

I see no big difference between the two approaches, is there any specific reason to do it in the way you suggest?

simahawk · 2025-08-12T07:58:44Z

api_log/models/api_log.py

+        try:
+            headers_dict = {key: value for key, value in headers.items()}
+            return self._sanitize_headers_dict(headers_dict)
+        except AttributeError:


when do you expect to have this kind of error?

I'm suspicious about the headers argument, if it does not have the items method then an AttributeError would be raised.
But you're right, let's be optimistic and remove the exception handling 😄

simahawk · 2025-08-12T08:00:24Z

api_log/models/api_log.py

+    def log_request(self, request, override_log_values=None):
+        log_request_values = self._prepare_log_request(request)
+        if override_log_values:
+            log_request_values.update(override_log_values)
+        return self.sudo().create(log_request_values)


Suggested change

def log_request(self, request, override_log_values=None):

log_request_values = self._prepare_log_request(request)

if override_log_values:

log_request_values.update(override_log_values)

return self.sudo().create(log_request_values)

def log_request(self, request, **kw):

log_request_values = self._prepare_log_request(request)

log_request_values.update(kw)

return self.sudo().create(log_request_values)

I prefer to keep the log values in their own variable instead of using kwargs because this way more parameters can be added to the method to tweak its behavior.

I removed the if as suggested though.

simahawk · 2025-08-12T08:03:27Z

api_log/models/api_log.py

+    def _prepare_log_response(self, response):
+        return {
+            "response_status_code": response.status_code,
+            "response_headers": self._headers_to_dict(response.headers),


One of the things done in rest_log was to inject a reference to the log entry in the response.
There we assumed the response was always JSON data and added a key.
Here we could inject a response header (eg: API_LOG_ENTRY_URL)

I have added the identifier (api.log.id) of the log record, that is enough to find the log record to whoever has access to them.

Please keep in mind that I agree with the author of #501 that wrote:

This is not a migration of rest_log, this module aims to be simpler, at least in the first iterations.

A more elaborated feature (like building the full URL of the log record) can be added later, after the first simple module is merged.

simahawk · 2025-08-12T08:06:07Z

api_log/models/api_log_collection.py

+    def _compute_log_ids(self):
+        for collection in self:
+            collection.log_ids = self.env["api.log"].search(
+                [("collection_ref", "=", "%s,%s" % (collection._name, collection.id))]


see my comment above on how to search for logs

simahawk · 2025-08-12T08:07:23Z

api_log_mail/models/api_log.py

+from odoo import api, models
+
+
+class FastapiLog(models.Model):


Suggested change

class FastapiLog(models.Model):

class ApiLog(models.Model):

Nice catch! Thanks

SirPyTech · 2025-08-21T10:27:22Z

The error in the tests (https://github.com/OCA/rest-framework/actions/runs/17124124402/job/48571725570?pr=554#step:8:120) does not seem to be related to these changes

Stack

2025-08-21 10:24:04,335 340 INFO odoo odoo.modules.loading: Loading module graphql_base (12/53) 
2025-08-21 10:24:04,475 340 CRITICAL odoo odoo.modules.module: Couldn't load module graphql_base 
2025-08-21 10:24:04,475 340 CRITICAL odoo odoo.modules.module: cannot import name 'HttpQueryError' from 'graphql_server' (/opt/odoo-venv/lib/python3.10/site-packages/graphql_server/__init__.py) 
2025-08-21 10:24:04,477 340 WARNING odoo odoo.modules.loading: Transient module states were reset 
2025-08-21 10:24:04,477 340 ERROR odoo odoo.modules.registry: Failed to load registry 
Traceback (most recent call last):
  File "/opt/odoo/odoo/modules/registry.py", line 87, in new
    odoo.modules.load_modules(registry, force_demo, status, update_module)
  File "/opt/odoo/odoo/modules/loading.py", line 493, in load_modules
    processed_modules += load_marked_modules(cr, graph,
  File "/opt/odoo/odoo/modules/loading.py", line 374, in load_marked_modules
    loaded, processed = load_module_graph(
  File "/opt/odoo/odoo/modules/loading.py", line 190, in load_module_graph
    load_openerp_module(package.name)
  File "/opt/odoo/odoo/modules/module.py", line 471, in load_openerp_module
    __import__('odoo.addons.' + module_name)
  File "/__w/rest-framework/rest-framework/graphql_base/__init__.py", line 4, in 
    from .controllers import GraphQLControllerMixin
  File "/__w/rest-framework/rest-framework/graphql_base/controllers/__init__.py", line 1, in 
    from .main import GraphQLControllerMixin
  File "/__w/rest-framework/rest-framework/graphql_base/controllers/main.py", line 6, in 
    from graphql_server import (
ImportError: cannot import name 'HttpQueryError' from 'graphql_server' (/opt/odoo-venv/lib/python3.10/site-packages/graphql_server/__init__.py)
2025-08-21 10:24:04,478 340 CRITICAL odoo odoo.service.server: Failed to initialize database `odoo`. 
Traceback (most recent call last):
  File "/opt/odoo/odoo/service/server.py", line 1333, in preload_registries
    registry = Registry.new(dbname, update_module=update_module)
  File "", line 2, in new
  File "/opt/odoo/odoo/tools/func.py", line 87, in locked
    return func(inst, *args, **kwargs)
  File "/opt/odoo/odoo/modules/registry.py", line 87, in new
    odoo.modules.load_modules(registry, force_demo, status, update_module)
  File "/opt/odoo/odoo/modules/loading.py", line 493, in load_modules
    processed_modules += load_marked_modules(cr, graph,
  File "/opt/odoo/odoo/modules/loading.py", line 374, in load_marked_modules
    loaded, processed = load_module_graph(
  File "/opt/odoo/odoo/modules/loading.py", line 190, in load_module_graph
    load_openerp_module(package.name)
  File "/opt/odoo/odoo/modules/module.py", line 471, in load_openerp_module
    __import__('odoo.addons.' + module_name)
  File "/__w/rest-framework/rest-framework/graphql_base/__init__.py", line 4, in 
    from .controllers import GraphQLControllerMixin
  File "/__w/rest-framework/rest-framework/graphql_base/controllers/__init__.py", line 1, in 
    from .main import GraphQLControllerMixin
  File "/__w/rest-framework/rest-framework/graphql_base/controllers/main.py", line 6, in 
    from graphql_server import (
ImportError: cannot import name 'HttpQueryError' from 'graphql_server' (/opt/odoo-venv/lib/python3.10/site-packages/graphql_server/__init__.py)

SirPyTech

Thanks for the review!
Please check the latest changes

SirPyTech · 2025-08-21T08:45:03Z

api_log/models/api_log.py

+    collection_model = fields.Char(
+        compute="_compute_collection",
+        store=True,
+        index=True,


SirPyTech · 2025-08-21T08:56:45Z

api_log/models/api_log.py

+    time = fields.Float(compute="_compute_time", store=True)
+    request_preview = fields.Text(compute="_compute_request_preview")
+    response_preview = fields.Text(compute="_compute_response_preview")
+    request_b64 = fields.Binary(


I thought they would be useful to store the binary value of the request/response, but now I see request/response body is already Binary.
Removed

SirPyTech · 2025-08-21T09:00:50Z

api_log/models/api_log.py

+    @api.depends(
+        "collection_ref",
+    )
+    def _compute_collection(self):


Sure, you can compute collection_ref based on model + id; or you can compute model + id from collection_ref.

I see no big difference between the two approaches, is there any specific reason to do it in the way you suggest?

SirPyTech · 2025-08-21T09:07:51Z

api_log/models/api_log.py

+        try:
+            headers_dict = {key: value for key, value in headers.items()}
+            return self._sanitize_headers_dict(headers_dict)
+        except AttributeError:


I'm suspicious about the headers argument, if it does not have the items method then an AttributeError would be raised.
But you're right, let's be optimistic and remove the exception handling 😄

SirPyTech · 2025-08-21T09:08:45Z

api_log/models/api_log.py

+    def log_request(self, request, override_log_values=None):
+        log_request_values = self._prepare_log_request(request)
+        if override_log_values:
+            log_request_values.update(override_log_values)
+        return self.sudo().create(log_request_values)


I prefer to keep the log values in their own variable instead of using kwargs because this way more parameters can be added to the method to tweak its behavior.

I removed the if as suggested though.

SirPyTech · 2025-08-21T09:15:34Z

api_log/models/api_log.py

+    def _prepare_log_response(self, response):
+        return {
+            "response_status_code": response.status_code,
+            "response_headers": self._headers_to_dict(response.headers),


I have added the identifier (api.log.id) of the log record, that is enough to find the log record to whoever has access to them.

Please keep in mind that I agree with the author of #501 that wrote:

This is not a migration of rest_log, this module aims to be simpler, at least in the first iterations.

A more elaborated feature (like building the full URL of the log record) can be added later, after the first simple module is merged.

SirPyTech · 2025-08-21T09:15:44Z

api_log/models/api_log_collection.py

+    def _compute_log_ids(self):
+        for collection in self:
+            collection.log_ids = self.env["api.log"].search(
+                [("collection_ref", "=", "%s,%s" % (collection._name, collection.id))]


SirPyTech · 2025-08-21T09:16:51Z

api_log_mail/models/api_log.py

+from odoo import api, models
+
+
+class FastapiLog(models.Model):


Nice catch! Thanks

paradoxxxzero

Good work thanks. Nothing blocking except the exception commit problem. Once it’s fixed I’ll close my old PR.

paradoxxxzero · 2025-09-04T10:23:11Z

api_log/models/api_log.py

+
+    def _prepare_log_response(self, response):
+        headers_dict = self._headers_to_dict(response.headers)
+        headers_dict["API_LOG_ENTRY_ID"] = self.id


This does not add the id to the headers of the response, only in the logs. Set it in the response instead.

Right, thanks for catching that!
I have added a quick assert in the tests too

paradoxxxzero · 2025-09-04T10:25:33Z

api_log/views/api_log_views.xml

+                            <field name="response_status_code" />
+                            <field name="response_headers_preview" />
+                            <field
+                                name="response_preview"


There is no way to access the full response in the view anymore since the removal of the b64 field.
I think it should be kept one way or another.

👍 Answered to the same query in #554 (comment).

paradoxxxzero · 2025-09-04T10:43:01Z

fastapi_log/fastapi_dispatcher.py

+                    _logger.warning("Failed to log exception", exc_info=e)
+                else:
+                    # Be sure to commit/save the exception's log
+                    env.cr.commit()


This is very wrong. You are committing a transaction of a failed request.

Consider this simple case :

@router.post("/create_partner") def create_partner(env: Annotated[api.Environment, Depends(optionally_authenticated_partner_env)]): partner = env["res.partner"].create({...}) if is_something_wrong_with_new_partner(partner): raise UserError("Something is wrong with new partner") return "Ok"

If logging is enabled the wrong partner will be stored in database.

Please restore the separate cursor (https://github.com/OCA/rest-framework/pull/501/files#diff-67e6f9ad0c4d04c8de3dcd507c53fa5bbebf76b169c660d976f36c84c236ea68R38) or find another way to commit the exception.

I verified this and you are correct.
I added the separate cursor in a commit with you as a co-author to fix this, could you please check?

SirPyTech

Thanks for the review @paradoxxxzero, please check the latest changes

SirPyTech · 2025-09-09T10:20:36Z

api_log/models/api_log.py

+
+    def _prepare_log_response(self, response):
+        headers_dict = self._headers_to_dict(response.headers)
+        headers_dict["API_LOG_ENTRY_ID"] = self.id


Right, thanks for catching that!
I have added a quick assert in the tests too

SirPyTech · 2025-09-09T10:26:47Z

fastapi_log/fastapi_dispatcher.py

+                    _logger.warning("Failed to log exception", exc_info=e)
+                else:
+                    # Be sure to commit/save the exception's log
+                    env.cr.commit()


I verified this and you are correct.
I added the separate cursor in a commit with you as a co-author to fix this, could you please check?

SirPyTech · 2025-09-09T10:30:42Z

api_log/models/api_log.py

+    time = fields.Float(compute="_compute_time", store=True)
+    request_preview = fields.Text(compute="_compute_request_preview")
+    response_preview = fields.Text(compute="_compute_response_preview")
+    request_b64 = fields.Binary(


They were used to dowload the payload directly from the log form in case of a file upload/download.

Thanks for letting us know, I added them back.

SirPyTech · 2025-09-09T10:31:59Z

api_log/views/api_log_views.xml

+                            <field name="response_status_code" />
+                            <field name="response_headers_preview" />
+                            <field
+                                name="response_preview"


👍 Answered to the same query in #554 (comment).

paradoxxxzero

Except for missing header in exception, LGTM

paradoxxxzero · 2025-09-18T10:09:20Z

api_log/models/api_log.py

+        return self.sudo().create(log_request_values)
+
+    def _prepare_log_response(self, response):
+        response.headers["API_LOG_ENTRY_ID"] = self.id


I think it could be better to use kebab-case here

Right, it is the common convention for headers keys but I didn't know that, thanks!

paradoxxxzero · 2025-09-18T10:09:51Z

api_log/models/api_log.py

+        return self.sudo().write(log_response_values)
+
+    def _prepare_log_exception(self, exception):
+        values = {


API_LOG_ENTRY_ID header is not set in case of exception

Thanks, I added it, please check.

Note that it won't be returned in the response to the client but only stored in the log record, because in case of an exception the dispatcher simply propagates the exception raised by the endpoint.

paradoxxxzero · 2025-10-20T10:34:52Z

api_log/models/api_log.py

+        values = {
+            "stack_trace": "".join(format_exception(exception)),
+            "response_headers": self._inject_log_entry(dict()),


I think it is especially important to return the header in case of exception, luckily we can do this by setting exception headers attribute, see: https://github.com/OCA/rest-framework/blob/18.0/fastapi/fastapi_dispatcher.py#L46

Suggested change

values = {

"stack_trace": "".join(format_exception(exception)),

"response_headers": self._inject_log_entry(dict()),

exception.headers = getattr(exception, "headers", {})

values = {

"stack_trace": "".join(format_exception(exception)),

"response_headers": self._inject_log_entry(exception.headers),

}

I agree, that's why I mentioned it specifically, but I didn't know a way to inject it; the solution you proposed is awesome and works great!
I included it, please check the new changes.

paradoxxxzero

LGTM

Also please rename this PR to something like: [16.0][ADD] fastapi_log

This way other APIs might use the new module `api_log` to store logs.

Co-authored-by: Florian Mounier <paradoxxx.zero@gmail.com>

SirPyTech · 2025-11-26T09:00:22Z

Rebased to check if this is affected by fastapi/fastapi@51ad909.
It still looks good 😎

SirPyTech marked this pull request as ready for review August 4, 2025 10:13

SirPyTech mentioned this pull request Aug 4, 2025

Split logging features of fastapi_log akretion/rest-framework#8

Closed

1 task

simahawk requested changes Aug 12, 2025

View reviewed changes

SirPyTech force-pushed the 16.0-add-fastapi_log branch from 7674bd7 to d1f3eec Compare August 21, 2025 10:21

SirPyTech commented Aug 21, 2025

View reviewed changes

SirPyTech requested a review from simahawk August 21, 2025 10:28

sebastienbeau added this to the 16.0 milestone Sep 1, 2025

paradoxxxzero suggested changes Sep 4, 2025

View reviewed changes

SirPyTech force-pushed the 16.0-add-fastapi_log branch from d1f3eec to 4e1d2b8 Compare September 9, 2025 11:01

SirPyTech commented Sep 9, 2025

View reviewed changes

SirPyTech force-pushed the 16.0-add-fastapi_log branch from 4e1d2b8 to d7bf717 Compare September 9, 2025 11:05

aleuffre approved these changes Sep 10, 2025

View reviewed changes

SirPyTech requested a review from paradoxxxzero September 10, 2025 06:59

paradoxxxzero approved these changes Sep 18, 2025

View reviewed changes

paradoxxxzero mentioned this pull request Sep 18, 2025

[16.0] [ADD] fastapi_log #501

Closed

SirPyTech force-pushed the 16.0-add-fastapi_log branch from d7bf717 to fb9fad2 Compare September 24, 2025 07:43

SirPyTech requested a review from paradoxxxzero September 24, 2025 07:56

paradoxxxzero suggested changes Oct 20, 2025

View reviewed changes

SirPyTech force-pushed the 16.0-add-fastapi_log branch from fb9fad2 to 67660fd Compare October 20, 2025 10:57

SirPyTech requested a review from paradoxxxzero October 20, 2025 11:00

paradoxxxzero approved these changes Oct 20, 2025

View reviewed changes

SirPyTech changed the title ~~Log FastAPI requests~~ [16.0][ADD] fastapi_log Oct 20, 2025

paradoxxxzero and others added 6 commits November 26, 2025 09:49

[ADD] fastapi_log

77d0242

[REF] fastapi_log: Extract common features to api_log

a6da320

This way other APIs might use the new module `api_log` to store logs.

[ADD] fastapi_log_mail

be38517

[FIX] fastapi_log: Manage multi-slash endpoints

6974dd1

[IMP] api_log: Hide sensitive headers

ca8b5e4

[IMP] api_log: Add collection of logs

4cf4899

SirPyTech and others added 7 commits November 26, 2025 09:49

[ADD] api_log_mail: Notify user about logged exceptions

feb7ea4

[IMP] fastapi_log: Adapt to log collection

710bdba

[IMP] fastapi_log_mail: Adapt to log collection

66c26f5

[IMP] api_log: Inject Log record identifier

4417639

[IMP] fastapi_log: Use dedicated cursor for logs

5dec230

Co-authored-by: Florian Mounier <paradoxxx.zero@gmail.com>

[IMP] fastapi_log_mail: Use dedicated cursor in tests

341195a

[IMP] fastapi_log: In tests, check Log identifier in response

0fae517

SirPyTech force-pushed the 16.0-add-fastapi_log branch from 67660fd to 0fae517 Compare November 26, 2025 08:54

Uh oh!

[16.0][ADD] fastapi_log #554

Are you sure you want to change the base?

[16.0][ADD] fastapi_log #554

Uh oh!

Conversation

SirPyTech commented Aug 4, 2025

Uh oh!

SirPyTech commented Aug 4, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SirPyTech commented Aug 21, 2025

Uh oh!

SirPyTech left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paradoxxxzero left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!