handle explore names with url encoding characters (#9213)

* handle explore names with url encoding characters

* lint

Author: pjain1

runtime/reconcilers/alert.go

@@ -9,7 +9,6 @@ import (
 	"fmt"
 	"io"
 	"net/url"
-	"strings"
 	"time"
 
 	runtimev1 "github.com/rilldata/rill/proto/gen/rill/runtime/v1"
@@ -305,30 +304,12 @@ func (r *AlertReconciler) ResolveTransitiveAccess(ctx context.Context, claims *r
 	}
 
 	// figure out explore or canvas for the alert
-	var explore, canvas string
-	if e, ok := spec.Annotations["explore"]; ok {
-		explore = e
-	}
+	explore := exploreNameFromAnnotations(spec.Annotations, mvName)
+	var canvas string
 	if c, ok := spec.Annotations["canvas"]; ok {
 		canvas = c
 	}
 
-	if explore == "" { // backwards compatibility, try to find explore
-		if path, ok := spec.Annotations["web_open_path"]; ok {
-			// parse path, extract explore name, it will be like /explore/{explore}
-			if strings.HasPrefix(path, "/explore/") {
-				explore = path[9:]
-				if explore[len(explore)-1] == '/' {
-					explore = explore[:len(explore)-1]
-				}
-			}
-		}
-		// still not found, use mv name as explore name
-		if explore == "" {
-			explore = mvName
-		}
-	}
-
 	// add explore and canvas to access and field access rule's condition resources
 	if explore != "" {
 		exp := &runtimev1.ResourceName{Kind: runtime.ResourceKindExplore, Name: explore}

runtime/reconcilers/report.go

@@ -5,7 +5,6 @@ import (
 	"errors"
 	"fmt"
 	"net/url"
-	"strings"
 	"time"
 
 	runtimev1 "github.com/rilldata/rill/proto/gen/rill/runtime/v1"
@@ -232,30 +231,12 @@ func (r *ReportReconciler) ResolveTransitiveAccess(ctx context.Context, claims *
 	}
 
 	// figure out explore or canvas for the report
-	var explore, canvas string
-	if e, ok := spec.Annotations["explore"]; ok {
-		explore = e
-	}
+	explore := exploreNameFromAnnotations(spec.Annotations, mvName)
+	var canvas string
 	if c, ok := spec.Annotations["canvas"]; ok {
 		canvas = c
 	}
 
-	if explore == "" { // backwards compatibility, try to find explore
-		if path, ok := spec.Annotations["web_open_path"]; ok {
-			// parse path, extract explore name, it will be like /explore/{explore}
-			if strings.HasPrefix(path, "/explore/") {
-				explore = path[9:]
-				if explore[len(explore)-1] == '/' {
-					explore = explore[:len(explore)-1]
-				}
-			}
-		}
-		// still not found, use mv name as explore name
-		if explore == "" {
-			explore = mvName
-		}
-	}
-
 	// add explore and canvas to access and field access rule's condition resources
 	if explore != "" {
 		exp := &runtimev1.ResourceName{Kind: runtime.ResourceKindExplore, Name: explore}

runtime/reconcilers/util.go

@@ -4,6 +4,7 @@ import (
 	"context"
 	"errors"
 	"fmt"
+	"net/url"
 	"strings"
 	"time"
 
@@ -143,6 +144,30 @@ func nextRefreshTime(t time.Time, schedule *runtimev1.Schedule) (time.Time, erro
 	return t2, nil
 }
 
+// exploreNameFromAnnotations extracts the explore name from resource annotations.
+// It checks the "explore" annotation first, then falls back to parsing the "web_open_path" annotation.
+// If neither is found, it returns fallback.
+func exploreNameFromAnnotations(annotations map[string]string, fallback string) string {
+	if e, ok := annotations["explore"]; ok {
+		return e
+	}
+	if path, ok := annotations["web_open_path"]; ok {
+		if strings.HasPrefix(path, "/explore/") {
+			explore := path[9:]
+			if explore != "" && explore[len(explore)-1] == '/' {
+				explore = explore[:len(explore)-1]
+			}
+			if decoded, err := url.PathUnescape(explore); err == nil {
+				explore = decoded
+			}
+			if explore != "" {
+				return explore
+			}
+		}
+	}
+	return fallback
+}
+
 // analyzeTemplatedVariables analyzes strings nested in the provided props for template tags that reference instance variables.
 // It returns a map of variable names referenced in the props mapped to their current value (if known).
 func analyzeTemplatedVariables(ctx context.Context, c *runtime.Controller, props map[string]any) (map[string]string, error) {

runtime/reconcilers/util_test.go

@@ -0,0 +1,77 @@
+package reconcilers
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/require"
+)
+
+func TestExploreNameFromAnnotations(t *testing.T) {
+	tests := []struct {
+		name        string
+		annotations map[string]string
+		fallback    string
+		want        string
+	}{
+		{
+			name:        "explicit explore annotation",
+			annotations: map[string]string{"explore": "my_explore"},
+			fallback:    "fallback",
+			want:        "my_explore",
+		},
+		{
+			name:        "explicit explore takes precedence over web_open_path",
+			annotations: map[string]string{"explore": "my_explore", "web_open_path": "/explore/other"},
+			fallback:    "fallback",
+			want:        "my_explore",
+		},
+		{
+			name:        "web_open_path without encoding",
+			annotations: map[string]string{"web_open_path": "/explore/my_explore"},
+			fallback:    "fallback",
+			want:        "my_explore",
+		},
+		{
+			name:        "web_open_path with percent encoding",
+			annotations: map[string]string{"web_open_path": "/explore/publisher%20overview%20explore"},
+			fallback:    "fallback",
+			want:        "publisher overview explore",
+		},
+		{
+			name:        "web_open_path with trailing slash",
+			annotations: map[string]string{"web_open_path": "/explore/my_explore/"},
+			fallback:    "fallback",
+			want:        "my_explore",
+		},
+		{
+			name:        "web_open_path with encoding and trailing slash",
+			annotations: map[string]string{"web_open_path": "/explore/publisher%20overview/"},
+			fallback:    "fallback",
+			want:        "publisher overview",
+		},
+		{
+			name:        "non-explore web_open_path falls back",
+			annotations: map[string]string{"web_open_path": "/canvas/my_canvas"},
+			fallback:    "fallback",
+			want:        "fallback",
+		},
+		{
+			name:        "no annotations falls back",
+			annotations: map[string]string{},
+			fallback:    "fallback",
+			want:        "fallback",
+		},
+		{
+			name:        "nil annotations falls back",
+			annotations: nil,
+			fallback:    "fallback",
+			want:        "fallback",
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got := exploreNameFromAnnotations(tt.annotations, tt.fallback)
+			require.Equal(t, tt.want, got)
+		})
+	}
+}