Skip to content
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

Adding capability to take max upload size per request.Fileupload 1 3 1 #22

Conversation

codecracker2014
Copy link

As of now FileUploadBase has sizeMax which is global for all uploads, in many cases it is required to calculate this limit based on request.

https://stackoverflow.com/questions/16585866/changing-file-size-limit-maxuploadsize-depending-on-the-controller

As of now FileUploadBase has sizeMax which is global for all uploads, in many cases it is required to calculate this limit based on request.

https://stackoverflow.com/questions/16585866/changing-file-size-limit-maxuploadsize-depending-on-the-controller
As of now FileUploadBase has sizeMax which is global for all uploads, in many cases it is required to calculate this limit based on request.

https://stackoverflow.com/questions/16585866/changing-file-size-limit-maxuploadsize-depending-on-the-controller
Copy link
Member

@garydgregory garydgregory left a comment

Choose a reason for hiding this comment

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

I'm pretty sure no one is looking at old branches for this component. You should based your PR off of master.
This PR needs tests. There is also no documentation for the new feature.

@codecracker2014 codecracker2014 changed the base branch from master to FILEUPLOAD_1_3_1 September 21, 2019 09:19
@jochenw
Copy link
Contributor

jochenw commented Sep 21, 2019

  1. I'd support Gary's request, that you provide a PR for the current master, and not the stable branch.

  2. Well spotted, that you should use the RequestContext, to support both the servlet, and the portlet case!

  3. Rather than using a method getSizeMaxForThisRequest, I'd suggest a more general approach, (The 1.4 branch is still open for API changes.) such as:

    • Replace

FileItemStream FileUploadBase.getItemIterator(RequestContext pCtx)

with a method

FileItemRequest getItemIterator(RequestContext pCtx)

where FileItemRequest would contain methods like

void setMaxUploadSizeExceeded(Predicate<Long> pPredicate) {
}
FileItemStream getItemIterator();

On the other hand, rethinking that, it is probably a better idea, if I do that myself.

@jochenw
Copy link
Contributor

jochenw commented Sep 23, 2019

Please close in favour of FILEUPLOAD-300.

@codecracker2014
Copy link
Author

@jochenw i implemented current solution avoiding breaking changes for 1.3.1, but i agree with you for master, we can do it with api changes. Also i'll do more brainstorming on this feature. Closing this PR in favour of FILEUPLOAD-300.

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