5.x API Feedback All enumerable results should be immutable and always return a result

[niemyjski]

In 1.x all collections were empty collections regardless of if the api result was valid or invalid. In 5.x this is very different and personally I don't like it as it makes me question what the api is going to do. Let me explain..

Previously this code along with bulk results would return an empty collection

            var alias = await _client.GetAliasAsync(descriptor => descriptor.Name(_configuration.DailyLogEvents.Name));
            _logger.Trace(() => alias.GetRequest());
            Assert.False(alias.IsValid);
            Assert.Equal(0, alias.Indices.Count);

Now alias.Indicies throws a null reference exception, the same with bulk operations... This leads us to have to do null checking everywhere and if blocks around foreachs...

What we have been doing in our api (apps and libs) is simple, we return readonly collections everywhere and in the case where there are no results (and it's an enumerable result) we return an shared empty result instance like:

private static IReadOnlyCollection<Aliases> EmptyAliases = new List<Aliases>(0).AsReadOnly();

This really cuts down on extra required code and keeps allocations to a minimal.

TLDR: Always return an empty read only collection instead of null + make everything immutable.

[niemyjski]

This just bit us again with aggregations:

        public override void Visit(GroupNode node, IQueryVisitorContext context) {
            base.Visit(node, context);

            var elasticContext = context as IElasticQueryVisitorContext;
            if (elasticContext == null)
                throw new ArgumentException("Context must be of type IElasticQueryVisitorContext", nameof(context));

            var container = GetParentContainer(node, context);
            foreach (var child in node.Children.OfType<IFieldQueryNode>()) {
                var aggregation = child.GetAggregation(() => child.GetDefaultAggregation(context));
                if (aggregation != null) {
                    container.Aggregations[((IAggregation)aggregation).Name] = (AggregationContainer)aggregation;
                }
            }

            if (node.Parent == null)
                node.SetAggregation(container);
        }

container.Aggregations was previously initialized and now it's not.

[Mpdreamz]

This is now implemented as per #2383, we we're not doing this consistently thanks @niemyjski 🍺 ++;