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.
Created attachment 19857 [details] Sample file of barcodes for testing
Created attachment 19858 [details] [review] Bug 10625 - Inventory/Stocktaking tool cannot handle windows file uploads 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. Test Plan: 1) Download the attached sample file of barcodes 2) Upload this file to the inventory/stocktaking tool. For this test, it doesn't matter that these barcodes aren't cataloged in your system. 3) Note that the "barcode not found" is a single extremely long barcode 4) Apply this patch 5) Repeat step 2 6) Note that you now get a "barcode not found" error for each barcode individually.
Created attachment 19970 [details] [review] Bug 10625 - Inventory/Stocktaking tool cannot handle windows file uploads 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. Test Plan: 1) Download the attached sample file of barcodes 2) Upload this file to the inventory/stocktaking tool. For this test, it doesn't matter that these barcodes aren't cataloged in your system. 3) Note that the "barcode not found" is a single extremely long barcode 4) Apply this patch 5) Repeat step 2 6) Note that you now get a "barcode not found" error for each barcode individually. Signed-off-by: Mirko Tietgen <mirko@abunchofthings.net>
Created attachment 20466 [details] [review] Bug 10625 - Inventory/Stocktaking tool cannot handle windows file uploads 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. Test Plan: 1) Download the attached sample file of barcodes 2) Upload this file to the inventory/stocktaking tool. For this test, it doesn't matter that these barcodes aren't cataloged in your system. 3) Note that the "barcode not found" is a single extremely long barcode 4) Apply this patch 5) Repeat step 2 6) Note that you now get a "barcode not found" error for each barcode individually. Signed-off-by: Mirko Tietgen <mirko@abunchofthings.net> Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
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.
(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.
With vim, you can use :set ff=[unix dos mac]
(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.
> 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.
Created attachment 37846 [details] [review] 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; $barcode =~ s/\n$//; $barcode =~ s/^\n//; 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) Create a file with at least three different barcodes. --- BEGIN EXAMPLE --- BARCODE1\n BARCODE2\r BARCODE3\r\n --- END EXAMPLE --- $ echo "BARCODE1\nBARCODE2\rBARCODE3\r\n" | sed -e "s#\\\\n#\\n#g" | sed -e "s#\\\\r#\\r#g" > test.txt 2) Log into staff client 3) Tools -> Inventory/stocktaking 4) Click 'Browse' and select the file to upload (e.g. test.txt) 5) Click 'Submit' 6) A Javascript warning comes up, click 'Yes' -- This will take a LONG time, at least 1 minute. -- eventual results will be wrong. 7) Apply patch 8) repeat steps 3-6 -- This time correct number of barcodes will be given. 9) run koha qa test tools
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.
Created attachment 37848 [details] [review] [Signed-off] 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; $barcode =~ s/\n$//; $barcode =~ s/^\n//; 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) Create a file with at least three different barcodes. --- I tested with the test file provided and with a file where some lines have CR and other lines have CR NL (inserted with Win text editor). Worked as expected. Signed-off-by: Marc Véron <veron@veron.ch>
Signed-off on second patch (by M.Tompsett)
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 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.
(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 :)
Created attachment 38572 [details] Three Barcodes and blank lines (\n delimiter)
Created attachment 38573 [details] Three Barcodes and blank lines (\r delimiter)
Created attachment 38574 [details] Three Barcodes and blank lines (\r\n delimiter)
Created attachment 38575 [details] [review] 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.
Created attachment 38576 [details] [review] 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.
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.
Since it is my patch that needs sign off. See comment #21 for a test plan. :)
Created attachment 38730 [details] [review] 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>
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>
Patch pushed to master. Thanks Mark!
Pushed to 3.20.x will be in 3.20.1
Patch pushed to 3.18.x will be in 3.18.08