[bugfix] Store LastModified for domain perm subs + send as If-Modified-Since (#3655)

8 files changed, 306 insertions, 34 deletions

diff --git a/internal/cache/size.go b/internal/cache/size.go
index 4cca91666..24101683a 100644
--- a/internal/cache/size.go
+++ b/internal/cache/size.go
@@ -372,6 +372,7 @@ func sizeofDomainPermissionSubscription() uintptr {
 		FetchedAt:             exampleTime,
 		SuccessfullyFetchedAt: exampleTime,
 		ETag:                  exampleID,
+		LastModified:          exampleTime,
 		Error:                 exampleTextSmall,
 	}))
 }
diff --git a/internal/db/bundb/migrations/20241022153016_domain_permission_subscriptions/domainpermissionsubscription.go b/internal/db/bundb/migrations/20241022153016_domain_permission_subscriptions/domainpermissionsubscription.go
index 851f44f15..b24dc7da5 100644
--- a/internal/db/bundb/migrations/20241022153016_domain_permission_subscriptions/domainpermissionsubscription.go
+++ b/internal/db/bundb/migrations/20241022153016_domain_permission_subscriptions/domainpermissionsubscription.go
@@ -33,6 +33,7 @@ type DomainPermissionSubscription struct {
 	FetchPassword         string    `bun:",nullzero"`
 	FetchedAt             time.Time `bun:"type:timestamptz,nullzero"`
 	SuccessfullyFetchedAt time.Time `bun:"type:timestamptz,nullzero"`
+	LastModified          time.Time `bun:"type:timestamptz,nullzero"`
 	ETag                  string    `bun:"etag,nullzero"`
 	Error                 string    `bun:",nullzero"`
 }
diff --git a/internal/db/bundb/migrations/20250119112746_domain_perm_sub_caching_tweaks.go b/internal/db/bundb/migrations/20250119112746_domain_perm_sub_caching_tweaks.go
new file mode 100644
index 000000000..c37e4aa7f
--- /dev/null
+++ b/internal/db/bundb/migrations/20250119112746_domain_perm_sub_caching_tweaks.go
@@ -0,0 +1,76 @@
+// GoToSocial
+// Copyright (C) GoToSocial Authors admin@gotosocial.org
+// SPDX-License-Identifier: AGPL-3.0-or-later
+//
+// This program is free software: you can redistribute it and/or modify
+// it under the terms of the GNU Affero General Public License as published by
+// the Free Software Foundation, either version 3 of the License, or
+// (at your option) any later version.
+//
+// This program is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU Affero General Public License for more details.
+//
+// You should have received a copy of the GNU Affero General Public License
+// along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+package migrations
+
+import (
+	"context"
+	"fmt"
+	"reflect"
+
+	"github.com/superseriousbusiness/gotosocial/internal/gtsmodel"
+	"github.com/superseriousbusiness/gotosocial/internal/log"
+	"github.com/uptrace/bun"
+)
+
+func init() {
+	up := func(ctx context.Context, db *bun.DB) error {
+		return db.RunInTx(ctx, nil, func(ctx context.Context, tx bun.Tx) error {
+			// Bail if "last_modified"
+			// column already created.
+			if exists, err := doesColumnExist(
+				ctx,
+				tx,
+				"domain_permission_subscriptions",
+				"last_modified",
+			); err != nil {
+				return err
+			} else if exists {
+				return nil
+			}
+
+			// Derive column definition.
+			var permSub *gtsmodel.DomainPermissionSubscription
+			permSubType := reflect.TypeOf(permSub)
+			colDef, err := getBunColumnDef(tx, permSubType, "LastModified")
+			if err != nil {
+				return fmt.Errorf("error making column def: %w", err)
+			}
+
+			log.Info(ctx, "adding domain_permission_subscriptions.last_modified column...")
+			if _, err := tx.
+				NewAddColumn().
+				Model(permSub).
+				ColumnExpr(colDef).
+				Exec(ctx); err != nil {
+				return fmt.Errorf("error adding column: %w", err)
+			}
+
+			return nil
+		})
+	}
+
+	down := func(ctx context.Context, db *bun.DB) error {
+		return db.RunInTx(ctx, nil, func(ctx context.Context, tx bun.Tx) error {
+			return nil
+		})
+	}
+
+	if err := Migrations.Register(up, down); err != nil {
+		panic(err)
+	}
+}
diff --git a/internal/gtsmodel/domainpermissionsubscription.go b/internal/gtsmodel/domainpermissionsubscription.go
index b6a0b8f43..724b2164f 100644
--- a/internal/gtsmodel/domainpermissionsubscription.go
+++ b/internal/gtsmodel/domainpermissionsubscription.go
@@ -34,7 +34,8 @@ type DomainPermissionSubscription struct {
 	FetchPassword         string                   `bun:",nullzero"`                                // Password to send when doing a GET of URI using basic auth.
 	FetchedAt             time.Time                `bun:"type:timestamptz,nullzero"`                // Time when fetch of URI was last attempted.
 	SuccessfullyFetchedAt time.Time                `bun:"type:timestamptz,nullzero"`                // Time when the domain permission list was last *successfuly* fetched, to be transmitted as If-Modified-Since header.
-	ETag                  string                   `bun:"etag,nullzero"`                            // Etag last received from the server (if any) on successful fetch.
+	LastModified          time.Time                `bun:"type:timestamptz,nullzero"`                // "Last-Modified" time received from the server (if any) on last successful fetch. Used for HTTP request caching.
+	ETag                  string                   `bun:"etag,nullzero"`                            // "ETag" header last received from the server (if any) on last successful fetch. Used for HTTP request caching.
 	Error                 string                   `bun:",nullzero"`                                // If latest fetch attempt errored, this field stores the error message. Cleared on latest successful fetch.
 }
 
diff --git a/internal/subscriptions/domainperms.go b/internal/subscriptions/domainperms.go
index ee4730023..b94f284bf 100644
--- a/internal/subscriptions/domainperms.go
+++ b/internal/subscriptions/domainperms.go
@@ -253,13 +253,9 @@ func (s *Subscriptions) ProcessDomainPermissionSubscription(
 	// to indicate a successful fetch, and return.
 	if resp.Unmodified {
 		l.Debug("received 304 Not Modified from remote")
+		permSub.ETag = resp.ETag
+		permSub.LastModified = resp.LastModified
 		permSub.SuccessfullyFetchedAt = permSub.FetchedAt
-		if permSub.ETag == "" && resp.ETag != "" {
-			// We didn't have an ETag before but
-			// we have one now: probably the remote
-			// added ETag support in the meantime.
-			permSub.ETag = resp.ETag
-		}
 		return nil, nil
 	}
 
@@ -308,6 +304,7 @@ func (s *Subscriptions) ProcessDomainPermissionSubscription(
 	// This can now be considered a successful fetch.
 	permSub.SuccessfullyFetchedAt = permSub.FetchedAt
 	permSub.ETag = resp.ETag
+	permSub.LastModified = resp.LastModified
 	permSub.Error = ""
 
 	// Keep track of which domain perms are
diff --git a/internal/subscriptions/subscriptions_test.go b/internal/subscriptions/subscriptions_test.go
index 6c47fed21..d86d98691 100644
--- a/internal/subscriptions/subscriptions_test.go
+++ b/internal/subscriptions/subscriptions_test.go
@@ -107,7 +107,7 @@ func (suite *SubscriptionsTestSuite) TestDomainBlocksCSV() {
 	}
 
 	// The just-fetched perm sub should
-	// have ETag and count etc set now.
+	// have cache meta and count etc set now.
 	permSub, err := testStructs.State.DB.GetDomainPermissionSubscriptionByID(
 		ctx, testSubscription.ID,
 	)
@@ -121,7 +121,8 @@ func (suite *SubscriptionsTestSuite) TestDomainBlocksCSV() {
 		suite.FailNow(err.Error())
 	}
 
-	suite.Equal("bigbums6969", permSub.ETag)
+	suite.Equal("\"bigbums6969\"", permSub.ETag)
+	suite.EqualValues(1726956000, permSub.LastModified.Unix())
 	suite.EqualValues(3, count)
 	suite.WithinDuration(time.Now(), permSub.FetchedAt, 1*time.Minute)
 	suite.WithinDuration(time.Now(), permSub.SuccessfullyFetchedAt, 1*time.Minute)
@@ -186,7 +187,7 @@ func (suite *SubscriptionsTestSuite) TestDomainBlocksJSON() {
 	}
 
 	// The just-fetched perm sub should
-	// have ETag and count etc set now.
+	// have cache meta and count etc set now.
 	permSub, err := testStructs.State.DB.GetDomainPermissionSubscriptionByID(
 		ctx, testSubscription.ID,
 	)
@@ -200,7 +201,8 @@ func (suite *SubscriptionsTestSuite) TestDomainBlocksJSON() {
 		suite.FailNow(err.Error())
 	}
 
-	suite.Equal("don't modify me daddy", permSub.ETag)
+	suite.Equal("\"don't modify me daddy\"", permSub.ETag)
+	suite.EqualValues(1726956000, permSub.LastModified.Unix())
 	suite.EqualValues(3, count)
 	suite.WithinDuration(time.Now(), permSub.FetchedAt, 1*time.Minute)
 	suite.WithinDuration(time.Now(), permSub.SuccessfullyFetchedAt, 1*time.Minute)
@@ -265,7 +267,7 @@ func (suite *SubscriptionsTestSuite) TestDomainBlocksPlain() {
 	}
 
 	// The just-fetched perm sub should
-	// have ETag and count etc set now.
+	// have cache meta and count etc set now.
 	permSub, err := testStructs.State.DB.GetDomainPermissionSubscriptionByID(
 		ctx, testSubscription.ID,
 	)
@@ -279,13 +281,14 @@ func (suite *SubscriptionsTestSuite) TestDomainBlocksPlain() {
 		suite.FailNow(err.Error())
 	}
 
-	suite.Equal("this is a legit etag i swear", permSub.ETag)
+	suite.Equal("\"this is a legit etag i swear\"", permSub.ETag)
+	suite.EqualValues(1726956000, permSub.LastModified.Unix())
 	suite.EqualValues(3, count)
 	suite.WithinDuration(time.Now(), permSub.FetchedAt, 1*time.Minute)
 	suite.WithinDuration(time.Now(), permSub.SuccessfullyFetchedAt, 1*time.Minute)
 }
 
-func (suite *SubscriptionsTestSuite) TestDomainBlocksCSVETag() {
+func (suite *SubscriptionsTestSuite) TestDomainBlocksCSVCaching() {
 	var (
 		ctx           = context.Background()
 		testStructs   = testrig.SetupTestStructs(rMediaPath, rTemplatePath)
@@ -297,7 +300,7 @@ func (suite *SubscriptionsTestSuite) TestDomainBlocksCSVETag() {
 		)
 
 		// Create a subscription for a CSV list of baddies.
-		// Include the ETag so it gets sent with the request.
+		// Include ETag + LastModified so they get sent with the request.
 		testSubscription = &gtsmodel.DomainPermissionSubscription{
 			ID:                 "01JGE681TQSBPAV59GZXPKE62H",
 			Priority:           255,
@@ -309,7 +312,8 @@ func (suite *SubscriptionsTestSuite) TestDomainBlocksCSVETag() {
 			CreatedByAccount:   testAccount,
 			URI:                "https://lists.example.org/baddies.csv",
 			ContentType:        gtsmodel.DomainPermSubContentTypeCSV,
-			ETag:               "bigbums6969",
+			ETag:               "\"bigbums6969\"",
+			LastModified:       testrig.TimeMustParse("2024-09-21T22:00:00Z"),
 		}
 	)
 	defer testrig.TearDownTestStructs(testStructs)
@@ -339,7 +343,7 @@ func (suite *SubscriptionsTestSuite) TestDomainBlocksCSVETag() {
 	}
 
 	// The just-fetched perm sub should
-	// have ETag and count etc set now.
+	// have cache meta and count etc set now.
 	permSub, err := testStructs.State.DB.GetDomainPermissionSubscriptionByID(
 		ctx, testSubscription.ID,
 	)
@@ -353,12 +357,157 @@ func (suite *SubscriptionsTestSuite) TestDomainBlocksCSVETag() {
 		suite.FailNow(err.Error())
 	}
 
-	suite.Equal("bigbums6969", permSub.ETag)
+	suite.Equal("\"bigbums6969\"", permSub.ETag)
+	suite.EqualValues(1726956000, permSub.LastModified.Unix())
 	suite.Zero(count)
 	suite.WithinDuration(time.Now(), permSub.FetchedAt, 1*time.Minute)
 	suite.WithinDuration(time.Now(), permSub.SuccessfullyFetchedAt, 1*time.Minute)
 }
 
+func (suite *SubscriptionsTestSuite) TestDomainBlocksCSVFutureLastModified() {
+	var (
+		ctx           = context.Background()
+		testStructs   = testrig.SetupTestStructs(rMediaPath, rTemplatePath)
+		testAccount   = suite.testAccounts["admin_account"]
+		subscriptions = subscriptions.New(
+			testStructs.State,
+			testStructs.TransportController,
+			testStructs.TypeConverter,
+		)
+
+		// Create a subscription for a CSV list of baddies.
+		// Request the future last modified value.
+		testSubscription = &gtsmodel.DomainPermissionSubscription{
+			ID:                 "01JGE681TQSBPAV59GZXPKE62H",
+			Priority:           255,
+			Title:              "whatever!",
+			PermissionType:     gtsmodel.DomainPermissionBlock,
+			AsDraft:            util.Ptr(false),
+			AdoptOrphans:       util.Ptr(true),
+			CreatedByAccountID: testAccount.ID,
+			CreatedByAccount:   testAccount,
+			URI:                "https://lists.example.org/baddies.csv?future=true",
+			ContentType:        gtsmodel.DomainPermSubContentTypeCSV,
+		}
+	)
+	defer testrig.TearDownTestStructs(testStructs)
+
+	// Store test subscription.
+	if err := testStructs.State.DB.PutDomainPermissionSubscription(
+		ctx, testSubscription,
+	); err != nil {
+		suite.FailNow(err.Error())
+	}
+
+	// Process all subscriptions.
+	subscriptions.ProcessDomainPermissionSubscriptions(ctx, testSubscription.PermissionType)
+
+	// We should now have blocks for
+	// each domain on the subscribed list.
+	for _, domain := range []string{
+		"bumfaces.net",
+		"peepee.poopoo",
+		"nothanks.com",
+	} {
+		var (
+			perm gtsmodel.DomainPermission
+			err  error
+		)
+		if !testrig.WaitFor(func() bool {
+			perm, err = testStructs.State.DB.GetDomainBlock(ctx, domain)
+			return err == nil
+		}) {
+			suite.FailNowf("", "timed out waiting for domain %s", domain)
+		}
+
+		suite.Equal(testSubscription.ID, perm.GetSubscriptionID())
+	}
+
+	// The just-fetched perm sub should have ETag
+	// set now, but last modified should be thrown away.
+	permSub, err := testStructs.State.DB.GetDomainPermissionSubscriptionByID(
+		ctx, testSubscription.ID,
+	)
+	if err != nil {
+		suite.FailNow(err.Error())
+	}
+
+	suite.Equal("\"bigbums6969\"", permSub.ETag)
+	suite.Zero(permSub.LastModified)
+}
+
+func (suite *SubscriptionsTestSuite) TestDomainBlocksCSVGarbageLastModified() {
+	var (
+		ctx           = context.Background()
+		testStructs   = testrig.SetupTestStructs(rMediaPath, rTemplatePath)
+		testAccount   = suite.testAccounts["admin_account"]
+		subscriptions = subscriptions.New(
+			testStructs.State,
+			testStructs.TransportController,
+			testStructs.TypeConverter,
+		)
+
+		// Create a subscription for a CSV list of baddies.
+		// Request the garbage last modified value.
+		testSubscription = &gtsmodel.DomainPermissionSubscription{
+			ID:                 "01JGE681TQSBPAV59GZXPKE62H",
+			Priority:           255,
+			Title:              "whatever!",
+			PermissionType:     gtsmodel.DomainPermissionBlock,
+			AsDraft:            util.Ptr(false),
+			AdoptOrphans:       util.Ptr(true),
+			CreatedByAccountID: testAccount.ID,
+			CreatedByAccount:   testAccount,
+			URI:                "https://lists.example.org/baddies.csv?garbage=true",
+			ContentType:        gtsmodel.DomainPermSubContentTypeCSV,
+		}
+	)
+	defer testrig.TearDownTestStructs(testStructs)
+
+	// Store test subscription.
+	if err := testStructs.State.DB.PutDomainPermissionSubscription(
+		ctx, testSubscription,
+	); err != nil {
+		suite.FailNow(err.Error())
+	}
+
+	// Process all subscriptions.
+	subscriptions.ProcessDomainPermissionSubscriptions(ctx, testSubscription.PermissionType)
+
+	// We should now have blocks for
+	// each domain on the subscribed list.
+	for _, domain := range []string{
+		"bumfaces.net",
+		"peepee.poopoo",
+		"nothanks.com",
+	} {
+		var (
+			perm gtsmodel.DomainPermission
+			err  error
+		)
+		if !testrig.WaitFor(func() bool {
+			perm, err = testStructs.State.DB.GetDomainBlock(ctx, domain)
+			return err == nil
+		}) {
+			suite.FailNowf("", "timed out waiting for domain %s", domain)
+		}
+
+		suite.Equal(testSubscription.ID, perm.GetSubscriptionID())
+	}
+
+	// The just-fetched perm sub should have ETag
+	// set now, but last modified should be thrown away.
+	permSub, err := testStructs.State.DB.GetDomainPermissionSubscriptionByID(
+		ctx, testSubscription.ID,
+	)
+	if err != nil {
+		suite.FailNow(err.Error())
+	}
+
+	suite.Equal("\"bigbums6969\"", permSub.ETag)
+	suite.Zero(permSub.LastModified)
+}
+
 func (suite *SubscriptionsTestSuite) TestDomainBlocks404() {
 	var (
 		ctx           = context.Background()
diff --git a/internal/transport/derefdomainpermlist.go b/internal/transport/derefdomainpermlist.go
index c81117bc6..ba61c8b39 100644
--- a/internal/transport/derefdomainpermlist.go
+++ b/internal/transport/derefdomainpermlist.go
@@ -21,9 +21,11 @@ import (
 	"context"
 	"io"
 	"net/http"
+	"time"
 
 	"github.com/superseriousbusiness/gotosocial/internal/gtserror"
 	"github.com/superseriousbusiness/gotosocial/internal/gtsmodel"
+	"github.com/superseriousbusiness/gotosocial/internal/log"
 )
 
 type DereferenceDomainPermissionsResp struct {
@@ -39,6 +41,10 @@ type DereferenceDomainPermissionsResp struct {
 	// May be set
 	// if 200 or 304.
 	ETag string
+
+	// May be set
+	// if 200 or 304.
+	LastModified time.Time
 }
 
 func (t *transport) DereferenceDomainPermissions(
@@ -60,27 +66,27 @@ func (t *transport) DereferenceDomainPermissions(
 	// Set relevant Accept headers.
 	// Allow fallback in case target doesn't
 	// negotiate content type correctly.
-	req.Header.Add("Accept-Charset", "utf-8")
-	req.Header.Add("Accept", permSub.ContentType.String()+","+"*/*")
+	req.Header.Set("Accept-Charset", "utf-8")
+	req.Header.Set("Accept", permSub.ContentType.String()+","+"*/*")
 
 	// If skipCache is true, we want to skip setting Cache
 	// headers so that we definitely don't get a 304 back.
 	if !skipCache {
-		// If we've successfully fetched this list
-		// before, set If-Modified-Since to last
-		// success to make the request conditional.
+		// If we've got a Last-Modified stored for this list,
+		// set If-Modified-Since to make the request conditional.
 		//
 		// See: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/If-Modified-Since
-		if !permSub.SuccessfullyFetchedAt.IsZero() {
-			timeStr := permSub.SuccessfullyFetchedAt.Format(http.TimeFormat)
-			req.Header.Add("If-Modified-Since", timeStr)
+		if !permSub.LastModified.IsZero() {
+			// http.Time wants UTC.
+			lmUTC := permSub.LastModified.UTC()
+			req.Header.Set("If-Modified-Since", lmUTC.Format(http.TimeFormat))
 		}
 
 		// If we've got an ETag stored for this list, set
 		// If-None-Match to make the request conditional.
 		// See https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/ETag#caching_of_unchanged_resources.
-		if len(permSub.ETag) != 0 {
-			req.Header.Add("If-None-Match", permSub.ETag)
+		if permSub.ETag != "" {
+			req.Header.Set("If-None-Match", permSub.ETag)
 		}
 	}
 
@@ -99,11 +105,12 @@ func (t *transport) DereferenceDomainPermissions(
 		return nil, err
 	}
 
-	// Check already if we were given an ETag
-	// we can use, as ETag is often returned
-	// even on 304 Not Modified responses.
+	// Check already if we were given a valid ETag or
+	// Last-Modified we can use, as these cache headers
+	// are often returned even on Not Modified responses.
 	permsResp := &DereferenceDomainPermissionsResp{
-		ETag: rsp.Header.Get("Etag"),
+		ETag:         rsp.Header.Get("ETag"),
+		LastModified: validateLastModified(ctx, rsp.Header.Get("Last-Modified")),
 	}
 
 	if rsp.StatusCode == http.StatusNotModified {
@@ -119,3 +126,43 @@ func (t *transport) DereferenceDomainPermissions(
 
 	return permsResp, nil
 }
+
+// Validate Last-Modified to ensure it's not
+// garbagio, and not more than a minute in the
+// future (to allow for clock issues + rounding).
+func validateLastModified(
+	ctx context.Context,
+	lastModified string,
+) time.Time {
+	if lastModified == "" {
+		// Not set,
+		// no problem.
+		return time.Time{}
+	}
+
+	// Try to parse and see what we get.
+	switch lm, err := http.ParseTime(lastModified); {
+	case err != nil:
+		// No good,
+		// chuck it.
+		log.Debugf(ctx,
+			"discarding invalid Last-Modified header %s: %+v",
+			lastModified, err,
+		)
+		return time.Time{}
+
+	case lm.Unix() > time.Now().Add(1*time.Minute).Unix():
+		// In the future,
+		// chuck it.
+		log.Debugf(ctx,
+			"discarding in-the-future Last-Modified header %s",
+			lastModified,
+		)
+		return time.Time{}
+
+	default:
+		// It's fine,
+		// keep it.
+		return lm
+	}
+}
diff --git a/internal/transport/transport.go b/internal/transport/transport.go
index 45d43ff18..eb8f0cace 100644
--- a/internal/transport/transport.go
+++ b/internal/transport/transport.go
@@ -81,7 +81,7 @@ type Transport interface {
 	// DereferenceDomainPermissions dereferences the
 	// permissions list present at the given permSub's URI.
 	//
-	// If "force", then If-Modified-Since and If-None-Match
+	// If "skipCache", then If-Modified-Since and If-None-Match
 	// headers will *NOT* be sent with the outgoing request.
 	//
 	// If err == nil and Unmodified == false, then it's up
@@ -89,7 +89,7 @@ type Transport interface {
 	DereferenceDomainPermissions(
 		ctx context.Context,
 		permSub *gtsmodel.DomainPermissionSubscription,
-		force bool,
+		skipCache bool,
 	) (*DereferenceDomainPermissionsResp, error)
 
 	// Finger performs a webfinger request with the given username and domain, and returns the bytes from the response body.