Create explicit scope for mappers in UmbracoMapper #9995
Conversation
|
Just a quick thought without really knowing the background - but did you consider amending the It does of course bring a downside and lose a benefit of what you've done, in that it will be up to the developer using a mapping operation to consider if and when a scope should be created, rather than it just happening automatically anyway. |
|
I like the idea, but the problem with doing that is that then you will have to know if a specific object mapping needs a scope, and that requires that you know the implementation of the mapper itself, for instance, just picking a random place the Current.Mapper.Map<IDataType>(dataType);At this point, you have no way of knowing if this requires a scope without knowing the implementation of I do think that it might be possible though to do something similar when registering a mapper in the |
There was a problem hiding this comment.
Nice and simple! Had one comment in there. I also double checked that it is correct, creating a Scope with the provider just allocates an instance and it doesn't automatically create any database instances or start any transactions, that is all done lazily only if/when the Database property of a Scope is accessed so the downside is minimal.
| /// <summary> | ||
| /// Initializes a new instance of the <see cref="UmbracoMapper"/> class. | ||
| /// </summary> | ||
| /// <param name="profiles"></param> | ||
| public UmbracoMapper(MapDefinitionCollection profiles) | ||
| /// <param name="scopeProvider"></param> | ||
| public UmbracoMapper(MapDefinitionCollection profiles, IScopeProvider scopeProvider) |
There was a problem hiding this comment.
We should make this ctor non-breaking by obsoleting the old one and adding a new one and calling: this(profiles, Current.ScopeProvider)
There was a problem hiding this comment.
Ah very good point, didn't think of that, I've re-added the old constructor using Current.ScopeProvider and marked it as obsolete 👍
Shazwazza
added
the
category/performance
|
@nikolajlauridsen Once that is fixed up feel free to merge. But we'll need to check with @nul800sebastiaan or someone what version this should be tagged in. |
|
We just closed 8.13 yesterday, I checked if I could merge this but I saw there was some outstanding questions, 8.14 it is! |
nikolajlauridsen
deleted the
v8/feature/10615-mappers-with-explicit-scope
branch
Description
We have a lot of mappers that makes several calls to multiple services, resulting in multiple transactions. This PR wraps all calls to the mappers' Map methods in an explicit scope, making all Map operation result in a single transaction.
I've tried to investigate potential issues and pros and cons. I was unable to find any issues, but the benefit of this change is also the downside to it, there will always be created an explicit scope, even for the mapping operations that don't require the scope, however, as far as I can tell, it's only a matter of object allocation, and therefore I think it's a beneficial trade-off.
Another minor downside to this is that it can be argued that this introduces more "magic". If you don't take a look at the UmbracoMapper implementation, it's not obvious that a scope is created for mapping operations, however, I don't think that this is a big deal.
A thing worth noting is that with the current implementation when mapping with IEnumberables, a single scope will be created for all the mapping operations (line 260 in UmbracoMapper).