using new Field does not set ComparisonValue

[thespatt]

I just upgraded from 2.0.2 to 2.0.5 and discovered an issue in our code to create a highlight request. When converting Highlights to the Fields dictionary on HighlightRequest it was throwing an ArgumentNullException in Equals (elasticsearch-net/src/Nest/CommonAbstractions/Infer/Field/Field.cs line 101) and finally discovered that I had to change
return new Nest.Field { Boost = field.Boost, Name = field.Name };
to
return Nest.Field.Create(field.Name, field.Boost);

[gmarz]

Thanks for raising this @thespatt.

Field is implicitly convertible from a string or expression, which will set ComparisonValue, so you shouldn't have to instantiate one directly, or use the static Create() method. For instance, you can pass the string literal "myfield" (or with a boost value: "myfield^1.2") to any method that takes a Field, or cast (Field)"myfield".

That said, we shouldn't allow Field to be misused like this (e.g. Nest.Field { Boost = field.Boost, Name = field.Name };) and should enforce the string/expression via constructors that call Create() internally. That however would be a binary breaking change, so we'll have to target this for 5.0.

[gmarz]

As a temporary solution in 2.x we could hack the property setters.

Thoughts @Mpdreamz @russcam ?

[russcam]

It'll address the problem for 2.x, so I'm 👍 for it there.

Can we make the ctor private in 5.x so that you have to use .Create() or implicit conversion, both of which set ComparisonValue?

[gmarz]

Cool. Removed the 5.0 label :).

Can we make the ctor private in 5.x so that you have to use .Create() or implicit conversion, both of which set ComparisonValue?

I'd rather make Create() private and call it in the constructors.

[gmarz]

...or just remove Create() entirely and move the implementation to the ctors.

[russcam]

I can see a slight issue with hacking the property accessors for 2.x - if an API consumer were to set more than one of Name, Expression or PropertyInfo, then the last set accessor will be the one used for ComparisonValue. This seems like a reasonable compromise to fix this bug without breaking binary compatibility?