Experiment with extra check for an uploaded file #341
Conversation
whytheplatypus
force-pushed
the
feature/double-check-file
branch
from
4b33f10
to
969cbc9
Compare
| @@ -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.
Why not use the post finish hook?
There was a problem hiding this comment.
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.
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.
| ctx := event.Context | ||
|
|
||
| var fileInfo map[string]any | ||
| for i := 0; i < 5; i++ { |
There was a problem hiding this comment.
Make this configurable
| if fileInfo != nil { | ||
| break | ||
| } | ||
| time.Sleep(1 * time.Second) |
There was a problem hiding this comment.
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.
Include the number of attempts in all logs
|
|
||
| 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.
normalize errors and include the upload id
| time.Sleep(1 * time.Second) | ||
| } | ||
| var uploadedFileInfo map[string]any | ||
| for i := 0; i < 5; i++ { |
There was a problem hiding this comment.
why iterate limit to 5?
There was a problem hiding this comment.
could create a variable called maxRetries to represent the iteration limit
|
|
||
| "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 { |
There was a problem hiding this comment.
is this UploadInspector or UploadInspecter? in info.go, you call a method called createInspector()`
|
|
quick benchmark shows no meaningful change in performance with or without verification. |
This double checks an uploaded file's existence in the pre-finish hook, which is called after a file is uploaded but before a response to a client.