-
Notifications
You must be signed in to change notification settings - Fork 3
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Experiment with extra check for an uploaded file #341
base: main
Are you sure you want to change the base?
Changes from 2 commits
969cbc9
15b67f3
540d237
de01db8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,12 +6,15 @@ import ( | |
"errors" | ||
"fmt" | ||
"net/http" | ||
"time" | ||
|
||
"github.com/cdcgov/data-exchange-upload/upload-server/internal/appconfig" | ||
"github.com/cdcgov/data-exchange-upload/upload-server/internal/storeaz" | ||
"github.com/cdcgov/data-exchange-upload/upload-server/pkg/azureinspector" | ||
"github.com/cdcgov/data-exchange-upload/upload-server/pkg/fileinspector" | ||
"github.com/cdcgov/data-exchange-upload/upload-server/pkg/info" | ||
"github.com/tus/tusd/v2/pkg/handler" | ||
"github.com/tus/tusd/v2/pkg/hooks" | ||
) | ||
|
||
type UploadInspecter interface { | ||
|
@@ -46,6 +49,49 @@ func (ih *InfoHandler) ServeHTTP(rw http.ResponseWriter, r *http.Request) { | |
enc.Encode(response) | ||
} | ||
|
||
func (ih *InfoHandler) Hook(event handler.HookEvent, resp hooks.HookResponse) (hooks.HookResponse, error) { | ||
id := event.Upload.ID | ||
ctx := event.Context | ||
|
||
var fileInfo map[string]any | ||
for i := 0; i < 5; i++ { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Make this configurable |
||
var err error | ||
fileInfo, err = ih.inspecter.InspectInfoFile(ctx, id) | ||
if err != nil { | ||
logger.Info("Failed to validate manifest file after upload", "id", id, "error", err, "manifest", fileInfo) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Include the number of attempts in all logs |
||
} | ||
if fileInfo != nil { | ||
break | ||
} | ||
time.Sleep(1 * time.Second) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Make this configurable |
||
} | ||
var uploadedFileInfo map[string]any | ||
for i := 0; i < 5; i++ { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why iterate limit to 5? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. could create a variable called |
||
var err error | ||
uploadedFileInfo, err = ih.inspecter.InspectUploadedFile(ctx, id) | ||
if err != nil { | ||
logger.Info("Failed to validate upload file", "id", id, "error", err, "manifest", fileInfo, "upload", uploadedFileInfo) | ||
} | ||
if uploadedFileInfo != nil { | ||
break | ||
} | ||
time.Sleep(1 * time.Second) | ||
} | ||
|
||
if fileInfo == nil { | ||
logger.Error("Failed to validate manifest file after upload", "id", id) | ||
return resp, errors.New("failed to find manifest") | ||
} | ||
|
||
if uploadedFileInfo == nil { | ||
logger.Error("Failed to validate upload file", "id", id, "manifest", fileInfo, "upload", uploadedFileInfo) | ||
return resp, errors.New("failed to find the uploaded file") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. normalize errors and include the upload id |
||
} | ||
|
||
logger.Info("Upload validated", "id", id, "manifest", fileInfo, "upload_info", uploadedFileInfo) | ||
return resp, nil | ||
} | ||
|
||
func getStatusFromError(err error) int { | ||
if errors.Is(err, info.ErrNotFound) { | ||
return http.StatusNotFound | ||
|
@@ -71,7 +117,7 @@ func createInspector(appConfig *appconfig.AppConfig) (UploadInspecter, error) { | |
return nil, errors.New("unable to create inspector given app configuration") | ||
} | ||
|
||
func GetUploadInfoHandler(appConfig *appconfig.AppConfig) (http.Handler, error) { | ||
func GetUploadInfoHandler(appConfig *appconfig.AppConfig) (*InfoHandler, error) { | ||
inspector, err := createInspector(appConfig) | ||
return &InfoHandler{ | ||
inspector, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,9 +9,11 @@ import ( | |
"github.com/cdcgov/data-exchange-upload/upload-server/internal/handlertusd" | ||
"github.com/cdcgov/data-exchange-upload/upload-server/internal/health" | ||
"github.com/cdcgov/data-exchange-upload/upload-server/internal/pshealth" | ||
prebuilthooks "github.com/cdcgov/data-exchange-upload/upload-server/pkg/hooks" | ||
"github.com/cdcgov/data-exchange-upload/upload-server/pkg/redislocker" | ||
"github.com/prometheus/client_golang/prometheus/promhttp" | ||
"github.com/tus/tusd/v2/pkg/hooks" | ||
tusHooks "github.com/tus/tusd/v2/pkg/hooks" | ||
"github.com/tus/tusd/v2/pkg/memorylocker" | ||
) | ||
|
||
|
@@ -57,6 +59,9 @@ func Serve(appConfig appconfig.AppConfig) (http.Handler, error) { | |
logger.Error("error configuring tusd handler: ", "error", err) | ||
return nil, err | ||
} | ||
if h, ok := hookHandler.(*prebuilthooks.PrebuiltHook); ok { | ||
h.Register(tusHooks.HookPreFinish, uploadInfoHandler.Hook) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why not use the post finish hook? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. https://tus.github.io/tusd/advanced-topics/hooks/#list-of-available-hooks pre-finish should fire after the upload is done but before responding to the client. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Alternatively, you can also use the PreFinishResponseCallback from Config, which is the basis for the pre-finish hook and therefore provides the same capabilities. This could be a useful alternative if this change is the only reason why you start adding hooks. |
||
} | ||
|
||
// initialize tusd handler | ||
handlerTusd, err := handlertusd.New(store, locker, hookHandler, appConfig.TusdHandlerBasePath) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this
UploadInspector
or UploadInspecter? in
info.go, you call a method called
createInspector()`