BulkAll requires DefaultIndex or DefaultMappingFor with index

[russcam]

From: https://discuss.elastic.co/t/nest-client-bulkall-requires-defaultindexfor-or-defaultindex-to-work/189766

The BulkAll method of ElasticClient requires

• DefaultMappingFor<T>() with Index

or

• DefaultIndex()

or

• .Index() on the BulkAll call

to work.

You'll get an ArgumentException otherwise :

Index name is null for the given type and no default index is set. Map an index name using ConnectionSettings.DefaultMappingFor<TDocument>() or set a default index using ConnectionSettings.DefaultIndex()

Repro

static void Main(string[] args)
{
	var connectionSettings = new ConnectionSettings(new Uri("http://localhost:9200"))
	.DisableDirectStreaming()
	.PrettyJson()
	.OnRequestCompleted(callDetails =>
	{
		if (callDetails.RequestBodyInBytes != null)
		{
			Console.WriteLine(
				$"{callDetails.HttpMethod} {callDetails.Uri} \n" +
				$"{Encoding.UTF8.GetString(callDetails.RequestBodyInBytes)}");
		}
		else
		{
			Console.WriteLine($"{callDetails.HttpMethod} {callDetails.Uri}");
		}

		Console.WriteLine();

		if (callDetails.ResponseBodyInBytes != null)
		{
			Console.WriteLine($"Status: {callDetails.HttpStatusCode}\n" +
					 $"{Encoding.UTF8.GetString(callDetails.ResponseBodyInBytes)}\n" +
					 $"{new string('-', 30)}\n");
		}
		else
		{
			Console.WriteLine($"Status: {callDetails.HttpStatusCode}\n" +
					 $"{new string('-', 30)}\n");
		}
	});
	//uncomment one of the following lines to crash
	//connectionSettings.DefaultMappingFor<Document>(d => d.IndexName("myindex"));
	//connectionSettings.DefaultIndex("mydefaultindex");

	ElasticClient client = new ElasticClient(connectionSettings);

	BulkAllObservable<Document> bulk = client.BulkAll(GetDocuments(), b => b
		.BufferToBulk((descriptor, list) =>
		{
			foreach (Document doc in list)
			{
				descriptor.Index<Document>(bi => bi
					.Index($"myindex-{doc.Date:yyyy.mm.dd}")
					.Document(doc)
				);
			}
		})
		.Size(2)
	);

	bulk.Subscribe(new BulkAllObserver(
		onNext: r => Console.WriteLine(string.Join(";", r.Items.Select(i => i.Id))),
		onError: e => Console.Error.WriteLine(e),
		onCompleted: () => Console.WriteLine("Done")
	));
	bulk.Wait(TimeSpan.FromSeconds(30), null);
}

private static IEnumerable<Document> GetDocuments()
{
	for (int i = 0; i < 10; i++)
	{
		yield return new Document { Date = DateTime.UtcNow.AddDays(i) };
	}
}

private class Document
{
	public DateTime Date { get; set; }
}

[russcam]

This happens because the default IBulkAllRequest<T> implementations set the Index to typeof(T) so that index is set in the URL path for all bulk requests executed for the IEnumerable<T> collection. For example

elasticsearch-net/src/Nest/Document/Multiple/BulkAll/BulkAllRequest.cs

Lines 151 to 160 in c330114

	public class BulkAllDescriptor<T> : DescriptorBase<BulkAllDescriptor<T>, IBulkAllRequest<T>>, IBulkAllRequest<T>
	where T : class
	{
	private readonly IEnumerable<T> _documents;
	public BulkAllDescriptor(IEnumerable<T> documents)
	{
	_documents = documents;
	((IBulkAllRequest<T>)this).Index = typeof(T);
	}

and

elasticsearch-net/src/Nest/Document/Multiple/BulkAll/BulkAllRequest.cs

Lines 90 to 97 in c330114

	public class BulkAllRequest<T> : IBulkAllRequest<T>
	where T : class
	{
	public BulkAllRequest(IEnumerable<T> documents)
	{
	Documents = documents;
	Index = typeof(T);
	}

This reduces the size of bulk requests in the case where the index is the same for all operations, because it does not have to be repeated in the body of each operation. It does mean however that an index has to be mapped for T for this default to work, otherwise an exception will be thrown, as observed.

In the case where each item will explicitly define an index, the default index that the IBulkAllRequest<T> implementations will use can be explicitly overridden with .Index()

var observableBulk = Client.BulkAll(documents, f => f
	.Index(null)
	.BufferToBulk((b, smallObjects) =>
	{
		foreach (var o in objects)
		{
			b.Index<object>(bi => bi
				.Document(o)
				.Index("different_index")
			);
		}
	})
);

There's a trade-off in implementation here, I think:

1. Set a default index for IBulkAllRequest<T> based on T to reduce the size of bulk requests for the common index many use case.
2. Don't set a default index for IBulkAllRequest<T>, and allow index to be inferred for each operation from the document type T, if not explicitly specified.

The current implementation takes the first approach.

@Mpdreamz @codebrain we could potentially set Index to T for IBulkAllRequest<T> only when a default mapping is defined for T on ConnectionSettings? This does feel slightly more unintuitive to the current implementation though. Thoughts?

[Mpdreamz]

I can live with 2. We lose the benefit of 1 but agree it does make the API more intuitive when you always set index name on each operation.

[russcam]

Going with 2 would make the API behave differently to others e.g. Search<T>(), where index is inferred from T. I don't have a strong opinion about it, but think the current implementation is OK and we should document how it works in XML comments, site docs

[blemasle]

To reiterate my initial statement, the current implementation makes sense but has one major drawback in some use cases. If you have an index that change based on the document (say the document date for instance), you have to define a "default" index that will never be used.

For the person who code such a thing it's a "meh", but for the one who comes after it's a "what's the hell, I've never seen that index on our instances ?!" If the current implementation stays, I think I'll have to put something like DefaultIndexFor<T>("That index will never be used but I have to set it for the stack to work") 😄

[russcam]

you have to define a "default" index that will never be used.

You don't need to define this, you just need to set .Index(null) on the BulkAll call as in the example in #3951 (comment). Note that you may also need to explicitly specify an index for many other API calls that require an index too, which is the reason why .DefaultIndex() and .DefaultMappingFor<T>(m => m.IndexName()) exist 😃

There is a trade-off to be made either way in terms of implementation:

Current implementation

Set Index to typeof(T) to infer the index from T in IBulkAllRequest<T>

Pros

• the size of bulk requests is reduced because _index is not inferred from the document T of each bulk operation and sent in the bulk operation object, and instead forms part of the path, /{index}/_bulk, which means each bulk operation does not need to specify an index
• A user does not need to remember to set .Index(typeof(T)) for BulkAll() in order to benefit from reduced bulk request sizes
• Inferring the index from T for IBulkAllRequest<T> aligns with how the index is inferred from T for other API methods such as .Search<T>()

Cons

• if there's no default index specified for T, then it'll throw an exception.

The other possible implementation of Don't set Index to typeof(T) to infer the index from T in IBulkAllRequest<T> is the inverse of the pros and cons.

[blemasle]

You don't need to define this, you just need to set .Index(null) on the BulkAll call as in the example in #3951 (comment).

Oooh I didn't see that one :)
My bad, sorry. In this case, it is not very intuitive from the API definition and documentation or XML comments should indeed reflect that behaviour 👍

Thanks !

[russcam]

Totally agree that the XML docs and documentation can be better here!

[codebrain]

There's a trade-off in implementation here, I think:

1. Set a default index for IBulkAllRequest based on T to reduce the size of bulk requests for the common index many use case.
2. Don't set a default index for IBulkAllRequest, and allow index to be inferred for each operation from the document type T, if not explicitly specified.

Number 1. seems like the right thing to do. I imagine that many users of the Bulk API are inserting into a single index.

Number 2. would add the overhead of looking up the index per type (albeit this is cache-able). I think it is OK that it deviates from Search<T>() as the "direction" is different.

"we could potentially set Index to T for IBulkAllRequest only when a default mapping is defined for T on ConnectionSettings? This does feel slightly more unintuitive to the current implementation though. Thoughts?"

This does seem a little unintuitive, although we could always call out in the documentation...

Keep as-is and improve docs?

[russcam]

I'm ++ in keeping as is. and improving the documentation. I'll add something now