DateHistogramAggregation - changing Interval to FixedInterval produces an invalid request

[awelburn]

NEST/Elasticsearch.Net version: 7.8.1
Elasticsearch version: 7.8.0

Description of the problem including expected versus actual behavior:
Converting from 6.x to 7, using NEST.7xUpgradeAssistant (really helpful btw!)
When changing code from
.Interval(DateInterval.Day)
the comment on the Obsolete attribute implies that the equivalent call should be
.FixedInterval(DateInterval.Day)
but this seems to produce an invalid request? (showing only the aggs fragment):
"aggs": {"doccountbyday": {"date_histogram": {"field": "published","fixed_interval": "day"}}}
The Reason is
failed to parse setting [date_histogram.fixedInterval] with value [day] as a time value: unit is missing or unrecognized

I've figured out that the new code should be
.FixedInterval(new Time(1, TimeUnit.Day))
which does produce a valid request:
{"aggs":{"doccountbyday":{"date_histogram":{"field":"published","fixed_interval":"1d"}}}

Expected behavior
Either remove FixedInterval(DateInterval interval) - it's not a breaking change if it's already broken?!
or perform the conversion from DateInterval to Time within the method?

[russcam]

Thanks for opening @awelburn.

This is an unfortunate bug- FixedInterval should accept only a Time, since a unit needs to be supplied.

Either remove FixedInterval(DateInterval interval) - it's not a breaking change if it's already broken?!
or perform the conversion from DateInterval to Time within the method?

You make a good point about when is a breaking change acceptable :) My personal preference is to fix this in master by breaking, and implement a formatter in 7.x to resolve at serialization; my reasoning here is that it's a small fix to workaround in 7.x, provides a "just works" path for folks upgrading, and doesn't break the signature IDateHistogramAggregation for everyone. FixedInterval doesn't support all of the units of DateInterval such as Month, Quarter and Year. For these, I don't think we have any choice with these but to throw an exception.

What are your thoughts, @Mpdreamz?