Helper for ConstructingObjectParser for record classes

[prdoyle]

Motivation

ConstructingObjectParser requires a builder parameter that accepts a Object[] and constructs the desired object using those as constructor parameters. This leads to boilerplate like this:

    @SuppressWarnings("unchecked")
    static final ConstructingObjectParser<ProfileDocumentUser, Void> PROFILE_DOC_USER_PARSER = new ConstructingObjectParser<>(
        "user_profile_document_user",
        false,
        (args, v) -> new ProfileDocumentUser(
            (String) args[0],
            (List<String>) args[1],
            (Authentication.RealmRef) args[2],
            (String) args[3],
            (String) args[4]
        )
    );

This is awkward when first developing such a record, when you might add, remove, or rearrange the fields often. It's especially hazardous if two of the arguments have the same datatype, as with the fourth and fifth arguments in the example above, because if you reorder those record components and forget to change this lambda expression, the compiler will not detect the mistake.

And the final insult is, it requires @SuppressWarnings("unchecked") if any of the arguments have generic types, like List<String> above, meaning you need to opt out of those warnings entirely.

Change

• Add a factory method ConstructingObjectParser.forRecord that accepts the record class and returns a parser for that record
• Change a few cases where we use the boilerplate above, to serve as examples.

[elasticsearchmachine]

Pinging @elastic/es-core-infra (Team:Core/Infra)

[prdoyle]

(For the future... if this were able to consume the context argument too, there would be a lot more cases where it would apply. The current one applies only to cases where context is ignored, which seems to be roughly half the places where records are used.)

[prdoyle]

Actually, looking into this some more, my own CPS requirements actually need this functionality, so that means this PR is insufficient for my own needs in its current form.

[mosche]

Love the idea of improving this, this is awful at the moment!
Looks good, though I suggest to restructure the code a little bit.

I'm not sure how much we have to worry about performance, but if recall right parsing has come up repeatedly in flame graphs. On the other hand, that was mostly chunking I think and none of the converted records look performance critical.

[mosche]

How about restructuring this a bit for readability and have recordBuilder return the actual builder BiFunction as expected by ConstructingObjectParser using

• <R extends Record> MethodHandle recordConstructor(Class<R> recordClass, MethodHandles.Lookup lookup) and
• <R extends Record, Context> BiFunction<Object[], Context, R> recordBuilder(MethodHandle ctor)

[prdoyle]

Yeah I thought the same thing actually. I should do that.

[mosche]

Typically this area is rather performance sensitive, not sure how much we have to worry here.

[mosche]

Wondering how invoke on an array-spreading method handle (asSpreader(Object[].class, ctorArgs.length)) compares to invokeWithArguments...

[prdoyle]

The javadocs say invokeWithArguments is equivalent to this:

MethodHandle invoker = MethodHandles.spreadInvoker(this.type(), 0);
Object result = invoker.invokeExact(this, arguments);

I could perhaps do the spreadInvoker ahead of time, leaving only the invokeExact.

[prdoyle]

Part of the motivation for this is to enable some performance optimization. By hiding the conversion from Object[] to a record, we could in principle make that intermediate Object[] unnecessary, and instantiate the record directly. That would require custom bytecode, but hey, we're not necessarily adverse to that in general.