Do not merge: ModelContainer memory leak review

src/Microsoft.AspNet.OData.Shared/EnableQueryAttribute.cs

@@ -670,6 +670,9 @@ internal static object SingleOrDefault(
             var enumerator = queryable.GetEnumerator();
             try
             {
+                // ATTEMPT 1: throws here
+                // Exception: 
+                // NotSupportedException: Unable to create a constant value of type 'Microsoft.OData.Edm.EdmModel'. Only primitive types or enumeration types are supported in this context.
                 var result = enumerator.MoveNext() ? enumerator.Current : null;
 
                 if (enumerator.MoveNext())

src/Microsoft.AspNet.OData.Shared/Formatter/ODataOutputFormatterHelper.cs

@@ -247,6 +247,14 @@ private static ODataSerializer GetSerializer(Type type, object value, IWebApiReq
             IEdmObject edmObject = value as IEdmObject;
             if (edmObject != null)
             {
+                /*
+                // Part of ATTEMPT 3.
+                SelectExpandWrapper selectExpandWrapper = edmObject as SelectExpandWrapper;
+                if (selectExpandWrapper != null)
+                {
+                    selectExpandWrapper.Model = model; // Need to modify the method parameters to pass in model from the caller (WriteToStream method)
+                } 
+                */
                 IEdmTypeReference edmType = edmObject.GetEdmType();
                 if (edmType == null)
                 {

src/Microsoft.AspNet.OData.Shared/Formatter/Serialization/ODataSerializerContext.cs

@@ -5,6 +5,7 @@
 using System.Collections.Generic;
 using Microsoft.AspNet.OData.Common;
 using Microsoft.AspNet.OData.Interfaces;
+using Microsoft.AspNet.OData.Query.Expressions;
 using Microsoft.OData.Edm;
 using ODataPath = Microsoft.AspNet.OData.Routing.ODataPath;
 using SelectExpandClause = Microsoft.OData.UriParser.SelectExpandClause;
@@ -154,6 +155,29 @@ internal IEdmTypeReference GetEdmType(object instance, Type type)
             IEdmObject edmObject = instance as IEdmObject;
             if (edmObject != null)
             {
+                /*
+                // ATTEMPT 3: For this attempt, the Model remains null in the SelectExpandWrapper until
+                // we are writing to the response soon. At this point, we still have access to the Edm model
+                // corresponding to the request. Unfortunately not all code paths seem to have access to the model,
+                // so those code paths will throw.
+                // 
+                // This is not an ideal approach in any case; we should try to set the Model close to when the SelectExpandWrapper
+                // is instantiated.
+                //
+                // Sample unit tests that fail: ODataQueryOptions_SetToApplied, QueryComposition_WorkAsExpect_ForOptionalDollarSignPrefixForSystemQuery
+                // Sample E2E case that fails: QueryAnEntryAndIncludeTheRelatedEntriesForAGivenNavigationPropertyInline
+                SelectExpandWrapper selectExpandWrapper = edmObject as SelectExpandWrapper;
+                if (selectExpandWrapper != null)
+                {
+                    if (Model == null)
+                    {
+                        throw Error.InvalidOperation(SRResources.RequestMustHaveModel);
+                    }
+
+                    selectExpandWrapper.Model = Model;
+                }
+                */
+
                 edmType = edmObject.GetEdmType();
                 if (edmType == null)
                 {

src/Microsoft.AspNet.OData.Shared/IEdmStructuredObject.cs

@@ -8,6 +8,9 @@ namespace Microsoft.AspNet.OData
 {
     /// <summary>
     /// Represents an instance of an <see cref="IEdmStructuredType"/>.
+    /// ATTEMPT 4: attempt to expose an IEdmModel property. Lots of code churn due to inheritance and comes across
+    /// some diamond inheritance problems because some interfaces or classes already define IEdmModel property with
+    /// something conflicting.
     /// </summary>
     public interface IEdmStructuredObject : IEdmObject
     {

src/Microsoft.AspNet.OData.Shared/Query/Expressions/SelectExpandBinder.cs

@@ -318,6 +318,19 @@ private Expression ProjectElement(Expression source, SelectExpandClause selectEx
             wrapperProperty = wrapperType.GetProperty("ModelID");
             wrapperTypeMemberAssignments.Add(Expression.Bind(wrapperProperty, Expression.Constant(_modelID)));
 
+            /*
+            // ATTEMPT 1: Attempt to bind the a reference to the model in a similar way to the other properties that exist
+            // in SelectExpandWrapper. This passes the unit test suite but fails 26 cases in E2E suite. This throws
+            // in ODataQueryOptions.cs : 686 (TruncatedCollection<T> truncatedCollection = new TruncatedCollection<T>(queryable, limit);)
+            // and in EnableQueryAttribute.cs : 673 (var result = enumerator.MoveNext() ? enumerator.Current : null;)
+            // Exception: 
+            // NotSupportedException: Unable to create a constant value of type 'Microsoft.OData.Edm.EdmModel'. Only primitive types or enumeration types are supported in this context.
+            //
+            // Sample E2E test that fails: NestedTopSkipOrderByInDollarExpandWorksWithEF
+            wrapperProperty = wrapperType.GetProperty("Model");
+            wrapperTypeMemberAssignments.Add(Expression.Bind(wrapperProperty, Expression.Constant(_model)));
+            */
+
             // Initialize property 'Instance' on the wrapper class
             // source => new Wrapper { Instance = element }
             wrapperProperty = wrapperType.GetProperty("Instance");

src/Microsoft.AspNet.OData.Shared/Query/Expressions/SelectExpandWrapper.cs

@@ -30,6 +30,11 @@ internal abstract class SelectExpandWrapper : IEdmEntityObject, ISelectExpandWra
         /// </summary>
         public string ModelID { get; set; }
 
+        /// <summary>
+        /// Gets or sets the Edm model.
+        /// </summary>
+        public IEdmModel Model { get; set; }
+
         /// <inheritdoc />
         public object UntypedInstance { get; set; }
 
@@ -42,6 +47,7 @@ internal abstract class SelectExpandWrapper : IEdmEntityObject, ISelectExpandWra
         public IEdmTypeReference GetEdmType()
         {
             IEdmModel model = GetModel();
+            // IEdmModel model = Model;
             Type elementType = GetElementType();
             return model.GetEdmTypeReference(elementType);
         }
@@ -129,6 +135,7 @@ private IEdmModel GetModel()
             Contract.Assert(ModelID != null);
 
             return ModelContainer.GetModel(ModelID);
+            // return Model;
         }
     }
 }

src/Microsoft.AspNet.OData.Shared/Query/Expressions/SelectExpandWrapperConverter.cs

@@ -40,6 +40,8 @@ public override void WriteJson(JsonWriter writer, object value, JsonSerializer s
             ISelectExpandWrapper selectExpandWrapper = value as ISelectExpandWrapper;
             if (selectExpandWrapper != null)
             {
+                // ATTEMPT 3: sample unit test throws here: ODataQueryOptions_SetToApplied because
+                // SelectExpandWrapper.Model is null.
                 serializer.Serialize(writer, selectExpandWrapper.ToDictionary(_mapperProvider));
             }
         }

src/Microsoft.AspNet.OData.Shared/Query/ODataQueryOptions.cs

@@ -682,6 +682,11 @@ internal static IQueryable LimitResults(IQueryable queryable, int limit, out boo
         [SuppressMessage("Microsoft.Design", "CA1021:AvoidOutParameters", Justification = "Not intended for public use, only public to enable invocation without security issues.")]
         public static IQueryable<T> LimitResults<T>(IQueryable<T> queryable, int limit, out bool resultsLimited)
         {
+            // ATTEMPT 1: throws here
+            // Exception: 
+            // NotSupportedException: Unable to create a constant value of type 'Microsoft.OData.Edm.EdmModel'. Only primitive types or enumeration types are supported in this context.
+            // 
+            // Sample E2E test that fails: NestedTopSkipOrderByInDollarExpandWorksWithEF
             TruncatedCollection<T> truncatedCollection = new TruncatedCollection<T>(queryable, limit);
             resultsLimited = truncatedCollection.IsTruncated;
             return truncatedCollection.AsQueryable();

src/Microsoft.AspNet.OData.Shared/Query/SelectExpandQueryOption.cs

@@ -189,7 +189,35 @@ public IQueryable ApplyTo(IQueryable queryable, ODataQuerySettings settings)
 
             ODataQuerySettings updatedSettings = Context.UpdateQuerySettings(settings, queryable);
 
-            return SelectExpandBinder.Bind(queryable, updatedSettings, this);
+            IQueryable result = SelectExpandBinder.Bind(queryable, updatedSettings, this);
+
+            /*
+            // ATTEMPT 2: Attempt to add the model to the SelectExpandWrapper once the object has been created. I didn't find any points
+            // where the SelectExpandWrapper is created and the Bind method is the closest I've traced to it; therefore I have to cast the object
+            // to SelectExpandWrapper first and then add the model. This approach doesn't work as there are some parts of the code later on that
+            // expect Expressions instead of SelectExpandWrapper. (Perhaps there's a way to cast it back? Just thought this recently, haven't tried it.)
+            // This approach is also a bit non-performant as it's converting back and forth between IQueryable and List, but it does certainly cast
+            // to SelectExpandWrapper and we are able to set the Model. There is also an issue later on in a test run where for some reason,
+            // all but one object in the IQueryable have the Model set and therefore throws at that point.
+            //
+            // Example expected type: System.Linq.EnumerableQuery<Microsoft.AspNet.OData.Query.Expressions.SelectExpandBinder.SelectAllAndExpand<Microsoft.AspNet.OData.Test.SelectExpandTestCustomer>>
+            // Resulting type with this change: System.Linq.EnumerableQuery<Microsoft.AspNet.OData.Query.Expressions.SelectExpandWrapper>
+            //
+            // Sample test case to debug that will hit this code path: SelectExpand_Works_WithExpandStar (unit test project)
+            var list = result.OfType<SelectExpandWrapper>().ToList(); // need to assert that all items are actually SelectExpandWrapper
+            foreach (var item in list)
+            {
+                SelectExpandWrapper selectExpandWrapper = item as SelectExpandWrapper;
+                if (selectExpandWrapper != null)
+                {
+                    selectExpandWrapper.Model = Context.Model;
+                }
+            }
+
+            return list.AsQueryable();
+            */
+
+            return result;
         }
 
         /// <summary>
@@ -215,7 +243,19 @@ public object ApplyTo(object entity, ODataQuerySettings settings)
 
             ODataQuerySettings updatedSettings = Context.UpdateQuerySettings(settings, query: null);
 
-            return SelectExpandBinder.Bind(entity, updatedSettings, this);
+            object result = SelectExpandBinder.Bind(entity, updatedSettings, this);
+
+            /*
+            // Part of ATTEMPT 2.
+            // Sample test case that will hit this code path: Levels_Works_WithMaxLevelInNestedExpand (unit test project)
+            SelectExpandWrapper selectExpandWrapper = result as SelectExpandWrapper;
+            if (selectExpandWrapper != null)
+            {
+                selectExpandWrapper.Model = Context.Model;
+            }
+            */
+
+            return result;
         }
 
         /// <summary>

src/Microsoft.AspNet.OData.Shared/ResourceContext.cs

@@ -60,6 +60,11 @@ private ResourceContext(ODataSerializerContext serializerContext, IEdmStructured
             EdmObject = edmObject;
         }
 
+        /// <summary>
+        /// Gets or sets the <see cref="IEdmStructuredObject"/>.
+        /// </summary>
+        private IEdmStructuredObject edmObject;
+
         /// <summary>
         /// Gets or sets the <see cref="ODataSerializerContext"/>.
         /// </summary>
@@ -88,6 +93,15 @@ public IEdmModel EdmModel
             set
             {
                 SerializerContext.Model = value;
+
+                /*
+                // Part of ATTEMPT 5.
+                SelectExpandWrapper selectExpandWrapper = this.edmObject as SelectExpandWrapper;
+                if (selectExpandWrapper != null)
+                {
+                    selectExpandWrapper.Model = this.EdmModel;
+                }
+                */
             }
         }
 
@@ -114,7 +128,39 @@ public IEdmNavigationSource NavigationSource
         /// <summary>
         /// Gets or sets the <see cref="IEdmStructuredObject"/> backing this instance.
         /// </summary>
-        public IEdmStructuredObject EdmObject { get; set; }
+        public IEdmStructuredObject EdmObject
+        {
+            get
+            {
+                return edmObject;
+            }
+            set
+            {
+                /*
+                // ATTEMPT 5: this is more or less like Konstantin's change:
+                // https://github.com/kosinsky/WebApi/commit/40020a1121a288fe64255ed7a785c25d010cd4b9#diff-5449bed3a006bfaa5c95f3959bf4cb96R574
+                // There don't seem to be any SelectExpand cases that go through this code path. For the test cases of SelectExpand,
+                // either ResourceContext is never instantiated (I checked this by putting debug points on all constructors of ResourceContext)
+                // or ResourceContext is instantiated but never passes SelectExpand to this set method and because of that, the Model remains null.
+                // Thus Konstantin's fix doesn't seem to be applicable to this code; this means that ATTEMPT 4 as this attempt is a shortcut for that attempt.
+                //
+                // Sample SelectExpand unit test that doesn't instantiate ResourceContext: SelectExpand_WithAllDollarCount_Works
+                // Sample SelectExpand unit test that does instantiate ResourceContext but doesn't pass SelectExpand to this setter: WriteObjectInline_Calls_CreateSelectExpandNode
+                SelectExpandWrapper selectExpandWrapper = value as SelectExpandWrapper;
+                if (selectExpandWrapper != null)
+                {
+                    selectExpandWrapper.Model = EdmModel;
+                    edmObject = selectExpandWrapper;
+                }
+                else
+                {
+                    edmObject = value;
+                }
+                */
+
+                edmObject = value;
+            }
+        }
 
         /// <summary>
         /// Gets or sets the value of this resource instance.