- solved #840 #849
- solved #840 #849
Conversation
Codecov Report
@@ Coverage Diff @@
## 1.6.x #849 +/- ##
============================================
+ Coverage 48.96% 49.00% +0.03%
- Complexity 1941 1951 +10
============================================
Files 267 267
Lines 12081 12112 +31
Branches 1692 1704 +12
============================================
+ Hits 5916 5936 +20
- Misses 5462 5463 +1
- Partials 703 713 +10
Continue to review full report at Codecov.
|
audit/src/test/resources/audit.yml
Outdated
| - serviceId | ||
|
|
||
| # Request Body, this is optional and must be set by the service in its implementation | ||
| - request |
There was a problem hiding this comment.
can we rename this to requestBody and responseBody? Just saying request sounds like it will audit the whole request including query params, path params, cookies etc
There was a problem hiding this comment.
Thanks for pointing out, I also think renaming would be better. But seems these names have been used in previous versions. If we change them now, it may cause backward compatibility. People who previously used these names to audit requests may not be able to continue auditing them if they don't change their configuration. @stevehu Do you think we can change it?
There was a problem hiding this comment.
I think it makes sense to rename it to clearly define what is the audited object. Regarding the backward compatibility in the config, we can mention that in the next release notes. It is not a coding issue but only the configuration file and it is easier for users to adjust. Thank you for pointing it out. We need to update the document to refect the change in this PR as well.
There was a problem hiding this comment.
I see. Will rename them.
audit/src/test/resources/audit.yml
Outdated
| - request | ||
|
|
||
| # Response payload, this is optional and must be set by the service in its implementation | ||
| - response |
There was a problem hiding this comment.
same comment as comment for request
| - response | ||
|
|
||
| # Request query parameters | ||
| - queryParameters |
There was a problem hiding this comment.
you added queryParams, what if somebody wants to audit path params and cookies?
There was a problem hiding this comment.
Thanks for pointing out.
Since my client now only needs audit query parameters, I was limited to audit query parameters before. I think we can try to cover more audit point.
There was a problem hiding this comment.
I have realized that serviceId is in the audit list but it is not implemented. It is only mentioned in the comment section at the beginning of the handler. When the audit log aggregated to a central location, the serviceId should be the key info. @jiachen1120 Could you please add this to this PR? It should be retrieved from the server.yml file. Let me know if you have questions or concerns.
- auditing serviceId
stevehu
deleted the
feat/#840_request_response_audit_missing_when_auditOnError
branch
* - solved networknt#840 - audit request if bodyHandler enabled - audit query parameters if request contains it - audit response on error * - auditing all request components - auditing serviceId * - added more test cases
issue: #840