un-Privatizing newFieldDescription and newTypeDescription

[plagupta]

Hi there!

I noticed that these were privatized in #592 but it's not clear why. We've been on v0.12.5 for a while and have a couple of places where we've been using NewFieldDescription and NewTypeDescription. Now that we are trying to upgrade, we've found it difficult to substitute our usage of these functions using the still-exposed functions available to us. Would y'all be willing to consider reverting this small change? This would be the diff, in case that's helpful:

diff --git a/common/types/pb/file.go b/common/types/pb/file.go
index e323afb..82b521e 100644
--- a/common/types/pb/file.go
+++ b/common/types/pb/file.go
@@ -32,7 +32,7 @@ func newFileDescription(fileDesc protoreflect.FileDescriptor, pbdb *Db) (*FileDe
        }
        types := make(map[string]*TypeDescription)
        for name, msgType := range metadata.msgTypes {
-               types[name] = newTypeDescription(name, msgType, pbdb.extensions)
+               types[name] = NewTypeDescription(name, msgType, pbdb.extensions)
        }
        fileExtMap := make(extensionMap)
        for typeName, extensions := range metadata.msgExtensionMap {
@@ -42,7 +42,7 @@ func newFileDescription(fileDesc protoreflect.FileDescriptor, pbdb *Db) (*FileDe
                }
                for _, ext := range extensions {
                        extDesc := dynamicpb.NewExtensionType(ext).TypeDescriptor()
-                       messageExtMap[string(ext.FullName())] = newFieldDescription(extDesc)
+                       messageExtMap[string(ext.FullName())] = NewFieldDescription(extDesc)
                }
                fileExtMap[typeName] = messageExtMap
        }
diff --git a/common/types/pb/type.go b/common/types/pb/type.go
index 6cc95c2..1106d78 100644
--- a/common/types/pb/type.go
+++ b/common/types/pb/type.go
@@ -38,16 +38,16 @@ type description interface {
        Zero() proto.Message
 }

-// newTypeDescription produces a TypeDescription value for the fully-qualified proto type name
+// NewTypeDescription produces a TypeDescription value for the fully-qualified proto type name
 // with a given descriptor.
-func newTypeDescription(typeName string, desc protoreflect.MessageDescriptor, extensions extensionMap) *TypeDescription {
+func NewTypeDescription(typeName string, desc protoreflect.MessageDescriptor, extensions extensionMap) *TypeDescription {
        msgType := dynamicpb.NewMessageType(desc)
        msgZero := dynamicpb.NewMessage(desc)
        fieldMap := map[string]*FieldDescription{}
        fields := desc.Fields()
        for i := 0; i < fields.Len(); i++ {
                f := fields.Get(i)
-               fieldMap[string(f.Name())] = newFieldDescription(f)
+               fieldMap[string(f.Name())] = NewFieldDescription(f)
        }
        return &TypeDescription{
                typeName:    typeName,
@@ -131,8 +131,8 @@ func (td *TypeDescription) Zero() proto.Message {
        return td.zeroMsg
 }

-// newFieldDescription creates a new field description from a protoreflect.FieldDescriptor.
-func newFieldDescription(fieldDesc protoreflect.FieldDescriptor) *FieldDescription {
+// NewFieldDescription creates a new field description from a protoreflect.FieldDescriptor.
+func NewFieldDescription(fieldDesc protoreflect.FieldDescriptor) *FieldDescription {

Thank you for your consideration!

[plagupta]

Also apologies if this should have been an Issue instead - it did not feel like a bug or big enough to warrant a feature request.

[TristonianJones]

@plagupta can you help me understand what you're trying to accomplish with these methods? From a CEL perspective, these are internal implementation details which were improperly exposed early in development and which have proven to be problematic to upgrade. If you can point me toward your repo, perhaps, I can suggest an alternative approach?

[plagupta]

Thank you for the swift response, @TristonianJones! Unfortunately I'm unable to share the repo as it is private and belongs to my employer. But at a high level, we are using CEL to implement a DSL to define anti-fraud rules that we can apply to incoming traffic. The DSL is based on certain protobuf message/enum definitions that we have. For example, if we have the following:

enum Action {
  ACTION_ONE = 0;
  ACTION_TWO = 1;
  ACTION_THREE = 2;
}

enum Decision {
  DECISION_ALLOW = 0;
  DECISION_BLOCK = 1;
}

message Features {
  int64 id = 1;
  Action action = 2;
  string request_id = 3;
  string session_id = 4;
  bool is_bot = 5;
}

then a DSL rule may look something like:

rule: block_bots
description: returns BLOCK if bot was detected
statements:
  - if: features.is_bot && features.action == ACTION_ONE
    then: BLOCK "bot detected"

We use NewTypeDescription and NewFieldDescription to make sure that detailed field/type information can be shared between proto and CEL so that we can properly validate rules, if that makes sense? To be frank, I did not write the code and don't have a perfect understanding of it myself so apologies if this explanation is a little shaky.

[TristonianJones]

@plagupta you should be able to use the protoreflect library to determine whether the enum being referenced was configured with CEL since CEL is effectively taking in a set of descriptors and then indexing them internally. I know it may not be convenient, but these methods are very much internal implementation details of CEL.

[hbelmiro]

@TristonianJones can you please suggest an alternative for https://github.com/kubeflow/pipelines/blob/1b107eb4bb2510ecb99fd5f4fb438cbf7c96a87a/backend/src/v2/expression/expression.go#L193?

[TristonianJones]

Hi @hbelmiro, I would recommend looking at the code for cel.ContextProtoVars which is doing something similar. If you'd like to contribute a PR to take an option for ContextProtoVars that allows for skipping unset fields, this would likely be roughly equivalent to what you're doing and sets up future changes for publishing fields as optional_type as well.

cel-go/cel/options.go

Lines 588 to 612 in 24a38ef

	func ContextProtoVars(ctx proto.Message) (interpreter.Activation, error) {
	if ctx == nil || !ctx.ProtoReflect().IsValid() {
	return interpreter.EmptyActivation(), nil
	}
	reg, err := types.NewRegistry(ctx)
	if err != nil {
	return nil, err
	}
	pbRef := ctx.ProtoReflect()
	typeName := string(pbRef.Descriptor().FullName())
	fields := pbRef.Descriptor().Fields()
	vars := make(map[string]any, fields.Len())
	for i := 0; i < fields.Len(); i++ {
	field := fields.Get(i)
	sft, found := reg.FindStructFieldType(typeName, field.TextName())
	if !found {
	return nil, fmt.Errorf("no such field: %s", field.TextName())
	}
	fieldVal, err := sft.GetFrom(ctx)
	if err != nil {
	return nil, err
	}
	vars[field.TextName()] = fieldVal
	}
	return interpreter.NewActivation(vars)