Umbraco-cms: .NetCore: Remove direct database queries from services and controllers

[AndyButland]

Umbraco services shouldn't be doing direct queries of the database, rather they should go through their respective repository for database access. This is a dependency we'd need to remove in order to migrate to a "clean architecture", with all models and services (business logic) in "core" and persistence concerns in "infrastructure".

There look to be a couple of services where this occurs:

• KeyValueService
• UserService

Task is to remove these concerns, migrating to an existing or new repository layer.

[kashfshah]

Any other services still need this treatment?

[AndyButland]

I'm not aware of any, no. Looks like you closed this @bergmania, and re-opened, but I think it can actually be closed now. I believe that at least any issues of SQL statements in the services were removed from the linked PR.

[bergmania]

Hi @kashfshah and @AndyButland .
At least EntityService are still building SQL and using Scope.Database directly.

[bergmania]

Also LogController makes SQL

[umbrabot]

Hi @AndyButland,

We're writing to let you know that we would love some help with this issue. We feel that this issue is ideal to flag for a community member to work on it. Once flagged here, folk looking for issues to work on will know to look at yours. Of course, please feel free work on this yourself ;-). If there are any changes to this status, we'll be sure to let you know.

For more information about issues and states, have a look at this blog post

Thanks muchly, from your friendly Umbraco GitHub bot :-)

[ageroh]

Hi,
Still need help?
I willing to take over one this one

[bergmania]

Hi @ageroh...

Yes, this is still unresolved and it would be super nice if you would take a look. The issue will not be closed by a single PR.

I'm aware of multiple usages of ISqlContext in controllers. These are all Fat controllers, where business logic should be handled in services and database queries should be handled in repositories.

Consider LogController as an example. All logic could be refactored to be handled by a new LogService.

Let me know if you have any questions :)

[ageroh]

I will have a look and let you know! Happy to help!

[ageroh]

@bergmania could you check the below:

https://github.com/umbraco/Umbraco-CMS/pull/8968

[lars-erik]

After the unicorner with @bergmania on Unicore, this issue has piqued my interest due to the possible long run outcome.
Regarding that (for now undefined) outcome, I would say that moving towards services et al. using IQueryable's from a repository is a very very valid way of querying. The age old repository.ThisQuery(params) and repository.ThatQuery(params) is a very rigid way to build data layers. I do consent that it's "The way" to clean up leaked SQL right now. But in the long run services and other things should defo. be able to do ad hoc queries against the logical model. I suggest that would be through
IQueryable<T> IRepository<T>.Query(string[] eagerLoadedAssociationNames).

It would probably cause a requirement for any future data layers to use a "proper" ORM with controllable query building and eager loading, but in my opinion that's a good thing.

Just my two cents.
Hope to be able to contrib more than words (🎵) to this sooner or later. 😇

[bergmania]

@lars-erik, I agree with what you say regarding IQueryable, as long as this is only exposed by repositories and not by services. IMO services should solve the use cases, and I see no good argument for having fat controllers 😸

[Shazwazza]

IQueryable is great! But it's also very difficult to fully implement - of course not impossible even with what we have today. We also want this with the front-end cache https://github.com/umbraco/Umbraco-CMS/issues/8367#issuecomment-652761645 which comes with it's own complexities.

It would probably cause a requirement for any future data layers to use a "proper" ORM with controllable query building and eager loading, but in my opinion that's a good thing.

Throwing a 'real' ORM at Umbraco doesn't solve these issues/requests. I know people like EF and other ORMs that support IQueryable, but the Umbraco database structure does not support this style of ORM and querying. The Umbraco database schema is like a meta schema, it itself creates it's own data structure. If Umbraco tables were 'normal' then a 'normal' ORM would work. If we had a table per document type composition - then this would work but we don't. Doing that has been a consideration in my mind for years and I believe is what some other CMS do (i.e. IIRC Orchard does this) but this itself adds tremendous complexity with atomic operations and modifying schema types, compositions, etc... Until we re-think the Umbraco database structure EF isn't going to fix anything. The implementation would end up being very similar to what we already have with very specific SQL queries, entity tracking turned off (because entities don't really exist), no IQueryable... because of the specific requirements for SQL ... it would be basically what we have today.

In the meantime, if all of the underlying DB interaction is done in repositories, then it's possible to replace the repositories without replacing the services and then all sorts of experimentation could happen and maybe I can be proved wrong :)

[lars-erik]

IMO services should solve the use cases, and I see no good argument for having fat controllers 😸

In the meantime, if all of the underlying DB interaction is done in repositories, then it's possible to replace the repositories without replacing the services and then all sorts of experimentation could happen and maybe I can be proved wrong :)

We all completely agree. 👍

If we had a table per document type composition - then this would work but we don't.

I think this is a complete fallacy. There is no difference in handling todays entity representations with an ORM than pure SQL. Nobody said anything about strongly typing the properties. We've got ModelsBuilder for that.

A document entity in Umbraco IS a content header, a list of named properties, a content type, a list of property definitions etc.
Perfectly valid! It's Umbraco's _domain model_ at core. Whatever crosses the ports / borders to outside the cache does not, and probably should not represent those entities.

I'm entierly sure we'll have a cleaner codebase by using LINQ against the current entity model. Not eager loading where not necessary will likely yield better performance. Using IQueryable will yield way more flexibility to services and use cases than hard coded SQL queries. This goes for pretty much everything. :P Transforming, eager loading, association traversal, paging, filtering etc.

None of these benefits mandate that the entities should match the document type structure.

I'm sure we'll prove you wrong, @Shazwazza. 😁

[lars-erik]

entity tracking turned off

This is a use case dependent setting. Read a lot of stuff? Turn it off. Update one entity? Turn it on.

[Shazwazza]

Looking forward to it 😊

I suppose a reason why I'm skeptical is because this was tried with NHibernate years ago. I realize that's old tech and things have evolved but the concepts were the same/similar and we ended up with a hugely inefficient DB access layer that suffered from crazy N+1 issues. I understand that with very careful consideration every step of the way this could be avoided but it is still easy (with our DB structure) to have someone accidentally cause N+1 spikes.

But I've been out of that game for a very long time so once all things are at repo level I would be happy to be very happy to be proven wrong and see proof of concepts 😀

[lars-erik]

There are bad ways of using any library. 😁 Also NHibernate with XML was a huge pain, not to mention using "one size fits all caching strategies". EF with Code First is super sweet, and you're in total control. You can even go pure SQL if you need to in some use cases. And there's the eager vs. lazy loading. I say drop lazy loading all together.

In any case, really looking forward to the future!

[rustybox]

I find Repository slightly overloaded as a name, its roots are in domain driven design for a class responsible for persisting and loading an aggregate root.

I'm guessing the refactor here is likely to be closer to data access objects, perhaps I'm wrong.

---

we ended up with a hugely inefficient DB access layer

You don't have to use the same tech everywhere, perhaps some performance critical / hot areas demand a hyper optimized stored procedure, and elsewhere a bit of linq against a DbSet will suffice, when data access is abstracted away it shouldn't matter to the caller which is the case.

[lars-erik]

its roots are in domain driven design

I dare contradict that and say it's an ancient common pattern documented by Martin Fowler in PoEAA. ;)

I also die on the hill of a simple IRepository/IUnitOfWork abstraction leaves enough elbow room to easily migrate to the next iteration of MS' (or any "proper") ORM. I've had nothing by luck doing that through LINQ to SQL, EF 1 + poco adapters, EF 4, EF 6, EF Core... Nothing but the two generic implementations need change. Further elaboration on demand.

I do believe you of all peeps would agree that removing dependencies is a good thing, @rustybox . 😉

[rustybox]

Hah I will stand corrected, Evans expanded on the work 2 years later, although I'd considder Fowler very much part of the DDD movement.

What would the advantage of exposing IQueryable be over Criteria/Specification?

I do believe you of all peeps would agree that removing dependencies is a good thing

Sometimes, decouple from light inject sure, but you know I'd glady add MediatR especially with an abstraction on top :p

[lars-erik]

What would the advantage of exposing IQueryable be over Criteria/Specification?

@rustybox, IQueryable is basically the ultimate implementation of a provider based crieteria/specification converter.

I presume you know all about it, but I'll throw in a summary for good measure.
At runtime uncompiled lambda expressions, Expression<Func<Entity, Object>>, are visited by the provider's (EF) parser that builds raw SQL based on the abstract syntax tree. (select expression where expression)
This means that we can write any C# code compatible with the _provider_ in our expressions and they will act as the specifications.
AFAIK, the only feature EF has that generic expressions and IQueryable _does not have_ is the EF specific extension Include.
I generally solve that by abstracting the expression based Include extensions behind an params string[] argument to repository.Query().

So the only abstractions needed for repo/uow is as follows:

public interface IRepository<T>
{
    void Add(T entity);
    void Delete(T entity);
    T Get(object id);
    IQueryable<T> Query(params string[] include);
}

public interface IUnitOfWork
{
    void SaveChanges();
}

Evergreen! Mine haven't changed since LINQ to SQL.

How 'bout going something like:

contentRepository
    .Select(x => new {x.Name, x.ContentType.Alias})
    .Where(x => x.CurrentVersion.Properties.Any(x => x.Alias == "title" && x.Text.Contains("umbraco"));

converting to something like (forgive wrong names)

select
    name, contenttype.alias
from
    umbraconode
    inner join cmscontent on id = id
    inner join cmsproperty on contentid = id
    inner join cmspropertyvalues on version = currentversion
where
    cmsproperty.alias = 'title'
    and
    cmspropertyvalues.value like '%umbraco%'

I'll throw in modification as well:

using (var transaction = Factory.CreateTransaction())
{
    using (var uow = Factory.CreateUoW())
    {
        var repo = new GenericRepo<Entity>(uow);
        var entity = repo.Get(id); // ID can be whatever, including composite, hence object
        entity.Property = newValue;
        uow.SaveChanges();
    }
}

Although I prefer scoping transaction and uow + savechanges to once per request, rolling back or committing based on success of all operations. Which of course gives me the ability to inject IRepository and not mind acid, transactions or UOW at all. :) (Because any request is one command and one transaction anyway)

[JimBobSquarePants]

You'll need to provide an abstraction for a Migrate() method also and make your methods async.

What happens with existing migration infrastructure should you use a different DB provider? I'll confess I'm not 100% clued up on how they work currently on upgrade.

Regarding EF and meta. I'm with @lars-erik here it's super possible. I've actually implemented custom properties in a similar fashion to Umbraco in EF Core and (_combined with a very lightweight, injectable mapper_) you can easily map to your view models.

```c#
///

/// Defines the contract for all custom properties.
///

public interface ICustomProperty : IEntity
{
///

/// Gets or sets the name.
///

string Name { get; set; }

/// <summary>
/// Gets or sets the name.
/// Normalized to be compliant with C# property naming restrictions.
/// </summary>
string NormalizedName { get; set; }

/// <summary>
/// Gets or sets the description.
/// </summary>
string Description { get; set; }

/// <summary>
/// Gets or sets the string value.
/// </summary>
string StringValue { get; set; }

/// <summary>
/// Gets or sets the bool value.
/// </summary>
bool? BoolValue { get; set; }

/// <summary>
/// Gets or sets the date time value.
/// </summary>
DateTime? DateTimeValue { get; set; }

/// <summary>
/// Gets or sets the integer value.
/// </summary>
int? IntValue { get; set; }

/// <summary>
/// Gets or sets the decimal value.
/// </summary>
decimal? DecimalValue { get; set; }

/// <summary>
/// Gets or sets the guid value.
/// </summary>
Guid? GuidValue { get; set; }

}

///

/// Defines the contract for all custom Properties.
///

/// The parent class that the property belongs to.
public interface ICustomProperty : ICustomProperty
where TParent : class, new()
{
///

/// Gets or sets the properties parent.
///

TParent Parent { get; set; }
}
```