Split logging features of fastapi_log #8

SirPyTech · 2025-04-23T08:20:16Z

As suggested in

Thanks for working on this. I wanted to do it since a while 😅

Early feedback...

The way I was planning to handle this was to have a base module to collect any kind of "request/response log".
This can be used by fastapi, base_rest, edi framework, endpoints and any custom implementation of request/response feature.

Would you consider splitting this part to a module like request_log or something like that?

Originally posted by @simahawk in OCA#501 (comment)

Also add the module fastapi_log_mail to optionally send an email when an exception occurs.

Depends on:

[16.0][IMP]fastapi: enable multi-slash routes OCA/rest-framework#515

ak-git-bot · 2025-04-23T08:20:25Z

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

PicchiSeba · 2025-04-30T10:18:33Z

Regarding the multi-slash PR maybe we should wait to include it in its entirety until we receive some feedback (and I implement some tests).

I could extract the get_endpoint method without the multi-slash part and put it in its own PR and then apply my PR on it.
This way we have a common method to work on.

What do you think?

SirPyTech · 2025-05-05T09:01:34Z

Regarding the multi-slash PR maybe we should wait to include it in its entirety until we receive some feedback (and I implement some tests).

I could extract the get_endpoint method without the multi-slash part and put it in its own PR and then apply my PR on it. This way we have a common method to work on.

What do you think?

Yes that's a good conservative solution.
So you'll create a simple refactoring PR that only extracts the occurrences of self.search([("root_path", "=", root_path)]) in a get_endpoint method, leaving the rest unchanged; that should be merged easily.

Then OCA#515 will build on the new PR to edit the get_endpoint implementation and root_path definition.

I will then include the new refactoring PR instead of OCA#515.

PicchiSeba · 2025-05-05T13:00:52Z

@SirPyTech

OCA#524

I moved the common method to this PR

PicchiSeba · 2025-05-07T06:59:09Z

@SirPyTech the commit 018f44d is missing.

SirPyTech · 2025-05-07T07:20:18Z

@SirPyTech the commit 018f44d is missing.

That is not how PRs are included in OCA, see https://github.com/OCA/maintainer-tools/wiki/Use-temporary-reference(s)-to-another-pull-request(s).
To switch from OCA#515 to OCA#524 I pushed this change https://github.com/akretion/rest-framework/compare/6632ff858bcc9370e46d75853b4edfe7d1fc5314..641d4df4235e0c20f1efe482c926fecdd586d47d yesterday.

PicchiSeba · 2025-05-07T07:34:29Z

@SirPyTech the commit 018f44d is missing.

That is not how PRs are included in OCA, see https://github.com/OCA/maintainer-tools/wiki/Use-temporary-reference(s)-to-another-pull-request(s). To switch from OCA#515 to OCA#524 I pushed this change https://github.com/akretion/rest-framework/compare/6632ff858bcc9370e46d75853b4edfe7d1fc5314..641d4df4235e0c20f1efe482c926fecdd586d47d yesterday.

Oh thanks, I didn't know

PicchiSeba

Code review: issue when processing requests with consumable bodies

PicchiSeba · 2025-05-08T10:16:11Z

api_log/models/api_log.py

+    def log_request(self, request):
+        log_request_values = {
+            "request_url": request.url,
+            "request_method": request.method,
+            "request_headers": self._headers_to_dict(request.headers),
+            "request_body": request.data,
+            "request_date": fields.Datetime.now(),
+            "request_time": self._current_time(),
+        }


issue: we should check if we can read request.data because if we don't and it is consumed then we get stuck into an infinite loop later on when calling ASGIMiddleware.__call__() (from https://github.com/OCA/rest-framework/blob/16.0/fastapi/middleware.py)

We could solve this like that

We change the method's signature to allow other parameters, then we move part of fastapi_log.models.api_log's implementation into the original method

Suggested change

def log_request(self, request):

log_request_values = {

"request_url": request.url,

"request_method": request.method,

"request_headers": self._headers_to_dict(request.headers),

"request_body": request.data,

"request_date": fields.Datetime.now(),

"request_time": self._current_time(),

}

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

log_request_values = {

"request_url": request.url,

"request_method": request.method,

"request_headers": self._headers_to_dict(request.headers),

"request_date": fields.Datetime.now(),

"request_time": self._current_time(),

}

stream = kwargs.get("fastapi_environ", {}).get("wsgi.input")

if stream and stream.seekable():

log_request_values["request_body"] = stream.read()

stream.seek(0)

else:

log_request_values["request_body"] = request.data

An easier solution would have been to copy the request object but it doesn't have access to such a method

EDIT: nvm, request should have access to a method called deepcopy

Thanks for the proposal, I prefer to avoid adding fastapi code in the generic module api_log so I have added a couple of hooks, please check

praise: that's actually a better way to do it, gj

PicchiSeba

Code review: good work! A couple of points

PicchiSeba · 2025-05-14T08:35:49Z

api_log/security/ir_model_access.xml

-        <field name="name">Fastapi Log: Read access</field>
-        <field name="model_id" ref="model_fastapi_log" />
-        <field name="group_id" ref="group_fastapi_log" />
+    <record id="access_api_log" model="ir.model.access">


question: does this change require a migration?

Since fastapi_log isn't merged yet, I wouldn't bother doing migrations in general.

If we want to keep the user's edits on fastapi_log's record then a migration is needed, otherwise Odoo will remove the old record and create a new one; since this is not a record usually edited by the user I wouldn't do a migration.

PicchiSeba · 2025-05-14T08:36:11Z

api_log/security/res_groups.xml

@@ -6,8 +6,8 @@
 -->
 <odoo>

-    <record id="group_fastapi_log" model="res.groups">
-        <field name="name">Fastapi Log Access</field>
+    <record id="group_api_log" model="res.groups">


question: (same as above)

See response for the question above.

PicchiSeba · 2025-05-14T08:39:49Z

api_log/tests/test_api_log.py

+        request = requests.Request(
+            headers={
+                "Api-Key": secret_api_key,
+                "Cookie": secret_cookie,


non-blocking: let's add another header, we should check that it does not get deleted when logged. For completeness

Right, done

PicchiSeba

We changed a few things in the multi-slash PR. Let's adapt to them

PicchiSeba · 2025-05-15T07:39:22Z

fastapi_log/fastapi_dispatcher.py

        fastapi_endpoint = (
            self.request.env["fastapi.endpoint"]
            .sudo()
-            .search([("root_path", "=", root_path)])


issue: the latest changes from @lmignon renamed this method to _get_endpoint, we should adapt its use

Done!
Actually, you did it 😉
Thanks

PicchiSeba · 2025-05-15T07:39:36Z

fastapi_log/models/api_log.py

+            log.fastapi_endpoint_id = (
+                request.env["fastapi.endpoint"]
+                .sudo()
+                .get_endpoint(environ["PATH_INFO"])


issue: same as above

Done!
Actually, you did it 😉
Thanks

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

PicchiSeba

Code and functional review: LGTM

SirPyTech force-pushed the 16.0-add-fastapi_log branch from 1e08ade to ff9d6af Compare April 28, 2025 09:48

SirPyTech marked this pull request as draft April 28, 2025 09:54

SirPyTech force-pushed the 16.0-add-fastapi_log branch from ff9d6af to 1e08ade Compare April 28, 2025 09:57

SirPyTech marked this pull request as ready for review April 28, 2025 10:05

SirPyTech mentioned this pull request Apr 28, 2025

[16.0] [ADD] fastapi_log OCA/rest-framework#501

Open

SirPyTech force-pushed the 16.0-add-fastapi_log branch from 6632ff8 to 641d4df Compare May 6, 2025 07:26

PicchiSeba suggested changes May 8, 2025

View reviewed changes

SirPyTech marked this pull request as draft May 12, 2025 12:14

SirPyTech force-pushed the 16.0-add-fastapi_log branch from 641d4df to 4e86b39 Compare May 13, 2025 09:05

SirPyTech requested a review from PicchiSeba May 13, 2025 10:33

SirPyTech marked this pull request as ready for review May 13, 2025 10:33

PicchiSeba reviewed May 14, 2025

View reviewed changes

PicchiSeba suggested changes May 15, 2025

View reviewed changes

PicchiSeba force-pushed the 16.0-add-fastapi_log branch 2 times, most recently from 49e7872 to 11defea Compare May 16, 2025 07:45

SirPyTech added 5 commits May 20, 2025 11:03

[REF] fastapi_log: Extract common features to api_log

86c7696

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

[ADD] fastapi_log_mail

81168a1

[FIX] fastapi_log: Manage multi-slash endpoints

9ee914c

[DON'T MERGE] test-requirements.txt

64598a4

[IMP] api_log: Hide sensitive headers

05fb39b

SirPyTech force-pushed the 16.0-add-fastapi_log branch from 11defea to 05fb39b Compare May 20, 2025 09:05

SirPyTech requested a review from PicchiSeba May 20, 2025 09:49

PicchiSeba approved these changes May 20, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Split logging features of fastapi_log #8

Split logging features of fastapi_log #8

SirPyTech commented Apr 23, 2025 •

edited

Loading

ak-git-bot commented Apr 23, 2025

PicchiSeba commented Apr 30, 2025 •

edited

Loading

SirPyTech commented May 5, 2025

PicchiSeba commented May 5, 2025

PicchiSeba commented May 7, 2025

SirPyTech commented May 7, 2025

PicchiSeba commented May 7, 2025

PicchiSeba left a comment

PicchiSeba May 8, 2025 •

edited

Loading

SirPyTech May 13, 2025

PicchiSeba May 13, 2025

PicchiSeba left a comment

PicchiSeba May 14, 2025

SirPyTech May 20, 2025

PicchiSeba May 14, 2025

SirPyTech May 20, 2025

PicchiSeba May 14, 2025

SirPyTech May 20, 2025

PicchiSeba left a comment

PicchiSeba May 15, 2025

SirPyTech May 20, 2025

PicchiSeba May 15, 2025

SirPyTech May 20, 2025

PicchiSeba left a comment

Split logging features of fastapi_log #8

Are you sure you want to change the base?

Split logging features of fastapi_log #8

Conversation

SirPyTech commented Apr 23, 2025 • edited Loading

ak-git-bot commented Apr 23, 2025

PicchiSeba commented Apr 30, 2025 • edited Loading

SirPyTech commented May 5, 2025

PicchiSeba commented May 5, 2025

PicchiSeba commented May 7, 2025

SirPyTech commented May 7, 2025

PicchiSeba commented May 7, 2025

PicchiSeba left a comment

Choose a reason for hiding this comment

PicchiSeba May 8, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PicchiSeba left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PicchiSeba left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PicchiSeba left a comment

Choose a reason for hiding this comment

SirPyTech commented Apr 23, 2025 •

edited

Loading

PicchiSeba commented Apr 30, 2025 •

edited

Loading

PicchiSeba May 8, 2025 •

edited

Loading