Skip to content
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.

Sign up for GitHub

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

Handle spaces in file names in the import scripts #4189

Open
wants to merge 3 commits into
base: release-10.1
Choose a base branch
from
Open

Handle spaces in file names in the import scripts #4189

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
90cfef1
Handle spaces in file names in the import scripts
dltj Jan 16, 2025
18ea640
Remove debugging
dltj Jan 16, 2025
7b68e85
Improve indentation of bash commands
dltj Jan 17, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 14 additions & 12 deletions harvest/batch-import-marc.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,13 +127,13 @@ else
local LOGFILE
if [ $# -eq 1 ]
then
LOGFILE=$BASEPATH/log/`basename $1`.log
> $LOGFILE
LOGFILE=$BASEPATH/log/`basename "$1"`.log
> "$LOGFILE"
else
LOGFILE=$BASEPATH/log/`basename $1`_and_more.log
LOGFILE=$BASEPATH/log/`basename "$1"`_and_more.log
echo -e "This log is for the following files: \n$FILES\n" > $LOGFILE
fi
cat -u - >> $LOGFILE
cat -u - >> "$LOGFILE"
}
fi

Expand All @@ -143,12 +143,14 @@ find -L $BASEPATH -maxdepth 1 \( -iname "*.xml" -o -iname "*.mrc" -o -iname "*.m
do
# Logging output handled by log() function
# PROPERTIES_FILE passed via environment
$VUFIND_HOME/import-marc.sh $files 2> >(log $files)
if [ "$?" -eq "0" ] && [ $MOVE_DATA == true ]
then
for file in $files
do
mv $file $BASEPATH/processed/`basename $file`
done
fi
$VUFIND_HOME/import-marc.sh "$files" 2> >(log "$files")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to have gotten indented an extra two spaces for some reason...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, since this script is largely the work of others and my bash expertise is limited, I may just be confused... but I thought the purpose of some of this code was to pass multiple matching files to the import script at once in a space-delimited list. How do we get around confusion between space-delimited files and files containing spaces? Is the find command smart enough to quote things appropriately for us? Do you need me to try to set up any edge case test scenarios to confirm proper functionality, or have you already done so?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes are mostly dealing with the output of the find command for filenames that have spaces in them. If one puts a filename with spaces (properly escaped) on the command line itself, it works okay. The -print0 option from the find command separates file entries by null characters...that was already there. The rest of the changes deal with handling that correctly. The script had some notable pitfalls when handling filenames with spaces...notably the use of the for control flow command. Needing to double-quote variable names that deal with values with spaces is also a notable pitfall.

This is working in production for me now. I'm not sure how we'd set up a test with 'bash' and hook it up to the PHP test harness.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we wanted an automated test of this, we could create a directory containing MARC records with unusual filenames, and then run a batch import of this directory as part of the build.xml MARC indexing process. Then we would need a Mink test to confirm that the records actually exist in the index. A bit roundabout and complicated, and very possibly not worth the effort, but that's how I would approach it if I had to. (I'm perfectly content to just do some manual testing of my own when time permits, unless you're eager to automate something in the meantime).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's go the manual testing route, and I'll post a message to Slack to get others to look at this. Normally I'd be up for learning new tools, but it is feeling like such a crunch time at the moment.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I tried some manual testing, and nothing seems to be working for me... but maybe I'm operating on incorrect assumptions. Here's what I did:

1.) I created two test directories under $VUFIND_LOCAL_DIR/harvest: $VUFIND_LOCAL_DIR/harvest/marc and $VUFIND_LOCAL_DIR/harvest/marc records, so I could run tests with or without a space in the harvest directory name.

2.) I copied the same two sample records from the test data into both of these directories. I copied testbug2.mrc as is (for a "normal" filename example) and I copied journals.mrc as journal data.mrc to introduce a filename containing a space.

Here's what happens when I try to run the batch import on these two directories:

"marc" example:

dkatz@dkatz-VirtualBox:/usr/local/vufind$ harvest/batch-import-marc.sh marc
Now Importing: /testbug2.mrc
DEBUG [main] (Boot.java:713) - Adding jars files in directory: /usr/local/vufind/import/lib_local
DEBUG [main] (Boot.java:713) - Adding jars files in directory: /usr/local/vufind/solr/vendor/modules/analysis-extras/lib
 INFO [main] (ValueIndexerFactory.java:119) - Using directory: /usr/local/vufind/import/index_java as location of java sources
 INFO [main] (PropertyUtils.java:337) - Opening file (instead of 1 other options): /usr/local/vufind/local/import/import.properties
DEBUG [main] (SolrCoreLoader.java:48) - Pinging Solr at URL:  http://localhost:8983/solr/biblio/admin/ping
DEBUG [main] (SolrCoreLoader.java:65) -     ...
DEBUG [main] (SolrCoreLoader.java:54) -       "status":"OK"
DEBUG [main] (SolrCoreLoader.java:96) - Found Solrj class org.apache.solr.client.solrj.impl.HttpSolrClient$Builder
 INFO [main] (IndexDriver.java:166) - Reading and compiling index specifications: marc.properties, marc_local.properties
 INFO [main] (IndexDriver.java:257) - Opening index spec file: /usr/local/vufind/import/marc.properties
 INFO [main] (IndexDriver.java:257) - Opening index spec file: /usr/local/vufind/import/marc_local.properties
 INFO [main] (IndexDriver.java:100) - Opening input files: [/testbug2.mrc]
FATAL [main] (Boot.java:240) - ERROR: Error while invoking main method in specified class: org.solrmarc.driver.IndexDriver
java.lang.IllegalArgumentException: /testbug2.mrc (No such file or directory)
	at org.solrmarc.driver.IndexDriver.configureReader(IndexDriver.java:235)
	at org.solrmarc.driver.IndexDriver.execute(IndexDriver.java:101)
	at org.solrmarc.driver.IndexDriver.main(IndexDriver.java:72)
	at java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:103)
	at java.base/java.lang.reflect.Method.invoke(Method.java:580)
	at org.solrmarc.driver.Boot.invokeMain(Boot.java:229)
	at org.solrmarc.driver.ConfigDriver.main(ConfigDriver.java:136)
	at java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:103)
	at java.base/java.lang.reflect.Method.invoke(Method.java:580)
	at org.solrmarc.driver.Boot.invokeMain(Boot.java:229)
	at org.solrmarc.driver.Boot.main(Boot.java:79)
Caused by: java.io.FileNotFoundException: /testbug2.mrc (No such file or directory)
	at java.base/java.io.FileInputStream.open0(Native Method)
	at java.base/java.io.FileInputStream.open(FileInputStream.java:213)
	at java.base/java.io.FileInputStream.<init>(FileInputStream.java:152)
	at java.base/java.io.FileInputStream.<init>(FileInputStream.java:106)
	at org.marc4j.MarcReaderFactory.makeReader(MarcReaderFactory.java:65)
	at org.marc4j.MarcReaderFactory.makeReader(MarcReaderFactory.java:43)
	at org.solrmarc.driver.IndexDriver.configureReader(IndexDriver.java:231)
	... 10 more

"marc records" example:

dkatz@dkatz-VirtualBox:/usr/local/vufind$ harvest/batch-import-marc.sh "marc records"
harvest/batch-import-marc.sh: line 94: [: /usr/local/vufind/local/harvest/marc: binary operator expected
harvest/batch-import-marc.sh: line 103: [: /usr/local/vufind/local/harvest/marc: binary operator expected
harvest/batch-import-marc.sh: line 110: [: /usr/local/vufind/local/harvest/marc: binary operator expected
find: ‘records’: No such file or directory
harvest/batch-import-marc.sh: line 131: /usr/local/vufind/local/harvest/marc records/log/testbug2.mrc.log: No such file or directory
harvest/batch-import-marc.sh: line 136: /usr/local/vufind/local/harvest/marc records/log/testbug2.mrc.log: No such file or directory
Now Importing: /testbug2.mrc

Are you seeing different results?

Also, I notice that you have targeted the release-10.1 branch here; if you feel strongly that this is a bug fix, I'm not entirely opposed to going forward on that path... but since the changes are significant, maybe it would be more conservative to target this against dev for inclusion in release 11.0.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I hadn't considered a space in the directory name, only the file name of the records to be loaded. To be honest, I hadn't looked at the handling of spaces in the directory name...there are probably pitfalls there, too. Does it change your test results if you use a directory with no spaces in it?

I'm happy to target this against dev for inclusion in the next release. I could go either way.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I'm still seeing the same behavior as noted above for the marc directory containing testbug2.mrc and journal data.mrc. Looks to me from the error message like the path to the directory containing the file is somehow getting lost and the code is trying to find it in the root directory.


if [ "$?" -eq 0 ] && [ "$MOVE_DATA" == true ]; then
# Convert the space-separated file string into an array
IFS= read -r -a file_array <<< "$files"

for file in "${file_array[@]}"; do
mv "$file" "$BASEPATH/processed/$(basename "$file")"
done
fi
done
41 changes: 29 additions & 12 deletions import-marc.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,12 +130,14 @@ fi
#####################################################
# Normalize file paths to absolute paths
#####################################################
NORMALIZED_PATHS=""
for f in $*; do
MARC_PATH=`dirname $f`
MARC_PATH=`cd $MARC_PATH && pwd`
MARC_FILE=`basename $f`
NORMALIZED_PATHS="${NORMALIZED_PATHS} $MARC_PATH/$MARC_FILE"
NORMALIZED_PATHS=()

for f in "$@"; do
MARC_PATH=$(dirname "$f")
MARC_PATH=$(cd "$MARC_PATH" && pwd) # Resolve the full path to prevent relative path issues
MARC_FILE=$(basename "$f")
# Add the full path to the array
NORMALIZED_PATHS+=("$MARC_PATH/$MARC_FILE")
done

#####################################################
Expand All @@ -153,16 +155,31 @@ then
mkdir -p $SOLRJ_DIR
for file in $VUFIND_HOME/solr/vendor/server/solr-webapp/webapp/WEB-INF/lib/solr*.jar $VUFIND_HOME/solr/vendor/server/solr-webapp/webapp/WEB-INF/lib/http*.jar
do
ln -s $file $SOLRJ_DIR/`basename $file`
ln -s $file $SOLRJ_DIR/`basename "$file"`
done
fi

#####################################################
# Execute Importer
#####################################################

RUN_CMD="$JAVA $INDEX_OPTIONS -Duser.timezone=UTC -Dlog4j.configuration=file://$LOG4J_CONFIG $EXTRA_SOLRMARC_SETTINGS -jar $JAR_FILE $PROPERTIES_FILE -solrj $SOLRJ_DIR -lib_local "$VUFIND_HOME/import/lib_local\;$VUFIND_HOME/solr/vendor/modules/analysis-extras/lib" $NORMALIZED_PATHS"
echo "Now Importing $NORMALIZED_PATHS ..."
# solrmarc writes log messages to stderr, write RUN_CMD to the same place
echo "`date '+%h %d, %H:%M:%S'` $RUN_CMD" >&2
exec $RUN_CMD
# Build the command as an array
RUN_CMD=(
"$JAVA"
$INDEX_OPTIONS
-Duser.timezone=UTC
-Dlog4j.configuration="file://$LOG4J_CONFIG"
$EXTRA_SOLRMARC_SETTINGS
-jar "$JAR_FILE"
"$PROPERTIES_FILE"
-solrj "$SOLRJ_DIR"
-lib_local "$VUFIND_HOME/import/lib_local;$VUFIND_HOME/solr/vendor/modules/analysis-extras/lib"
"${NORMALIZED_PATHS[@]}"
)

# Debugging output
echo "Now Importing: ${NORMALIZED_PATHS[*]}"
echo "$(date '+%h %d, %H:%M:%S') ${RUN_CMD[*]}" >&2

# Execute the command using the array
exec "${RUN_CMD[@]}"
dltj marked this conversation as resolved.
Show resolved Hide resolved
Loading