Skip to content

Conversation

SpursJiang
Copy link

  • Add file upload size validation with user-friendly error messages
  • Implement HandshakeController to communicate size limits to frontend
  • Add GlobalExceptionHandler for handling file size exceeded errors
  • Update FileTransferForm to show clear size limit warnings
  • Add bilingual documentation for file upload configuration
  • Support both English and Chinese error messages

- Add file upload size validation with user-friendly error messages
- Implement HandshakeController to communicate size limits to frontend
- Add GlobalExceptionHandler for handling file size exceeded errors
- Update FileTransferForm to show clear size limit warnings
- Add bilingual documentation for file upload configuration
- Support both English and Chinese error messages

Signed-off-by: jiangpeng <949474720@qq.com>
- Fix test compatibility issue by making MultipartProperties parameter optional
- Add null check to prevent NullPointerException in test environments
- Ensure file size limit feature works when MultipartProperties is available

Signed-off-by: spursjiang <949474720@qq.com>
@D-D-H D-D-H requested review from D-D-H and Copilot and removed request for D-D-H September 15, 2025 09:31
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR implements configurable file upload size limits with comprehensive user experience improvements. It adds dynamic size validation, friendly error messages in both English and Chinese, and seamless communication between backend configuration and frontend interface.

  • Dynamic file size limit configuration retrieved from backend via handshake API
  • Comprehensive file upload error handling with size-specific messages
  • Frontend validation with immediate feedback and bilingual support

Reviewed Changes

Copilot reviewed 16 out of 17 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
site/docs/zh/guide/file-upload-configuration.md New comprehensive Chinese documentation for file upload configuration
site/docs/zh/guide/configuration.md Added Chinese documentation for multipart configuration properties
site/docs/guide/file-upload-configuration.md New comprehensive English documentation for file upload configuration
site/docs/guide/configuration.md Added English documentation for multipart configuration properties
site/docs/guide/changelog.md Updated changelog with file upload enhancement features
server/src/main/java/org/eclipse/jifa/server/util/ErrorUtil.java Added utility method for creating structured error responses
server/src/main/java/org/eclipse/jifa/server/enums/ServerErrorCode.java Added FILE_TOO_LARGE error code for file size violations
server/src/main/java/org/eclipse/jifa/server/domain/dto/HandshakeResponse.java Extended handshake response to include maximum file size configuration
server/src/main/java/org/eclipse/jifa/server/controller/HandshakeController.java Enhanced controller to communicate file size limits to frontend
server/src/main/java/org/eclipse/jifa/server/controller/GlobalExceptionHandler.java Added comprehensive file upload size exception handling
frontend/src/stores/env.ts Added file size limit storage and management in environment store
frontend/src/i18n/zh.ts Added Chinese translations for file upload messages
frontend/src/i18n/en.ts Added English translations for file upload messages
frontend/src/components/forms/FileTransferForm.vue Enhanced upload form with size validation and error handling
analysis/gc-log/src/test/java/org/eclipse/jifa/gclog/TestFragmentedParserToMetrics.java Fixed comma placement in printf statement
analysis/gc-log/src/main/java/org/eclipse/jifa/gclog/parser/AbstractPreUnifiedGCLogParser.java Fixed parenthesis character in comment
Files not reviewed (1)
  • frontend/package-lock.json: Language not supported

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@D-D-H
Copy link
Contributor

D-D-H commented Sep 15, 2025

eca has passed.

@D-D-H D-D-H closed this Sep 15, 2025
@D-D-H D-D-H reopened this Sep 15, 2025
- Fix null MultipartProperties handling in HandshakeController
- Simplify fragile string parsing in GlobalExceptionHandler
- Improve error.response type safety in FileTransferForm
@SpursJiang
Copy link
Author

Thanks for the suggestions! I've addressed all issues。

@SpursJiang
Copy link
Author

Could you please merge my pull request?

}
// Format file size
function formatFileSize(bytes) {
Copy link
Contributor

@D-D-H D-D-H Sep 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can reuse the simmilar function in

export function prettySize(size: number): string {

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes,done


disabledFileTransferMethods: []
disabledFileTransferMethods: [],
maxFileSize: 536870912 // Default 512MB
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unlimited size for the default may be a better choice.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes,done

import {formatDate} from '@vueuse/core';
import {ElNotification} from 'element-plus';
import {h} from 'vue';
import { useI18n } from 'vue-i18n';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

imported but not used?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes,done

/**
* Default maximum file size limit (512MB) - matches Spring Boot's default
*/
long DEFAULT_MAX_FILE_SIZE_BYTES = 512L * 1024L * 1024L;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unlimited size for the default may be a better choice.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes,done

@@ -1,5 +1,5 @@
/********************************************************************************
* Copyright (c) 2023 Contributors to the Eclipse Foundation
* Copyright (c) 2023, 2024 Contributors to the Eclipse Foundation
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2024 -> 2025

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

int exp = (int) (Math.log(bytes) / Math.log(1024));
String pre = "KMGTPE".charAt(exp - 1) + "";
return String.format("%.1f %sB", bytes / Math.pow(1024, exp), pre);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if we already have a similar method in our Java code.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes,done

/**
* Handle file upload size exceeded exceptions with simple, reliable error message
*/
private void handleFileUploadSizeExceeded(MaxUploadSizeExceededException e, HttpServletResponse response) throws IOException {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need to handle this exception especially?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What this achieves:
Automatic interception: When Spring MVC throws MaxUploadSizeExceededException, our handler catches it immediately
User-friendly response: Returns a structured error with FILE_TOO_LARGE error code and formatted file size
Frontend integration: Provides the exact data needed for frontend internationalization
Proper HTTP status: Returns 413 (Payload Too Large) status code following HTTP standards
Logging for debugging: Captures detailed information for troubleshooting while keeping user-facing messages clean

config.getDisabledFileTransferMethods(),
user);
user,
maxFileSizeBytes);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If maxFileSizeBytes only limits the files uploaded through web pages, I think we should use a more appropriate name, like maxUploadSize.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes,done

return json.toString().getBytes(Constant.CHARSET);
}

public static byte[] toJson(ErrorCode errorCode, String message) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why don't we reuse the method: public static byte[] toJson(Throwable throwable).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes,done

@D-D-H
Copy link
Contributor

D-D-H commented Sep 25, 2025

@SpursJiang

Hi, sorry for the late response.

I left some comments for this patch.

Thanks for the contribution.

…back

- Remove duplicate formatFileSize function, reuse existing prettySize utility
- Use unlimited size as default instead of hardcoded 512MB limit
- Update copyright year to 2025 in GlobalExceptionHandler
- Replace custom formatFileSize with Apache Commons FileUtils.byteCountToDisplaySize
- Rename maxFileSizeBytes to maxUploadSize for better clarity
- Refactor ErrorUtil.toJson to eliminate code duplication with shared buildJsonBytes method
- Simplify file upload error handling logic to use 200 status code
- Remove unused imports and clean up code structure
- Ensure consistent configuration logic between HandshakeController and GlobalExceptionHandler
@SpursJiang
Copy link
Author

Thanks for the suggestions! I've addressed all issues。Could you please merge my pull request?

@SpursJiang

Hi, sorry for the late response.

I left some comments for this patch.

Thanks for the contribution.

Thanks for the suggestions! I've addressed all issues。Could you please merge my pull request?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants