Better validation of mapping JSON

[colings86]

The parsers for the mappers now remove each setting as they parse it and an error will be thrown if any settings are left after parsing is complete

Closes #7205

[colings86]

I would like to get people's thoughts on this in particular as I am not sure which way to go on it

[brwe]

null_value is undocumented for murmur3 and also it seemed to have never worked at all (was not parsed and the set value ~0L was never used). I think we should remove it. Replacing the ~0L in ctor with null?

[jpountz]

Arbitrary null values don't really make sense for this mapper so I think it makes sense to remove support for it.

[jpountz]

However I think it is important that the default configuration emits an empty json

[brwe]

Was the intention of this to also work for root mappers like _all, _boost etc? I thought so because you changed AllFieldMapper but the change as is does not help detecting typos in fields of root mappers.

[brwe]

For the context suggester, one can still put arbitrary fields into the context, for example:

PUT testidx
{
  "mappings": {
    "service": {
      "properties": {
        "suggest_field": {
          "type": "completion",
          "context": {
            "location": {
              "type": "geo",
              "precision": "5m",
              "neighbors": true,
              "default": "u33",
              "foo": "bar"
            }
          }
        }
      }
    }
  }
}

[brwe]

This does not seem to work work for multi_fields yet, example below.
Maybe we need to check for each field directly after parsing if the mapping that is passed to the parser is empty instead of checking at the very end?

This should throw an exception but does not:

PUT testidx
{
  "mappings": {
    "doc": {
      "properties": {
        "text": {
          "type": "string",
          "fields": {
            "another_text": {
              "type": "string",
              "store": true,
              "foo": "bar"
            }
          }
        }
      }
    }
  }
}

[markharwood]

Does this message go back to the end user? If so the fact that a map must be empty is more of an implementation detail than an meaningful error message for the end user. Something like "Mapping definition for field X has unsupported parameters: foo, bar" would be more appropriate.

[brwe]

@colings86 I left some comments but I love this change already so much!

One thing I noticed is that if I add a faulty dynamic_mapping, the parser will still not complain:

DELETE testidx

DELETE _template/my_template
PUT _template/my_template
{
  "template": "test*",
  "mappings": {
    "_default_": {
      "dynamic_templates": [
        {
          "string_fields": {
            "mapping": {
              "type": "string",
              "foo":"bar"
            },
            "match": "*"
          }
        }
      ]
    }
  }
}
POST testidx/doc/1
{
  "text":1
}

GET _mapping

Should we do something about that as well?

[colings86]

@brwe I have implemented most of your comments and left some replies on some others. I agree that the dynamic mapping should also be fixed but maybe we should open a new issue for this so that this one can be merged first?

[clintongormley]

@colings86 please could you confirm whether this PR would fix #8317?

[colings86]

@clintongormley yes this PR will fix #8317

[colings86]

Suggestion from @brwe and @jpountz: Make it so the strict validation is only turned on if the index version is on or after the version that releases this change. This will eliminate the possibility of having a mapping that was created in a previous version that cannot be read by the current version. This is a precaution rather than a necessity

[clintongormley]

@brwe and @jpountz - please could you review?

[jpountz]

or just message + getRemainingFields(fieldNodeMap)?

[jpountz]

@clintongormley @colings86 As far as I am concerned, I think it's ready, I just left minor notes. There is an issue with fielddata and norms settings, but this PR solves most of the issue so I think we should not try to solve it in this PR and merge soon.

[brwe]

agree with @jpountz