Skip to content

Feat: implementation of includes#118

Merged
csantero merged 12 commits intoJSONAPIdotNET:masterfrom
spike83:includes
Sep 12, 2016
Merged

Feat: implementation of includes#118
csantero merged 12 commits intoJSONAPIdotNET:masterfrom
spike83:includes

Conversation

@spike83
Copy link
Contributor

@spike83 spike83 commented Aug 17, 2016

added basic IncludeExpressionExractor and introduced includes on EFDocumentMaterializer
will do more testing

/// If the client doesn't request any sort expressions, these expressions will be used for sorting instead.
/// </summary>
/// <returns></returns>
protected virtual string[] GetDefaultSortExpressions()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The more I think about it... it's may needed and overriden by custom subclasses.
I've seen the same method in MappedDocumentMaterializer.
Is this it needed?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you want to sub-class QueryableToManyRelatedResourceDocumentMaterializer and provide default sort expressions for a particular endpoint in the case the client does not provide their own sort expressions, this would be the only way to do it. I was doing precisely this in my own project at one point, which is why I added the method.(although I've since switched to my own custom implementation of IRelatedResourceDocumentMaterializer).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I supposed so just after deletation. I'll bring it back.

This one was not clear to me at the first glance... I'll bring it back too.

            if (sortExpressions == null || sortExpressions.Length < 1)
                sortExpressions = GetDefaultSortExpressions();`

@spike83
Copy link
Contributor Author

spike83 commented Sep 9, 2016

In EntityFrameworkDocumentMaterializer<T> and in EntityFrameworkToManyRelatedResourceDocumentMaterializer<TPrimaryResource, TRelated> there are methods like this.

        private IQueryable<TResource> Filter<TResource>(Expression<Func<TResource, bool>> predicate,
            params Expression<Func<TResource, object>>[] includes) where TResource : class
        {
            IQueryable<TResource> query = DbContext.Set<TResource>();
            if (includes != null && includes.Any())
                query = includes.Aggregate(query, (current, include) => current.Include(include));

            if (predicate != null)
                query = query.Where(predicate);

            return query.AsQueryable();
        }

It seems to me there is an implication on includes but I don't unterstand exactly what should happend and where the includes should be filled in. At the moment query = includes.Aggregate(query, (current, include) => current.Include(include)); is never hit because includes are never filled.

Thanks for some suggestions.

@csantero
Copy link
Collaborator

csantero commented Sep 9, 2016

@spike83 You're right - somehow that capability got lost at some point. Both related resource materializer classes should add something like the following:

protected virtual Expression<Func<TResource, object>>[] GetIncludes()
{
    return new Expression<Func<TResource, object>>[] {};
}

to allow overriding classes to specify navigation properties to include.

…rkDocumentMaterializer because they are never used and replaced with EntityFrameworkToManyRelatedResourceDocumentMaterializer and EntityFrameworkToOneRelatedResourceDocumentMaterializer
@spike83
Copy link
Contributor Author

spike83 commented Sep 10, 2016

@csantero ok if I got it right the GetIncludes method you suggested is not directly related to the includes in the resulting document. It's for eager loading...? since queryable.Include() adds joins to resulting linq to entities sql statement.
So I added the method you suggested and also added a default implementation wich eager loads relations specified by the URL parameter &include=.

@spike83 spike83 changed the title [WIP/RFC]: implementation of includes Feat: implementation of includes Sep 12, 2016
@spike83
Copy link
Contributor Author

spike83 commented Sep 12, 2016

I think it's almost done.

@csantero
Copy link
Collaborator

@spike83 let me know when you're done and I'll take a closer look for review.

@spike83
Copy link
Contributor Author

spike83 commented Sep 12, 2016

For my needs it works like a charm. So I'm done. Please have a strict look what is missing and can be improved before merge. I also enabled the epic github new feature Allow edits from maintainers ;-)
Thanks.

@csantero csantero merged commit 4635c82 into JSONAPIdotNET:master Sep 12, 2016
@csantero
Copy link
Collaborator

@spike83, thanks, this is great!

ClintGood added a commit to ClintGood/JSONAPI.NET that referenced this pull request Sep 13, 2016
@spike83 spike83 deleted the includes branch September 16, 2016 07:44
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

Successfully merging this pull request may close these issues.

2 participants