Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CompositeAggregationSource is not serialized correctly #7822

Closed
asal-hesehus opened this issue Jul 12, 2023 · 7 comments
Closed

CompositeAggregationSource is not serialized correctly #7822

asal-hesehus opened this issue Jul 12, 2023 · 7 comments
Labels
8.x Relates to 8.x client version Category: Bug

Comments

@asal-hesehus
Copy link

asal-hesehus commented Jul 12, 2023

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
}
@asal-hesehus asal-hesehus added the 8.x Relates to 8.x client version label Jul 12, 2023
@flobernd flobernd added the bug label Jul 18, 2023
@flobernd
Copy link
Member

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

[JsonInclude, JsonPropertyName("terms")]
public Elastic.Clients.Elasticsearch.Aggregations.TermsAggregation? Terms { get; set; }

but the TermsAggregation type does this again:

writer.WriteStartObject();
writer.WritePropertyName("terms");

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

@nadiiaboichuk
Copy link

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

@sufyannisar
Copy link

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..

@flobernd flobernd added Category: Bug and removed bug labels Apr 4, 2024
@thebmo
Copy link

thebmo commented Apr 4, 2024

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
Copy link
Member

flobernd commented Apr 4, 2024

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
Copy link

thebmo commented Apr 4, 2024

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
Copy link
Member

flobernd commented Apr 5, 2024

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
8.x Relates to 8.x client version Category: Bug
Projects
None yet
Development

No branches or pull requests

5 participants