Skip to content

Commit fc6198a

Browse files
Copilotpelikhan
andauthored
feat(workflow): cache repository owner type per compilation run
Initialize ownerTypeCache in NewCompiler so the owner-type API call ("Check repository owner type...") is made at most once per repo during a single compilation run. Add unit tests that verify the cache is consulted before making a network call and that it persists across multiple workflow compilations on the same Compiler instance. Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
1 parent e004d90 commit fc6198a

3 files changed

Lines changed: 122 additions & 4 deletions

File tree

Lines changed: 120 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,120 @@
1+
//go:build !integration
2+
3+
package workflow
4+
5+
import (
6+
"testing"
7+
)
8+
9+
// TestRepositoryOwnerIsIndividualUser_CacheHit verifies that repositoryOwnerIsIndividualUser
10+
// returns the cached owner type without making a network call when the owner is already
11+
// in the cache. This ensures the owner-type check is performed at most once per repo
12+
// during a single compilation run.
13+
func TestRepositoryOwnerIsIndividualUser_CacheHit(t *testing.T) {
14+
tests := []struct {
15+
name string
16+
cachedType string
17+
expectedResult bool
18+
}{
19+
{
20+
name: "cached User type returns true",
21+
cachedType: "User",
22+
expectedResult: true,
23+
},
24+
{
25+
name: "cached Organization type returns false",
26+
cachedType: "Organization",
27+
expectedResult: false,
28+
},
29+
{
30+
name: "cached empty string (API error) returns false",
31+
cachedType: "",
32+
expectedResult: false,
33+
},
34+
}
35+
36+
for _, tt := range tests {
37+
t.Run(tt.name, func(t *testing.T) {
38+
c := NewCompiler()
39+
c.SetRepositorySlug("myowner/myrepo")
40+
41+
// Pre-populate the cache so no network call is made.
42+
// If the cache is not consulted, RunGH would be called without a real gh binary
43+
// available in unit tests, causing the function to return false even for the
44+
// "User" case — the test would then fail on that case.
45+
c.ownerTypeCache["myowner"] = tt.cachedType
46+
47+
got := c.repositoryOwnerIsIndividualUser()
48+
if got != tt.expectedResult {
49+
t.Errorf("repositoryOwnerIsIndividualUser() = %v, want %v (cached owner type %q)",
50+
got, tt.expectedResult, tt.cachedType)
51+
}
52+
})
53+
}
54+
}
55+
56+
// TestRepositoryOwnerIsIndividualUser_MalformedSlug verifies that the function
57+
// returns false immediately when repositorySlug is missing or malformed, without
58+
// consulting the cache or making a network call.
59+
func TestRepositoryOwnerIsIndividualUser_MalformedSlug(t *testing.T) {
60+
tests := []struct {
61+
name string
62+
slug string
63+
}{
64+
{name: "empty slug", slug: ""},
65+
{name: "no slash", slug: "owneronly"},
66+
{name: "trailing slash only", slug: "/"},
67+
{name: "missing owner", slug: "/repo"},
68+
{name: "missing repo", slug: "owner/"},
69+
}
70+
71+
for _, tt := range tests {
72+
t.Run(tt.name, func(t *testing.T) {
73+
c := NewCompiler()
74+
c.SetRepositorySlug(tt.slug)
75+
// Populate the cache with "User" for any conceivable owner — if the slug
76+
// parsing were bypassed and a lookup happened, it would incorrectly return true.
77+
c.ownerTypeCache["owneronly"] = "User"
78+
c.ownerTypeCache["owner"] = "User"
79+
80+
got := c.repositoryOwnerIsIndividualUser()
81+
if got {
82+
t.Errorf("repositoryOwnerIsIndividualUser() = true for malformed slug %q, want false", tt.slug)
83+
}
84+
})
85+
}
86+
}
87+
88+
// TestRepositoryOwnerIsIndividualUser_CacheSharedAcrossCompilations verifies that
89+
// the owner-type cache persists across multiple calls on the same Compiler instance,
90+
// which represents multiple workflow files compiled in a single `gh aw compile` run.
91+
func TestRepositoryOwnerIsIndividualUser_CacheSharedAcrossCompilations(t *testing.T) {
92+
c := NewCompiler()
93+
c.SetRepositorySlug("myorg/repo-a")
94+
95+
// Seed the cache as if a previous call already resolved the owner type.
96+
c.ownerTypeCache["myorg"] = "Organization"
97+
98+
// Simulate compiling a second workflow in the same repo (different repo name, same owner).
99+
c.SetRepositorySlugIfUnlocked("myorg/repo-b")
100+
101+
// The cache entry for "myorg" must be reused — no network call is made.
102+
got := c.repositoryOwnerIsIndividualUser()
103+
if got {
104+
t.Error("repositoryOwnerIsIndividualUser() = true for Organization owner, want false")
105+
}
106+
107+
// The cache should still hold the original value (not been cleared or overwritten).
108+
if val := c.ownerTypeCache["myorg"]; val != "Organization" {
109+
t.Errorf("ownerTypeCache[myorg] = %q after second call, want %q", val, "Organization")
110+
}
111+
}
112+
113+
// TestRepositoryOwnerIsIndividualUser_CacheInitializedByNewCompiler verifies that
114+
// NewCompiler initialises ownerTypeCache so callers never encounter a nil map panic.
115+
func TestRepositoryOwnerIsIndividualUser_CacheInitializedByNewCompiler(t *testing.T) {
116+
c := NewCompiler()
117+
if c.ownerTypeCache == nil {
118+
t.Fatal("NewCompiler() left ownerTypeCache nil; expected an initialized map")
119+
}
120+
}

pkg/workflow/compiler_types.go

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -136,7 +136,8 @@ func NewCompiler(opts ...CompilerOption) *Compiler {
136136
artifactManager: NewArtifactManager(),
137137
actionPinWarnings: make(map[string]bool), // Initialize warning cache
138138
priorManifests: make(map[string]*GHAWManifest),
139-
gitRoot: gitRoot, // Auto-detected git root
139+
ownerTypeCache: make(map[string]string), // Initialize owner-type cache (keyed by owner login)
140+
gitRoot: gitRoot, // Auto-detected git root
140141
}
141142

142143
// Apply functional options

pkg/workflow/permissions_compiler_validator.go

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -186,9 +186,6 @@ func (c *Compiler) repositoryOwnerIsIndividualUser() bool {
186186
return false
187187
}
188188

189-
if c.ownerTypeCache == nil {
190-
c.ownerTypeCache = make(map[string]string)
191-
}
192189
ownerType, cached := c.ownerTypeCache[owner]
193190
if !cached {
194191
workflowLog.Printf("Checking owner type for: %s", owner)

0 commit comments

Comments
 (0)