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

TypeNameCache does not handle type duplicates #539

Closed
gpomykala opened this issue Jul 30, 2018 · 7 comments
Closed

TypeNameCache does not handle type duplicates #539

gpomykala opened this issue Jul 30, 2018 · 7 comments
Milestone

Comments

@gpomykala
Copy link

Scenario:

  1. assembly A defines type X
  2. assembly B defines type X (same FullName)
  3. both A and B are being registered in a bootstrapper and added to AssemblySource

Result: boostrapping fails due to type duplicates being added to TypeNameCache

Although it may not be the best practice to use same types from multiple assemblies within an app it does happen and TypeNameCache should not rely on Type.FullName only as it is not uniquely identtifying information (FullName + assembly is). It would not be too hard to define aggregated dictionary key consisting of assembly name + type name

AssemblySource.Instance.CollectionChanged += (s, e) => { switch (e.Action) { case NotifyCollectionChangedAction.Add: e.NewItems.OfType<Assembly>() .SelectMany(a => ExtractTypes(a)) .Apply(t => TypeNameCache.Add(t.FullName, t)); break; case NotifyCollectionChangedAction.Remove: case NotifyCollectionChangedAction.Replace: case NotifyCollectionChangedAction.Reset: TypeNameCache.Clear(); AssemblySource.Instance .SelectMany(a => ExtractTypes(a)) .Apply(t => TypeNameCache.Add(t.FullName, t)); break; } };

@popcatalin81
Copy link

@gpomykala there is an Type.AssemblyQualifiedName

However, this would create conflicts, when looking up a view for PersonViewModel which of the following should be chosen?

AppNamespace.Views.PersonView, Lib1, Version=1.0.0.0, Culture=neutral, PublikeyToken=5c5619...
AppNamespace.Views.PersonView, Lib2, Version=1.0.0.0, Culture=neutral, PublikeyToken=13cefg...

?

@gpomykala
Copy link
Author

I am evaluating an assembly aware cache along with an update LocateTypeForModelType

` static IDictionary<Assembly, IDictionary<string, Type>> _assemblyScopedTypeNameCaches = new Dictionary<Assembly, IDictionary<string, Type>>();

    protected override void Configure()
    {
        base.Configure();

        var assemblies = SelectAssemblies();
        assemblies.Apply(a =>
                                   {
                                       _assemblyScopedTypeNameCaches.TryAdd(a, new Dictionary<string, Type>());

                                       var assemblyScopedCache = _assemblyScopedTypeNameCaches[a];

                                       AssemblySourceCache.ExtractTypes(a).Apply(t => assemblyScopedCache.TryAdd(t.FullName, t));
                                   });

        ViewLocator.LocateTypeForModelType = (modelType, displayLocation, context) => {
                                                 var viewTypeName = modelType.FullName;
                                                 var typeAssembly = modelType.Assembly;

                                                 if (View.InDesignMode)
                                                 {
                                                     viewTypeName = ViewLocator.ModifyModelTypeAtDesignTime(viewTypeName);
                                                 }

                                                 viewTypeName = viewTypeName.Substring(
                                                                                       0,
                                                                                       viewTypeName.IndexOf('`') < 0
                                                                                           ? viewTypeName.Length
                                                                                           : viewTypeName.IndexOf('`')
                                                                                      );

                                                 var viewTypeList = ViewLocator.TransformName(viewTypeName, context);
                                                 var viewType = FindTypeByNames(typeAssembly, viewTypeList);

                                                 if (viewType == null)
                                                 {
                                                     Debug.WriteLine("View not found. Searched: {0}.", string.Join(", ", viewTypeList.ToArray()));
                                                 }

                                                 return viewType;
                                             };
    }

    private static Type FindTypeByNames(Assembly typeAssembly, IEnumerable<string> names)
    {

        if (names == null || typeAssembly == null)
        {
            return null;
        }

        var cache = _assemblyScopedTypeNameCaches[typeAssembly];

        var type = names.Select(n => cache.GetValueOrDefault(n)).FirstOrDefault(t => t != null);
        return type;
    }`

@nigel-sampson
Copy link
Contributor

I have some concerns with adding this capability in much the same way @popcatalin81 does.

All of the conventions in CM work off the FullName, if you have a view with the full name of Project.Feature.Views.SampleView and there are two different classes with full name Project.Feature.ViewModels.SampleViewModel which one should it chose to use in a deterministic manner.

@gpomykala
Copy link
Author

@nigel-sampson i recognize that the scope of changing it properly for all scenarios supported by the framework is much wider than an example I submitted above.

Answering your question: ideally it should return Project.Feature.ViewModels.SampleViewModel from an assembly Project.Feature.Views.SampleView was defined in.

Just my 2 cents...

@popcatalin81
Copy link

popcatalin81 commented Jul 31, 2018

@gpomykala Yes that would work from an application developer perspective.

If you're a library author, building a componentized UI library you would want it the other way around, you'd want users to be able to selectively override UI components imported from your library in their applications.

The problem is there's no universal best choice here, so there are only two universal solutions in my opinion:

  1. Do not allow duplicate Full names
  2. Allow duplicates and build tie breaker mechanisms everywhere in the framework, so the user can choose.

Option 2) Is allot more complicated to support and implement and therefore really needs a compelling use case to make it worthwhile. (One such use case would be reusable View libraries, but this pattern is not really common, even thouhg I've encountered and was pretty neat)

@nigel-sampson
Copy link
Contributor

This is certainly an extreme example, if nothing else I'll probably do an audit to make sure the assembly cache is extensible in the right way such that if someone wants to build a "tie breaking cache" it could be wired in correctly.

@nigel-sampson nigel-sampson added this to the v4.0.0 milestone Nov 28, 2018
@nigel-sampson
Copy link
Contributor

Pushed change to make AssemblySource extensible ad unlock this scenario if needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants