Specify better error message and code for invalid field selections

PiperOrigin-RevId: 625085969

Author: l46kok

runtime/src/main/java/dev/cel/runtime/BUILD.bazel

@@ -220,7 +220,9 @@ java_library(
     srcs = ["RuntimeTypeProviderLegacyImpl.java"],
     deps = [
         ":unknown_attributes",
+        "//common:error_codes",
         "//common:options",
+        "//common:runtime_exception",
         "//common/annotations",
         "//common/internal:cel_descriptor_pools",
         "//common/internal:dynamic_proto",

runtime/src/main/java/dev/cel/runtime/DescriptorMessageProvider.java

@@ -168,7 +168,7 @@ public Object selectField(Object message, String fieldName) {
       }
     }
 
-    MessageOrBuilder typedMessage = assertFullProtoMessage(message);
+    MessageOrBuilder typedMessage = assertFullProtoMessage(message, fieldName);
     FieldDescriptor fieldDescriptor = findField(typedMessage.getDescriptorForType(), fieldName);
     // check whether the field is a wrapper type, then test has and return null
     if (isWrapperType(fieldDescriptor) && !typedMessage.hasField(fieldDescriptor)) {
@@ -202,7 +202,7 @@ public Object hasField(Object message, String fieldName) {
       return map.containsKey(fieldName);
     }
 
-    MessageOrBuilder typedMessage = assertFullProtoMessage(message);
+    MessageOrBuilder typedMessage = assertFullProtoMessage(message, fieldName);
     FieldDescriptor fieldDescriptor = findField(typedMessage.getDescriptorForType(), fieldName);
     if (fieldDescriptor.isRepeated()) {
       return typedMessage.getRepeatedFieldCount(fieldDescriptor) > 0;
@@ -228,16 +228,16 @@ private FieldDescriptor findField(Descriptor descriptor, String fieldName) {
     return fieldDescriptor;
   }
 
-  private static MessageOrBuilder assertFullProtoMessage(Object candidate) {
+  private static MessageOrBuilder assertFullProtoMessage(Object candidate, String fieldName) {
     if (!(candidate instanceof MessageOrBuilder)) {
-      // This is an internal error. It should not happen for type checked expressions.
+      // This can happen when the field selection is done on dyn, and it is not a message.
       throw new CelRuntimeException(
-          new IllegalStateException(
+          new IllegalArgumentException(
               String.format(
-                  "[internal] expected an instance of 'com.google.protobuf.MessageOrBuilder' "
-                      + "but found '%s'",
-                  candidate.getClass().getName())),
-          CelErrorCode.INTERNAL_ERROR);
+                  "Error resolving field '%s'. Field selections must be performed on messages or"
+                      + " maps.",
+                  fieldName)),
+          CelErrorCode.ATTRIBUTE_NOT_FOUND);
     }
     return (MessageOrBuilder) candidate;
   }

runtime/src/main/java/dev/cel/runtime/RuntimeTypeProviderLegacyImpl.java

@@ -19,7 +19,9 @@
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Preconditions;
 import com.google.errorprone.annotations.Immutable;
+import dev.cel.common.CelErrorCode;
 import dev.cel.common.CelOptions;
+import dev.cel.common.CelRuntimeException;
 import dev.cel.common.annotations.Internal;
 import dev.cel.common.internal.CelDescriptorPool;
 import dev.cel.common.internal.DynamicProto;
@@ -69,17 +71,37 @@ public Object createMessage(String messageName, Map<String, Object> values) {
   @Override
   @SuppressWarnings("unchecked")
   public Object selectField(Object message, String fieldName) {
-    SelectableValue<CelValue> selectableValue =
-        (SelectableValue<CelValue>) protoCelValueConverter.fromJavaObjectToCelValue(message);
+    CelValue convertedCelValue = protoCelValueConverter.fromJavaObjectToCelValue(message);
+    if (!(convertedCelValue instanceof SelectableValue)) {
+      throw new CelRuntimeException(
+          new IllegalArgumentException(
+              String.format(
+                  "Error resolving field '%s'. Field selections must be performed on messages or"
+                      + " maps.",
+                  fieldName)),
+          CelErrorCode.ATTRIBUTE_NOT_FOUND);
+    }
+
+    SelectableValue<CelValue> selectableValue = (SelectableValue<CelValue>) convertedCelValue;
 
     return unwrapCelValue(selectableValue.select(StringValue.create(fieldName)));
   }
 
   @Override
   @SuppressWarnings("unchecked")
   public Object hasField(Object message, String fieldName) {
-    SelectableValue<CelValue> selectableValue =
-        (SelectableValue<CelValue>) protoCelValueConverter.fromJavaObjectToCelValue(message);
+    CelValue convertedCelValue = protoCelValueConverter.fromJavaObjectToCelValue(message);
+    if (!(convertedCelValue instanceof SelectableValue)) {
+      throw new CelRuntimeException(
+          new IllegalArgumentException(
+              String.format(
+                  "Error resolving field '%s'. Field selections must be performed on messages or"
+                      + " maps.",
+                  fieldName)),
+          CelErrorCode.ATTRIBUTE_NOT_FOUND);
+    }
+
+    SelectableValue<CelValue> selectableValue = (SelectableValue<CelValue>) convertedCelValue;
 
     return selectableValue.find(StringValue.create(fieldName)).isPresent();
   }

runtime/src/test/java/dev/cel/runtime/DescriptorMessageProviderTest.java

@@ -165,8 +165,8 @@ public void selectField_nonProtoObjectError() {
     CelRuntimeException e =
         Assert.assertThrows(
             CelRuntimeException.class, () -> provider.selectField("hello", "not_a_field"));
-    assertThat(e).hasCauseThat().isInstanceOf(IllegalStateException.class);
-    assertThat(e.getErrorCode()).isEqualTo(CelErrorCode.INTERNAL_ERROR);
+    assertThat(e).hasCauseThat().isInstanceOf(IllegalArgumentException.class);
+    assertThat(e.getErrorCode()).isEqualTo(CelErrorCode.ATTRIBUTE_NOT_FOUND);
   }
 
   @Test

runtime/src/test/resources/dyn_error.baseline

@@ -0,0 +1,23 @@
+Source: dyn('hello').invalid
+=====>
+bindings: {}
+error:    evaluation error at test_location:12: Error resolving field 'invalid'. Field selections must be performed on messages or maps.
+error_code:    ATTRIBUTE_NOT_FOUND
+
+Source: has(dyn('hello').invalid)
+=====>
+bindings: {}
+error:    evaluation error at test_location:3: Error resolving field 'invalid'. Field selections must be performed on messages or maps.
+error_code:    ATTRIBUTE_NOT_FOUND
+
+Source: dyn([]).invalid
+=====>
+bindings: {}
+error:    evaluation error at test_location:7: Error resolving field 'invalid'. Field selections must be performed on messages or maps.
+error_code:    ATTRIBUTE_NOT_FOUND
+
+Source: has(dyn([]).invalid)
+=====>
+bindings: {}
+error:    evaluation error at test_location:3: Error resolving field 'invalid'. Field selections must be performed on messages or maps.
+error_code:    ATTRIBUTE_NOT_FOUND

testing/src/main/java/dev/cel/testing/BaseInterpreterTest.java

@@ -1600,6 +1600,21 @@ public void dynConversions() throws Exception {
     runTest(Activation.EMPTY);
   }
 
+  @Test
+  public void dyn_error() throws Exception {
+    source = "dyn('hello').invalid";
+    runTest(Activation.EMPTY);
+
+    source = "has(dyn('hello').invalid)";
+    runTest(Activation.EMPTY);
+
+    source = "dyn([]).invalid";
+    runTest(Activation.EMPTY);
+
+    source = "has(dyn([]).invalid)";
+    runTest(Activation.EMPTY);
+  }
+
   // This lambda implements @Immutable interface 'Function', but 'InterpreterTest' has field 'eval'
   // of type 'com.google.api.expr.cel.testing.Eval', the declaration of
   // type