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

DefaultStreamedContent.contentLength is Integer #9485

Closed
morvael opened this issue Dec 12, 2022 · 27 comments · Fixed by #9493
Closed

DefaultStreamedContent.contentLength is Integer #9485

morvael opened this issue Dec 12, 2022 · 27 comments · Fixed by #9493
Assignees
Labels
enhancement Additional functionality to current component
Milestone

Comments

@morvael
Copy link

morvael commented Dec 12, 2022

I think Long would be better for bigger files.

@tandraschko
Copy link
Member

see ExternalContext:

public void setResponseContentLength(int length) {

@morvael
Copy link
Author

morvael commented Dec 12, 2022

I have a 5GB file to push through it, and it gets cut off at 2GB. What can be done?

@morvael
Copy link
Author

morvael commented Dec 12, 2022

See

  setContentLengthLong

setContentLengthLong

@morvael
Copy link
Author

morvael commented Dec 12, 2022

ExternalContext says it must push it through ServletResponse and ServletResponse now has the correct method for longs.

@morvael
Copy link
Author

morvael commented Dec 12, 2022

Seems like a fckp of the ExternalContext guys.

@morvael
Copy link
Author

morvael commented Dec 12, 2022

Can you get to ServletResponse bypassing ExternalContext?

@tandraschko
Copy link
Member

I think you should create a faces specification issue first
we rely on faces api and try to avoid direct servlet calls as we also need to care about porlets

@morvael
Copy link
Author

morvael commented Dec 12, 2022

Yes: "Servlet: This must be performed by calling the javax.servlet.http.HttpServletResponse setContentLength method."
but ServletResponse has now new method for Long length.

@morvael
Copy link
Author

morvael commented Dec 12, 2022

I think you should create a faces specification issue first we rely on faces api and try to avoid direct servlet calls as we also need to care about porlets

And wait a few years? I think I need a hack around your code and ExternalContext as well for today (business reality). Do you have an idea how?

@tandraschko
Copy link
Member

Faces 4.0 is on the track, might be a good chance currently to get it in

@tandraschko
Copy link
Member

i would copy StreamedContentHandler in your project in the same package, so yours is used

@morvael
Copy link
Author

morvael commented Dec 12, 2022

Somehow I guess I need to be able to override FileDownloadActionListener.regularDownload and plug my implementation if that's possible?

@morvael
Copy link
Author

morvael commented Dec 12, 2022

i would copy StreamedContentHandler in your project in the same package, so yours is used

Clever. What about the other place I saw StreamedContent.getContentLength used?

@morvael
Copy link
Author

morvael commented Dec 12, 2022

I hope this will be enough:
jakartaee/faces#1764

@jepsar jepsar closed this as not planned Won't fix, can't repro, duplicate, stale Dec 12, 2022
@melloware
Copy link
Member

From BalusC: FYI: work around: ec.setResponseHeader("Content-Length", String.valueOf(contentLengthLong)). Has been in among others OmniFaces Faces#sendFile() for ages.

@morvael
Copy link
Author

morvael commented Dec 12, 2022

Not very helpful if all I have to work with p:fileDownload and StreamedContent is Integer contentLength. That part of PF API needs enchancement too, in anticipation of JSF API enchancement. Basically the workaround is only for PF internal classes, not for someone using PF API.

@morvael
Copy link
Author

morvael commented Dec 12, 2022

  1. you change StreamedContent from Integer to Long
  2. temporarily you use setResponseHeader workaround in FileDownloadActionListener and StreamedContentHandler
  3. once JSF API 4.0 is ready you switch from workaround to new Long methods in same classes, without affecting PF users

That seems like a good plan for me :)

@melloware
Copy link
Member

agreed care to submit a PR?

@melloware melloware reopened this Dec 12, 2022
@melloware melloware added the enhancement Additional functionality to current component label Dec 12, 2022
@melloware melloware added this to the 13.0.0 milestone Dec 12, 2022
@morvael
Copy link
Author

morvael commented Dec 12, 2022

Lol, now I'll become a laughingstock for not knowing how you submit a PR. As for code modification, I can do it. Plus there's a decision to make whether keep old methods in StreamedContent and add Long ones, or just change existing ones to Long.

@morvael
Copy link
Author

morvael commented Dec 12, 2022

Quickly, I can provide only this (changed files in zip):
primefaces.zip

Issues:

  • DefaultStreamedContent serialized form changes
  • classes extending DefaultStreamedContent may not work correctly without overriding getContentLengthLong (wondered about default implementation in StreamedContent, but don't remember what is your lowest supported Java version, plus it doesn't help when extending DefaultStreamedContent which has its own implementation).

@melloware
Copy link
Member

@morvael i will take a look.

@melloware melloware assigned melloware and unassigned morvael Dec 12, 2022
@melloware
Copy link
Member

PR submitted using OmniFaces trick.

melloware added a commit to melloware/primefaces that referenced this issue Dec 13, 2022
@melloware
Copy link
Member

Looks like BalusC just submitted this to Mojarra: eclipse-ee4j/mojarra#5187

@tandraschko
Copy link
Member

@melloware will you take of this in MF?

@melloware
Copy link
Member

Yep it looks like he submitted that for Mojarra 4.1 but we don't have a 4.1 can I just put it in 4.0.RC3?

@tandraschko
Copy link
Member

oh then lets wait till we have a 4.1 branch

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

Successfully merging a pull request may close this issue.

4 participants