elastic / elasticsearch-net

This strongly-typed, client library enables working with Elasticsearch. It is the official client maintained and supported by Elastic.
https://www.elastic.co/guide/en/elasticsearch/client/net-api/current/index.html
Apache License 2.0
9 stars 1.15k forks source link

CompositeAggregationSource is not serialized correctly #7822

Closed asal-hesehus closed 7 months ago

asal-hesehus commented 1 year ago

Elastic.Clients.Elasticsearch version: 8.6.2

Elasticsearch version: 8.1.1

.NET runtime version: 6.0

Operating system version: Windows 11

Description of the problem including expected versus actual behavior: CompositeAggregationSource is not serialized correctly. The query below is serialized to

{
    "aggregations": {
        "my_aggregation": {
            "composite": {
                "size": 10000,
                "sources": [
                    {
                        "path": {
                            "terms": {
                                "terms": {
                                    "field": "my_field"
                                }
                            }
                        }
                    }
                ]
            }
        }
    },
    "size": 0
}

Which fails to parse on the elastic search side as there are one nested "terms" to much. The error returned is: [composite] failed to parse field [sources]","caused_by":{"type":"x_content_parse_exception","reason":"[1:91] [terms] unknown field [terms]"}}

Steps to reproduce:

        var response = await elasticClient
            .SearchAsync<MyDocument>(
                x => x.Index(indexName)
                    .Aggregations(aggregationDescriptor => aggregationDescriptor
                    .Composite(CompositeName, compositeAggregationDescriptor =>
                    {
                        var term = new TermsAggregation("my_aggregation")
                        {
                            Field = "my_field"
                        };
                        compositeAggregationDescriptor.Sources(new IDictionary<string, CompositeAggregationSource>[]
                        {
                            new Dictionary<string, CompositeAggregationSource>
                            {
                                { CompositeKey, new CompositeAggregationSource { Terms = term } }
                            }
                        }).Size(10000).Size(0);
                    })));

Expected behavior I would expect the json to be:

{
    "aggs": {
        "my_aggregation": {
            "composite": {
                "size": 10000
            },
            "sources": {
                "path": {
                    "terms": {
                        "field": "my_field"
                    }
                }
            }
        }
    },
    "size": 0
}
flobernd commented 1 year ago

I can confirm that this is a bug. The CompositeAggregrationSource serializes the "terms" property on top level:

https://github.com/elastic/elasticsearch-net/blob/a61e8037635e3c91e8da38363ed67f4cde28600b/src/Elastic.Clients.Elasticsearch/_Generated/Types/Aggregations/CompositeAggregationSource.g.cs#L38-L39

but the TermsAggregation type does this again:

https://github.com/elastic/elasticsearch-net/blob/a61e8037635e3c91e8da38363ed67f4cde28600b/src/Elastic.Clients.Elasticsearch/_Generated/Types/Aggregations/TermsAggregation.g.cs#L272-L273

This is a bug in the code generator and as well affects DateHistogramAggregation, GeotileGridAggregation and HistogramAggregation.

nadiiaboichuk commented 9 months ago

Hi, when can we expect that CompositeAggregation is fixed in v8 Elasticsearch client?

sufyannisar commented 8 months ago

Hi, in the latest release (8.12.0) CompositeAggregationSource class no longer has any fields, it's an empty sealed class: public sealed partial class CompositeAggregationSource { } CompositeAggregationSource.cs

So you can't perform any Sources query..

thebmo commented 7 months ago

Any update on this? Our solution would really love to make use of that paging feature. Sending clients a 65k+ entry response is very unideal.

flobernd commented 7 months ago

Hi @thebmo,

could you please check if it's working for you in 8.13.1?

This version is based on a completely revised code generator.

thebmo commented 7 months ago

Thanks @flobernd It looks to be working now, thanks for a zippy response. To your point, the fluent api patterns seemed to have dramatically changed but we were able to append our aggregations such. Hopefully this helps others as the documentation isnt quite there yet. Note that the searchDescriptor here was built earlier with the query we needed.

(dotnet v8 on the 8.13.1 version of Elastic.Clients.Elasticsearch )

var compositeAggregation = new CompositeAggregation
{
    Sources = new Dictionary<string, CompositeAggregationSource>[]
        {
            new Dictionary<string, CompositeAggregationSource>
            {
                {
                    "fieldName", new CompositeAggregationSource
                    {
                        Terms =  new CompositeTermsAggregation
                        {
                            Field = "some-field-to-bucket"
                        }
                    }
                }
            }
        },
    Size = 10
};

searchDescriptor.Aggregations(aggs =>
    aggs.Add("myAggregation", new AggregationDescriptor<ProjectType>()
        .Composite(compositeAggregation)
    ))
.Size(0);
flobernd commented 7 months ago

@thebmo Glad to hear that it's working now 🙂

Thank you as well for the example. This will indeed be useful to other users.

This particular change is documented in the release notes of the GitHub release (that's probably not a prominent place ... I'll make sure to port the release notes over to our official documentation).

Closing this issue right now as the original bug is fixed. Feel free to open a new one in case you encounter another bug or if you need assistance with anything else.