Fix wrong type being passed to IInterceptorSelector.SelectInterceptors for class proxies
#359
Conversation
stakx
left a comment
stakx
left a comment
There was a problem hiding this comment.
Just a couple of explanatory notes.
| new MethodInvocationExpression(null, | ||
| TypeUtilMethods.GetTypeOrNull, | ||
| getTargetExpression(@class, MethodToOverride)), | ||
| targetTypeExpression, |
There was a problem hiding this comment.
The simplest possible bugfix (which wouldn't require any changes in locations other than this one) would be to wrap the result of getTargetExpression(...) with a call to TypeUtil.GetTypeOrNull only if it is not a TypeTokenExpression already. I discarded this simpler solution because it feels like a hack. (It requires out-of-band knowledge about the kind of Expression that class proxies will produce inside their version of getTargetExpression.) The fix proposed in this PR is more elaborate, but arguably cleaner.
| OverrideMethodDelegate createMethod, IInvocationCreationContributor contributor) | ||
| : this(method, interceptors, invocation, getTargetExpression, null, createMethod, contributor) | ||
| { | ||
| } |
There was a problem hiding this comment.
Adding the new parameter to the existing (publicly visible) ctor would be a breaking change, hence a new ctor.
There was a problem hiding this comment.
No one should be using these classes anyway as they really are internal implementation, but happy for this to stay as is for now and can be removed later.
stakx
force-pushed
the
issue-74
branch
2 times, most recently
from
e786159 to
7f55def
Compare
These verify that the `type` argument passed to `SelectInterceptors` is equal to the type of the proxy's target. (There are also tests for `IInvocation.TargetType` in order to ensure consistency between `IInvocation` and `IInterceptorSelector`.) One of these tests will fail: When a proxy is created using `Create- ClassProxy`, `SelectInterceptors` will receive a `type` representing `System.RuntimeType` instead of the target's type.
This is achieved by the following three measures:
1. Fix the incorrect XML documentation for the `type` parameter of
`IInterceptorSelector.SelectInterceptors`. This parameter receives
the target type, *not* the intercepted method's declaring type.
2. Add a new ctor to `MethodWithInvocationGenerator` that accepts an
additional parameter `getTargetTypeExpression` (next to `get-
TargetExpression`) to account for the fact that invocation classes
sometimes require a target object (e.g. `CompositionInvocation`)
while others require a target type (e.g. `InheritanceInvocation`).
The new parameter is optional; if `getTargetTypeExpression` is not
provided, the target type is derived from the target object.
3. Use the new constructor for class proxies. We do *not* want to
have the target type derived from the target object, because that
would make the dynamically generated proxy type visible. (For
class proxies, the target object and the proxy object are one and
the same). Instead we want the target type to be the proxied type.
jonorossi
left a comment
jonorossi
left a comment
There was a problem hiding this comment.
Looks great, thanks for getting that fixed.
2 participants
This resolves #74.