Use NamedWritable to enable GeoBoundingBox serialisation

[craigtaverner]

BoundingBox allows inter-node serialisation only within aggs results as components of aggregations that themselves use NamedWritable. However, if a GeoBoundingBox is added to search hits, serialisation fails as this type is not explicitly supported by StreamOuput/StreamInput. This was never noticed before because the only way to get a BoundingBox into the hits is through a painless script, something that appears to have not been used by customers, and is only tested internally using a single-node, single-shard yaml test, which does not touch this code path. We only found this issue due to recent effort to run all yaml tests in a serverless environment, which appears to always require serialisation from backend data nodes to a coordinator node.

Having said that, it is a surface API that could be used by users, and so should be fixed. While the missing type could be added explicitly, a much more flexible approach is to allow named classes to encapsulate their own serialization/deserialization logic. Here we use NamedWritable for that purpose.

Tasks:

• Implement GenericNamedWriteable with version checking capabilities (based on VersionedNamedWriteable)
• Manual testing with a multi-node cluster to verify this fixes Failed Serialization of Scripted Geo fields  #99089
• Unit tests for version checking
• Unit tests for serialization of GeoBoundingBox
• Support CartesianBoundingBox by registering the named writable in SpatialPlugin

Fixes #99089

[elasticsearchmachine]

Pinging @elastic/es-analytics-geo (Team:Analytics)

[elasticsearchmachine]

Hi @craigtaverner, I've created a changelog YAML for you.

[DaveCTurner]

Looks great, just a few small comments.

[DaveCTurner]

I think we should just verify that writing to an older stream fails, no need to make a copyInstance which tries to read it back again.

[craigtaverner]

OK. After simplifying the test I replaced this with just the writing code.

[DaveCTurner]

I think this doesn't really make sense, we are always sure that reading and writing always use the same transport version, and the message we're creating isn't actually something an older node would be able to create.

[craigtaverner]

OK. I've simplified this failure test to only test the failure case. The first part with (valid,valid) was already tested in another test testSerialization, and the last part is unnecessary:

[DaveCTurner]

This is no longer needed, we can just use the copyInstance implementation in the subclass.

Suggested change

	@Override
	protected GenericNamedWriteable copyInstance(GenericNamedWriteable original, TransportVersion version) throws IOException {
	return copyInstance(original, version, version);
	}
	/**
	* Read and write with different serialization versions
	*/
	protected GenericNamedWriteable copyInstance(
	GenericNamedWriteable original,
	TransportVersion writeVersion,
	TransportVersion readVersion
	) throws IOException {
	try (BytesStreamOutput output = new BytesStreamOutput()) {
	output.setTransportVersion(writeVersion);
	output.writeGenericValue(original);
	try (StreamInput in = new NamedWriteableAwareStreamInput(output.bytes().streamInput(), getNamedWriteableRegistry())) {
	in.setTransportVersion(readVersion);
	return readGenericValue(in);
	}
	}
	}
	private GenericNamedWriteable readGenericValue(StreamInput in) throws IOException {
	Object obj = in.readGenericValue();
	if (obj instanceof GenericNamedWriteable result) {
	return result;
	}
	throw new IllegalStateException("Read result of wrong type: " + obj.getClass().getSimpleName());
	}

(also needs a spotless run to clean up imports)

[craigtaverner]

Unfortunately the parent version does not call output.writeGenericValue or in.readGenericValue which are the methods we added support for this to. Instead it calls directly the writeNamedWriteable and readNamedWriteable methods, skipping all the new code and version checks. But I see your point that at least I do not need the two-version version of the method, so I removed that.

[DaveCTurner]

acked, I think I can see a way to make this a little less special, but let's merge this and I'll follow up with a suggestion.

[DaveCTurner]

LGTM (left a comment about some now-unnecessary test code tho)