Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the API's efficiency for managing authentication files by introducing robust batch processing capabilities. Users can now upload and delete multiple authentication files simultaneously, improving the overall user experience and reducing the overhead of individual file operations. The changes include comprehensive error handling and detailed status reporting for batch actions. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces batch upload and delete functionalities for authentication files. The UploadAuthFile handler has been refactored to support multiple file uploads via multipart form data, providing detailed status responses for individual file successes and failures. Similarly, the DeleteAuthFile handler now supports batch deletion of files specified through query parameters or the request body. New helper functions were added to modularize the file handling logic. The review comments suggest improvements such as correcting the content type check for multipart requests, standardizing error messages for JSON file validation, simplifying error handling in batch upload loops, ensuring file cleanup if auth record registration fails, and restoring the request body after reading it to prevent issues with subsequent handlers.
| } | ||
|
|
||
| func (h *Handler) multipartAuthFileHeaders(c *gin.Context) ([]*multipart.FileHeader, error) { | ||
| if h == nil || c == nil || c.ContentType() != "multipart/form-data" { |
There was a problem hiding this comment.
The check c.ContentType() != "multipart/form-data" is incorrect because the Content-Type header for multipart requests includes a boundary parameter (e.g., multipart/form-data; boundary=...). This will cause the function to incorrectly return early for valid multipart requests. You should check if the content type starts with multipart/form-data instead.
| if h == nil || c == nil || c.ContentType() != "multipart/form-data" { | |
| if h == nil || c == nil || !strings.HasPrefix(c.ContentType(), "multipart/form-data") { |
| callbackForwarders = make(map[int]*callbackForwarder) | ||
| callbackForwardersMu sync.Mutex | ||
| callbackForwarders = make(map[int]*callbackForwarder) | ||
| errAuthFileMustBeJSON = errors.New("auth file must be .json") |
There was a problem hiding this comment.
The error message for errAuthFileMustBeJSON is "auth file must be .json", but in UploadAuthFile you are creating responses with "file must be .json". To improve consistency and simplify the error handling logic in UploadAuthFile, consider aligning the error message here.
| errAuthFileMustBeJSON = errors.New("auth file must be .json") | |
| errAuthFileMustBeJSON = errors.New("file must be .json") |
| if _, errUpload := h.storeUploadedAuthFile(ctx, fileHeaders[0]); errUpload != nil { | ||
| if errors.Is(errUpload, errAuthFileMustBeJSON) { | ||
| c.JSON(http.StatusBadRequest, gin.H{"error": "file must be .json"}) | ||
| return | ||
| } | ||
| } | ||
| if errSave := c.SaveUploadedFile(file, dst); errSave != nil { | ||
| c.JSON(500, gin.H{"error": fmt.Sprintf("failed to save file: %v", errSave)}) | ||
| c.JSON(http.StatusInternalServerError, gin.H{"error": errUpload.Error()}) | ||
| return | ||
| } |
There was a problem hiding this comment.
This error handling can be made more concise. By checking for errAuthFileMustBeJSON you can set the appropriate status code and avoid duplicating logic. Assuming errAuthFileMustBeJSON's message is updated as per another suggestion, this becomes even cleaner.
| if _, errUpload := h.storeUploadedAuthFile(ctx, fileHeaders[0]); errUpload != nil { | |
| if errors.Is(errUpload, errAuthFileMustBeJSON) { | |
| c.JSON(http.StatusBadRequest, gin.H{"error": "file must be .json"}) | |
| return | |
| } | |
| } | |
| if errSave := c.SaveUploadedFile(file, dst); errSave != nil { | |
| c.JSON(500, gin.H{"error": fmt.Sprintf("failed to save file: %v", errSave)}) | |
| c.JSON(http.StatusInternalServerError, gin.H{"error": errUpload.Error()}) | |
| return | |
| } | |
| if _, errUpload := h.storeUploadedAuthFile(ctx, fileHeaders[0]); errUpload != nil { | |
| status := http.StatusInternalServerError | |
| if errors.Is(errUpload, errAuthFileMustBeJSON) { | |
| status = http.StatusBadRequest | |
| } | |
| c.JSON(status, gin.H{"error": errUpload.Error()}) | |
| return | |
| } |
| if errUpload != nil { | ||
| failureName := "" | ||
| if file != nil { | ||
| failureName = filepath.Base(file.Filename) | ||
| } | ||
| msg := errUpload.Error() | ||
| if errors.Is(errUpload, errAuthFileMustBeJSON) { | ||
| msg = "file must be .json" | ||
| } | ||
| failed = append(failed, gin.H{"name": failureName, "error": msg}) | ||
| continue |
There was a problem hiding this comment.
This error handling block can be simplified. The if file != nil check is redundant since file is a loop variable and will not be nil here. Also, if errAuthFileMustBeJSON is updated as suggested in another comment, the special handling for the error message can be removed.
if errUpload != nil {
failed = append(failed, gin.H{
"name": filepath.Base(file.Filename),
"error": errUpload.Error(),
})
continue
}| if err := h.upsertAuthRecord(ctx, auth); err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
There's a potential for an inconsistent state here. If h.upsertAuthRecord(ctx, auth) fails, the auth file has already been written to disk but won't be tracked by the auth manager. This could lead to orphaned auth files. Consider removing the file if the registration fails to maintain consistency.
| if err := h.upsertAuthRecord(ctx, auth); err != nil { | |
| return err | |
| } | |
| if err := h.upsertAuthRecord(ctx, auth); err != nil { | |
| // Attempt to clean up the file if registration fails. | |
| if errRemove := os.Remove(dst); errRemove != nil && !os.IsNotExist(errRemove) { | |
| log.WithError(errRemove).Warnf("failed to remove auth file after registration failure: %s", dst) | |
| } | |
| return err | |
| } |
| body, err := io.ReadAll(c.Request.Body) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to read body") | ||
| } |
There was a problem hiding this comment.
Reading the request body with io.ReadAll(c.Request.Body) consumes it, which can lead to unexpected behavior if other handlers or middlewares attempt to read the body later. It's a good practice to restore the request body after reading it so it can be read again if needed.
| body, err := io.ReadAll(c.Request.Body) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to read body") | |
| } | |
| body, err := io.ReadAll(c.Request.Body) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to read body") | |
| } | |
| c.Request.Body = io.NopCloser(bytes.NewBuffer(body)) |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9e5693e74f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| } | ||
| defer src.Close() | ||
|
|
||
| data, err := io.ReadAll(src) |
There was a problem hiding this comment.
storeUploadedAuthFile now calls io.ReadAll(src) for every multipart file before writing it, which means request memory usage scales with file size; a large (or intentionally oversized) upload can spike RSS and potentially OOM the process before any disk write occurs. This is a regression from the prior streamed SaveUploadedFile path and is especially risky now that batch upload is supported, so this path should stream to disk (or enforce strict size limits) rather than fully buffering.
Useful? React with 👍 / 👎.
2 participants
Implement functionality to handle batch uploads and deletions of authentication files, enhancing the API's efficiency in managing multiple files simultaneously.