Custom analyzer to reject unknown parameters #89112

likzn · 2022-08-04T11:34:03Z

Main Change

Nowtime, when we create custom analyzer with some not required params("foo":"nar"), it will succeed. This PR will make check with these parms to ensure these were our need.

Close: #85710

PUT customanlyzerindex
{
    "settings": {
        "analysis": {
            "analyzer": {
                "my_analyzer": {
                    "type": "custom",
                    "tokenizer": "standard",
                    "foo": "bar"
                }
            }
        }
    }
}

likzn · 2022-08-04T11:48:09Z

@javanna @romseygeek @jtibshirani Hi, PTAL if free~

elasticsearchmachine · 2022-08-04T19:13:39Z

Pinging @elastic/es-search (Team:Search)

likzn · 2022-08-12T12:33:30Z

Hi, can someone take a look about it

likzn · 2022-08-20T09:01:15Z

@javanna Hi, PTAL

javanna

I left a couple of comments, the approach looks good to me. I am going to run tests and edit the title which is going to go in the changelog entry.

javanna · 2022-09-09T12:40:19Z

server/src/main/java/org/elasticsearch/index/analysis/AnalyzerComponents.java

+        for (String key : analyzerSettings.keySet()) {
+            switch (key) {
+                case "tokenizer", "char_filter", "filter", "type", "position_increment_gap" -> {}
+                default -> throw new IllegalArgumentException("Custom Analyzer not support [" + key + "] now");


could you rephrase the error to "Custom analyzer [" + name + "] does not support [" + key + "]" ?

javanna · 2022-09-09T12:42:27Z

modules/analysis-common/src/yamlRestTest/resources/rest-api-spec/test/search.query/10_match.yml

                  tokenizer: standard
                  filter: [lowercase]
                search:
-                  rest_total_hits_as_int: true


why was this change necessary?

javanna · 2022-09-09T12:42:41Z

...analysis-common/src/yamlRestTest/resources/rest-api-spec/test/indices.analyze/10_analyze.yml

+
+  - match: { status: 400 }
+  - match: { error.type: illegal_argument_exception }
+  - match: { error.reason: "Custom Analyzer not support [foo] now" }


++ thanks for adding this test!

javanna · 2022-09-09T12:44:59Z

server/src/main/java/org/elasticsearch/index/analysis/AnalyzerComponents.java

    ) {
+        for (String key : analyzerSettings.keySet()) {
+            switch (key) {
+                case "tokenizer", "char_filter", "filter", "type", "position_increment_gap" -> {}


another way of doing this, without deduplicating the expected analyzer keys, would be to duplicate the settings into a mutable map, which we'd remove from every time we read a certain setting. At the end, if any item is left in the map it means that some unsupported param has been provided.

javanna · 2022-09-09T12:46:36Z

server/src/test/java/org/elasticsearch/index/analysis/AnalysisTests.java

+        try {
+            createComponents("my_analyzer", analyzerSettings, testAnalysis.tokenizer, testAnalysis.charFilter, testAnalysis.tokenFilter);
+            fail("expected failure");
+        } catch (IllegalArgumentException e) {


you can use expectThrows instead .

javanna · 2022-09-09T12:47:34Z

server/src/test/java/org/elasticsearch/index/analysis/AnalysisTests.java

+    @BeforeClass
+    public static void setup() throws IOException {
+        testAnalysis = createTestAnalysis(new Index("test", "_na_"), settings);
+    }


do you need this as before class? It looks like it's only used in the new method that you introduced?

javanna · 2022-09-09T12:48:02Z

server/src/test/java/org/elasticsearch/index/analysis/AnalysisTests.java

        assertEquals(Arrays.asList("hello", "world"), wordList);
    }
+
+    public void testCustomAnalyzerWithNotSupportKey() {


testCustomAnalyzerWithUnsupportedKey ?

javanna · 2022-09-09T12:49:42Z

@elasticsearchmachine test this please

javanna · 2022-09-09T13:52:42Z

docs/changelog/89112.yaml

+area: Search/Analysis
+type: enhancement
+issues:
+ - 85710


the changelog generation is automatic in most situations, no need to create the entry yourself. And it gets updated as the labels and title of the PR get updated. Could you remove the changelog from your PR, then it should get automatically created with the expected area and all other fields. The current build errors are around the wrong area.

javanna · 2022-09-09T14:20:09Z

server/src/main/java/org/elasticsearch/index/analysis/AnalyzerComponents.java

        final Map<String, CharFilterFactory> charFilters,
        final Map<String, TokenFilterFactory> tokenFilters
    ) {
+        for (String key : analyzerSettings.keySet()) {


one additional suggestion: this is technically a breaking change, as existing indices will not be loaded if they have have an unknown param in the definition of a custom analyzer. We could mitigate this by making the new behaviour depend on the index created version, so that we'd throw error only for newly created indices. We'd need to have the index created version propagated from AnalysisRegistry though.

Sorry. I am late. But i find the method of createComponents is stateless. How can we propagate the index created version to it?

OK. I got it.

likzn · 2022-09-18T14:29:51Z

@javanna Hi, i resolved all comments. PTAL~

elasticsearchmachine · 2024-07-16T14:08:23Z

Pinging @elastic/es-search-relevance (Team:Search Relevance)

benwtrent · 2025-08-04T17:56:30Z

@javanna I am not sure we should actually make this change.

All analyzer builders accept unknown parameters without throwing, and it has been that way since the start. Seems like an expensive and broad ranging breaking change that might not be worth the squeeze.

javanna · 2025-08-12T14:31:09Z

I agree that it's a breaking change. I agree it is rather late to fix it. It does feel like something we'll want to fix though at some point, in that incorrect configuration is being silently accepted, as opposed to how Elasticsearch works in many other places. Ideally we'd promptly fail instead and provide feedback to users.

benwtrent · 2025-08-12T14:51:32Z

@javanna the real fix is to implement something like:

#41299 (comment)

And provide a deprecation warning on unknown parameters and deprecate the old ctor for custom analyzers that do not provide a "known settings" value.

I am closing this PR as it is very far away from a good path forward.

New work can be reopened through the plan laid out in the linked issue.

javanna · 2025-08-13T09:58:48Z

Sounds good to me @benwtrent thanks

make param check when create custom analyzer

91dacc3

elasticsearchmachine added needs:triage Requires assignment of a team area label external-contributor Pull request authored by a developer outside the Elasticsearch team v8.5.0 labels Aug 4, 2022

likzn added 2 commits August 4, 2022 19:40

add changelog

250959d

update issue in changelog

ea7106c

nik9000 added :Search Relevance/Analysis How text is split into tokens team-discuss and removed needs:triage Requires assignment of a team area label labels Aug 4, 2022

elasticsearchmachine added the Team:Search Meta label for search team label Aug 4, 2022

nik9000 added the >bug label Aug 4, 2022

javanna removed team-discuss v8.5.0 labels Aug 9, 2022

javanna reviewed Sep 9, 2022

View reviewed changes

javanna self-assigned this Sep 9, 2022

javanna added the v8.5.0 label Sep 9, 2022

javanna changed the title ~~Make param check when create custom analyzer~~ Custom analyzer to reject unknown parameters Sep 9, 2022

javanna reviewed Sep 9, 2022

View reviewed changes

resolve comment

90ba7bf

Merge branch 'main' into check_analyzer_param

0ed1304

likzn requested a review from javanna September 20, 2022 02:19

csoulios added v8.6.0 and removed v8.5.0 labels Sep 21, 2022

pugnascotia added v8.10.0 and removed v8.9.0 labels Jun 22, 2023

quux00 added v8.11.0 and removed v8.10.0 labels Aug 16, 2023

mattc58 added v8.12.0 and removed v8.11.0 labels Oct 4, 2023

brianseeders added v8.13.0 and removed v8.12.0 labels Dec 6, 2023

elasticsearchmachine added v8.14.0 and removed v8.13.0 labels Feb 14, 2024

elasticsearchmachine added v8.15.0 and removed v8.14.0 labels Apr 17, 2024

elasticsearchmachine added v8.16.0 and removed v8.15.0 labels Jul 4, 2024

javanna added Team:Search Relevance Meta label for the Search Relevance team in Elasticsearch and removed Team:Search Meta label for search team labels Jul 16, 2024

mark-vieira added v9.0.0 and removed v8.16.0 labels Sep 11, 2024

elasticsearchmachine added v9.1.0 and removed v9.0.0 labels Jan 30, 2025

elasticsearchmachine added v9.2.0 and removed v9.1.0 labels Jun 26, 2025

mayya-sharipova added the team-discuss label Aug 12, 2025

javanna removed their assignment Aug 12, 2025

benwtrent closed this Aug 12, 2025

Custom analyzer to reject unknown parameters #89112

Custom analyzer to reject unknown parameters #89112

Uh oh!

Conversation

likzn commented Aug 4, 2022

Main Change

Uh oh!

likzn commented Aug 4, 2022

Uh oh!

elasticsearchmachine commented Aug 4, 2022

Uh oh!

likzn commented Aug 12, 2022

Uh oh!

likzn commented Aug 20, 2022

Uh oh!

javanna left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

javanna commented Sep 9, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

likzn commented Sep 18, 2022

Uh oh!

elasticsearchmachine commented Jul 16, 2024

Uh oh!

benwtrent commented Aug 4, 2025

Uh oh!

javanna commented Aug 12, 2025

Uh oh!

benwtrent commented Aug 12, 2025

Uh oh!

javanna commented Aug 13, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

16 participants