9.0 Preview 1 Feedback

[niemyjski]

References: #8485

Let me know if this format works best or you'd like new issues for each. I just got started as it was a busy weekend. Anything logged here is feedback addressing 9.0.

1. Query logical operators compiler errors.

Error CS0201 : Only assignment, call, increment, decrement, await, and new object expressions can be used as a statement

public record MyType
{
    public string Id { get; set; }
    public string Field1 { get; set; }
    public string Field2 { get; set; }
    public string Field3 { get; set; }
    public int Field4 { get; set; }
    public DateTime Field5 { get; set; }
    public string MultiWord { get; set; }
    public Dictionary<string, object> Data { get; set; } = new Dictionary<string, object>();
}

await Client.SearchAsync<MyType>(d => d
            .Indices("blah").Query(f => f.Term(m => m.Field(tf => tf.Field1).Value("value1")) || !f.Term(m => m.Field(tf => tf.Field2).Value("value2"))));

2. Query.TryGet<>(out)

This method seems to be missing on query we used this to do some parent child behavior in our query visitors.

 Query container = query;
        if (query.TryGet<NestedQuery>(out var nested) && node.Parent != null)
            container = null;

3. ExtendedBounds conversion to DateHistogramAggregation.ExtendedBounds

We might need an implicit conversion here?

Cannot convert source type 'Elastic.Clients.Elasticsearch.Aggregations.ExtendedBounds<System.DateTime>' to target type 'Elastic.Clients.Elasticsearch.Aggregations.ExtendedBounds<Elastic.Clients.Elasticsearch.Aggregations.FieldDateMath>?'

var bounds = new ExtendedBounds<DateTime> { Min = start.Value, Max = end.Value };
var agg = new DateHistogramAggregation
{
    Field = field,
    ExtendedBounds = bounds
};

4. SortOptions.Field usage

Constructor 'FieldSort' has 1 parameter(s) but is invoked with 0 argument(s)

This was working in 8.0 but not mentioned in release notes.

return SortOptions.Field(field, new FieldSort
{
    UnmappedType = fieldType == FieldType.None ? FieldType.Keyword : fieldType,
    Order = node.IsNodeOrGroupNegated() ? SortOrder.Desc : SortOrder.Asc
});

converted to

return new SortOptions 
{ 
    Field = new FieldSort(field)
    {
        UnmappedType = fieldType == FieldType.None ? FieldType.Keyword : fieldType,
        Order = node.IsNodeOrGroupNegated() ? SortOrder.Desc : SortOrder.Asc
    }
};

5. GetMappingResponse.Indicies

I'm logging this one as I didn't see anything in the previous release notes and this was compiling in 8 to my knowledge. Previously we were calling

public static ElasticMappingResolver Create<T>(ElasticsearchClient client)
{
    return Create(() =>
    {
        client.Indices.Refresh(Indices.Index<T>());
        var response = client.Indices.GetMapping(new GetMappingRequest(Indices.Index<T>()));

        // use first returned mapping because index could have been an index alias
        var mapping = response.Indices.Values.FirstOrDefault()?.Mappings;
        return mapping;
    }, client.Infer, logger);
}

Seems like this might need to change to:

var mapping = response.Mappings.FirstOrDefault().Value?.Mappings;

6. GeohashPrecision (constructor overloads)

Previous we passed a double to GeohashPrecision which now only takes a long or a string. I think this is now the correct behavior but just wanted to double check.

var precision = new GeohashPrecision(1);
if (!String.IsNullOrEmpty(node.UnescapedProximity) && Double.TryParse(node.UnescapedProximity, out double parsedPrecision))
{
    if (parsedPrecision is < 1 or > 12)
        throw new ArgumentOutOfRangeException(nameof(node.UnescapedProximity), "Precision must be between 1 and 12");

    precision = new GeohashPrecision(parsedPrecision);
}

7. CountAsync requires parameters.

This seems like it should be allowed by default to get the count of everything in a repository. This is a breaking change from 8.

var total = await Client.CountAsync<InvertTest>();

8. Range Queries breaking change.

Undocumented breaking change with TermRange and DateRange

 .Query(f => f.Range(r => r.DateRange(dr => dr.Field()))

to

 .Query(f => f.Range(r => r.Date(dr => dr.Field()))

9. Nested Mappings Breaking Changes

This was undocumented, really like this change for consistency but it might break a lot of people.

Error CS0308 : The non-generic method 'PropertiesDescriptor.Nested(PropertyName, NestedProperty)' cannot be used with type arguments

  string index = await CreateRandomIndexAsync<MyNestedType>(d => d.Properties(p => p
      .Text(e => e.Field1, o => o.Index())
      .Nested<MyType>(r => r.Name(n => n.Nested.First()).Properties(p1 => p1
          .Text(e => e.Field1, o => o.Index())
          .IntegerNumber(e => e.Field4)
      ))
  ));

to

string index = await CreateRandomIndexAsync<MyNestedType>(d => d.Properties(p => p
    .Text(e => e.Field1, o => o.Index())
    .Text(e => e.Field2, o => o.Index())
    .Text(e => e.Field3, o => o.Index())
    .IntegerNumber(e => e.Field4)
    .Nested(e => e.Nested, o => o.Properties(p1 => p1
        .Text(e1 => e1.Field1, o1 => o1.Index())
        .Text(e1 => e1.Field2, o1 => o1.Index())
        .Text(e1 => e1.Field3, o1 => o1.Index())
        .IntegerNumber(e => e.Field4)
    ))
));

However the dictionary case might be breaking intellisense as I'm getting auto completion for the top level fields..

string index = await CreateRandomIndexAsync<MyType>(m => m.Properties(p => p
    .Object<Dictionary<string, object>>(f => f.Name(e => e.Data).Properties(p2 => p2
        .Text(e => e.Name("@browser_version"))
        .FieldAlias(a => a.Name("browser.version").Path("data.@browser_version"))))));
       

to

string index = await CreateRandomIndexAsync<MyType>(m => m.Properties(p => p
    .Object(f => f.Data, o => o.Properties(p2 => p2
        .Text("@browser_version")
        .FieldAlias("browser.version", o1 => o1.Path("data.@browser_version"))))));

10. GeoDistanceQuery, GeoBoundingBoxQuery, TopLeftBottomRightGeoBounds parameter order.

I typed field in wrong which feels like what most of the api surface accepts as the first parameter. Feels like this should be looked over to make it feel more uniform.

for TopLeftBottomRightGeoBounds, it seems like the order should match the method name.

11. Query Generics for easier query generation.

We have the generic query client which gives us field inference/autocomplete. Any chance this can be brought back? Or a great solution for it?

    public class CompanyQueryBuilder : IElasticQueryBuilder
    {
        public Task BuildAsync<T>(QueryBuilderContext<T> ctx) where T : class, new()
        {
            var companyIds = ctx.Source.GetCompanies();
            if (companyIds.Count <= 0)
                return Task.CompletedTask;

            if (companyIds.Count == 1)
                ctx.Filter &= Query<Employee>.Term(f => f.CompanyId, companyIds.Single());
            else
                ctx.Filter &= Query<Employee>.Terms(d => d.Field(f => f.CompanyId).Terms(companyIds));

            return Task.CompletedTask;
        }
    }

[flobernd]

Hey @niemyjski, thanks for the feedback. This format works perfectly well for me!

I'll have a detailed look in a few hours :-)

[flobernd]

@niemyjski

1.

Ah, I see. This does not work, because all (most) of the fluent methods are using this signature:

public SearchRequestDescriptor<TDocument> Query(Action<QueryDescriptor<TDocument>> action)

The instance of QueryDescriptor<TDocument> gets created internally and passed to the action where it can be mutated.

Your code example does not mutate the original instance, but implicitly creates a new one by using the || operator. We could support this by introducing an overload with the following signature:

public SearchRequestDescriptor<TDocument> Query(Func<QueryDescriptor<TDocument>, QueryDescriptor<TDocument>> action)

I can see that this might be useful, but I have to think about the implications in detail. It would definitely increase complexity quite a bit, since it would have to be implemented for all complex cases (Query[], Dictionary<T, Query>, etc.).

This might be something that can be added in one of the upcoming patch releases.

Workaround:

Not as convenient, but declaring/creating the descriptor beforehand, works fine:

var combined =
    new QueryDescriptor<MyType>().Term(m => m.Field(tf => tf.Field1).Value("value1")) ||
    !new QueryDescriptor<MyType>().Term(m => m.Field(tf => tf.Field2).Value("value2"));

await client.SearchAsync<MyType>(d => d
    .Indices("blah")
    .Query(combined));

2.

This method is no longer required due to the container type improvements. It was as well problematic since it was only matching the type, but not the actual variant name (some containers have multiple variants with the same type in which case this method would just "randomly" returns a result).

The new recommended way of inspecting container types is to use simple pattern matching:

var query = new Query();

if (query.Nested is { } nested)
{
    // We have a nested query.
}

Thanks for pointing this out. I'll add a few lines about this to the release notes/changelog 🙂

3.

Just use the ExtendedBounds<FieldDateMath> type that is expected by DateHistogramAggregation directly:

var bounds = new ExtendedBounds<FieldDateMath> { Min = DateTime.UtcNow, Max = DateTime.UtcNow };
var agg = new DateHistogramAggregation
{
    Field = "field",
    ExtendedBounds = bounds
};

FieldDateMath itself does now have an implicit conversion from DateTime which allows you to initialize Min / Max from DateTime.

4.

Thanks, I'll make sure to explicitly mention this in the release notes. This is part of the ordering improvements.

5.

Right, this is a specification related change. The codegen name hint changed to mappings.

6.

Yes, this is correct. GeohashPrecision:

A precision that can be expressed as a geohash length between 1 and 12, or a distance measure like "1km", "10m".

I'll hand-craft this union in the future to improve usability. This had rather low priority and did not make it in the 9.0 release.

7.

This change is not intended. Thanks for reporting, I'll try to fix it before the 9.0 release.

8.

Good spot as well! I handcrafted the overloads with the old names, but used the wrong name for the partial class which caused them to not show up.

9.

This is actually a breaking change from 8.0 (or maybe 8.13) and the related usability issue is tracked in #8455.

From a codegen perspective, this is a horrible function since the "nested property" semantics are not captured in the specification and hardcoding conditions in the code generator is always something I try to avoid at all costs.

For usability, it would be good, if the generic type of the selected property is pushed down when using .Nested() instead of always pushing down the generic type of the top level property.

Currently, this leads to longer property select expressions, since the whole property path from the top level type to the nested property must always be specified.

10.

A very valid point that I as well noticed. Sadly, I don't have a good solution for this right now. The arguments are ordered alphabetically which causes these little inconsistencies.

One option is to use named arguments field: "my_field", but I think most C# devs won't do that. What's the TFM you are using btw.? For NET7+, the parameterless constructor is not deprecated which allows to use the object initializer to set the required properties. For backwards compatibility reasons I sadly could not enable this feature for generic TFMs like e.g. netstandard2.0.

11.

Oh, I never saw that syntax before 😅 I can definitely bring that back with a slightly different syntax like e.g. QueryDescriptor<T>.Build(x => x.Term(...)) which is actually a method that already exists internally for all descriptors:

[System.Runtime.CompilerServices.MethodImpl(System.Runtime.CompilerServices.MethodImplOptions.AggressiveInlining)]
internal static Elastic.Clients.Elasticsearch.QueryDsl.Query Build(System.Action<Elastic.Clients.Elasticsearch.QueryDsl.QueryDescriptor> action)
{
    var builder = new Elastic.Clients.Elasticsearch.QueryDsl.QueryDescriptor(new Elastic.Clients.Elasticsearch.QueryDsl.Query(Elastic.Clients.Elasticsearch.Serialization.JsonConstructorSentinel.Instance));
    action.Invoke(builder);
    return builder.Instance;
}

[niemyjski]

1. That makes sense. It's how we generate the queries in all of our apps and other apps that I come across where the new query container was being created. This overload would be a massive help as this would be a massive change to hundreds of files.
2. Library targets netstandard 2.0, our apps target LTS (8). All of this is due to code gen? Seems like that could be a massive issue if a new property is ever added (starting with a) and breaks all existing call sites.
3. That works, the type was QueryContainer which I changed to Query. I was wondering if I could change that to QueryDescriptor but didn't have time. We probably have 20+ statements like this in our apps when we need to do statement based logic.

Thanks again.