degroff/0.3.5.maintenance file manager #35
Conversation
| this.temporaryFilenameSuffix = other.temporaryFilenameSuffix; | ||
| } | ||
|
|
||
| public boolean deleteTemporaryFiles() { |
There was a problem hiding this comment.
This seems to break the pattern below of everything being a getter.
There was a problem hiding this comment.
yeah, good point. I could rename to isDeleteTemporaryFiles or getDeleteTemporaryFiles to make it more consistent.
| throw new ContentTooLarge(maximumRequestSize, detailedMessage); | ||
| } | ||
|
|
||
| end += read; |
There was a problem hiding this comment.
Is this correct? The condition end == -1 can only happen if the first read is empty right? That would make this like be 0 += -1 and then the if-statement would return false on the next line?
Just want to make sure this block of code will eventually complete even in edge cases.
There was a problem hiding this comment.
I think this is correct based upon how end is set above. If I condition this with read > 0 the tests hang.
There is a bug in this code though that I see now - I was not looking for read > 0 before calculating bytesRead.
| new MultipartStream(inputStream, boundary, getMultipartFileManager(), multipartConfiguration).process(parameters, files); | ||
| } | ||
|
|
||
| public void setMultipartConfiguration(MultipartConfiguration multipartConfiguration) { |
There was a problem hiding this comment.
I'm not a fan of the pattern where this is always clobbered. Why not back the variable here final and then copy things over using the with methods? Or add a copyFrom method to the MultipartConfiguration object?
| this.request = new HTTPRequest(configuration.getContextPath(), listener.isTLS() ? "https" : "http", listener.getPort(), ipAddress); | ||
|
|
||
| // Create a deep copy of the MultipartConfiguration so that the request may optionally modify the configuration on a per-request basis. | ||
| this.request.getMultiPartStreamProcessor().setMultipartConfiguration(new MultipartConfiguration(configuration.getMultipartConfiguration())); |
There was a problem hiding this comment.
See my comment in the Configuration object about clobbering objects.
| @@ -157,7 +169,7 @@ public ProcessorState read(ByteBuffer buffer) throws IOException { | |||
| // If the next state is not preamble, that means we are done processing that and ready to handle the request in a separate thread | |||
| if (requestState != RequestState.Preamble && requestState != RequestState.Expect) { | |||
| logger.trace("(RWo)"); | |||
| future = threadPool.submit(new HTTPWorker(configuration.getHandler(), configuration.getLoggerFactory(), this, request, response)); | |||
| future = threadPool.submit(new HTTPWorker(configuration, configuration.getHandler(), configuration.getLoggerFactory(), this, request, response)); | |||
There was a problem hiding this comment.
If you are going to pass in the configuration, why not remove the other objects and just pull them from the configuration in the constructor?
There was a problem hiding this comment.
Yes, I considered that, happy to make that change.
| public void post_server_configuration_file_upload_disabled(String scheme) throws Exception { | ||
| // File uploads disabled | ||
| withScheme(scheme) |
There was a problem hiding this comment.
Is this correct? If files are disabled, should we be returning an error or should we just ignore them and still handle the rest of the request? I can go either way, but seems like you were planning on looking at the action in Prime and disabling files if the action didn't need them, which would blow up if the request has files, which we could just ignore.
There was a problem hiding this comment.
I guess it just depends on how we wish to handle. I'd rather not ignore them because I assume if a multipart request contains both files and form data they are all required to be complete. But open to other ideas.
Summary
Add in additional control for multipart streams.
Reviewer notes
This is on a maintenance branch. The plan will be to either merge this change into
mainor possibly just rebuild something very similar onmaindepending upon how much of this translates.