-
-
Notifications
You must be signed in to change notification settings - Fork 208
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
Fix recorded requests skipped by request logger #346
Fix recorded requests skipped by request logger #346
Conversation
- When proxy is enabled the recorded requests are mistaken (IMO) for admin requests and skipped
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 take a look?
@@ -99,7 +100,7 @@ private async Task InvokeInternal(IContext ctx) | |||
return; | |||
} | |||
|
|||
logRequest = !targetMapping.IsAdminInterface; | |||
logRequest = !targetMapping.IsAdminInterface || targetMapping.Provider is ProxyAsyncResponseProvider; |
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.
The mapping class already has this code:
public bool IsAdminInterface => Provider is DynamicResponseProvider || Provider is DynamicAsyncResponseProvider || Provider is ProxyAsyncResponseProvider;
So no need to add this here?
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.
That's the root cause of the problem (there is negation). In this check it is actually assumes that proxy response is NOT admin interface in order to log it. I have tried to remove that condition on mapping but it actually conflicts with Admin interface. There is a test for that - FluentMockServer_Proxy_Should_Not_overrule_AdminMappings. So it is kind of workaround to still log proxy requests.
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.
Ah yes, Sorry.
In that case. Maybe it's better to to add a property to the mapping class, like this:
public bool LogRequest => !(Provider is DynamicResponseProvider || Provider is DynamicAsyncResponseProvider).
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.
What about smth like:
public bool IsProxyRequest => Provider is ProxyAsyncResponseProvider
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.
But then you still need to check if it's not an admin request.
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.
In the MiddleWare - yes. Let me update the PR
src/WireMock.Net/Mapping.cs
Outdated
@@ -46,7 +46,10 @@ public class Mapping : IMapping | |||
public bool IsStartState => Scenario == null || Scenario != null && NextState != null && ExecutionConditionState == null; | |||
|
|||
/// <inheritdoc cref="IMapping.IsAdminInterface" /> | |||
public bool IsAdminInterface => Provider is DynamicResponseProvider || Provider is DynamicAsyncResponseProvider || Provider is ProxyAsyncResponseProvider; | |||
public bool IsAdminInterface => Provider is DynamicResponseProvider || Provider is DynamicAsyncResponseProvider; |
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.
It is too dangerous to update this code because .IsAdminInterface
is used in more places in the application.
I think the best solution is to add a property LogRequest
as proposed earlier.
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.
It actually logs both, request and response. Is LogMapping
better name for this property?
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.
LogMapping is fine.
Codecov Report
@@ Coverage Diff @@
## master #346 +/- ##
==========================================
+ Coverage 80.25% 80.26% +<.01%
==========================================
Files 111 111
Lines 4442 4443 +1
==========================================
+ Hits 3565 3566 +1
Misses 877 877
Continue to review full report at Codecov.
|
Thanks. |
When proxy is enabled the recorded requests are mistaken (IMO) for admin interface requests and skipped in LogEntries. Actually none of requests logged in proxy mode. It is valuable to monitor that while recording.