Skip to content

Override is linq to objects#229

Merged
StefH merged 4 commits intozzzprojects:masterfrom
david-garcia-garcia:override-is-linq-to-objects
Nov 28, 2018
Merged

Override is linq to objects#229
StefH merged 4 commits intozzzprojects:masterfrom
david-garcia-garcia:override-is-linq-to-objects

Conversation

@david-garcia-garcia
Copy link
Copy Markdown
Contributor

@david-garcia-garcia david-garcia-garcia commented Nov 25, 2018

Current implementation of IsLinqToObjects might no always return the correct result because there is no specificatin or reliable way of actually answering the question "IsLinqToObjects".

I..e in our project we are using LinqKit's AsExpandable() and we had to extend the current implementation by forking:

private static bool IsProviderEnumerableQuery(IQueryProvider provider, IQueryable query)
        {
            Type baseType = provider.GetType().GetTypeInfo().BaseType;

            if (baseType == typeof(EnumerableQuery))
            {
                return true;
            }

            // Support for QueryTranslatorProvider
            if (baseType.Name == "QueryTranslatorProvider")
            {
                try
                {
                    IQueryProvider provider1 = baseType.GetProperty("OriginalProvider").GetValue(provider, (object[])null) as IQueryProvider;
                    return provider1 != null && QueryExtensions.IsProviderEnumerableQuery(provider1, query);
                }
                catch
                {
                    return false;
                }
            }

            // Support for LinqKit's ExpandableQuery
            if (provider.GetType().Name.StartsWith("ExpandableQuery"))
            {
                try
                {
                    IQueryable innerQuery = query.GetType().GetProperty("InnerQuery", BindingFlags.NonPublic | BindingFlags.Instance).GetValue(query) as IQueryable;
                    return innerQuery != null && QueryExtensions.IsProviderEnumerableQuery(innerQuery.Provider, innerQuery);
                }
                catch
                {
                    return false;
                }
            }

            return false;
        }

The proposal is to move IsLinqToObjects to the ParsingConfig object, so that users can customize it depending on what libraries they are using.

This is of course a breaking change as it breaks the current's library public contract.

@codecov
Copy link
Copy Markdown

codecov bot commented Nov 25, 2018

Codecov Report

Merging #229 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #229      +/-   ##
==========================================
+ Coverage   96.63%   96.64%   +0.01%     
==========================================
  Files          41       41              
  Lines        6654     6683      +29     
==========================================
+ Hits         6430     6459      +29     
  Misses        224      224
Impacted Files Coverage Δ
...stem.Linq.Dynamic.Core/DefaultQueryableAnalyzer.cs 100% <100%> (ø)
src/System.Linq.Dynamic.Core/ParsingConfig.cs 100% <100%> (ø) ⬆️
...em.Linq.Dynamic.Core/DynamicQueryableExtensions.cs 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1ee8997...71bc716. Read the comment docs.

@StefH
Copy link
Copy Markdown
Collaborator

StefH commented Nov 25, 2018

Good point, thanks for the PR.

However I would propose this:

  1. Create a new Interface and Default implementation class.
  2. Move the logic from IsProviderEnumerableQuery to that class.
  3. Create a new property in the ParsingConfig class and build this property like public IExpressionPromoter ExpressionPromoter

For item 1 and 2:
Interface name could be:

public interface IQueryProviderAnalyzer
{
    bool IsProviderEnumerableQuery(IQueryProvider provider)
}

And default implementation is

public class DefaultQueryProviderAnalyzer
{
    public bool IsProviderEnumerableQuery(IQueryProvider provider)
    {
      // copy code from ParsingConfig -> IsProviderEnumerableQuery method here
    }
}

Item 3 would be like:

public IQueryableProviderAnalyzer QueryableProviderAnalyzer
        {
            get
            {
                return _queryableProviderAnalyzer ?? (_queryableProviderAnalyzer = new DefaultQueryableProviderAnalyzer());
            }

            set
            {
                if (_queryableProviderAnalyzer != value)
                {
                    _queryableProviderAnalyzer = value;
                }
            }
}

StefH
StefH previously requested changes Nov 25, 2018
Comment thread src/System.Linq.Dynamic.Core/ParsingConfig.cs
@StefH
Copy link
Copy Markdown
Collaborator

StefH commented Nov 26, 2018

I will take a look and try to refactor the code...

@StefH
Copy link
Copy Markdown
Collaborator

StefH commented Nov 26, 2018

@david-garcia-garcia - I did update the code, please take a look.

@StefH
Copy link
Copy Markdown
Collaborator

StefH commented Nov 27, 2018

@david-garcia-garcia : can you please take a look at my updates for this PR?

@StefH StefH added the feature label Nov 27, 2018
@david-garcia-garcia
Copy link
Copy Markdown
Contributor Author

Looks impecable! Been bussy...

@StefH StefH merged commit b4c24dd into zzzprojects:master Nov 28, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Development

Successfully merging this pull request may close these issues.

2 participants