Injectable IPropertyIndexValueFactory implementations into data editors

[whitter]

Just throwing this out as an idea/getting some input, before I start fiddling with any source code as a proof of concept.

At the moment near enough all the out of the box data types either use the DefaultPropertyIndexValueFactory or NoopPropertyIndexValueFactory with no mechanisms in place to change the implementation out without creating your own data type, which in turn would then require making a PropertyValueConverter etc... lots of overhead when you just want to change what data is presented to the index.

At the moment there are only two places that we have control of to change what data is indexed. That is either implementing IndexFieldValueType within Examine, this is fine but I'm personally starting to like the idea of Examine just dealing with primitive data so not concerning its self with what data should be indexed and only detailing with the how data should be indexed.

The other place is handling the Examine event to mutate the value set before it past on for indexing, again this is an okay place to change things but within this event we no longer have access to what the data type is/was to be able to change the data based on property information.

Current thinking is to just to make an explicit factory for each data type and inject it.

internal class DateTimePropertyIndexValueFactory : DefaultPropertyIndexValueFactory, IDateTimePropertyIndexValueFactory
{
}

public class DateTimePropertyEditor : DataEditor
{
    ...
    private readonly IDateTimePropertyIndexValueFactory _dateTimePropertyIndexValueFactory;
    ...

    public DateTimePropertyEditor(
        ...,
        IDateTimePropertyIndexValueFactory dateTimePropertyIndexValueFactory)
    {
        ...
        _dateTimePropertyIndexValueFactory = dateTimePropertyIndexValueFactory;
        ...
    }

    public override IPropertyIndexValueFactory PropertyIndexValueFactory => _dateTimePropertyIndexValueFactory;
    ...
}

This is great for a first pass of changes, but would there be any issues that would arise from injecting a collection of factories? Main thinking is that it allows to have individual factories that can contain their own processes without replacing the defaults or fighting against any other factories the community might make.

internal class DateTimePropertyIndexValueFactory : DefaultPropertyIndexValueFactory, IDateTimePropertyIndexValueFactory
{
}

public class DateTimePropertyEditor : DataEditor
{
    ...
    private readonly IEnumerable<IDateTimePropertyIndexValueFactory> _dateTimePropertyIndexValueFactories;
    ...

    public DateTimePropertyEditor(
        ...,
        IEnumerable<IDateTimePropertyIndexValueFactory> dateTimePropertyIndexValueFactories)
    {
        ...
        _dateTimePropertyIndexValueFactories = dateTimePropertyIndexValueFactories;
        ...
    }

    public override IEnumerable<IPropertyIndexValueFactory> PropertyIndexValueFactories => _dateTimePropertyIndexValueFactories;
    ...
}

As a couple of examples of what this would allow:-

• At the moment the media picker uses the noop factory. The ability to add a second factory that could look at a property and understand what type of media item has been selected. If its an item, such as a PDF, the content of the item can then be pulled out and indexed along side the page.
• With all the new extension points in the v14 UI, I've had a mad thought about data types having configuration options to decide what data is presented for indexing. I'm imagining the ability to add a content app to the multi node picker that will allow the selection of which data from the selected nodes should be indexed along side its default content. So along with the comma list of UUIDs, we could have a collection of IDs, URL segments, names, and maybe any other property. All achieved with a zero code config.

Ramble over 😆

[bielu]

I don't think you need that collection to inject at all, you can decorate interface(scrutor is your friend) and make it conditional in decorator if use base implementation or specific to your need.
Adding this collection adds complexity in really basic implementations which is not needed.

Also I don't think this implementation of indexing factories should stay in current form, especially when umbraco abstract out examine. I think factories should be independent from data types and have method is suitable factory and then you can have multiple factories for same data type or even shared ones based on some type of logic.

[whitter]

Don't worry too much about my code examples, they are just there illustrate the idea of swappable/stackable factories and promote conversation.

I have been thinking about the decorator pattern as the most likely contender as a solution as, at least on paper, would introduce no breaking changes. Toying with the idea of an abstract PropertyIndexValueFactoryDecorator class, something like:

public abstract class PropertyIndexValueFactoryDecorator<TFactory> : IPropertyIndexValueFactory
    where TFactory : IPropertyIndexValueFactory

{
    private readonly TFactory _factory;

    public PropertyIndexValueFactoryDecorator(TFactory factory)
    {
        _factory = factory;
    }

    public IEnumerable<KeyValuePair<string, IEnumerable<object?>>> GetIndexValues(
        IProperty property,
        string? culture,
        string? segment,
        bool published,
        IEnumerable<string> availableCultures,
        IDictionary<Guid, IContentType> contentTypeDictionary)
    {
        // return items for the _factory.GetIndexValues(...) and the abstract Handle(...) functions
    }

    protected abstract IEnumerable<KeyValuePair<string, IEnumerable<object?>>> Handle(
        IProperty property,
        string? culture,
        string? segment,
        bool published,
        IEnumerable<string> availableCultures,
        IDictionary<Guid, IContentType> contentTypeDictionary);
}

Then an extension method on the IUmbracoBuilder to add some function to aid in adding factory decorators. AddPropertyIndexValueFactory<TFactory, TDecorator>() where TFactory has to be of type IPropertyIndexValueFactory and TDecorator needs to be of type PropertyIndexValueFactoryDecorator<TFactory>.

Also did think about disconnecting the factories from the data types, in a similar way to how value converters work. But have put that thinking aside for now as that would introduce a big breaking change and, not that I've given it much thought, would need proper consideration around performance.

[bielu]

@whitter it should be included in RFC for new search abstractions as agree they shouldn't be attach in way as they are currently, I also dont like how they works at all ;)

I would probably introduce separate interface for them and simplify it. Also umbraco is unlikely to introduce decorator pattern as it will introduce depedency on 3rd party orchestrator like scrutor :) so more likely Wrapper pattern (it is similar but different!) i proposed decator for you to use it currently to avoid umbraco core changes :)

[whitter]

That is true on the bring in another 3rd party library. Find certain aspects of search out of the box to be quite restrictive now a days.

Might raise a PR in the first instance to have all the factories injected into the data types, at least that then opens it up for us developers to bring in decorators etc... when we see fit to use it.

[bielu]

@whitter i raised pr(Actually few 😂) to abstract out examine and separate concern of engine and indexing, but it got closed and somewhen this year there is supposed be rfc for new search abstractions