Fix pattern replace by making flag optional as on api

[grouh]

Description

This PR makes the flag optional on the pattern replace char filter, as on OS API

Issues Resolved

Fixes #886

[reta]

Thank you, @grouh , please add CHANGELOG entry and at least one test for the change.

[harawata]

I could be wrong, but replacement also is optional, isn't it?

[reta]

I could be wrong, but replacement also is optional, isn't it?

Hm ... I don't think so, the filter is not very useful if it replaces nothing (by and large, this is replace charfilter)

[harawata]

@reta ,

Thank you for the reply!

I don't disagree about the usefulness and replacement being required does not affect me, so please carry on. :)

I commented only because 1) the server accepts pattern_replace without replacement and 2) the replacement is optional in the Elasticsearch doc (I couldn't find OpenSeach doc about the filter).
https://www.elastic.co/guide/en/elasticsearch/reference/current/analysis-pattern-replace-charfilter.html

PUT my-index-000001
{
  "settings": {
    "analysis": {
      "analyzer": {
        "my_analyzer": {
          "tokenizer": "standard",
          "char_filter": [
            "my_char_filter"
          ],
          "filter": [
            "lowercase"
          ]
        }
      },
      "char_filter": {
        "my_char_filter": {
          "type": "pattern_replace",
          "pattern": "(?<=\\p{Lower})(?=\\p{Upper})"
        }
      }
    }
  },
  "mappings": {
    "properties": {
      "text": {
        "type": "text",
        "analyzer": "my_analyzer"
      }
    }
  }
}

[reta]

I don't disagree about the usefulness and replacement being required does not affect me, so please carry on. :)

Thank you @harawata , what would be the function of the char filter when pattern_replace is provided without replacement? The fact server accepts this combination points out there could be a problem there (and yes, we do have large gaps in documentation sadly).

[harawata]

@reta ,

I just did a quick test and it seems that when replacement is omitted, it simply removes the matched string (so, same behavior as replacement: "", probably?).

For example, with this index...

PUT my-index-000001
{
  "settings": {
    "analysis": {
      "analyzer": {
        "my_analyzer": {
          "type": "custom",
          "tokenizer": "standard",
          "char_filter": [
            "my_char_filter"
          ],
          "filter": [
            "lowercase"
          ]
        }
      },
      "char_filter": {
        "my_char_filter": {
          "type": "pattern_replace",
          "pattern": "Ba."
        }
      }
    }
  },
  "mappings": {
    "properties": {
      "text": {
        "type": "text",
        "analyzer": "my_analyzer"
      }
    }
  }
}

... the following returns ["the", "foo", "method"].

POST my-index-000001/_analyze
{
  "analyzer": "my_analyzer",
  "text": "The fooBarBaz method"
}

I'm still new to Elasticsearch/OpenSearch, so please forgive me if I'm missing something basic.

[reta]

I just did a quick test and it seems that when replacement is omitted, it simply removes the matched string (so, same behavior as replacement: "", probably?).

Thanks @harawata , you are totally and absolutely correct in your observations (pasting below the server side code that uses empty string if value is not provided):

replacement = settings.get("replacement", ""); // when not set or set to "", use "".

With that being said, I would prefer the client to require replacement, as an explicit intent what the replacement value should be, thank you.

[harawata]

Personally, I am OK with that.

I should, however, mention that the mismatch between server and client could cause an issue when retrieving the index.
Assuming that the index in my previous comment exists on the server, the following code throws the MissingRequiredPropertyException when the client internally deserializes the server response.

osClient.indices().get(x -> x.index("my-index-000001"));

This was actually how I encountered #886 at first.

[grouh]

If the consensus is to let replacement as mandatory for explicit use of the replace value, can this PR considered for merging ? or is there any other blocking point ?

[reta]

If the consensus si to let replacement as mandatory for explicit use of the replace value, can this PR considered for merging ? or is there any other blocking point ?

I think the deserialization failure mentioned by @harawata (mismatch between server and client could cause an issue when retrieving the index) leave us no choice - we have to make replacement optional. Could you please address that and also have a test for it? Thank you and thanks to @harawata to digging it through.

[grouh]

Thanks @reta @dblock @harawata @VachaShah