Reintroduce convenience where appropriate

[stefanobranco]

First of all I want to point out I'm very happy that with 8.13 and that it finally feels like we're moving towards the point where we can actually depreciate the nest client in our solution rather than using two clients at once.

When migrating however, I noticed that most of the changes that broke our code required rewrites that do the same thing, just in more code, most of which feels unnecessary. Maybe I've overlooked some obvious solutions though, so I'd be happy for suggestions.

Otherwise I want to at least point them out here, even though I fully understand these probably have not been at the top of the priorities when writing 8.13, so here are some examples.

First and foremost obviously the aggregation change, that requires an additional add, straight from the release notes:

var aggExampleResponse = await client.SearchAsync<StockData>(s => s
    .Aggregations(aggs => aggs
        .DateHistogram("by-month", dh => dh
            .CalendarInterval(CalendarInterval.Month)
            .Field(fld => fld.Date)
            .Aggregations(subaggs => subaggs
                .Sum("trade-volumes", sum => sum.Field(fld => fld.Volume))
            )
        )
    )
);

to:

var aggExampleResponse = await client.SearchAsync<StockData>(s => s
    .Aggregations(aggs => aggs
        .Add("by-month", agg => agg
            .DateHistogram(dh => dh
                .CalendarInterval(CalendarInterval.Month)
                .Field(fld => fld.Date)
            )
            .Aggregations(subaggs => subaggs
                .Add("trade-volumes", agg => agg
                    .Sum(sum => sum.Field(fld => fld.Volume))
                )
            )
        )
    )
);

In addition, I find subbaggregations alot clearer in the original example.

MatchAll now needs a parameter:

q => q.MatchAll()

to:

q => q.MatchAll(m => m.QueryName("all"))

Indexing:

await this.Client.IndexAsync(elasticCustomer, indexName, token);

to:

await this.Client.IndexAsync(elasticCustomer, i => i.Index(indexName), token);

Highlighting (simplified):

private HighlightField HighlightField => new() { PreTags = new List<string> { "<span style=\"style\">" }, PostTags = new List<string> { "</span>" } };


.Highlight(h => h.Fields(fs => fs.Add(new Field("field"), this.HighlightField)))

to:

private HighlightField HighlightField => new() { PreTags = new List<string> { "<span style=\"style\">" }, PostTags = new List<string> { "</span>" } };


.Highlight(h => h.Fields(fs => fs.Add(new Field("field"), hf => hf.PreTags(this.HighlightField.PreTags).PostTags(this.HighlightField.PostTags))))

Again I'm grateful for the effort I can live with all these, but I figured I'd at least point them out in case some of these are just oversights (e.g. MatchAll).

[flobernd]

Hi @stefanobranco,

1. The aggregation change requires a little bit of additional code, but on the same time this new structure maps closely to the corresponding JSON request. This is a good thing as it's consistent now with the REST documentation and as well allowed me to remove a lot of internal hacks that were previously required to support the old strucutre in the .NET client (these hacks caused several issues in the past).
2. I will bring back the parameterless MatchAll(). This one somehow got lost in the refactoring process 🙂
3. The HighlightField case I have to check in detail, but it seems like you are mixing up descriptors and class initializer style here. A solution would be to initialize a HighlightFieldDescriptor with your default values instead of HighlightField:

private HighlightFieldDescriptor HighlightField = new().PreTags(...).PostTags(...);

.Highlight(h => h.Fields(fs => fs.Add(new Field("field"), HighlightField)))

[stefanobranco]

Hi @flobernd!

Thanks for the quick response.

1. The aggregation change requires a little bit of additional code, but on the same time this new structure maps closely to the corresponding JSON request. This is a good thing as it's consistent now with the REST documentation and as well allowed me to remove a lot of

I figured it was something like that. I guess I find the 'Add' a bit 'unattractive', but that's probably a personal thing and it's completely reasonable as it is. I did notice one more small thing, as I'm trying to now completely move away from our nest client. When ordering aggregations:

Before (Nest):

.Order(o => o.CountDescending())

Now (unless I missed something):

.Order(new List<KeyValuePair<Field, SortOrder>>(new []{new KeyValuePair<Field, SortOrder>(Field.CountField, SortOrder.Desc)})))

2. I will bring back the parameterless MatchAll(). This one somehow got lost in the refactoring process 🙂

Perfect, I figured this was just an oversight

3. The HighlightField case I have to check in detail, but it seems like you are mixing up descriptors and class initializer style here. A solution would be to initialize a HighlightFieldDescriptor with your default values instead of HighlightField:

You're right. I'm pretty sure the previous variant did work that way but either way your solution works just as well and is more consistent, so there's probably no need to spend more time on this.

[stijnherreman]

.Order(new List<KeyValuePair<Field, SortOrder>>(new []{new KeyValuePair<Field, SortOrder>(Field.CountField, SortOrder.Desc)})))

You don't need the List, and depending on the C# version you can write it like this:

// C# 9
.Order(new KeyValuePair<Field, SortOrder>[] { new(Field.CountField, SortOrder.Desc) })

// C# 12
.Order([new(Field.CountField, SortOrder.Desc)])

[stefanobranco]

.Order(new List<KeyValuePair<Field, SortOrder>>(new []{new KeyValuePair<Field, SortOrder>(Field.CountField, SortOrder.Desc)})))

You don't need the List, and depending on the C# version you can write it like this:

// C# 9
.Order(new KeyValuePair<Field, SortOrder>[] { new(Field.CountField, SortOrder.Desc) })

// C# 12
.Order([new(Field.CountField, SortOrder.Desc)])

Hahaha you're right of course, the list is beyond silly here. I guess the C# 12 version is reasonable enough in that case.