EntityRepository is fetching all property data for media

[clausjensen]

connect #3457

Changes

• Adds internal methods for getting media entities through the entity repository/service without fetching all property data.
• Ensures the trees use this method to avoid fetching property data in the media tree.

This is not the prettiest way to do it since it requires casting of the service/repo to use the internal methods. It is however a way of preventing this becoming a breaking change in the APIs.
This should be fixed when merged to V8, to we make sure we do not do this in the EntityRepository for media items.

Test

• Ensure that the content and media tree renders as expected (they render nodes without throwing errors).
• Ensure that the "folder browser" feature on media folders still work (this feature may be the reason why this property data was added initially? - and it should still work fine since this feature uses paging)

[Shazwazza]

I think for a v7 'quick fix' we can accept that this is hacky and a work around.

For v8 i think we need to fix this:

• The EntityService has methods that expose a parameter: loadBaseType which if it's false it will use the normal service to load the entity ... but why? This is very old code and I don't think it should exist at all
• The EntityRepository for entities shouldn't load in any property data at all, but because we will probably still require property data for media when using entities we should do this:
  • Expose another method(s) on the EntityService/EntityRepository for looking up property data directly where we can specify what properties to load (i.e. by property type alias, object type, etc...) we cannot allow searching on actual property data since that is not indexed but this would allow us to get all media entities and then when required lookup all media properties: fileName, width, height for the provided INT ids

This would cleanup a huge portion of this code

[clausjensen]

@Shazwazza I'm good with calling this a 'quick fix' and also know that we should fix it differently for V8.
Can you merge it when done reviewing and create a new task with details for fixing this in V8?

[sitereactor]

@Shazwazza one thing i don't understand, if you need properties on a media item, why not use the media service? The whole point of the entity service/repository was to load the bare minimum of what makes it an entity and thus without properties. If the media item contains a property that is essential to its 'entity' then why not look at that first, so entities can remain entities.

[Shazwazza]

@sitereactor many many moons ago when this was done, the EntityService was used for things that it didn't support because it was assumed that media and content were almost the 'same'. As it turns out when we went down this path, they weren't the same, things like URLs and dimensions were needed for the most basic media item. We then went down a path to treat some entities differently than others with this service because we needed to use it to be able to lookup the basic requirements of media. (it's been a very long time, i cannot tell you exactly the entire story) We figured out we could load the property values and media values in a single query, because it was a single query it was perceived that the performance was going to be good (oh how we learn new things over time ;) Obviously this was not the case but it was never changed because there was never seemed to be a side affect. Of course now there is and with all things learned we will need to fix it properly eventually.

If we used the Media Service for loading the tree, it would be worse.

[sitereactor]

I'm sure the past has its reasons and that is fine. I would much rather look ahead to see how we can fix it and improve future umbraco, and I think we are on the same page with your v8 suggestion. My point is just that maybe we should look at the various usages and what is required where and why, so we can keep the EntityService for what its meant for and so the usages where properties are not needed can use that happy path. Or expose suitable methods through the MediaService. Whatever you prefer. As long as its clear what you are getting from the various services/repositories/etc.

[Shazwazza]

Sure thing, just we can't change it (properly) in v7 now because of breaking changes and all that

[Shazwazza]

why are property data being loaded?

I've Investigating this further with regards to where/why the media properties are requires. It seems that that most of the loading of property values for media with the EntityService is only required for very old legacy reasons which are not in use anymore, however there is still a couple of scenarios where this is still required:

• In angular various methods of mediaHelper requires that one or more of the media properties exist - and this is to extract the media file url or the thumbnail url. From what i can tell this is used in various places - however doing some rudimentary tests, the usages of this might also be legacy
  • media pickers
  • the media grid
  • file uploaders
  • image cropper
  • link picker

So I believe this is really purely about resolving image file paths - though we can't only rely on the umbracoFile or the property editor Umbraco.UploadField property editor alias since file paths can be resolved from media from any property and a developer can register their own 'file resolvers'

solutions

solution 1

This PR work around seems OK and could work for the time being. I'll probably change the GetByQueryWithoutPropertyData to not accept an object type since this is purely only for media.

solution 2

Another slightly more consistent solution for v7 which could be:

• Add the loadBaseType parameter overload to the GetChildren/Descendants/Paged methods like there is for the normal Get methods (i still am unsure why these overloads exist but they are there). The way these overloads currently work is If loadBaseType == false then it will actually lookup that entity using it's native service (i.e. IMediaService).
• We could then use the same logic for the GetChildren/Descendants/Paged and if loadBaseType == false then it would load in the media the way it currently does, else it would only load the entity
• We'd then change the calls in the EntityController (which is where angular requests the entity data from for the above mentioned stuff) to use loadBaseType == false for media so it loads in all properties

That said, this is a bunch more work and considering I don't think the loadBaseType == false makes sense anyways, we should just stick to the hack and clean all of this up in v8.

solution 3

Considering this is really only used to determine the media file path/url, we already now store this data in the database from 7.10 (? something like that) in the cmsMedia table. We could easily just join this table when query media in the entity repo and include the file path there.

The problems with this are:

• angular will need to be updated to deal with where/how this file path is stored
• angular currently has this method mediaHelper.registerFileResolver where developers could register their own media file resolvers if they have a custom property editor for storing media (this is rare), but if this is being used then it would be a breaking change to do this

This is the most robust solution and would cause very little extra overhead for the entity queries. I propose we make this change in v8.

v8 changes

Cleaning this up in v8 will take some work and will need to be done before 8.0 since it is breaking changes that cannot be unbroken in a minor version.

We should implement solution 3 above which will mean

• a bunch of cleanup with the mediaHelper and all references to it
• remove the idea of mediaHelper.registerFileResolver, i don't think this is at all necessary
• cleaning up the EntityService/repo
  • Remove the loadBaseType thing
  • Adjust the query for media to join the cmsMedia table to get the file path

[Shazwazza]

The methods for media without properties should just be for media and named accordingly the rest could just stay as it was. I'll update so you can see changes.

[Shazwazza]

This note should be in a block

[Shazwazza]

Also should be in a block

[Shazwazza]

There's no reason to have the objectTypeId param here, this methods should just be called GetMediaByQueryWithoutPropertyData

[Shazwazza]

Have added v8 task here #3498