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

Removed usage of stream.available for stream size (it is the entire size in some implementations but it is not guaranteed) #4481

Closed
wants to merge 1 commit into from

Conversation

mrydzy
Copy link

@mrydzy mrydzy commented May 18, 2015

* Checking file size for JarUrlConnection and FileUrlConnection
* Sending chuncked response otherwise

@@ -475,6 +489,18 @@ class AssetsBuilder(errorHandler: HttpErrorHandler) extends Controller {
}
}

def getResult(file: String, assetInfo: AssetInfo, connection: URLConnection, gzipRequested: Boolean): Result = {
Copy link
Member

Choose a reason for hiding this comment

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

Make this private, also perhaps rename to createResult.

@jroper
Copy link
Member

jroper commented May 18, 2015

We should probably have some tests that assert the content length is right - they can be added to framework/src/play-integration-test/src/test/scala/play/it/http/assets/AssetsSpec.scala. I don't think any of the existing tests check for a content length header.

Also, would be good to test both the file case and the jar case. All the existing tests there, in practice, are testing the file case. You'll need to come up with a resource that can be served from a jar off the classpath. This may best be achieved by creating a custom URLClassLoader that gets passed to the ServerConfig constructor used in that spec, that zips up the testresources directory to ensure it's on the classpath. Let me know if you need help doing that.

    * Checking file size for JarUrlConnection and FileUrlConnection
    * Sending chuncked response otherwise
@mrydzy
Copy link
Author

mrydzy commented May 18, 2015

I'm not sure if I understood why to create custom URLClassLoader.
Maybe I could simply use JarOutputStream to create some jar file?

@benmccann
Copy link
Contributor

@mrydzy are you still interested in getting this change in? if so, it would be helpful to rebase the PR and add some tests

@gmethvin gmethvin closed this Jun 2, 2016
@gmethvin gmethvin reopened this Jun 3, 2016
@wsargent
Copy link
Member

closing due to lack of activity

@wsargent wsargent closed this Dec 24, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants