Bug 10625 - Inventory/Stocktaking tool cannot handle windows file uploads
Summary: Inventory/Stocktaking tool cannot handle windows file uploads
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Tools (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Mark Tompsett
QA Contact: Jonathan Druart
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-07-22 18:24 UTC by Kyle M Hall
Modified: 2016-12-05 21:23 UTC (History)
9 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Sample file of barcodes for testing (67.49 KB, text/plain)
2013-07-22 18:25 UTC, Kyle M Hall
Details
Bug 10625 - Inventory/Stocktaking tool cannot handle windows file uploads (1.95 KB, patch)
2013-07-22 18:34 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10625 - Inventory/Stocktaking tool cannot handle windows file uploads (2.00 KB, patch)
2013-07-27 23:22 UTC, Mirko Tietgen
Details | Diff | Splinter Review
Bug 10625 - Inventory/Stocktaking tool cannot handle windows file uploads (2.06 KB, patch)
2013-08-19 14:40 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10625: Inventory/Stocktaking tool cannot handle windows file uploads (2.99 KB, patch)
2015-04-15 05:20 UTC, Mark Tompsett
Details | Diff | Splinter Review
[Signed-off] Bug 10625: Inventory/Stocktaking tool cannot handle windows file uploads (2.74 KB, patch)
2015-04-15 06:31 UTC, Marc Véron
Details | Diff | Splinter Review
Three Barcodes and blank lines (\n delimiter) (17 bytes, application/octet-stream)
2015-04-27 19:50 UTC, Mark Tompsett
Details
Three Barcodes and blank lines (\r delimiter) (17 bytes, application/octet-stream)
2015-04-27 19:50 UTC, Mark Tompsett
Details
Three Barcodes and blank lines (\r\n delimiter) (22 bytes, application/octet-stream)
2015-04-27 19:51 UTC, Mark Tompsett
Details
Bug 10625: Inventory/Stocktaking tool cannot handle windows file uploads (2.79 KB, patch)
2015-04-27 20:01 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 10625: Inventory/Stocktaking tool cannot handle windows file uploads (2.94 KB, patch)
2015-04-27 20:05 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 10625: Inventory/Stocktaking tool cannot handle windows file uploads (3.12 KB, patch)
2015-04-30 21:34 UTC, Marc Véron
Details | Diff | Splinter Review
[PASSED QA] Bug 10625: Inventory/Stocktaking tool cannot handle windows file uploads (3.18 KB, patch)
2015-06-07 16:36 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Kyle M Hall 2013-07-22 18:24:31 UTC
The inventory/stocktaking tool interprets windows text editor file as a single large barcode because the text editor only uses carriage returns for line breaks, rather than a carriage return and line break combination. This tools should be able to handle \r, \n, and/or \r\n.
Comment 1 Kyle M Hall 2013-07-22 18:25:16 UTC
Created attachment 19857 [details]
Sample file of barcodes for testing
Comment 2 Kyle M Hall 2013-07-22 18:34:18 UTC Comment hidden (obsolete)
Comment 3 Mirko Tietgen 2013-07-27 23:22:05 UTC Comment hidden (obsolete)
Comment 4 Jonathan Druart 2013-08-19 14:40:30 UTC Comment hidden (obsolete)
Comment 5 Galen Charlton 2013-08-19 17:46:18 UTC
The sample file does not actually use \r\n as its delimiter -- it's using \r.

Can you confirm the source of the sample file you attached to the bug, as does not appear to actually be produced by a Windows text editor -- or in any event, not one that uses the DOS newline.
Comment 6 Jonathan Druart 2013-08-20 07:44:54 UTC
(In reply to Galen Charlton from comment #5)
> The sample file does not actually use \r\n as its delimiter -- it's using \r.
> 
> Can you confirm the source of the sample file you attached to the bug, as
> does not appear to actually be produced by a Windows text editor -- or in
> any event, not one that uses the DOS newline.

Galen, yes you are right, \r\n is under windows and it worked before this patch.
But \r (Mac OS) did not worked before.
The sample file contains \r.
I tested the \r\n too : I added a newline character after \r in order to have a \r\n and it still works with the patch.

Maybe should we change the commit message.
Comment 7 Jonathan Druart 2013-08-20 07:50:22 UTC
With vim, you can use
:set ff=[unix dos mac]
Comment 8 Galen Charlton 2013-08-20 18:11:48 UTC
(In reply to Jonathan Druart from comment #6)
> Galen, yes you are right, \r\n is under windows and it worked before this
> patch.
> But \r (Mac OS) did not worked before.

Ancient versions of Mac OS, not recent ones.  Are there folks actually using MacOS 9 or earlier with Koha?

> The sample file contains \r.
> I tested the \r\n too : I added a newline character after \r in order to
> have a \r\n and it still works with the patch.

DOS/Windows line endings also worked without the patch -- see commit 890f673d64e2.

Since the use of File::Slurp means loading the entire input barcode file into memory, this patch also changes the performance characters of the inventory tool when processing very large files.  Admittedly, this may not be a big deal in practice, but I would appreciate confirmation as to whether there are barcode scanners or other utilities that are *actually* producing files that use \r as a delimiter.
Comment 9 Kyle M Hall 2013-08-21 10:50:35 UTC
> Since the use of File::Slurp means loading the entire input barcode file
> into memory, this patch also changes the performance characters of the
> inventory tool when processing very large files.  Admittedly, this may not
> be a big deal in practice, but I would appreciate confirmation as to whether
> there are barcode scanners or other utilities that are *actually* producing
> files that use \r as a delimiter.

I agree, that using File::Slurp was also a concern of mine, but one would need to upload a barcode file with hundreds of thousands of barcodes to have a plain text file of any appreciable size, right?

I can confirm at least one library is having this issue in practice, as the example file of barcodes was provided by the library having this issue.
Comment 10 Mark Tompsett 2015-04-15 05:20:33 UTC Comment hidden (obsolete)
Comment 11 Mark Tompsett 2015-04-15 05:30:09 UTC
I believe my counter patch solves the problem, and addresses concerns of memory use by File::Slurp by not using it. I'm setting this to needs sign off, but only apply the second patch. I did not obsolete the original one, in case my counter patch is deemed inadequate. Follow the test plan in comment #10 for testing the second patch.
Comment 12 Marc Véron 2015-04-15 06:31:15 UTC Comment hidden (obsolete)
Comment 13 Marc Véron 2015-04-15 06:32:37 UTC
Signed-off on second patch (by M.Tompsett)
Comment 14 Jonathan Druart 2015-04-20 11:06:27 UTC
Comment on attachment 37848 [details] [review]
[Signed-off] Bug 10625: Inventory/Stocktaking tool cannot handle windows file uploads

Review of attachment 37848 [details] [review]:
-----------------------------------------------------------------

::: tools/inventory.pl
@@ +173,5 @@
> +    while (my $barcode=<$uploadbarcodes>) {
> +        $barcode =~ s/\r/\n/g;
> +        $barcode =~ s/\n\n/\n/g;
> +        $barcode =~ s/\n$//;
> +        $barcode =~ s/^\n//;

why not $barcode =~ s/\n//g; to replace the last 3 lines?
Comment 15 Mark Tompsett 2015-04-21 08:31:51 UTC
Comment on attachment 37848 [details] [review]
[Signed-off] Bug 10625: Inventory/Stocktaking tool cannot handle windows file uploads

Review of attachment 37848 [details] [review]:
-----------------------------------------------------------------

::: tools/inventory.pl
@@ +174,5 @@
> +        $barcode =~ s/\r/\n/g;
> +        $barcode =~ s/\n\n/\n/g;
> +        $barcode =~ s/\n$//;
> +        $barcode =~ s/^\n//;
> +        my @data = split(/\n/,$barcode);

Because then I couldn't split the barcodes.
I agree the logic might be more optimizable, but the idea is that if $barcode = "foo\nbar"; it needs to split into two barcodes: foo and bar. And given that some OS's may use \r and \n and \r\n in ways different than how $barcode=<$uploadbarcodes> works, I wanted to be certain that all middle combinations get split, and that there are no beginning or end strangeness.
Comment 16 Jonathan Druart 2015-04-27 08:56:30 UTC
(In reply to M. Tompsett from comment #15)
> Comment on attachment 37848 [details] [review] [review]
> [Signed-off] Bug 10625: Inventory/Stocktaking tool cannot handle windows
> file uploads
> 
> Review of attachment 37848 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: tools/inventory.pl
> @@ +174,5 @@
> > +        $barcode =~ s/\r/\n/g;
> > +        $barcode =~ s/\n\n/\n/g;
> > +        $barcode =~ s/\n$//;
> > +        $barcode =~ s/^\n//;
> > +        my @data = split(/\n/,$barcode);
> 
> Because then I couldn't split the barcodes.
> I agree the logic might be more optimizable, but the idea is that if
> $barcode = "foo\nbar"; it needs to split into two barcodes: foo and bar. And
> given that some OS's may use \r and \n and \r\n in ways different than how
> $barcode=<$uploadbarcodes> works, I wanted to be certain that all middle
> combinations get split, and that there are no beginning or end strangeness.

Ok sorry, did not read it with the context.
So the last 2 replacements are useless :)
Comment 17 Mark Tompsett 2015-04-27 19:50:12 UTC
Created attachment 38572 [details]
Three Barcodes and blank lines (\n delimiter)
Comment 18 Mark Tompsett 2015-04-27 19:50:46 UTC
Created attachment 38573 [details]
Three Barcodes and blank lines (\r delimiter)
Comment 19 Mark Tompsett 2015-04-27 19:51:33 UTC
Created attachment 38574 [details]
Three Barcodes and blank lines (\r\n delimiter)
Comment 20 Mark Tompsett 2015-04-27 20:01:04 UTC Comment hidden (obsolete)
Comment 21 Mark Tompsett 2015-04-27 20:05:59 UTC Comment hidden (obsolete)
Comment 22 Mark Tompsett 2015-04-28 21:41:14 UTC
Comment on attachment 20466 [details] [review]
Bug 10625 - Inventory/Stocktaking tool cannot handle windows file uploads

I think my patch is the way we are going. Obsoleting this. Feel free to unobsolete, as required.
Comment 23 Mark Tompsett 2015-04-30 17:11:05 UTC
Since it is my patch that needs sign off. See comment #21 for a test plan. :)
Comment 24 Marc Véron 2015-04-30 21:34:22 UTC Comment hidden (obsolete)
Comment 25 Katrin Fischer 2015-06-07 16:36:12 UTC
Created attachment 39959 [details] [review]
[PASSED QA] Bug 10625: Inventory/Stocktaking tool cannot handle windows file uploads

The current code uses
    $barcode = <fh>;
logic. This reads until \n, as far as I can tell.
EOL is indicated by \n, \r, and \r\n depending on OS and software.
So, to this end, rather than File::Slurp (which is a potential
memory hog, which is already an issue with no filters), a loop
to pre-read the barcodes was written.

This loop includes:
    $barcode =~ s/\r/\n/g;
    $barcode =~ s/\n\n/\n/g;
    my @data = split(/\n/, $barcode);
    push @uploadedbarcodes,@data;

So, that means that lines ending in \n would have it stripped
and pushed into the uploaded barcodes array.

Lines ending in \r would likely be read as one giant block,
have everything converted to single \n's and then using a split,
the set of barcodes are pushed into the uploaded barcodes array.

Lines ending in \r\n would get that stripped and pushed into the
uploaded barcodes array.

It is then the uploaded barcodes array that is looped over for
validating the barcodes.

TEST PLAN
---------
 1) Back up your database
 2) Download the three sample files (or create your own)
 3) Log in to staff client
 4) Create a branch with no inventory.
 5) Home -> Tools -> Inventory/Stocktaking
 6) Browse for your '\r' test file.
 7) Limit to just that branch
 8) Click 'Submit'
    -- Confirm expected errors
 9) Repeat steps 5-8 with the '\n' test file.
10) Repeat steps 5-8 with the '\r\n' test file.
    -- one of these repetitions should have problems.
11) Apply patch
12) Repeat steps 5-8 for each of the 3 test files.
    -- there should be no issues.
13) run koha qa test tools.

Note: This is a tweak based on Jonathan Druart's comment #16
      I have reset it to needs sign off again.

Followed test plan. Works as expected. qa OK.
Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 26 Tomás Cohen Arazi 2015-06-11 13:17:30 UTC
Patch pushed to master.

Thanks Mark!
Comment 27 Chris Cormack 2015-06-11 20:10:25 UTC
Pushed to 3.20.x will be in 3.20.1
Comment 28 Liz Rea 2015-06-22 23:28:14 UTC
Patch pushed to 3.18.x will be in 3.18.08