From 69c6c70845d44ff833fa1c8efa4e1701239aca91 Mon Sep 17 00:00:00 2001 From: Earl Warren Date: Thu, 4 Sep 2025 14:38:50 +0000 Subject: [PATCH] chore: refactor act/artifactcache Handler to an interface (#934) - the Handler struct becomes handler (lowercase) - the Handler interface is defined to be the existing methods - isClosed() is added and used only in tests - setgcAt() is added and used only in tests --- This is to allow mocking the Handler interface for testing. - other - [PR](https://code.forgejo.org/forgejo/runner/pulls/934): chore: refactor act/artifactcache Handler to an interface Reviewed-on: https://code.forgejo.org/forgejo/runner/pulls/934 Reviewed-by: Mathieu Fenniak Co-authored-by: Earl Warren Co-committed-by: Earl Warren --- act/artifactcache/handler.go | 61 ++++++++++++++++++++++--------- act/artifactcache/handler_test.go | 14 +++---- act/artifactcache/mac.go | 2 +- act/artifactcache/mac_test.go | 2 +- internal/app/cmd/exec.go | 2 +- 5 files changed, 52 insertions(+), 29 deletions(-) diff --git a/act/artifactcache/handler.go b/act/artifactcache/handler.go index 8b8572ee..801cd93d 100644 --- a/act/artifactcache/handler.go +++ b/act/artifactcache/handler.go @@ -28,7 +28,26 @@ const ( urlBase = "/_apis/artifactcache" ) -type Handler struct { +type Handler interface { + ExternalURL() string + Close() error + isClosed() bool + openDB() (*bolthold.Store, error) + find(w http.ResponseWriter, r *http.Request, params httprouter.Params) + reserve(w http.ResponseWriter, r *http.Request, params httprouter.Params) + upload(w http.ResponseWriter, r *http.Request, params httprouter.Params) + commit(w http.ResponseWriter, r *http.Request, params httprouter.Params) + get(w http.ResponseWriter, r *http.Request, params httprouter.Params) + clean(w http.ResponseWriter, r *http.Request, _ httprouter.Params) + middleware(handler httprouter.Handle) httprouter.Handle + readCache(id uint64) (*Cache, error) + useCache(id uint64) error + setgcAt(at time.Time) + gcCache() + responseJSON(w http.ResponseWriter, r *http.Request, code int, v ...any) +} + +type handler struct { dir string storage *Storage router *httprouter.Router @@ -43,8 +62,8 @@ type Handler struct { outboundIP string } -func StartHandler(dir, outboundIP string, port uint16, secret string, logger logrus.FieldLogger) (*Handler, error) { - h := &Handler{ +func StartHandler(dir, outboundIP string, port uint16, secret string, logger logrus.FieldLogger) (Handler, error) { + h := &handler{ secret: secret, } @@ -114,14 +133,14 @@ func StartHandler(dir, outboundIP string, port uint16, secret string, logger log return h, nil } -func (h *Handler) ExternalURL() string { +func (h *handler) ExternalURL() string { port := strconv.Itoa(h.listener.Addr().(*net.TCPAddr).Port) // TODO: make the external url configurable if necessary return fmt.Sprintf("http://%s", net.JoinHostPort(h.outboundIP, port)) } -func (h *Handler) Close() error { +func (h *handler) Close() error { if h == nil { return nil } @@ -146,7 +165,11 @@ func (h *Handler) Close() error { return retErr } -func (h *Handler) openDB() (*bolthold.Store, error) { +func (h *handler) isClosed() bool { + return h.listener == nil && h.server == nil +} + +func (h *handler) openDB() (*bolthold.Store, error) { return bolthold.Open(filepath.Join(h.dir, "bolt.db"), 0o644, &bolthold.Options{ Encoder: json.Marshal, Decoder: json.Unmarshal, @@ -159,7 +182,7 @@ func (h *Handler) openDB() (*bolthold.Store, error) { } // GET /_apis/artifactcache/cache -func (h *Handler) find(w http.ResponseWriter, r *http.Request, params httprouter.Params) { +func (h *handler) find(w http.ResponseWriter, r *http.Request, params httprouter.Params) { rundata := runDataFromHeaders(r) repo, err := h.validateMac(rundata) if err != nil { @@ -216,7 +239,7 @@ func (h *Handler) find(w http.ResponseWriter, r *http.Request, params httprouter } // POST /_apis/artifactcache/caches -func (h *Handler) reserve(w http.ResponseWriter, r *http.Request, params httprouter.Params) { +func (h *handler) reserve(w http.ResponseWriter, r *http.Request, params httprouter.Params) { rundata := runDataFromHeaders(r) repo, err := h.validateMac(rundata) if err != nil { @@ -255,7 +278,7 @@ func (h *Handler) reserve(w http.ResponseWriter, r *http.Request, params httprou } // PATCH /_apis/artifactcache/caches/:id -func (h *Handler) upload(w http.ResponseWriter, r *http.Request, params httprouter.Params) { +func (h *handler) upload(w http.ResponseWriter, r *http.Request, params httprouter.Params) { rundata := runDataFromHeaders(r) repo, err := h.validateMac(rundata) if err != nil { @@ -310,7 +333,7 @@ func (h *Handler) upload(w http.ResponseWriter, r *http.Request, params httprout } // POST /_apis/artifactcache/caches/:id -func (h *Handler) commit(w http.ResponseWriter, r *http.Request, params httprouter.Params) { +func (h *handler) commit(w http.ResponseWriter, r *http.Request, params httprouter.Params) { rundata := runDataFromHeaders(r) repo, err := h.validateMac(rundata) if err != nil { @@ -374,7 +397,7 @@ func (h *Handler) commit(w http.ResponseWriter, r *http.Request, params httprout } // GET /_apis/artifactcache/artifacts/:id -func (h *Handler) get(w http.ResponseWriter, r *http.Request, params httprouter.Params) { +func (h *handler) get(w http.ResponseWriter, r *http.Request, params httprouter.Params) { rundata := runDataFromHeaders(r) repo, err := h.validateMac(rundata) if err != nil { @@ -417,7 +440,7 @@ func (h *Handler) get(w http.ResponseWriter, r *http.Request, params httprouter. } // POST /_apis/artifactcache/clean -func (h *Handler) clean(w http.ResponseWriter, r *http.Request, _ httprouter.Params) { +func (h *handler) clean(w http.ResponseWriter, r *http.Request, _ httprouter.Params) { rundata := runDataFromHeaders(r) _, err := h.validateMac(rundata) if err != nil { @@ -430,7 +453,7 @@ func (h *Handler) clean(w http.ResponseWriter, r *http.Request, _ httprouter.Par h.responseJSON(w, r, 200) } -func (h *Handler) middleware(handler httprouter.Handle) httprouter.Handle { +func (h *handler) middleware(handler httprouter.Handle) httprouter.Handle { return func(w http.ResponseWriter, r *http.Request, params httprouter.Params) { h.logger.Debugf("%s %s", r.Method, r.RequestURI) handler(w, r, params) @@ -488,7 +511,7 @@ func insertCache(db *bolthold.Store, cache *Cache) error { return nil } -func (h *Handler) readCache(id uint64) (*Cache, error) { +func (h *handler) readCache(id uint64) (*Cache, error) { db, err := h.openDB() if err != nil { return nil, err @@ -501,7 +524,7 @@ func (h *Handler) readCache(id uint64) (*Cache, error) { return cache, nil } -func (h *Handler) useCache(id uint64) error { +func (h *handler) useCache(id uint64) error { db, err := h.openDB() if err != nil { return err @@ -522,7 +545,11 @@ const ( keepOld = 5 * time.Minute ) -func (h *Handler) gcCache() { +func (h *handler) setgcAt(at time.Time) { + h.gcAt = at +} + +func (h *handler) gcCache() { if h.gcing.Load() { return } @@ -629,7 +656,7 @@ func (h *Handler) gcCache() { } } -func (h *Handler) responseJSON(w http.ResponseWriter, r *http.Request, code int, v ...any) { +func (h *handler) responseJSON(w http.ResponseWriter, r *http.Request, code int, v ...any) { w.Header().Set("Content-Type", "application/json; charset=utf-8") var data []byte if len(v) == 0 || v[0] == nil { diff --git a/act/artifactcache/handler_test.go b/act/artifactcache/handler_test.go index 1d813d56..c9e01ba3 100644 --- a/act/artifactcache/handler_test.go +++ b/act/artifactcache/handler_test.go @@ -76,8 +76,7 @@ func TestHandler(t *testing.T) { }) t.Run("close", func(t *testing.T) { require.NoError(t, handler.Close()) - assert.Nil(t, handler.server) - assert.Nil(t, handler.listener) + assert.True(t, handler.isClosed()) _, err := httpClient.Post(fmt.Sprintf("%s/caches/%d", base, 1), "", nil) assert.Error(t, err) }) @@ -983,7 +982,7 @@ func TestHandler_gcCache(t *testing.T) { } require.NoError(t, db.Close()) - handler.gcAt = time.Time{} // ensure gcCache will not skip + handler.setgcAt(time.Time{}) // ensure gcCache will not skip handler.gcCache() db, err = handler.openDB() @@ -1010,8 +1009,7 @@ func TestHandler_ExternalURL(t *testing.T) { assert.Equal(t, handler.ExternalURL(), "http://127.0.0.1:34567") require.NoError(t, handler.Close()) - assert.Nil(t, handler.server) - assert.Nil(t, handler.listener) + assert.True(t, handler.isClosed()) }) t.Run("reports correct URL on IPv6 zero host", func(t *testing.T) { @@ -1021,8 +1019,7 @@ func TestHandler_ExternalURL(t *testing.T) { assert.Equal(t, handler.ExternalURL(), "http://[2001:db8::]:34567") require.NoError(t, handler.Close()) - assert.Nil(t, handler.server) - assert.Nil(t, handler.listener) + assert.True(t, handler.isClosed()) }) t.Run("reports correct URL on IPv6", func(t *testing.T) { @@ -1032,7 +1029,6 @@ func TestHandler_ExternalURL(t *testing.T) { assert.Equal(t, handler.ExternalURL(), "http://[2001:db8::1:2:3:4]:34567") require.NoError(t, handler.Close()) - assert.Nil(t, handler.server) - assert.Nil(t, handler.listener) + assert.True(t, handler.isClosed()) }) } diff --git a/act/artifactcache/mac.go b/act/artifactcache/mac.go index 72e5fa2e..faefe2d7 100644 --- a/act/artifactcache/mac.go +++ b/act/artifactcache/mac.go @@ -16,7 +16,7 @@ import ( var ErrValidation = errors.New("validation error") -func (h *Handler) validateMac(rundata cacheproxy.RunData) (string, error) { +func (h *handler) validateMac(rundata cacheproxy.RunData) (string, error) { // TODO: allow configurable max age if !validateAge(rundata.Timestamp) { return "", ErrValidation diff --git a/act/artifactcache/mac_test.go b/act/artifactcache/mac_test.go index 0e1f3be6..a790bc1d 100644 --- a/act/artifactcache/mac_test.go +++ b/act/artifactcache/mac_test.go @@ -10,7 +10,7 @@ import ( ) func TestMac(t *testing.T) { - handler := &Handler{ + handler := &handler{ secret: "secret for testing", } diff --git a/internal/app/cmd/exec.go b/internal/app/cmd/exec.go index 371beb24..a0694805 100644 --- a/internal/app/cmd/exec.go +++ b/internal/app/cmd/exec.go @@ -54,7 +54,7 @@ type executeArgs struct { debug bool dryrun bool image string - cacheHandler *artifactcache.Handler + cacheHandler artifactcache.Handler network string enableIPv6 bool githubInstance string