Consider server interceptor to check for unknown fields

GrpcErrorHelper.checkHasNoUnkownFields/Recursive exists as a utility for validating protobuf messages from the client and ensuring that no tags are included in the payload that the server couldn't decode - presumably from future clients, or otherwise slightly-incompatible clients. Only a few grpc calls run this validation today, and many of those non-recursively.

This ticket is to propose adding a server interceptor that validates all already-marshalled messages to ensure they only have known fields. Checking this could be as simple as req instanceof Message, to ensure that any protobuf marshalling has already taken place before validating (we have cases such as barrage/flight where messages are not marshalled yet, and it is up to the service impl to validate payloads).

This change would require no client changes, only a new interceptor added in the server, for all messages.

I think it is nice to have hooks that allow custom validation based on context; that entry point today, at least for table operations, is io.deephaven.server.table.ops.GrpcTableOperation#validateRequest.

Up until now, I've (mostly) preferred to finely control the validation logic for new table operations. For example, AggregateRequest:

    @Override
    public void validateRequest(AggregateRequest request) throws StatusRuntimeException {
        GrpcErrorHelper.checkHasField(request, AggregateRequest.SOURCE_ID_FIELD_NUMBER);
        GrpcErrorHelper.checkRepeatedFieldNonEmpty(request, AggregateRequest.AGGREGATIONS_FIELD_NUMBER);
        GrpcErrorHelper.checkHasNoUnknownFields(request);
        Common.validate(request.getSourceId());
        if (request.hasInitialGroupsId()) {
            Common.validate(request.getInitialGroupsId());
        }
        for (Aggregation aggregation : request.getAggregationsList()) {
            AggregationAdapter.validate(aggregation);
        }
    }

It's specifically not doing a recursive check at this level, and is explicitly validating each submessage as appropriate; ostensibly, the non-usage of checkHasNoUnkownFieldsRecursive in this case (and other cases where explicit validation is done) is to not need to re-do the validation logic (ie, the "speed" argument). Of course, this leaves room for programmer error (for example, if AggregateRequest had a new field added and this validateRequest impl was not updated).

I think I would be happy having either 1) apply checkHasNoUnkownFieldsRecursive if explicit validation is not provided, or 2) apply checkHasNoUnkownFieldsRecursive and explicit validation (if provided).

We could also think more general validation via annotations. It looks like https://github.com/bufbuild/protovalidate is supported by envoyproxy.

deephaven / deephaven-core

Consider server interceptor to check for unknown fields #5067