-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Fix File handling as a JAX-RS body parameter #35659
Conversation
At the least the first commit should be backported. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
@FroMage can you have a look at this? |
@FroMage ping :) |
@FroMage can you have a look at this please? |
✔️ The latest workflow run for the pull request has completed successfully. It should be safe to merge provided you have a look at the other checks in the summary. |
Ouch, I'm late again. Looking. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM except questions to consider. But don't let that prevent you from merging.
@@ -40,4 +42,8 @@ public Boolean call() { | |||
} | |||
}); | |||
} | |||
|
|||
protected String fileSizeAsStr(File file) throws IOException { | |||
return "" + Files.readAllBytes(file.toPath()).length; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤢
throws IOException, WebApplicationException { | ||
// unfortunately we don't do much here to avoid the file leak | ||
// however this should never be called in a real world scenario | ||
return FileBodyHandler.doRead(httpHeaders, entityStream, Files.createTempFile(PREFIX, SUFFIX).toFile()).toPath(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there really no way to obtain the current context in this case? Feels a bit dangerous and optimistic to assume this never gets called. Perhaps we should at least log an INFO message so that people get something in their logs to explain why their /tmp
folder would keep growing and crash?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We control the calling of the method, so it should indeed never be called
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤞
@@ -21,8 +21,8 @@ | |||
import org.jboss.resteasy.reactive.common.headers.HeaderUtil; | |||
|
|||
public class FileBodyHandler implements MessageBodyReader<File>, MessageBodyWriter<File> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this even used outside of the Quarkus version, which now overrides it? If not, I wonder if it wouldn't be less complex to ditch the reader overrides build items and pass the proper config and cleaner to the non-quarkus RR, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIRC, it was used in some tests in the independent project
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤷
No description provided.