Skip to content
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

Provide design time validation for LifetimeManager derivatives #90

Closed
ENikS opened this issue Jan 18, 2019 · 5 comments
Closed

Provide design time validation for LifetimeManager derivatives #90

ENikS opened this issue Jan 18, 2019 · 5 comments
Labels
New Feature 🚀 New feature request Requires Investigation 👮 Requires further verification
Milestone

Comments

@ENikS
Copy link
Contributor

ENikS commented Jan 18, 2019

Description

Currently RegisterType, RegisterInstance, etc. will accept any lifetime manager with no regard if it is compatible with corresponding registration type.

Problem

It is possible to register instance with TransientLifetimeManager, for example.

Solution

Add following interfaces to specify compatible registration type for the lifetime managers:

public interface IFactoryLifetimeManager { }

public interface IInstanceLifetimeManager { }

public interface ITypeLifetimeManager { }

Change IUnityContainer interface to support these interfaces:

public interface IUnityContainer : IDisposable
{
... RegisterType(..., ITypeLifetimeManager lifetime, . . .);

... RegisterInstance(..., IInstanceLifetimeManager lifetime);

... RegisterFactory(..., IFactoryLifetimeManager lifetime);

. . .
}
@ENikS ENikS added the New Feature 🚀 New feature request label Jan 18, 2019
@ENikS ENikS added this to the 4.0.0 milestone Jan 18, 2019
@ENikS ENikS closed this as completed in 23b26c3 Jan 18, 2019
@GSPP
Copy link

GSPP commented Apr 9, 2019

When upgrading I hit issues with that change that forced me to cast between LifetimeManager and ITypeLifetimeManager in some places. This is because those two types are unrelated from an inheritance standpoint. It is a bit weird to have to declare variables of a an interface type that is empty. On the face of it such variables support no operations. They require casting to do anything with them.

I think it would be better to have a hierarchy like abstract class TypeLifetimeManager : LifetimeManager but that likely would be a big compatibility issue.

An alternative would be to make this into runtime validation. I think this change should be made. RegisterType would do if (!(arg is ITypeLifetimeManager)) throw ...;. This would provide the same safety, a little bit less of static checking and a cleaner type hierarchy.

It could also be done by adding an enum LifetimeManagerSupportedOperations { Type, Instance, Factory } and adding public virtual LifetimeManagerSupportedOperations? SupportedOperations => null; to LifetimeManager. This property would then be used to perform fully backwards compatible validation. If it returns null there is no validation and user code keeps working.

What do you think?

@ENikS
Copy link
Contributor Author

ENikS commented Apr 9, 2019

Any runtime validation is slower than current implementation.

Some lifetime managers support more than one type of registrations, to do this with enums would require doing bit masks and multiple if(...) checks, and etc.

The goal is to make Unity as fast as possible and casting to interface at design time does not introduce any overhead.

@GSPP
Copy link

GSPP commented Apr 13, 2019

Registration normally is a one-time operation and a single cast takes very little time compared to anything else. Runtime checking seems feasible.

I see your point why this enum idea would not work. I think it would be best to revert the change. It breaks the type system for too little gain.

@ENikS
Copy link
Contributor Author

ENikS commented Apr 13, 2019

I could add an interface ILifetimeManager with all other interfaces deriving from it. It should create the lowest denominator to 'cover it all' cases.

If you could point me to the case where type system is broken I could fix it as well.

@ENikS ENikS reopened this Apr 13, 2019
@ENikS ENikS modified the milestones: 4.0.0, 4.1.4 Apr 13, 2019
@ENikS ENikS added the Requires Investigation 👮 Requires further verification label Apr 13, 2019
@GSPP
Copy link

GSPP commented Apr 15, 2019

I had code similar to the following:

LifetimeManager CreateLifetimeManager() => new ScopedLifetimeManager(); //a custom type

public static LifetimeManager DatabaseConnectionLifetimeManager = CreateLifetimeManager();

...

container.Register<DatabaseConnection>(DatabaseConnectionLifetimeManager);

And then I used DatabaseConnectionLifetimeManager.SetValue(...) to set the value in certain places. I am using a custom scoped lifetime manager where you can say using (new LifetimeScope()) { ... } and get fresh instances in that scope. Sometimes but not always I want child scopes to share the database connection. I then use SetValue to copy over the value.

This code is broken by the type system changes. I need DatabaseConnectionLifetimeManager to be of type LifetimeManager so that I can call SetValue. But Register requires one of the marker interface types which has no members.

This is what I mean when I say the type system is broken.

I solved this issue for me simply by casting. So it's not too big of a deal but I wanted to write this up to give you an idea of what I was talking about. Feel free to close this if you plan no further action.

@ENikS ENikS closed this as completed Apr 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
New Feature 🚀 New feature request Requires Investigation 👮 Requires further verification
Projects
None yet
Development

No branches or pull requests

2 participants