-
Notifications
You must be signed in to change notification settings - Fork 1
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
[#2] Add option to log request response body #4
Conversation
f596dfe
to
5008ad8
Compare
Codecov Report
@@ Coverage Diff @@
## main #4 +/- ##
==========================================
+ Coverage 88.46% 92.20% +3.74%
==========================================
Files 7 9 +2
Lines 104 218 +114
Branches 9 25 +16
==========================================
+ Hits 92 201 +109
- Misses 7 11 +4
- Partials 5 6 +1
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you also check the test coverage? This PR makes it drop, instead the coverage for a PR should be 100% to make sure you don't add untested code.
c05af12
to
fcf7741
Compare
0f7befb
to
62ea45b
Compare
62ea45b
to
f8a6b30
Compare
f8a6b30
to
2688188
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still seeing many new tests that are not in pytest-style, what's the reasoning behind that?
d59bd02
to
fbf3db0
Compare
fbf3db0
to
6d1ef18
Compare
02df661
to
7b84da0
Compare
@vaszig feel free to try this out and do a double-check of Paul's changes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't looked too much at the tests yet - let's get the implementation sorted out and approved first before I dive deeper into the tests.
LOG_OUTGOING_REQUESTS_CONTENT_TYPES = [ | ||
"text/*", | ||
"application/json", | ||
"application/xml", | ||
"application/soap+xml", | ||
] # save request/response bodies with matching content type |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as discussed in the office - this would probably benefit from a dataclass so you can do things like:
LOG_OUTGOING_REQUESTS_CONTENT_TYPES = [
ContentType(pattern="text/xml", default_encoding="iso-8859-3"),
ContentType(pattern="text/*", default_encoding="utf-8"),
ContentType(pattern="application/json", default_encoding="utf-8"),
...
]
then algorithm-wise you:
- extract the content type of the body (request or response)
- for every item in
settings.LOG_OUTGOING_REQUESTS_CONTENT_TYPES
- check if there's an exact match
- if not, check if there's a pattern match (using the wildcard)
- if there's no match -> don't log it
- if there's a match
- try to get the charset from the content-type header
- if there's no charset, fall back to the
ContentType.default_encoding
- Log the body
Somewhere in between there's also the check if settings/configuration and the body size
We also discussed that you might provide a setting/escape hatch to derive the encoding, something along the lines of:
def extract_encoding_for_body(content_type: str, body: bytes) -> str:
... # default implementation is part of the above algorithm
LOG_OUTGOING_REQUESTS_EXTRACT_ENCODING = "log_outgoing_requests.utils.extract_encoding_for_body"
so the default setting value should be the library function, but projects can provide their own function. In the case of XML for example, you'd want to parse the first line and get the value from the <?xml encoding="...">
part, but that's for projects to figure out.
LOG_OUTGOING_REQUESTS_DB_SAVE_BODY = True # save request/response body | ||
LOG_OUTGOING_REQUESTS_EMIT_BODY = True # log request/response body | ||
LOG_OUTGOING_REQUESTS_CONTENT_TYPES = [ | ||
"text/*", | ||
"application/json", | ||
"application/xml", | ||
"application/soap+xml", | ||
] # save request/response bodies with matching content type | ||
LOG_OUTGOING_REQUESTS_MAX_CONTENT_LENGTH = 524_288 # maximal size (in bytes) for the request/response body | ||
LOG_OUTGOING_REQUESTS_LOG_BODY_TO_STDOUT = True | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the library is currently missing a proper default settings mechanism, so I created #6 for this.
7b84da0
to
6bae3ef
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work!This is getting a lot better!
After Sergei's remarks and because I am not so familiar with all the code which has been added, I don't have a lot to mention except some minor spelling mistakes and one remark-question concerning the model.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's some follow up to be done, but that is going to happen in separate issues/PRs
Closes open-formulieren/open-forms#2762
Closes #2 (partly)