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

Add Type check to ExpressionBuildingEventArgs.Expression property. #354

Closed
dotnetjunkie opened this Issue Dec 18, 2016 · 0 comments

Comments

1 participant
@dotnetjunkie
Collaborator

dotnetjunkie commented Dec 18, 2016

The ExpressionBuilding event can be used to intercept the creation of the Expression tree before caching gets applied by the Registration instance. The Expression built up by Simple Injector can be replaced by a new expression.

Since the Registration instance works primarily with implementation types, the replaced expression should return the exact same type (or a sub type). It should not return a type that inherits from the service type, but not from the implementation type.

Although this might seem to work at first, it will break when properties, initializers or other expression interceptors (other ExpressionBuilding events) get applied.

Unfortunately however, the ExpressionBuildingEventArgs.Expression property setter, does not check the correctness of the supplied Expression at all. It should do so, as follows:

    set
    {
        if (!this.KnownImplementationType.IsAssignableFrom(value.Type))
        {
           // TODO: Add text that explains that ExpressionBuilt might be the way to go for the user
           throw new Exception("The known implementation type should be assignable from the " +
               "type of the expression, but {0} is not assignable from {1}. "");
        }
    }

Problem of course is (as always) that users might actually depend on this behavior, since it works in simple cases. So this is a breaking change.

@dotnetjunkie dotnetjunkie added this to the v4 milestone Dec 18, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment