Use MultiFieldMapper as base for the core-mappers

[Tasteful]

Using MultiFieldMapper as a base class for the other core-mappers will make everything to work and we got the best of two solutions (adding all properties for mapping into MultiFieldMapper (#883) and depricate the MultiFieldMapper) see #873.

This change should not be any breaking change and we are still supporting version of ES (if it exists) that still sending out the "multi_field" as the mapping type.

[Tasteful]

@gmarz What do you think of this solution?

[gmarz]

@Tasteful, I like the general idea of this solution- great work. My only reservation is that I think it might be better to introduce a new abstract base class instead of using the existing MultiFieldMapping, so we can properly deprecate it, and at the same time be a little more generic with core types. Perhaps something like CoreMappingBase. What do you think?

[Tasteful]

The problem if a new abstract base class are introduced with the fields are
that the backward compatibility that will be lost and should go into 2.0.

The backward compatibility was the reason to still use the
MultiFieldMapping as the base class to not break any implementations in
project that using the client and be able to get that already in 1.x
release.

I agree that it should be better with probably two base classes (one for
all mappings that have all the common fields and one that extend that one
with the common core types fields).

Not sure if the backward compatibility can be solved with explicit/implicit
operators from a new abstract baseclass into the old MultiFieldMapping to
avoid breaking change with that implementation; otherwise it need to be in
2.0 release.
On 18 Aug 2014 18:03, "Greg Marzouka" [email protected] wrote:

@Tasteful https://github.com/Tasteful, I like the general idea of this
solution- great work. My only reservation is that I think it might be
better to introduce a new abstract base class instead of using the existing
MultiFieldMapping, so we can properly deprecate it, and at the same time
be a little more generic with core types. Perhaps something like
CoreMappingBase. What do you think?

—
Reply to this email directly or view it on GitHub
#885 (comment)
.

[gmarz]

Hm, but we can just leave MultiFieldMapping to keep backwards compatibility, extend its properties (#883) to fix #873, and then mark it obsolete. At the same time, we can add the new abstract base class with Fields that all of the core types can inherit from. This way, it's a bit cleaner, and we're doing most of the work that we'd end up doing in 2.0 anyway, while still keeping backwards compatibility- I think?. In 2.0, all we'll have to do is remove the now deprecated MultiFieldMapping. This would also require a change in the deserialization logic in ElasticTypeConverter in both 1.x and 2.0.

The only downside to this approach is that we'll have this hybrid MultiFieldMapping monster that contains properties from all of the core types. But at least it will be marked obsolete.

I can try this out and open a PR, then we can review both and decide which we like better. Thoughts?

[Tasteful]

If we in 1.x version let core-mappers (will use StringMapper as example) inherit the the MultiFieldMapper, then we can change the ElasticTypeConverter (included in this PR) to not make any special conversion for the MultiFieldMapper and instead create the StringMapper.

Then users that are using the library today can still use that but can also upgrade to the new version that are directly based on the StringMapper.

var field = (MultiFieldMapper)rootObjectMapping.Properties["my_multi_field"]; // will work
var field = (StringMapper)rootObjectMapping.Properties["my_multi_field"]; // will work

The json-serialization will be for the StringMapper that will include all the needed fields that not will break to postback the json that are fetched from ES.

For version 2.0 we are renaming the MultiFieldMapper as a breaking change and let that be the BaseCoreMapper.

This will also make it possible for users that are using the library to upgrade their code before doing that when the MultiFieldMapper are depricated.

---

If we in 1.x adding a new BaseCoreMapper for the core-types with the Fields-property we still need to add all the properties to the MultiFieldMapper to not break the json-serialization.

This will make it possible to create a multi-field direct direct from a StringMapping

var field = new StringMapper{Name = "my_field"};
field.Fields.Add(new StringMapper{Name = "sort", Analyzed = false});
// put mappings to the client

var mappingResponse = client.GetMapping(); // get the mappings again
var rootObjectMapping = mapping.Mapping;
var stringMapping = rootObjectMapping.Properties["my_field"] as StringMapping; // return null
var multiFieldMapping = rootObjectMapping.Properties["my_field"] as MultiFieldMapping; // works

but the result will be in consequent that we create from one type and getting another type back.

[gmarz]

Hey @Tasteful, take a look at #883, particularly this commit. I don't think this will introduce any breaking changes, and in 2.0 we can simply remove MultiFieldMapping. Let me know what you think.

[Tasteful]

Hi @gmarz, this implementation is good for me what I can see now the code will work in future, maybe some more cleanup/refactoring to move other common fields, like Name and Type to the base-class but that can also be done in step 2.

The only thing are if you are using the NEST library and have code like below; it will be a breaking change, indirect...

var mappingResponse = client.GetMapping(...);
var rootObjectMapping = mapping.Mapping;
var multiFieldMapping = rootObjectMapping.Properties["my_field"] as MultiFieldMapping;

the object multiFieldMapping will be null because that the parsed object from JSON are a StringMapping that not can be casted to MultiFieldMapping.

The only usage for the MultiFieldMapping are that the mappings can be created with that type but we are never deserializing to that object again.

It's a hard line to say where the breaking changes are in this case.

[gmarz]

Ah, good catch. 👍

rootObjectMapping.Properties["my_field"] as MultiFieldMapping;  

will definitely break.

I'll merge your changes into 1.1 and #883 into 2.0. I also agree that we can refactor the base-class further to include more of the common properties- I was actually contemplating doing so in that same PR but wanted to keep the intentions of the fix clear.