Preserve existing Examine FieldDefinitionCollection if it already exists

[callumbwhyte]

As discussed on #20267 v16 introduced a behavioural breaking change whereby Umbraco's own Examine index configuration now happens at an undetermined point in the startup lifecycle, compared with previously running as part of the core Umbraco boot steps.

This can cause issues when trying to customise the Examine index, e.g. setting field types, as you don't know whether your code is going to run before or after Umbraco's.

I explained a bit more about how this came to be here:

This was caused by #18988 - where the registration of Examine (and therefore Umbraco's own configuration of LuceneDirectoryIndexOptions) was moved from happening inside builder.AddBackOffice() to a composer. This meant it went from being (almost) guaranteed to run first, as AddBackOffice is called first from Program.cs, to an unknown order.

Other than the [ComposeBefore] / [ComposeAfter] attributes I can't see a way of forcing Umbraco's Examine composer to run before any user registered ones...

The solution is of course to add decorate your IComposer with [ComposeAfter(typeof(AddExamineComposer))] as per the docs - but this has been problematic for me in my Search Extensions package where I am multi-targeting 🙈 and can't reliably target this change to V16+ as V15 also uses .NET 9 where AddExamineComposer does not exist...

After digging into things I found the real issue is Umbraco assumes it's okay to set the FieldDefinitions property during configuration, without consideration for if this has already been set. The fix is simple: when Umbraco's configuration code runs, ensure any existing config is applied too. I have added an overload to UmbracoFieldDefinitionCollection that takes an existing FieldDefinitionCollection and tries to add each existing definition to the UmbracoFieldDefinitionCollection. Examine sets this property to an empty collection on initialisation (here) anyway so it should always be set.

I believe it would be good to get this fix into both V16 and V17 to prevent confusion as others have been experiencing!

[github-actions]

Hi there @callumbwhyte, thank you for this contribution! 👍

While we wait for one of the Core Collaborators team to have a look at your work, we wanted to let you know about that we have a checklist for some of the things we will consider during review:

• It's clear what problem this is solving, there's a connected issue or a description of what the changes do and how to test them
• The automated tests all pass (see "Checks" tab on this PR)
• The level of security for this contribution is the same or improved
• The level of performance for this contribution is the same or improved
• Avoids creating breaking changes; note that behavioral changes might also be perceived as breaking
• If this is a new feature, Umbraco HQ provided guidance on the implementation beforehand
• 💡 The contribution looks original and the contributor is presumably allowed to share it

Don't worry if you got something wrong. We like to think of a pull request as the start of a conversation, we're happy to provide guidance on improving your contribution.

If you realize that you might want to make some changes then you can do that by adding new commits to the branch you created for this work and pushing new commits. They should then automatically show up as updates to this pull request.

Thanks, from your friendly Umbraco GitHub bot 🤖 🙂

[Copilot]

Pull request overview

This pull request adds functionality to preserve existing field definitions when configuring Umbraco Examine indexes. Previously, the ConfigureIndexOptions class would always create a new UmbracoFieldDefinitionCollection from scratch, which would discard any custom field definitions that may have been registered by extension code before the configuration runs.

Key Changes:

• Added a new constructor to UmbracoFieldDefinitionCollection that accepts an existing FieldDefinitionCollection and merges its definitions with the base Umbraco field definitions
• Updated ConfigureIndexOptions to use the new constructor, passing the existing options.FieldDefinitions to preserve any pre-existing customizations

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
src/Umbraco.Infrastructure/Examine/UmbracoFieldDefinitionCollection.cs	Adds a new constructor that accepts and merges existing field definitions
src/Umbraco.Examine.Lucene/DependencyInjection/ConfigureIndexOptions.cs	Updates all three index configurations (Internal, External, Members) to preserve existing field definitions using the new constructor

[AndyButland]

Looks sensible to me @callumbwhyte, thanks for the suggestion and contribution. I've just pushed a small update with a little bit of resolving existing warnings, and added some unit tests (mostly so we have a confirmation of expected behaviour).

To test I've used this composer, configured to run before the core setup, and verified via breakpoints that the custom field remains after ConfigureIndexOptions is called.

using Examine;
using Examine.Lucene;
using Microsoft.Extensions.Options;
using Umbraco.Cms.Core.Composing;

namespace Umbraco.Cms.Web.UI.Custom.Examine
{
    [ComposeBefore(typeof(Infrastructure.Examine.AddExamineComposer))]
    public class ExamineComposer : IComposer
    {
        public void Compose(IUmbracoBuilder builder)
        {
            builder.Services.ConfigureOptions<ConfigureExternalIndexOptions>();
        }
    }

    internal class ConfigureExternalIndexOptions : IConfigureNamedOptions<LuceneDirectoryIndexOptions>
    {
        public void Configure(string? name, LuceneDirectoryIndexOptions options)
        {
            if (name == Core.Constants.UmbracoIndexes.ExternalIndexName)
            {
                options.FieldDefinitions.AddOrUpdate(new FieldDefinition("customField", "string"));
            }
        }

        public void Configure(LuceneDirectoryIndexOptions options) => throw new NotImplementedException();
    }
}

I'll get a second opinion from an HQ review just to check I haven't overlooked something important about making this change.

[Zeegaan]

This looks good to me as well 😁

[callumbwhyte]

Thanks team @AndyButland & @Zeegaan!

I don't know what the plans are for any future V16 releases beyond (v16.4.0 this week) - but what do you think about the possibility of back-porting this into a potential future release / patch too?

[Zeegaan]

I agree, we should definitely also be backporting this for a potential future patch/minor 💪