PropertyMappingValidationConvention throws for explicit interface implementation

[Sohra]

Tested on Full .NET, in a unit test project, with EntityFramework.Commands v7.0.0-rc2-16332.

When calling EnsureCreated() I get (remainder of stack trace omitted):

System.InvalidOperationException was unhandled
  HResult=-2146233079
  Message=The property 'wired.net.MP4Amp.Models.IPlaylistEntry.MediaFile' on entity type 'wired.net.MP4Amp.Models.PlaylistEntry<wired.net.MP4Amp.Models.MediaFile>' has not been added to the model or ignored.
  Source=EntityFramework.Core
  StackTrace:
       at Microsoft.Data.Entity.Metadata.Conventions.Internal.PropertyMappingValidationConvention.Apply(InternalModelBuilder modelBuilder)
       at Microsoft.Data.Entity.Metadata.Conventions.Internal.ConventionDispatcher.OnModelBuilt(InternalModelBuilder modelBuilder)
       at Microsoft.Data.Entity.Metadata.Internal.InternalModelBuilder.Validate()
       at Microsoft.Data.Entity.ModelBuilder.Validate()
...

Relevent code snippets:

        protected override void OnModelCreating(ModelBuilder _builder)
        {
            var mediaFile = _builder.Entity<MediaFile>();
            mediaFile.ToTable("MediaFiles");
            mediaFile.HasKey(e => e.Uri);
            mediaFile.HasIndex(e => e.Uri);
            mediaFile.Property(e => e.Title).HasMaxLength(256).IsRequired();
            mediaFile.Property(e => e.Album).HasMaxLength(256);  //Allow this to be unspecified
            mediaFile.Property(e => e.Artist).HasMaxLength(256).IsRequired();
            mediaFile.Property(e => e.Number).IsRequired();
            mediaFile.Property(e => e.Year).IsRequired();
            mediaFile.Property(e => e.Genre).HasMaxLength(256).IsRequired();
            mediaFile.Property(e => e.Rating).IsRequired();
            mediaFile.Property(e => e.Uri).HasMaxLength(256).IsRequired();
            mediaFile.Property(e => e.DurationInMilliseconds).IsRequired();

            /*
            var pseudoMediaFile = _builder.Entity<CPseudoMediaFile>();
            pseudoMediaFile.ToTable("PseudoMediaFiles");
            //NOTE: Now that TPH is supported in the prereleases, this no longer works, nor does HasAlternateKey
            //      Attempt to disable it by setting base type to null
            pseudoMediaFile.HasBaseType((string)null);
            pseudoMediaFile.HasKey(e => new { e.Uri, e.Number });
            pseudoMediaFile.HasIndex(e => e.Uri);
            pseudoMediaFile.Property(e => e.Title).HasMaxLength(256).IsRequired();
            pseudoMediaFile.Property(e => e.Album).HasMaxLength(256);  //Allow this to be unspecified
            pseudoMediaFile.Property(e => e.Artist).HasMaxLength(256).IsRequired();
            pseudoMediaFile.Property(e => e.Number).IsRequired();
            pseudoMediaFile.Property(e => e.Year).IsRequired();
            pseudoMediaFile.Property(e => e.Genre).HasMaxLength(256).IsRequired();
            pseudoMediaFile.Property(e => e.Rating).IsRequired();
            pseudoMediaFile.Property(e => e.Uri).HasMaxLength(256).IsRequired();
            pseudoMediaFile.Property(e => e.DurationInMilliseconds).IsRequired();
            //Unique to Pseudo
            pseudoMediaFile.Property(e => e.TrackNumber).IsRequired();
            pseudoMediaFile.Property(e => e.OffsetMilliseconds).IsRequired();
            */

            var mediaFilePlaylistEntry = _builder.Entity<PlaylistEntry<MediaFile>>();
            mediaFilePlaylistEntry.ToTable("MediaFileEntries");
            //Workaround for EF not supporting Entity as a Key
            //Suggestion here: http://data.uservoice.com/forums/72025-entity-framework-feature-suggestions/suggestions/8102202-support-another-entity-as-a-key
            //If attempt to set Entity as Key, SaveChanges throws: The 'MediaFile' on entity type 'CPlaylistEntry<CMediaFile>' does not have a value set and no value generator is available for properties of type 'CMediaFile'. Either set a value for the property before adding the entity or configure a value generator for properties of type 'CMediaFile'.
            //Although it is actually set, its value is an object (IMediaFile) and not a basic value type that I think is required
            //mediaFilePlaylistEntry.Key(e => e.MediaFile);
            //Trying to setup key as a nested property throws: The properties expression 'e => e.MediaFile.Uri' is not valid. The expression should represent a property access: 't => t.MyProperty'. When specifying multiple properties use an anonymous type: 't => new { t.MyProperty1, t.MyProperty2 }'.
            //mediaFilePlaylistEntry.Key(e => e.MediaFile.Uri);
            //Using a Shadow Property so as not to pollute our CLR type . . . (set this up in the Foreign Key below all in one go, it get's the data type)
            //Explicit declare the shadow state property since the foreign key below throws saying Uri can't exist on the type as its not marked as shadow state
            mediaFilePlaylistEntry.Property<string>("Uri");
            //Declare this as the key for the entity
            mediaFilePlaylistEntry.HasKey("Uri");
            //Should be tags collection, but EF7 can't handle complex types yet, so TagsCsv introduced to do the appropriate serialisation and deserialisation
            //Could make this a shadow property, but it means I'll have to override SaveChanges, DetectChanges to pick up any modified, and recalculate TagsCsv
            //each commit.  Might be easier to just leave this property in place in the meantime.
            mediaFilePlaylistEntry.Property(e => e.TagsCsv).IsRequired();
            //Workaround for EF not supporting Entity as a Key
            //mediaFilePlaylistEntry.Reference(e => e.MediaFile).InverseReference()
            mediaFilePlaylistEntry.HasOne(e => e.MediaFile)
                                  .WithOne()
                                  .HasForeignKey(typeof(PlaylistEntry<MediaFile>), "Uri");  //Want this to be e.MediaFile.Uri, but it can't go a level down, so using a shadow state property instead
            //This will actually create Key shadow property if not already defined on the type
            //Means I don't need to define it earlier, but do I need to define it as a primary key?
            //or is that a given?  Or can I continue to declare the navigation property as primary key
            //as it should be?  There's also questions about writing this, and loading navigation property
            //when reading entity from the database.  A bunch of things need to be tested.  All this
            //to get rid of the Key property messing up my CLR type, and yet it's still a workaround since
            //EF7 can't use an entity as a key
            //mediaFilePlaylistEntry.Reference(e => e.MediaFile)
            //                      .InverseReference()
            //                      .ForeignKey(typeof(CPlaylistEntry<CMediaFile>), "Key");  //Want this to be e.MediaFile.Uri, so I can remove e.Key from the class
            //Unit test project ought to do it
            //Contrary to the comment on the ForeignKey(type, params string[] foreignKeyPropertyNames) about auto creating shadow, the above throws:
            //The property 'Uri' cannot exist on entity type 'wired.net.MP4Amp.Models.CPlaylistEntry<wired.net.MP4Amp.Models.CMediaFile>'
            //because the property is not marked as shadow state and no corresponding CLR property exists on the underlying type.

            /*
            var pseudoMediaFilePlaylistEntry = _builder.Entity<CPlaylistEntry<CPseudoMediaFile>>();
            pseudoMediaFilePlaylistEntry.ToTable("PseudoMediaFileEntries");
            //Workaround for EF not supporting Entity as a Key
            //pseudoMediaFilePlaylistEntry.Key(e => e.MediaFile);
            //pseudoMediaFilePlaylistEntry.Key(e => new { e.MediaFile.Uri, e.MediaFile.Number });
            //Explicit declare the shadow state properties as Foreign Key fails to implicitly do it like the comment says it should
            pseudoMediaFilePlaylistEntry.Property<string>("Uri");
            pseudoMediaFilePlaylistEntry.Property<uint>("Chapter");
            pseudoMediaFilePlaylistEntry.HasKey("Uri", "Chapter");
            //Should be tags collection, but EF7 can't handle complex types yet, so TagsCsv introduced to do the appropriate serialisation and deserialisation
            pseudoMediaFilePlaylistEntry.Property(e => e.TagsCsv).IsRequired();
            //Workaround for EF not supporting Entity as a Key
            //pseudoMediaFilePlaylistEntry.Reference(e => e.MediaFile).InverseReference();
            pseudoMediaFilePlaylistEntry.HasOne(e => e.MediaFile)
                                        .WithOne()
                                        .HasForeignKey(typeof(CPlaylistEntry<CPseudoMediaFile>), "Uri", "Chapter");
            */
        }

    public enum Rating
    {
        Unrated = 0,
        Dislike = 1,
        Boring = 2,
        Average = 3,
        Like = 4,
        Love = 5
    }

    public interface IMediaFile// : IEquatable<IMediaFile>
    {
        string DisplayName { get; }
        string Title { get; set; }
        string Artist { get; set; }
        string Album { get; set; }
        uint Number { get; }
        uint Year { get; }
        string Genre { get; }
        Rating Rating { get; set; }
        string Uri { get; }
        uint DurationInMilliseconds { get; }
        uint DurationInSeconds { get; }
    }

    [DebuggerDisplay("{Title} {Artist}")]
    public class MediaFile : IMediaFile//, IEquatable<MediaFile>
    {
        //Used to force a unique constraint on db4o
        string mKey;
        string mUrl;

        /// <summary>
        /// Required by Entity Framework, and now used by unit test project too, rather than the other, now internal, constructor.
        /// Not intended for any other use.
        /// </summary>
        public MediaFile()
        {
        }

        internal MediaFile(string _url, uint _durationMs,
                            string _title, string _artist,
                            string _album, uint _number, uint _year, string _genre,
                            Rating _rating)
            : this(_url, _url, _durationMs,
                   _title, _artist,
                   _album, _number, _year, _genre, _rating)
        {
        }

        protected MediaFile(string _key, string _url, uint _durationMs,
                             string _title, string _artist,
                             string _album, uint _number, uint _year, string _genre,
                             Rating _rating)
        {
            mKey = _key;
            mUrl = _url;
            DurationInMilliseconds = _durationMs;

            Title = _title;
            Artist = _artist;
            Album = _album;
            Number = _number;
            Year = _year;
            Genre = _genre;
            Rating = _rating;
        }

        /// <summary>
        /// 
        /// </summary>
        /// <remarks>WPF XAML binding of an IPlaylistEntry with "MediaFile.DisplayName" will fail to display anything, despite IPlaylistEntry being typed as IMediaFile
        /// for some reason the concrete implementation needs to have the property ):
        /// TODO: Figure out what's going on with XAML here, and see if WinRT projects have the same issue
        /// UPDATED: Now made public rather than explicitly implementing the interface, so this issue goes away.
        /// Making this class more friendly with EF, so that it can be constructed and hydrated through properties,
        /// instead of relying on constructor.</remarks>
        public string DisplayName
            => $"{Artist} - {Title}";
        //return string.Format("{0} - {1}{2}", Artist, Title, mRating != Rating.Unrated ? String.Format(" ({0} stars)", (int)mRating) : string.Empty);

        public string Title { get; set; }
        public string Artist { get; set; }
        public string Album { get; set; }
        public virtual uint Number { get; set; }
        public uint Year { get; set; }
        public string Genre { get; set; }
        public Rating Rating { get; set; }
        public string Uri
        {
            get { return mUrl; }
            set { mUrl = value; }
        }
        public uint DurationInMilliseconds { get; set; }

        public uint DurationInSeconds => DurationInMilliseconds / 1000;

    }

    public interface IPlaylistEntry
    {
        IMediaFile MediaFile { get; }

        bool Tag(string[] _tags);
        bool SetTags(string[] _tags);
        IReadOnlyCollection<string> Tags { get; }
        IDictionary<string, IList<string>> TagHierarchy { get; }

        void Play(uint _startOffsetMs);
        void Seek(float _fraction);
        void Seek(int _seconds);
    }

    [System.Diagnostics.DebuggerDisplay("{((wired.net.MP4Amp.Models.IMediaFile)mMediaFile).DisplayName}")]
    public class PlaylistEntry<T> : IPlaylistEntry where T : IMediaFile
    {
        T mMediaFile;
        ISet<string> mTags;

        static readonly IDictionary<string, Rating> sRatingTags
            = new Dictionary<string, Rating>()
            {
                {"1star", Rating.Dislike},
                {"2star", Rating.Boring},
                {"3star", Rating.Average},
                {"4star", Rating.Like},
                {"5star", Rating.Love}
            };


        /// <summary>
        /// Required by Entity Framework.
        /// Not intended for any other use.
        /// </summary>
        public PlaylistEntry()
        {
            //The whole purpose of Func<ISet<string>> _createTagsSet
            //was to allow injection of a set implementation that was supported
            //by the db4o Transparent Persistence mechanism, which means nothing to EF.
            //Hence we can safely default to an ordinary set if this constructor is used.
            mTags = new SortedSet<string>();
        }

        /// <summary>
        /// 
        /// </summary>
        /// <param name="_mediaFile"></param>
        /// <param name="_createSet">Factory method intended to dependency inject a set to store tags in</param>
        public PlaylistEntry(T _mediaFile, Func<ISet<string>> _createTagsSet)
        {
            mMediaFile = _mediaFile;
            mTags = _createTagsSet();
        }

        public T MediaFile
        {
            get { return mMediaFile; }
            set { mMediaFile = value; }
        }

        IMediaFile IPlaylistEntry.MediaFile => mMediaFile;

        public bool Tag(string[] _tags)
        {
            foreach (var tag in _tags)
                if (!mTags.Contains(tag, StringComparer.OrdinalIgnoreCase))
                    mTags.Add(tag);
            return true;
        }

        public bool SetTags(string[] _tags)
        {
            mTags.Clear();
            mTags.UnionWith(_tags.Except(sRatingTags.Keys));
            var ratingTags = _tags.Intersect(sRatingTags.Keys);
            if (ratingTags.Any())
            {
                //if (ratingTags.Count() > 1)
                //{
                //    var logger = MetroLog.LogManagerFactory.DefaultLogManager.GetLogger<IPlaylistEntry>();
                //    logger.Warn($"{MediaFile.DisplayName} contains more than one rating");
                //}
                MediaFile.Rating = sRatingTags[ratingTags.First()];
            }
            return true;
        }

        /// <summary>
        /// Required for EF only, once they support ComplexTypes or some kind of persistance mapper, remove this.
        /// </summary>
        public string TagsCsv
        {
            get { return string.Join(",", mTags); }
            //This required for EF rehydration only
            set { SetTags(value.Split(new[] { ',' }, StringSplitOptions.RemoveEmptyEntries)); }
        }

        IReadOnlyCollection<string> IPlaylistEntry.Tags
            => new ReadOnlyCollection<string>(mTags.ToList());

        public IDictionary<string, IList<string>> TagHierarchy
        {
            get
            {
                //Works fine when referenced by WinForms project, but WPF generates invalid IL - despite this all
                //being in a totally separate assembly referenced by both WinForms and WPF projects.
                //var tagTaxonomy = Misc.GetTagTaxonomy();
                //var tagHierarchy = new Dictionary<string, IList<string>>();
                //foreach (var key in tagTaxonomy.Keys)
                //    tagHierarchy.Add(key, tagTaxonomy[key].Where(t => mTags.Contains(t, StringComparer.OrdinalIgnoreCase)).ToList());

                //var miscTags = mTags.Where(t => !tagHierarchy.Values.SelectMany(v => v)
                //                                                    .Contains(t, StringComparer.OrdinalIgnoreCase));
                //if (miscTags.Any())
                //    tagHierarchy.Add("Misc", miscTags.ToList());

                //return tagHierarchy;

                //Stub this
                IDictionary<string, IList<string>> tagTaxonomy = new Dictionary<string, IList<string>>();
                var tagHierarchy = new Dictionary<string, IList<string>>();
                foreach (var key in tagTaxonomy.Keys)
                    tagHierarchy.Add(key, tagTaxonomy[key].Where(t => mTags.Contains(t, StringComparer.OrdinalIgnoreCase)).ToList());

                //The single-line calculation of miscTags caused the IL corruption when referenced by WPF project
                //To see, reinstate the above, compile WinForms & WPF, then open their respective MP4Amp.Platform.dll
                //in ILSpy and view the content of this property.  In WPF, ILSpy will throw an exception.
                //Reinstate this code and rebuild each and compare (in particular the WinForms, since you have two copies then)
                //Anyway, splitting into two statements works around it and ensures WPF does not hit the InvalidProgramException
                var collapsedTagHierarchy = tagHierarchy.Values.SelectMany(v => v);
                var miscTags = mTags.Where(t => !collapsedTagHierarchy.Contains(t, StringComparer.OrdinalIgnoreCase));
                if (miscTags.Any())
                    tagHierarchy.Add("Misc", miscTags.ToList());

                return tagHierarchy;
            }
        }

        #region Stubbed
        void IPlaylistEntry.Play(uint _startOffsetMs) { }
        void IPlaylistEntry.Seek(float _fraction) { }
        void IPlaylistEntry.Seek(int _seconds) { }

        #endregion

    }

[smitpatel]

IMediaFile IPlaylistEntry.MediaFile => mMediaFile; is the cause of the exception.
Whenever the CLR type of entity type is available, conventions will try to include all the properties with getter to be included in the model unless they are ignored. Above property is not added to the model. The property configured with relationship is MediaFile. IPlaylistEntry.MediaFile is different property. Hence exception is thrown. Since return type of IPlaylistEntry.MediaFile is IMediaFile it cannot be added as navigation (entity's clr type cannot be interface type) hence it is showing that property is not added to the model.
If you can remove explicit interface implementation then above issue will go away. If you need it (due to generic class), ignore those properties using [NotMapped] attribute or fluent API.

[Sohra]

Thanks for the diagnosis Smit, must be something new since Beta6.
How can I ignore this property using the fluent API?
mediaFilePlaylistEntry.Ignore(e => (e as IPlaylistEntry).MediaFile); throws an ArgumentException
mediaFilePlaylistEntry.Ignore(e => ((IPlaylistEntry)e).MediaFile); doesn't throw until model validation, the exact same message as originally posted.

[smitpatel]

@Sohra - I was discussing the same thing with my colleague in last hour. Turns out that you cannot exclude explicit interface implementation using fluent API. You can use [NotMapped] attribute over properties, it worked for me.
As for the changes, it is due to #2588. Essentially if use has a property on clr type then the expectation would be to have it in the model. Previously we silently ignored it but we changed to notify use about such properties so that use can add/ignore them appropriately.

[smitpatel]

@divega , @rowanmiller
Our fluent api is not able to point to exact PropertyInfo of explicit interface. Essentially the property info returned from propertyExpression.GetPropertyAccess() method gives the propertyinfo defined on the interface. While the GetRuntimeProperties() gives the propertyinfo defined on the concrete type. Since they are different (with different names), you cannot ignore the correct property for validation. [NotMapped] works correctly because it uses the GetRuntimeProperties().
On higher level the question would be, Do we want to include the explicit interface implementation in the model? If yes then we have to modify the implementation of ignore so that we are able to ignore interface implementation using fluent API. If we don't want to include interface implementation then we should exclude it from validation convention.

[rowanmiller]

To me it would seem our rules remain the simplest if we still include it, but then (as you said) we need to support ignoring it.

[rowanmiller]

Triage decision - we should map it and fix the bug in Ignore

[Sohra]

@smitpatel just got around to testing this with rc2-16447, working great. Thanks!