ETT-405 pdf download fixes#211
Conversation
| my $fh; | ||
|
|
||
| if ( ! $self->output_filename || SRV::Utils::under_server() ) { | ||
| if ( ! $self->output_filename || SRV::Utils::under_server() || $ENV{PSGI_COMMAND} ) { |
There was a problem hiding this comment.
@eumalin I'd like to check this. I don't see the change specifically mentioned in the ticket history and I'd be hesitant to rely on this, since it potentially affects the handling of all PDF downloads, not just ku01. RV::Utils::under_server() returns ! defined $ENV{PSGI_COMMAND} so the second and third OR clauses essentially cancel. I would like to get rid of this and see if ku01 download still works.
There was a problem hiding this comment.
This doesn't seem to work with the final OR clause removed. The output PDF file is empty, so I don't know if there is an error in the Java code, or if it is being provided a bad parameter. I have the contents of the temp config file for the Stamper being dumped into /htapps/moseshll.babel/cache/imgsrv/debug_out.txt so maybe the next step would be to see if the Java command can be run successfully with the given parameters. Unfortunately some of the files are ephemeral, in particular the source PDF which is in /ram
There was a problem hiding this comment.
Without this change, I get zero-byte pdf downloads; adding the condition fixes it. Seems to work ok for both KU and non KU pdf downloads.
I believe the empty PDF is not a Java error. It's a file overwrite. Java runs fine and writes the correct output. The problem is what happens after: Plack::App::Command always renames {cache_file}.download over {cache_file} at the end of the background process. Without the stream, Process::PDF writes Java's output directly to the cache path, then the rename overwrites it with the empty .download file.
The stream fix routes Java's output through the PSGI writer into .download first, so the rename produces the correct file. The reason _fill_params() sets output_filename to a string before _generate_coderef() runs is what breaks the original condition, ! $self->output_filename is already false by the time the check happens.
There was a problem hiding this comment.
Agreed the logic is awkward. I could replace the whole condition with an unconditional stream creation that forces streaming in the PSGI case and a comment explaining why, to avoid the confusion.
There was a problem hiding this comment.
I'm guessing it would also be awkward to change SRV::Volume::PDF behavior based on the Process subclass it is using. Ugh.
There was a problem hiding this comment.
Yeah, but I don't think it's tied to the Process subclass. The streaming is now unconditional for all PDF downloads, KU or not.
I've simplified the fix: the condition is gone entirely and replaced with a comment. The original guard (! $self->output_filename) was meant to skip stream creation if a path was already set, but _fill_params() always sets output_filename to a string before _generate_coderef() runs, so the guard was never true in background mode anyway.
Also bumped the stamper to 1.1 (was 1.0-SNAPSHOT), updated stamp_pdf.pl and both pom files. Tests pass, but we need more coverage - it makes sense to me to do this as part of a follow up that will address the remaining issues.
2 participants
A follow up to #120, this is a bunch of short-term fixes to address the issue with downloading KU pdf files. The longer term fix will be to remove the jar from git and establish a java build workflow that follows the best practices.