address comments

Author: shivaspeaks

interop-testing/build.gradle

@@ -50,8 +50,7 @@ dependencies {
             project(':grpc-inprocess'),
             project(':grpc-core'),
             libraries.mockito.core,
-            libraries.okhttp,
-            libraries.cel.compiler
+            libraries.okhttp
 
     signature (libraries.signature.java) {
         artifact {

xds/src/main/java/io/grpc/xds/internal/matcher/CelCommon.java

@@ -18,8 +18,13 @@
 
 import dev.cel.common.CelAbstractSyntaxTree;
 import dev.cel.common.CelOptions;
+import dev.cel.common.ast.CelReference;
 import dev.cel.runtime.CelRuntime;
 import dev.cel.runtime.CelRuntimeFactory;
+import dev.cel.runtime.CelStandardFunctions;
+import dev.cel.runtime.CelStandardFunctions.StandardFunction;
+import dev.cel.runtime.standard.AddOperator.AddOverload;
+import java.util.Map;
 
 /**
  * Shared utilities for CEL-based matchers and extractors.
@@ -31,17 +36,16 @@ final class CelCommon {
       .build();
 
 
-  private static final dev.cel.runtime.CelStandardFunctions FUNCTIONS = 
-      dev.cel.runtime.CelStandardFunctions.newBuilder()
+  private static final CelStandardFunctions FUNCTIONS = 
+      CelStandardFunctions.newBuilder()
           .filterFunctions((func, over) -> {
-            if (func == dev.cel.runtime.CelStandardFunctions.StandardFunction.STRING) {
+            if (func == StandardFunction.STRING) {
               return false;
             }
-            if (func == dev.cel.runtime.CelStandardFunctions.StandardFunction.ADD) {
-              return !over.equals(
-                      (Object) dev.cel.runtime.standard.AddOperator.AddOverload.ADD_STRING)
-                  && !over.equals(
-                      (Object) dev.cel.runtime.standard.AddOperator.AddOverload.ADD_LIST);
+            if (func == StandardFunction.ADD) {
+              // TODO: fix this, remove (object) casting when we upgrade to 0.12.0
+              return !over.equals((Object) AddOverload.ADD_STRING)
+                  && !over.equals((Object) AddOverload.ADD_LIST);
             }
             return true;
           })
@@ -56,9 +60,9 @@ final class CelCommon {
   private CelCommon() {}
 
   static void checkAllowedVariables(CelAbstractSyntaxTree ast) {
-    for (java.util.Map.Entry<Long, dev.cel.common.ast.CelReference> entry : 
+    for (Map.Entry<Long, CelReference> entry : 
         ast.getReferenceMap().entrySet()) {
-      dev.cel.common.ast.CelReference ref = entry.getValue();
+      CelReference ref = entry.getValue();
       // If overload_id is empty, it's a variable reference or type name.
       // We only support "request".
       if (!ref.value().isPresent() && ref.overloadIds().isEmpty()) {

xds/src/main/java/io/grpc/xds/internal/matcher/CelStateMatcher.java

@@ -17,6 +17,8 @@
 package io.grpc.xds.internal.matcher;
 
 import com.github.xds.core.v3.TypedExtensionConfig;
+import com.github.xds.type.matcher.v3.CelMatcher; // Proto
+import com.github.xds.type.v3.CelExpression;
 import dev.cel.common.CelAbstractSyntaxTree;
 import dev.cel.common.CelProtoAbstractSyntaxTree;
 import dev.cel.runtime.CelEvaluationException;
@@ -25,9 +27,9 @@
  * Matcher for CEL expressions handling xDS CEL Matcher extension.
  */
 final class CelStateMatcher implements Matcher {
-  private final CelMatcher compiledEndpoint;
+  private final io.grpc.xds.internal.matcher.CelMatcher compiledEndpoint;
 
-  CelStateMatcher(CelMatcher compiledEndpoint) {
+  CelStateMatcher(io.grpc.xds.internal.matcher.CelMatcher compiledEndpoint) {
     this.compiledEndpoint = compiledEndpoint;
   }
 
@@ -47,21 +49,22 @@ public Class<?> inputType() {
 
   static final class Provider implements MatcherProvider {
     @Override
-    public Matcher getMatcher(TypedExtensionConfig config) {
+    public CelStateMatcher getMatcher(TypedExtensionConfig config) {
       try {
-        com.github.xds.type.matcher.v3.CelMatcher celProto = config.getTypedConfig()
-            .unpack(com.github.xds.type.matcher.v3.CelMatcher.class);
+        CelMatcher celProto = config.getTypedConfig()
+            .unpack(CelMatcher.class);
         if (!celProto.hasExprMatch()) {
           throw new IllegalArgumentException("CelMatcher must have expr_match");
         }
-        com.github.xds.type.v3.CelExpression expr = celProto.getExprMatch();
+        CelExpression expr = celProto.getExprMatch();
         if (!expr.hasCelExprChecked()) {
           throw new IllegalArgumentException("CelMatcher must have cel_expr_checked");
         }
         CelAbstractSyntaxTree ast = 
             CelProtoAbstractSyntaxTree.fromCheckedExpr(
                 expr.getCelExprChecked()).getAst();
-        CelMatcher compiled = CelMatcher.compile(ast);
+        io.grpc.xds.internal.matcher.CelMatcher compiled = 
+            io.grpc.xds.internal.matcher.CelMatcher.compile(ast);
 
         return new CelStateMatcher(compiled);
       } catch (Exception e) {

xds/src/main/java/io/grpc/xds/internal/matcher/CelStringExtractor.java

@@ -20,6 +20,7 @@
 import dev.cel.common.types.SimpleType;
 import dev.cel.runtime.CelEvaluationException;
 import dev.cel.runtime.CelRuntime;
+import dev.cel.runtime.CelVariableResolver;
 
 /**
  * Executes compiled CEL expressions that extract a string.
@@ -52,8 +53,8 @@ public static CelStringExtractor compile(CelAbstractSyntaxTree ast)
    */
   public String extract(Object input) throws CelEvaluationException {
     Object result;
-    if (input instanceof dev.cel.runtime.CelVariableResolver) {
-      result = program.eval((dev.cel.runtime.CelVariableResolver) input);
+    if (input instanceof CelVariableResolver) {
+      result = program.eval((CelVariableResolver) input);
     } else if (input instanceof java.util.Map) {
       @SuppressWarnings("unchecked")
       java.util.Map<String, ?> mapInput = (java.util.Map<String, ?>) input;

xds/src/main/java/io/grpc/xds/internal/matcher/HeaderMatchInput.java

@@ -51,7 +51,7 @@ final class HeaderMatchInput implements MatchInput {
   }
 
   @Override
-  public Object apply(MatchContext context) {
+  public String apply(MatchContext context) {
     if ("te".equals(headerName)) {
       return null;
     }
@@ -88,7 +88,7 @@ public Class<?> outputType() {
 
   static final class Provider implements MatchInputProvider {
     @Override
-    public MatchInput getInput(TypedExtensionConfig config) {
+    public HeaderMatchInput getInput(TypedExtensionConfig config) {
       try {
         HttpRequestHeaderMatchInput proto = config.getTypedConfig()
             .unpack(HttpRequestHeaderMatchInput.class);

xds/src/main/java/io/grpc/xds/internal/matcher/HttpAttributesCelMatchInput.java

@@ -28,7 +28,7 @@ final class HttpAttributesCelMatchInput implements MatchInput {
   private HttpAttributesCelMatchInput() {}
 
   @Override
-  public Object apply(MatchContext context) {
+  public GrpcCelEnvironment apply(MatchContext context) {
     return new GrpcCelEnvironment(context);
   }
 
@@ -39,7 +39,7 @@ public Class<?> outputType() {
 
   static final class Provider implements MatchInputProvider {
     @Override
-    public MatchInput getInput(TypedExtensionConfig config) {
+    public HttpAttributesCelMatchInput getInput(TypedExtensionConfig config) {
       return INSTANCE;
     }
 

xds/src/main/java/io/grpc/xds/internal/matcher/MatchInput.java

@@ -34,7 +34,5 @@ public interface MatchInput {
   /**
    * Returns the type of value extracted by this input.
    */
-  default Class<?> outputType() {
-    return Object.class;
-  }
+  Class<?> outputType();
 }

xds/src/main/java/io/grpc/xds/internal/matcher/MatchResult.java

@@ -22,28 +22,52 @@
 import java.util.ArrayList;
 import java.util.Collections;
 import java.util.List;
+import javax.annotation.Nullable;
 
 /**
  * Result of a matching operation.
  */
 public final class MatchResult {
-  public final List<TypedExtensionConfig> actions; 
+  @Nullable
+  public final TypedExtensionConfig action;
+  public final List<TypedExtensionConfig> keepMatchingActions;
   public final boolean matched;
 
-  private MatchResult(List<TypedExtensionConfig> actions, boolean matched) {
-    this.actions = checkNotNull(actions, "actions");
+  private MatchResult(
+      @Nullable TypedExtensionConfig action,
+      List<TypedExtensionConfig> keepMatchingActions,
+      boolean matched) {
+    this.action = action;
+    this.keepMatchingActions =
+        Collections.unmodifiableList(
+            new ArrayList<>(checkNotNull(keepMatchingActions, "keepMatchingActions")));
     this.matched = matched;
   }
 
+  /**
+   * Creates a result indicating a successful match with a terminal action.
+   */
+  public static MatchResult create(
+      @Nullable TypedExtensionConfig action,
+      List<TypedExtensionConfig> keepMatchingActions) {
+    return new MatchResult(action, keepMatchingActions, true);
+  }
+
+  /**
+   * Creates a result indicating a match with a terminal action and no accumulated actions.
+   */
   public static MatchResult create(TypedExtensionConfig action) {
-    return new MatchResult(Collections.singletonList(action), true);
+    return new MatchResult(action, Collections.emptyList(), true);
   }
 
-  public static MatchResult create(List<TypedExtensionConfig> actions) {
-    return new MatchResult(new ArrayList<>(actions), true);
+  /**
+   * Creates a result indicating no terminal match, but potentially with accumulated actions.
+   */
+  public static MatchResult noMatch(List<TypedExtensionConfig> keepMatchingActions) {
+    return new MatchResult(null, keepMatchingActions, false);
   }
 
   public static MatchResult noMatch() {
-    return new MatchResult(Collections.emptyList(), false);
+    return new MatchResult(null, Collections.emptyList(), false);
   }
 }

xds/src/main/java/io/grpc/xds/internal/matcher/MatcherList.java

@@ -21,14 +21,16 @@
 import io.grpc.xds.internal.matcher.MatcherRunner.MatchContext;
 import java.util.ArrayList;
 import java.util.List;
+import java.util.function.Predicate;
 import javax.annotation.Nullable;
 
 final class MatcherList extends UnifiedMatcher {
   private final List<FieldMatcher> matchers;
-  @Nullable private final OnMatch onNoMatch;
+  @Nullable 
+  private final OnMatch onNoMatch;
 
   MatcherList(Matcher.MatcherList proto, @Nullable Matcher.OnMatch onNoMatchProto,
-      java.util.function.Predicate<String> actionValidator) {
+      Predicate<String> actionValidator) {
     if (proto.getMatchersCount() == 0) {
       throw new IllegalArgumentException("MatcherList must contain at least one FieldMatcher");
     }
@@ -50,45 +52,43 @@ public MatchResult match(MatchContext context, int depth) {
     }
 
     List<TypedExtensionConfig> accumulated = new ArrayList<>();
-    boolean matchedAtLeastOnce = false;
     for (FieldMatcher matcher : matchers) {
       if (matcher.matches(context)) {
         MatchResult result = matcher.onMatch.evaluate(context, depth);
-        if (result.matched) {
-          accumulated.addAll(result.actions);
-          matchedAtLeastOnce = true;
-        }
+        accumulated.addAll(result.keepMatchingActions);
 
-        if (!matcher.onMatch.keepMatching) {
-          if (!matchedAtLeastOnce) {
-            return MatchResult.noMatch();
+        if (result.matched) {
+          if (matcher.onMatch.keepMatching) {
+            if (result.action != null) {
+              accumulated.add(result.action);
+            }
+          } else {
+            return MatchResult.create(result.action, accumulated);
+          }
+        } else {
+          if (!matcher.onMatch.keepMatching) {
+            return MatchResult.noMatch(accumulated);
           }
-          break;
         }
       }
     }
 
-    if (!matchedAtLeastOnce) {
-      if (onNoMatch != null) {
-        MatchResult noMatchResult = onNoMatch.evaluate(context, depth);
-        if (noMatchResult.matched) {
-          accumulated.addAll(noMatchResult.actions);
-          matchedAtLeastOnce = true;
-        }
+    if (onNoMatch != null) {
+      MatchResult noMatchResult = onNoMatch.evaluate(context, depth);
+      accumulated.addAll(noMatchResult.keepMatchingActions);
+      if (noMatchResult.matched) {
+        return MatchResult.create(noMatchResult.action, accumulated);
       }
     }
-    if (matchedAtLeastOnce) {
-      return MatchResult.create(accumulated);
-    }
-    return MatchResult.noMatch();
+    return MatchResult.noMatch(accumulated);
   }
 
   private static final class FieldMatcher {
     private final PredicateEvaluator predicate;
     private final OnMatch onMatch;
 
     FieldMatcher(Matcher.MatcherList.FieldMatcher proto, 
-        java.util.function.Predicate<String> actionValidator) {
+        Predicate<String> actionValidator) {
       this.predicate = PredicateEvaluator.fromProto(proto.getPredicate());
       this.onMatch = new OnMatch(proto.getOnMatch(), actionValidator);
     }