Further optimize client-server roundtrips by including deletion and dump requests in submit responses (follow up to 1e43de689fa5ee8fb07862cc007c298389670bdb)

backend/server/internal/server/api_handlers.go

@@ -1,7 +1,6 @@
 package server
 
 import (
-	"context"
 	"encoding/json"
 	"fmt"
 	"html"
@@ -49,34 +48,26 @@ func (s *Server) apiSubmitHandler(w http.ResponseWriter, r *http.Request) {
 		s.statsd.Count("hishtory.submit", int64(len(devices)), []string{}, 1.0)
 	}
 
+	resp := shared.SubmitResponse{}
+
 	deviceId := getOptionalQueryParam(r, "source_device_id", s.isTestEnvironment)
-	resp := shared.SubmitResponse{
+	if deviceId != "" {
-		HaveDumpRequests:     s.haveDumpRequests(r.Context(), userId, deviceId),
+		dumpRequests, err := s.db.DumpRequestForUserAndDevice(r.Context(), userId, deviceId)
-		HaveDeletionRequests: s.haveDeletionRequests(r.Context(), userId, deviceId),
+		checkGormError(err)
+		resp.DumpRequests = dumpRequests
+
+		deletionRequests, err := s.db.DeletionRequestsForUserAndDevice(r.Context(), userId, deviceId)
+		checkGormError(err)
+		resp.DeletionRequests = deletionRequests
+
+		// TODO: Update this code to call DeletionRequestInc() iff the version is new enough to be using these responses
 	}
+
 	if err := json.NewEncoder(w).Encode(resp); err != nil {
 		panic(err)
 	}
 }
 
-func (s *Server) haveDumpRequests(ctx context.Context, userId, deviceId string) bool {
-	if userId == "" || deviceId == "" {
-		return true
-	}
-	dumpRequests, err := s.db.DumpRequestForUserAndDevice(ctx, userId, deviceId)
-	checkGormError(err)
-	return len(dumpRequests) > 0
-}
-
-func (s *Server) haveDeletionRequests(ctx context.Context, userId, deviceId string) bool {
-	if userId == "" || deviceId == "" {
-		return true
-	}
-	deletionRequests, err := s.db.DeletionRequestsForUserAndDevice(ctx, userId, deviceId)
-	checkGormError(err)
-	return len(deletionRequests) > 0
-}
-
 func (s *Server) apiBootstrapHandler(w http.ResponseWriter, r *http.Request) {
 	userId := getRequiredQueryParam(r, "user_id")
 	deviceId := getRequiredQueryParam(r, "device_id")

backend/server/internal/server/server_test.go

@@ -81,7 +81,8 @@ func TestESubmitThenQuery(t *testing.T) {
 	w := httptest.NewRecorder()
 	s.apiSubmitHandler(w, submitReq)
 	require.Equal(t, 200, w.Result().StatusCode)
-	require.Equal(t, shared.SubmitResponse{HaveDumpRequests: true, HaveDeletionRequests: false}, deserializeSubmitResponse(t, w))
+	require.Empty(t, deserializeSubmitResponse(t, w).DeletionRequests)
+	require.NotEmpty(t, deserializeSubmitResponse(t, w).DumpRequests)
 
 	// Query for device id 1
 	w = httptest.NewRecorder()
@@ -346,7 +347,8 @@ func TestDeletionRequests(t *testing.T) {
 	w := httptest.NewRecorder()
 	s.apiSubmitHandler(w, submitReq)
 	require.Equal(t, 200, w.Result().StatusCode)
-	require.Equal(t, shared.SubmitResponse{HaveDumpRequests: true, HaveDeletionRequests: false}, deserializeSubmitResponse(t, w))
+	require.Empty(t, deserializeSubmitResponse(t, w).DeletionRequests)
+	require.NotEmpty(t, deserializeSubmitResponse(t, w).DumpRequests)
 
 	// And another entry for user1
 	entry2 := testutils.MakeFakeHistoryEntry("ls /foo/bar")
@@ -359,7 +361,8 @@ func TestDeletionRequests(t *testing.T) {
 	w = httptest.NewRecorder()
 	s.apiSubmitHandler(w, submitReq)
 	require.Equal(t, 200, w.Result().StatusCode)
-	require.Equal(t, shared.SubmitResponse{HaveDumpRequests: true, HaveDeletionRequests: false}, deserializeSubmitResponse(t, w))
+	require.Empty(t, deserializeSubmitResponse(t, w).DeletionRequests)
+	require.NotEmpty(t, deserializeSubmitResponse(t, w).DumpRequests)
 
 	// And an entry for user2 that has the same timestamp as the previous entry
 	entry3 := testutils.MakeFakeHistoryEntry("ls /foo/bar")
@@ -373,7 +376,8 @@ func TestDeletionRequests(t *testing.T) {
 	w = httptest.NewRecorder()
 	s.apiSubmitHandler(w, submitReq)
 	require.Equal(t, 200, w.Result().StatusCode)
-	require.Equal(t, shared.SubmitResponse{HaveDumpRequests: true, HaveDeletionRequests: false}, deserializeSubmitResponse(t, w))
+	require.Empty(t, deserializeSubmitResponse(t, w).DeletionRequests)
+	require.NotEmpty(t, deserializeSubmitResponse(t, w).DumpRequests)
 
 	// Query for device id 1
 	w = httptest.NewRecorder()
@@ -485,7 +489,8 @@ func TestDeletionRequests(t *testing.T) {
 	w = httptest.NewRecorder()
 	s.apiSubmitHandler(w, submitReq)
 	require.Equal(t, 200, w.Result().StatusCode)
-	require.Equal(t, shared.SubmitResponse{HaveDumpRequests: true, HaveDeletionRequests: true}, deserializeSubmitResponse(t, w))
+	require.NotEmpty(t, deserializeSubmitResponse(t, w).DeletionRequests)
+	require.NotEmpty(t, deserializeSubmitResponse(t, w).DumpRequests)
 
 	// Query for deletion requests
 	w = httptest.NewRecorder()
@@ -585,7 +590,8 @@ func TestCleanDatabaseNoErrors(t *testing.T) {
 	w := httptest.NewRecorder()
 	s.apiSubmitHandler(w, submitReq)
 	require.Equal(t, 200, w.Result().StatusCode)
-	require.Equal(t, shared.SubmitResponse{HaveDumpRequests: true, HaveDeletionRequests: false}, deserializeSubmitResponse(t, w))
+	require.Empty(t, deserializeSubmitResponse(t, w).DeletionRequests)
+	require.NotEmpty(t, deserializeSubmitResponse(t, w).DumpRequests)
 
 	// Call cleanDatabase and just check that there are no panics
 	testutils.Check(t, DB.Clean(context.TODO()))

client/cmd/saveHistoryEntry.go

@@ -187,8 +187,6 @@ func saveHistoryEntry(ctx context.Context) {
 	lib.CheckFatalError(err)
 
 	// Persist it remotely
-	shouldCheckForDeletionRequests := true
-	shouldCheckForDumpRequests := true
 	if !config.IsOffline {
 		jsonValue, err := lib.EncryptAndMarshal(config, []*data.HistoryEntry{entry})
 		lib.CheckFatalError(err)
@@ -200,49 +198,11 @@ func saveHistoryEntry(ctx context.Context) {
 			if err != nil {
 				lib.CheckFatalError(fmt.Errorf("failed to deserialize response from /api/v1/submit: %w", err))
 			}
-			shouldCheckForDeletionRequests = submitResponse.HaveDeletionRequests
+			lib.CheckFatalError(handleDumpRequests(ctx, submitResponse.DumpRequests))
-			shouldCheckForDumpRequests = submitResponse.HaveDumpRequests
+			lib.CheckFatalError(lib.HandleDeletionRequests(ctx, submitResponse.DeletionRequests))
 		}
 	}
 
-	// Check if there is a pending dump request and reply to it if so
-	if shouldCheckForDumpRequests {
-		dumpRequests, err := lib.GetDumpRequests(config)
-		if err != nil {
-			if lib.IsOfflineError(err) {
-				// It is fine to just ignore this, the next command will retry the API and eventually we will respond to any pending dump requests
-				dumpRequests = []*shared.DumpRequest{}
-				hctx.GetLogger().Infof("Failed to check for dump requests because we failed to connect to the remote server!")
-			} else {
-				lib.CheckFatalError(err)
-			}
-		}
-		if len(dumpRequests) > 0 {
-			lib.CheckFatalError(lib.RetrieveAdditionalEntriesFromRemote(ctx))
-			entries, err := lib.Search(ctx, db, "", 0)
-			lib.CheckFatalError(err)
-			var encEntries []*shared.EncHistoryEntry
-			for _, entry := range entries {
-				enc, err := data.EncryptHistoryEntry(config.UserSecret, *entry)
-				lib.CheckFatalError(err)
-				encEntries = append(encEntries, &enc)
-			}
-			reqBody, err := json.Marshal(encEntries)
-			lib.CheckFatalError(err)
-			for _, dumpRequest := range dumpRequests {
-				if !config.IsOffline {
-					_, err := lib.ApiPost("/api/v1/submit-dump?user_id="+dumpRequest.UserId+"&requesting_device_id="+dumpRequest.RequestingDeviceId+"&source_device_id="+config.DeviceId, "application/json", reqBody)
-					lib.CheckFatalError(err)
-				}
-			}
-		}
-	}
-
-	// Handle deletion requests
-	if shouldCheckForDeletionRequests {
-		lib.CheckFatalError(lib.ProcessDeletionRequests(ctx))
-	}
-
 	if config.BetaMode {
 		db.Commit()
 	}
@@ -253,6 +213,31 @@ func init() {
 	rootCmd.AddCommand(presaveHistoryEntryCmd)
 }
 
+func handleDumpRequests(ctx context.Context, dumpRequests []*shared.DumpRequest) error {
+	db := hctx.GetDb(ctx)
+	config := hctx.GetConf(ctx)
+	if len(dumpRequests) > 0 {
+		lib.CheckFatalError(lib.RetrieveAdditionalEntriesFromRemote(ctx))
+		entries, err := lib.Search(ctx, db, "", 0)
+		lib.CheckFatalError(err)
+		var encEntries []*shared.EncHistoryEntry
+		for _, entry := range entries {
+			enc, err := data.EncryptHistoryEntry(config.UserSecret, *entry)
+			lib.CheckFatalError(err)
+			encEntries = append(encEntries, &enc)
+		}
+		reqBody, err := json.Marshal(encEntries)
+		lib.CheckFatalError(err)
+		for _, dumpRequest := range dumpRequests {
+			if !config.IsOffline {
+				_, err := lib.ApiPost("/api/v1/submit-dump?user_id="+dumpRequest.UserId+"&requesting_device_id="+dumpRequest.RequestingDeviceId+"&source_device_id="+config.DeviceId, "application/json", reqBody)
+				lib.CheckFatalError(err)
+			}
+		}
+	}
+	return nil
+}
+
 func buildPreArgsHistoryEntry(ctx context.Context) (*data.HistoryEntry, error) {
 	var entry data.HistoryEntry
 

client/cmd/status.go

@@ -22,7 +22,6 @@ var statusCmd = &cobra.Command{
 		if *verbose {
 			fmt.Printf("User ID: %s\n", data.UserId(config.UserSecret))
 			fmt.Printf("Device ID: %s\n", config.DeviceId)
-			printDumpStatus(config)
 			printOnlineStatus(config)
 		}
 		fmt.Printf("Commit Hash: %s\n", lib.GitCommit)
@@ -42,16 +41,6 @@ func printOnlineStatus(config hctx.ClientConfig) {
 	}
 }
 
-func printDumpStatus(config hctx.ClientConfig) {
-	dumpRequests, err := lib.GetDumpRequests(config)
-	lib.CheckFatalError(err)
-	fmt.Printf("Dump Requests: ")
-	for _, d := range dumpRequests {
-		fmt.Printf("%#v, ", *d)
-	}
-	fmt.Print("\n")
-}
-
 func init() {
 	rootCmd.AddCommand(statusCmd)
 	verbose = statusCmd.Flags().BoolP("verbose", "v", false, "Display verbose hiSHtory information")

client/lib/lib.go

@@ -598,6 +598,10 @@ func ProcessDeletionRequests(ctx context.Context) error {
 	if err != nil {
 		return err
 	}
+	return HandleDeletionRequests(ctx, deletionRequests)
+}
+
+func HandleDeletionRequests(ctx context.Context, deletionRequests []*shared.DeletionRequest) error {
 	db := hctx.GetDb(ctx)
 	for _, request := range deletionRequests {
 		for _, entry := range request.Messages.Ids {
@@ -876,22 +880,6 @@ func unescape(query string) string {
 	return string(newQuery)
 }
 
-func GetDumpRequests(config hctx.ClientConfig) ([]*shared.DumpRequest, error) {
-	if config.IsOffline {
-		return make([]*shared.DumpRequest, 0), nil
-	}
-	resp, err := ApiGet("/api/v1/get-dump-requests?user_id=" + data.UserId(config.UserSecret) + "&device_id=" + config.DeviceId)
-	if IsOfflineError(err) {
-		return []*shared.DumpRequest{}, nil
-	}
-	if err != nil {
-		return nil, err
-	}
-	var dumpRequests []*shared.DumpRequest
-	err = json.Unmarshal(resp, &dumpRequests)
-	return dumpRequests, err
-}
-
 func SendDeletionRequest(deletionRequest shared.DeletionRequest) error {
 	data, err := json.Marshal(deletionRequest)
 	if err != nil {

shared/data.go

@@ -118,11 +118,11 @@ type Feedback struct {
 	Feedback string    `json:"feedback"`
 }
 
-// Response from submitting new history entries. Contains metadata that is used to avoid making additional round-trip
+// Response from submitting new history entries. Contains deletion requests and dump requests to avoid
-// requests to the hishtory backend.
+// extra round-trip requests to the hishtory backend.
 type SubmitResponse struct {
-	HaveDumpRequests     bool `json:"have_dump_requests"`
+	DumpRequests     []*DumpRequest     `json:"dump_requests"`
-	HaveDeletionRequests bool `json:"have_deletion_requests"`
+	DeletionRequests []*DeletionRequest `json:"deletion_requests"`
 }
 
 func Chunks[k any](slice []k, chunkSize int) [][]k {