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

Features/improvements for version 5 #84

Closed
fiseni opened this issue Feb 16, 2021 · 7 comments
Closed

Features/improvements for version 5 #84

fiseni opened this issue Feb 16, 2021 · 7 comments

Comments

@fiseni
Copy link
Collaborator

fiseni commented Feb 16, 2021

I'm listing a few features and changes to consider for the next major version v5.

  • Refactoring the include infrastructure (see PR Refactoring Include infrastructure to store expression info. #83). Contains breaking changes.
  • Refactor the evaluation infrastructure. Will contain breaking changes.
    The evaluator's content grew with time, and right now everything is squeezed within a single method. It's not easy to extend/modify the evaluator, since the order of the actions is important, and users are forced to fully implement their own evaluators (if needed). We should improve this, extract the bits into partial evaluators and make it more modular and open to extensions.
  • Add "transient" evaluator in the base package, which will act on IEnumerable types (see issue Specification usage improvement for preloaded data. #57). Most probably will be a breaking change.
  • Add generic constraints wherever is required. Right now we have no constraints, e.g. Specification<int> does not make sense, right? Breaking change.
  • Add addon/plugin package for EF Core 5
  • Add support for AsSplitQuery
  • Various other improvements.

@ardalis ping me whenever you're free and willing to discuss/work on this. I think we can fix this within a couple of hours.

@ardalis
Copy link
Owner

ardalis commented Feb 17, 2021

I'm not sure what generic constraints we could apply. I suppose we could restrict to only reference types or newable types but those seem arbitrarily restrictive. Technically you could use a Specification as a kind of validator or to encapsulate some range of values that that is complex to describe but simple to name. Imagine someone finds this useful as an example:

PrimesLessThanLimitSpec : Specification<int>
{
    public PrimesLessThanLimitSpec(int limit)
    {
        Query.Where(x => x.IsPrime() && x < limit);
    }
}

Other than that I think it's a good list.

@fiseni
Copy link
Collaborator Author

fiseni commented Feb 17, 2021

I meant to restrict it for reference types only where T : class. As a matter of fact, we're already confined in this context, e.g. the extension methods of EF have constraints for reference types.

public static IQueryable<TEntity> Include<TEntity>([NotNull] this IQueryable<TEntity> source, [NotNull][NotParameterized] string navigationPropertyPath) where TEntity : class;

Also, we have a generic constraint for the evaluator (T : class), since it utilizes these EF methods. In the existing infrastructure we have, if the specifications are not of reference type, it will throw on evaluation anyway. So, it's about being explicit of the intentions, and what is allowed and not.

Your example is great, but for those cases, we might define another type of specification that will work exclusively with BCL. Otherwise, we end up with a lot of implicit rules. The rule would be something like "Ok you can define it as of type int, but in that case you can't use Include, Search, AsNoTracking, etc. The users will have to have a deeper knowledge of the inner logic, so they don't end up with improper usages. The usage becomes ambiguous.

@ardalis
Copy link
Owner

ardalis commented Feb 17, 2021

Fair enough; I wasn't thinking about the EF-specific bits so much as the basic spec functionality. Sounds good.

@fiseni
Copy link
Collaborator Author

fiseni commented Feb 18, 2021

I gave a thought to this. We can do better. We won't define constraints to the specifications, but we'll play with the builder extensions. The extension methods will appear only if applicable. For example, if you define Specification<int>, the Include method won't appear at all.
There won't be an additional specification type, users will work with the same base class Specification, and the behavior will change based on the given context.

@RandomUser99
Copy link

RandomUser99 commented Feb 25, 2021

I know it's only new in EF Core 5.0, but adding support for AsNoTrackingWithIdentityResolution() would be great.

@fiseni
Copy link
Collaborator Author

fiseni commented Mar 1, 2021

We implemented all of the topics. Some additional stuff too, we'll provide more detailed documentation later on.

Few stuff that we might consider:

  • We're using Tuples for some of the properties in the specification. Should we add new specific types or not? It will add to clarity, but we'll have additional allocations.
  • Should we keep the non-generic repository as part of the package. If decide to keep, then we should clarify the following:
    • Since the T is not class generic, the GetById has this form GetByIdAsync<T, TId>(TId id). Since the T is not a parameter of the method, it can't be inferred, and it has to be written explicitly. The TId can be inferred, but since you have to write the other parameter, then you must write the second one too (either everything is inferred, or everything is written explicitly, you can't do just one of them). Then, the usage becomes GetByIdAsync<Store, int>. Should we add an overload for int, string, Guid, to simplify it for these types at least?
    • For the same reasons GetBySpecAsync has very bad usage in case we want to apply ISingleResultSpecification constraint. It's something like GetBySpecAsync<SpecificationName,Entity>(specification). I have no better solution for it now.
  • Now that we added ISingleResultSpecification, it might be wise instead of GetBySpecAsync method to have more verbosely named FirstOrDefaultAsync and SingleOrDefaultAsync methods. We should constrain only the Single method probably? Conceptually that makes more sense to me.
  • Do we want to apply ISingleResultSpecification to the GetBySpecAsync<T, TResult> (for the specifications which have selector defined). What would be the use case here?

@RandomUser99
As for your request. EF Core provides way too many options, we have to draw a line somewhere. Otherwise, we'll be way too EF Core centric. In case of an additional provider, we'll have to throw exceptions or just ommit all these features. But, on the other hand, I like AsNoTrackingWithIdentityResolution() :). Hard choice :)

@fiseni
Copy link
Collaborator Author

fiseni commented Mar 5, 2021

Implemented in version 5.0.3
@RandomUser99 AsNoTrackingWithIdentityResolution is implemented as well.

@fiseni fiseni closed this as completed Mar 5, 2021
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