Add limits for ngram and shingle settings

[colings86]

Currently the options for ngram and shingle tokenizers/token filters allow the user to set min_size and max_size to any values. This is dangerous as users can set values which produces huge numbers of terms and at best bloat their index but at worst cause problems such as #25841.

I think we should add soft (and/or maybe hard) limits so that neither min_size or max_size can be more than say 6 and the difference between min_size and max_size can't be more than 2 or 3 (we may even want to make this limit 1).

Note that this does not apply to edge_ngrams where it is useful to have higher values and a larger difference between min and max values. We should probably decide if there should be different limits here though.

[nik9000]

I think this is worth having a look at. A soft limit will catch anyone just playing around and give back an error so they know this can be problematic.

[martijnvg]

We discussed during fixit friday and we all agreed on the fact that there should be a soft limit for the difference between the min_size and max_size settings. In case of the ngram tokenizer / token filter the soft limit should be 0 and the shingle token filter's soft limit should be 3. For the edge_ngram tokenizer / token filter there should be no soft limit.

[matarrese]

Hello guys,
I was having a look at this issue.
Should be raised a new IllegalArgumentException in case the conditions are not satisfied?
Is the constructor of ShingleTokenFilter a good place to do these validations?

[mayya-sharipova]

I guess we will raise IllegalArgumentException if conditions are not satisfied .
I was thinking we can do a check at the level of org.elasticsearch.index.analysis.ShingleTokenFilterFactory and org.elasticsearch.index.analysis.NGramTokenizerFactory.

[colings86]

@mayya-sharipova If we change the settings in those classes to use org.elasticsearch.common.settings.Setting.intSetting(String, int, int, int, Property...) instead of Settings.getAsInt() then we should be able to define and validate the min and max values using the Settings infrastructure and the exception will be consistent with other similar validation errors thrown form settings.

[jimczi]

Analyzers use a group setting (analysis.*) to register their settings. We don't check the inner setting for analyzers which is why we have to use Settings.getAs.... We should at some point move the analyzers options to checked settings (org.elasticsearch.common.settings.Setting) but that would be a breaking change for plugins devs and not a low hanging fruit.
For this issue I think it's fine to check the conditions in the factories directly and throw an IllegalArgumentException if conditions are not met as @mayya-sharipova suggests.
I'll open an issue to discuss how we could validate inner settings for analyzers but that's not a low hanging fruit ;).

[mayya-sharipova]

@jimczi @colings86 Do we want to create a new Setting for the difference between min_size and max_size? If yes, where do keep this setting?
If no, does it mean that will always throw an exception for example when Ngram min_gram != max_gram?

When we say " soft limit should be 0" what do we mean? Does it mean always throwing an exception when the limit is not 0?