Upgrading analysis plugins fails

[dadoonet]

When an analysis plugins provides default index settings using PreBuiltAnalyzerProviderFactory, PreBuiltTokenFilterFactoryFactory, PreBuiltTokenizerFactoryFactory or PreBuiltCharFilterFactoryFactory it fails when upgrading it with elasticsearch superior or equal to 0.90.5.

Related issue: #4936

Fix is needed in core. But, in the meantime, analysis plugins developers can fix that issue by overloading default prebuilt factories.

For example:

public class StempelAnalyzerProviderFactory extends PreBuiltAnalyzerProviderFactory {

    private final PreBuiltAnalyzerProvider analyzerProvider;

    public StempelAnalyzerProviderFactory(String name, AnalyzerScope scope, Analyzer analyzer) {
        super(name, scope, analyzer);
        analyzerProvider = new PreBuiltAnalyzerProvider(name, scope, analyzer);
    }

    @Override
    public AnalyzerProvider create(String name, Settings settings) {
        return analyzerProvider;
    }

    public Analyzer analyzer() {
        return analyzerProvider.get();
    }
}

And instead of:

    @Inject
    public PolishIndicesAnalysis(Settings settings, IndicesAnalysisService indicesAnalysisService) {
        super(settings);
        indicesAnalysisService.analyzerProviderFactories().put("polish", new PreBuiltAnalyzerProviderFactory("polish", AnalyzerScope.INDICES, new PolishAnalyzer(Lucene.ANALYZER_VERSION)));
    }

do

    @Inject
    public PolishIndicesAnalysis(Settings settings, IndicesAnalysisService indicesAnalysisService) {
        super(settings);
        indicesAnalysisService.analyzerProviderFactories().put("polish", new StempelAnalyzerProviderFactory("polish", AnalyzerScope.INDICES, new PolishAnalyzer(Lucene.ANALYZER_VERSION)));
    }

Closes #5030

[s1monw]

can we just delegate to valueOf and if it throws an exception we return the default? I don't think we should do the linear checks here

[s1monw]

I think we should also name it get or getOrDefault

[s1monw]

It looks pretty cool though. now the hard part comes... can you write a test for it? We have the ability to set the index version in the settings so you could write a test that loads a dummy pluging that registers the prebuild stuff and create the index with a mapping and a different index version.

There are many utils for this like ElasticserachTestCase#randomVersion() or ElasticsearchTestCase#getPreviousVersion()...
To create the index you just need to do this:

Settings versionSettings = ImmutableSettings.builder().put(IndexMetaData.SETTING_VERSION_CREATED, randomVersion()).build();

[dadoonet]

Great! I was wondering how to test it automatically! Going to look at this.

[dadoonet]

@s1monw Could you review it please? Tests fails without the patch and works when I apply the patch

[s1monw]

this looks awesome! exactly what I expected

can you also try to add prebuild TokenFitler and Tokenizer? just for kicks

[dadoonet]

@s1monw PR updated with test and headers updated. I think we are good now.

[s1monw]

hmm why did you remove the mapping from here? I think that was a good change?
you should add the settings from from public Settings indexSettings() are only used if you use prepareCreate so you should add the settings to the versionSettings below.

other than that it looks awesome

[dadoonet]

Actually I ran the test without the patch and without the mapping and it was failing. So I guess that this analyzer is set by default when creating the index. So we don't need to set it on a specific field to reproduce the issue.

That said I can add the mapping again.

[dadoonet]

About indexSettings(), this is true. I added to versionSettings and I have now an error which I need to fix. Will push a new update soon.

[s1monw]

David this looks great +1 to push