Move char filters into analysis-common#24261
Conversation
Another step down the road to dropping the lucene-analyzers-common dependency from core. Note that this removes some tests that no longer compile from core. I played around with adding them to the analysis-common module where they would compile but we already test these in the tests generated from the example usage in the documentation. I'm not super happy with the way that `requriesAnalysisSettings` works with regards to plugins. I think it'd be fairly bug-prone for plugin authors to use. But I'm making it visible as is for now and I'll rethink later. A part of elastic#23658
| return normalizers; | ||
| } | ||
|
|
||
| private static <T> AnalysisModule.AnalysisProvider<T> requriesAnalysisSettings(AnalysisModule.AnalysisProvider<T> provider) { |
|
|
||
| import static org.elasticsearch.test.ESTestCase.createTestAnalysis; | ||
|
|
||
| public class CharFilterTests extends ESTokenStreamTestCase { |
There was a problem hiding this comment.
These are duplicates of the rest tests in analysis-common. I can move this to analysis-common and keep it if anyone feels strongly about it.
| return singletonMap("myfilter", MyFilterTokenFilterFactory::new); | ||
| } | ||
|
|
||
| @Override |
There was a problem hiding this comment.
I believe this is a leftover. I'll look into removing this.
| // verify characters mapping | ||
| analyzer = indexAnalyzers.get("custom5").analyzer(); | ||
| assertThat(analyzer, instanceOf(CustomAnalyzer.class)); | ||
| CustomAnalyzer custom5 = (CustomAnalyzer) analyzer; |
There was a problem hiding this comment.
I could recreate these with mock car filters if anyone feels strongly about it but I think we have these covered very well in other places.
| assertThat(analyzeResponse.getTokens().get(0).getPositionLength(), equalTo(1)); | ||
| } | ||
|
|
||
| public void testAnalyzeWithCharFilters() throws Exception { |
There was a problem hiding this comment.
Covered by the rest tests in analysis-common.
| AnalyzeResponse analyzeResponse = admin().indices().prepareAnalyze().setIndex(indexOrAlias()).setText("THIS IS A PHISH") | ||
| .setExplain(true).addCharFilter("my_mapping").setTokenizer("keyword").addTokenFilter("lowercase").get(); | ||
|
|
||
| assertThat(analyzeResponse.detail().analyzer(), IsNull.nullValue()); |
There was a problem hiding this comment.
Covered by the rest tests in analysis-common.
| } | ||
|
|
||
| public void testDetailAnalyzeWithMultiValuesWithCustomAnalyzer() throws Exception { | ||
| assertAcked(prepareCreate("test").addAlias(new Alias("alias")) |
There was a problem hiding this comment.
Also covered by the rest tests in analysis-common.
|
|
||
|
|
||
| public void testCustomCharFilterInRequest() throws Exception { | ||
| Map<String, Object> charFilterSettings = new HashMap<>(); |
rjernst
left a comment
There was a problem hiding this comment.
LGTM. There are certainly things I think we should cleanup eventually, like requiresAnalysisSettings(), but this change is just moving stuff around so I think it is good for now.
|
Thanks for reviewing @rjernst! I've merged to master only. No backporting for analysis-common. |
Another step down the road to dropping the
lucene-analyzers-common dependency from core.
Note that this removes some tests that no longer compile from
core. I played around with adding them to the analysis-common
module where they would compile but we already test these in
the tests generated from the example usage in the documentation.
I'm not super happy with the way that
requriesAnalysisSettingsworks with regards to plugins. I think it'd be fairly bug-prone
for plugin authors to use. But I'm making it visible as is for
now and I'll rethink later.
A part of #23658