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

Add ExternalContext.setResponseContentLengthLong #1764

Closed
morvael opened this issue Dec 12, 2022 · 13 comments
Closed

Add ExternalContext.setResponseContentLengthLong #1764

morvael opened this issue Dec 12, 2022 · 13 comments
Labels
mojarra-implemented API issue implemented by Mojarra myfaces-implemented API issue implemented by MyFaces
Milestone

Comments

@morvael
Copy link

morvael commented Dec 12, 2022

I have discovered that ExternalContext.setResponseContentLength is supposed to call ServletResponse.setContentLength.

Currently ServletResponse provides long variant as well for files bigger than 2GB - ServletResponse.setContentLengthLong (since version 3.1). Sadly there is no corresponding method for ExternalContext and thus it's harder to serve bigger files using ExternalContext. Please add the mising method to API.

@tandraschko
Copy link

@arjantijms @BalusC can we do this for 4.0? small and good API enhancement

@BalusC
Copy link
Member

BalusC commented Dec 12, 2022

I'm wondering if we can't just update setResponseContentLength to take long instead of int.

thus it's harder to serve bigger files using ExternalContext

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

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

@tandraschko maybe it could be used in PF as well 😉

@tandraschko
Copy link

@BalusC yeah, i think we can just switch int to long
even easier

@morvael
Copy link
Author

morvael commented Dec 13, 2022

Looks non-standard seeing how all the other additions of Long were handled (for example in ServletResponse). Something may not compile somewhere.

@BalusC
Copy link
Member

BalusC commented Dec 13, 2022

Java 1.5 introduced autoboxing. That servlet method might have been added with earlier Java version in mind. I can't think of Java 1.5+ cases where it would fail.

Just did a quick test for sake that:

public static void main(String... args) {
    int primitiveInt = 123;
    Integer integerObject = Integer.valueOf(123);

    setContentLength(primitiveInt);
    setContentLength((int) primitiveInt);
    setContentLength((Integer) primitiveInt);
    setContentLength(integerObject);
    setContentLength((int) integerObject);
    setContentLength((Integer) integerObject);
}

public static void setContentLength(long contentLength) {
    System.out.println("Content length: " + contentLength);
}

@BalusC
Copy link
Member

BalusC commented Dec 13, 2022

Ok nevermind. It will indeed fail when one extends ExternalContext and overrides setContentLength() method with an int argument. That'd be a breaking change.

@morvael
Copy link
Author

morvael commented Dec 13, 2022

My IDE is not a fan:
image

@morvael
Copy link
Author

morvael commented Dec 13, 2022

COMPILATION ERROR : 
-------------------------------------------------------------
MainLong.java:[29,20] incompatible types: int cannot be converted to java.lang.Long
MainLong.java:[31,31] incompatible types: java.lang.Long cannot be converted to java.lang.Integer

Compiler is neither. Autoboxing is one thing. But you can have different type mismatches here (primitive int to object Long and class mismatch).

@morvael
Copy link
Author

morvael commented Dec 13, 2022

So assuming setValue and getValue used to be Integer and now will change to Long it will break code for many. Not that it's hard to fix, but still. Override is also a valid point.

@morvael
Copy link
Author

morvael commented Dec 13, 2022

Ah my mistake, I was coming from PF Integer/Long methods, ExternalContext and ServletResponse use primitives. That helps a lot.

@BalusC
Copy link
Member

BalusC commented Dec 13, 2022

Moreover there's no getResponseContentLength().

@morvael
Copy link
Author

morvael commented Dec 13, 2022

Still, assymetry in APIs hurts my eyes :)

BalusC added a commit to eclipse-ee4j/mojarra that referenced this issue Dec 13, 2022
Specify ExternalContext#setResponseContentLengthLong
BalusC added a commit to eclipse-ee4j/mojarra that referenced this issue Dec 13, 2022
Implement ExternalContext#setResponseContentLengthLong
BalusC added a commit to eclipse-ee4j/mojarra that referenced this issue Dec 13, 2022
Add ExternalContextWrapper#setResponseContentLengthLong
@BalusC BalusC added this to the 4.1 milestone Dec 17, 2022
@BalusC BalusC closed this as completed Dec 17, 2022
@tandraschko tandraschko added mojarra-implemented API issue implemented by Mojarra myfaces-implemented API issue implemented by MyFaces labels Oct 4, 2023
@BalusC BalusC changed the title Add javax.faces.context.ExternalContext.setResponseContentLengthLong Add ExternalContext.setResponseContentLengthLong Nov 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
mojarra-implemented API issue implemented by Mojarra myfaces-implemented API issue implemented by MyFaces
Projects
None yet
Development

No branches or pull requests

3 participants