Save log once when request fully processed #55
Conversation
Actually, finalize_response is always called, even when an exception is throwed. So, we don't need to save status_code and fire save() in handle_exception as it will always be done in finalize_response.
| # tracking db constraints should not create a failure on the API | ||
| return | ||
| # create log | ||
| self.request.log = APIRequestLog( |
There was a problem hiding this comment.
As far as I know, this change would only create the object in memory and not persist to the db.
There was a problem hiding this comment.
Well, yes, that's the point. But then, the object is carried over through request processing and we do save in DB at the end.
With these changes we have only one call to the DB instead of:
I think that this kind of optimizations is not pointless, as we surely want to limit the overhead brought by the tracking.
There was a problem hiding this comment.
Yeah, I see the other save locations as part of the whole picture.
|
@frankie567 First off. I want to express appreciation for finding this library and contributing to it. Some questions:
The try catch was added (I do not have the time to find the PR/issues on this) because the save would could fail on initialize and caused users regular API views to stop functioning. The finalize response then tried to operate on an object that didn't exist. This led to the has attribute check. This info leads too:.
|
|
@frankie567 My questions above are a little nuanced but wanted to make sure we didn't reintroduce issues that were once closed is all. |
|
@avelis Don't worry, I perfectly understand your point. I'll try to bring as more details as possible. 1. In case of thread interruption 2. Prevent mixin from ruining the API What could happen indeed is a problem during the final However, in the current version, if something fails during the initial object creation, I'm not sure the API continues gently. As we see on line #57, the To address these two last points, we surely could make a test, mocking model That's it! I hope I answered all your questions :) Regards! |
|
@frankie567 Thank you for your thought out responses to my questions. I merged in the PR. I appreciate the contribution to this library! I am sure there will be a performance improvement for sure. |
|
@frankie567 To address your other shared thoughts. If you still see in an improvement to not returning prematurely and potentially moving to use a pass statement. I am all for it! |
|
You're welcome! Thanks for the merge :-) Well, now that this PR is merged, this Still, there is still the case where the log If you're okay with that, I will pop another PR with a test assuming DB failure and a proposal to prevent errors when it happens. Best regards ! |
|
@frankie567 I am absolutely ok with another PR. Look forward to it. |
This is a follow-up of the discussion in PR aschn#55. If something is going wrong during log saving, we don't want the API to fail and throw out an error to the end user. To do this, silently swallow any exception throwed by save(). A test is provided to check that the response status_code of the API is still 200 even in the event of a DB failure.
This is a follow-up of the discussion in PR #55. If something is going wrong during log saving, we don't want the API to fail and throw out an error to the end user. To do this, silently swallow any exception throwed by save(). A test is provided to check that the response status_code of the API is still 200 even in the event of a DB failure.
Hi,
Ok this is maybe completely dummy from me, but couldn't we save us some writings to DB by saving
APIRequestLogonly once ; at the end of the request processing (either fromhandle_exceptionorfinalize_response)?Here is a proposal for this. Tests seem to pass.
Regards 🙂