Changeset 21715

Timestamp:

2010-02-04T13:04:56+13:00 (14 years ago)

Author:

mdewsnip

Message:

Adding some comments and error checking. By Jeffrey Ke.

Location:

main/trunk/greenstone2/perllib/cgiactions

Files:

• baseaction.pm (modified) (1 diff)
• metadataaction.pm (modified) (51 diffs)

main/trunk/greenstone2/perllib/cgiactions/baseaction.pm

@@ -370 +370 @@
 
 
+# Release the gli.lck otherwise no one else will be able to use the collection again.
+sub unlock_collection
+{
+    my $self = shift @_;
+    my ($username, $collection) = @_;
+    my $gsdl_cgi = $self->{'gsdl_cgi'};
+
+    # Obtain the path to the collection GLI lock file
+    my $lock_file_path = &util::filename_cat($ENV{'GSDLHOME'}, "collect", $collection, "gli.lck");
+
+    # If the lock file does exist, check if it is ours
+    if (-e $lock_file_path)
+    {
+    my $lock_file_content = "";
+    open(LOCK_FILE, "<$lock_file_path");
+    while (<LOCK_FILE>) {
+        $lock_file_content .= $_;
+    }
+    close(LOCK_FILE);
+
+    # Pick out the owner of the lock file
+    $lock_file_content =~ /\<User\>(.*?)\<\/User\>/;
+    my $lock_file_owner = $1;
+
+    # If we are the owner of this lock, we have the right to delete it
+    if ($lock_file_owner eq $username) {
+            unlink($lock_file_path );
+    }
+        else {
+        $gsdl_cgi->generate_error("Collection is locked by: $lock_file_owner. Cannot be unlocked");
+        }
+    }
+}
+
+
 sub send_mail
 {

main/trunk/greenstone2/perllib/cgiactions/metadataaction.pm

@@ -43 +43 @@
 
 
-
 my $action_table =
 { 
@@ -88 +87 @@
 
 
-
 sub get_live_metadata
 {
@@ -98 +96 @@
     my $gsdlhome  = $self->{'gsdlhome'};
 
+    # Note: Not sure why get_live_metadata doesn't need the authentication check
+
+    # Obtain the collect dir
     my $collect_dir = &util::filename_cat($gsdlhome, "collect");
-#    $gsdl_cgi->checked_chdir($collect_dir);
-
 
     # Make sure the collection isn't locked by someone else
@@ -106 +105 @@
 
     # look up additional args
-
     my $docid  = $self->{'d'};
     if ((!defined $docid) || ($docid =~ m/^\s*$/)) {
-    $gsdl_cgi->generate_error("No docid (d=...) specified.");
-    }
-
+       $gsdl_cgi->generate_error("No docid (d=...) specified.");
+    }
+
+    # Generate the dbkey
     my $metaname  = $self->{'metaname'};
-
     my $dbkey = "$docid.$metaname";
 
+    # To people who know $collect_tail please add some comments
+    # Obtain path to the database
     my $collect_tail = $collect;
     $collect_tail =~ s/^.*[\/|\\]//;
-
     my $index_text_directory = &util::filename_cat($collect_dir,$collect,"index","text");
     my $infodb_file_path = &dbutil::get_infodb_file_path("gdbm", "live-$collect_tail", $index_text_directory);
-
+    
+    # Obtain the content of the key
     my $cmd = "gdbmget $infodb_file_path $dbkey";
-
     if (open(GIN,"$cmd |") == 0) {
+        # Catch error if gdbmget failed
     my $mess = "Failed to get metadata key: $metaname\n";
     $mess .= "$!\n";
@@ -130 +130 @@
     $gsdl_cgi->generate_error($mess);
     }
-
     else {
+        # Read everything in and concatenate them into $metavalue
     my $metavalue = "";
-
     my $line;
     while (defined ($line=<GIN>)) {
@@ -139 +138 @@
     }
     close(GIN);
-
-    chomp($metavalue);
-
+    chomp($metavalue); # Get rid off the tailing newlines
     $gsdl_cgi->generate_ok_message("$metavalue");
     }
-}
-
+
+    # Release the lock once it is done
+    $self->unlock_collection($username, $collect);
+}
 
 
@@ -157 +156 @@
     my $gsdlhome  = $self->{'gsdlhome'};
 
-
+    # Authenticate user if it is enabled
     if ($baseaction::authentication_enabled) {
     # Ensure the user is allowed to edit this collection
@@ -163 +162 @@
     }
 
+    # Obtain the collect dir
     my $collect_dir = &util::filename_cat($gsdlhome, "collect");
-#    $gsdl_cgi->checked_chdir($collect_dir);
-
 
     # Make sure the collection isn't locked by someone else
@@ -171 +169 @@
 
     # look up additional args
-
     my $docid     = $self->{'d'};
     my $metaname  = $self->{'metaname'};
     my $metapos   = $self->{'metapos'};
 
+    # To people who know $collect_tail please add some comments
+    # Obtain path to the database
     my $collect_tail = $collect;
     $collect_tail =~ s/^.*[\/\\]//;
-
     my $index_text_directory = &util::filename_cat($collect_dir,$collect,"index","text");
     my $infodb_file_path = &dbutil::get_infodb_file_path("gdbm", $collect_tail, $index_text_directory);
+
+    # Read the docid entry
     my $doc_rec_string = &dbutil::read_infodb_entry("gdbm", $infodb_file_path, $docid);
     my $doc_rec = &dbutil::convert_infodb_string_to_hash($doc_rec_string);
+
+    # Basically loop through and unescape_html the values
     foreach my $k (keys %$doc_rec) {
     my @escaped_v = ();
@@ -189 +191 @@
         push(@escaped_v, $ev);
     }
-
     $doc_rec->{$k} = \@escaped_v;
     }
 
+    # Obtain the specified metadata value
     $metapos = 0 if (!defined $metapos);
-
     my $metavalue = $doc_rec->{$metaname}->[$metapos];
-
     $gsdl_cgi->generate_ok_message("$metavalue");
-
-}
-
-
+    
+    # Release the lock once it is done
+    $self->unlock_collection($username, $collect);
+}
 
 
@@ -213 +213 @@
     my $gsdlhome  = $self->{'gsdlhome'};
 
-
     # don't user authenticate for now
     if ($baseaction::authentication_enabled) {
@@ -220 +219 @@
     }
 
+    # Obtain the collect dir
     my $collect_dir = &util::filename_cat($gsdlhome, "collect");
-#    $gsdl_cgi->checked_chdir($collect_dir);
-
 
     # Make sure the collection isn't locked by someone else
@@ -228 +226 @@
 
     # look up additional args
-
     my $docid     = $self->{'d'};
+    if ((!defined $docid) || ($docid =~ m/^\s*$/)) {
+      $gsdl_cgi->generate_error("No docid (d=...) specified.");
+    }
+    my $metavalue = $self->{'metavalue'};
+
+    # Generate the dbkey    
     my $metaname  = $self->{'metaname'};
-    my $metavalue = $self->{'metavalue'};
-
     my $dbkey = "$docid.$metaname";
 
+    # To people who know $collect_tail please add some comments
+    # Obtain path to the database
     my $collect_tail = $collect;
     $collect_tail =~ s/^.*[\/\\]//;
-
     my $index_text_directory = &util::filename_cat($collect_dir,$collect,"index","text");
     my $infodb_file_path = &dbutil::get_infodb_file_path("gdbm", "live-$collect_tail", $index_text_directory);
 
+    # Set the new value
     my $cmd = "gdbmset \"$infodb_file_path\" \"$dbkey\" \"$metavalue\"";
-
     my $status = system($cmd);
-
     if ($status != 0) {
+        # Catch error if gdbmget failed
     my $mess = "Failed to set metadata key: $dbkey\n";
-    
+
     $mess .= "PATH: $ENV{'PATH'}\n";
     $mess .= "cmd = $cmd\n";
@@ -258 +260 @@
     $gsdl_cgi->generate_ok_message("set-live-metadata successful: Key[$metaname]=$metavalue");
     }
-
+    
+    # Release the lock once it is done
+    $self->unlock_collection($username, $collect);
 }
 
@@ -271 +275 @@
     my $gsdlhome  = $self->{'gsdlhome'};
 
-
     # don't user authenticate for now
     if ($baseaction::authentication_enabled) {
@@ -278 +281 @@
     }
 
+    # Obtain the collect dir
     my $collect_dir = &util::filename_cat($gsdlhome, "collect");
-#    $gsdl_cgi->checked_chdir($collect_dir);
-
 
     # Make sure the collection isn't locked by someone else
@@ -286 +288 @@
 
     # look up additional args
-
     my $docid     = $self->{'d'};
     my $metaname  = $self->{'metaname'};
@@ -292 +293 @@
     my $metavalue = $self->{'metavalue'};
 
+    # To people who know $collect_tail please add some comments
+    # Obtain path to the database
     my $collect_tail = $collect;
     $collect_tail =~ s/^.*[\/\\]//;
-
     my $index_text_directory = &util::filename_cat($collect_dir,$collect,"index","text");
     my $infodb_file_path = &dbutil::get_infodb_file_path("gdbm", $collect_tail, $index_text_directory);
+
+    # Read the docid entry
     my $doc_rec_string = &dbutil::read_infodb_entry("gdbm", $infodb_file_path, $docid);
     my $doc_rec = &dbutil::convert_infodb_string_to_hash($doc_rec_string);
@@ -313 +317 @@
         }
     }
-
     $doc_rec->{$k} = \@escaped_v;
     }
-
     ## print STDERR "**** metavalue = $metavalue\n";
 
+    # Protect the quotes
     $metavalue =~ s/\"/\\\"/g;
 
+    # Set the metadata value
     if (defined $metapos) {
     $doc_rec->{$metaname}->[$metapos] = $metavalue;
@@ -327 +331 @@
     $doc_rec->{$metaname} = [ $metavalue ];
     }
-
     ## print STDERR "**** metavalue = $metavalue\n";
 
+    # Generate the record string
     my $serialized_doc_rec = &dbutil::convert_infodb_hash_to_string($doc_rec);
-
     ## print STDERR "**** ser dr\n$serialized_doc_rec\n\n\n";
 
+    # Store it into GDBM
     my $cmd = "gdbmset \"$infodb_file_path\" \"$docid\" \"$serialized_doc_rec\"";
-
     my $status = system($cmd);
-
     if ($status != 0) {
+        # Catch error if gdbmget failed
     my $mess = "Failed to set metadata key: $docid\n";
     
@@ -356 +359 @@
     $gsdl_cgi->generate_ok_message($mess);
     }
-
-}
-
+    
+    # Release the lock once it is done
+    $self->unlock_collection($username, $collect);
+}
 
 
 sub dxml_metadata
 {
-
     my ($tagname, $attrHash, $contextArray, $parentDataArray, $parser) = @_;
-
-
     my $metaname = $parser->{'parameters'}->{'metaname'};
     my $metamode = $parser->{'parameters'}->{'metamode'};
 
+    # Find the right metadata tag and checks if we are going to override it
+    # Note: This over writes the first metadata block it encountered. If there are multiple Sections in the doc.xml, it might not behave as you would expect
     my $name_attr = $attrHash->{'name'};
-
     if (($name_attr eq $metaname) && ($metamode eq "override")) {
-
+        # Get the value and override the current value
     my $metavalue = $parser->{'parameters'}->{'metavalue'};
-
     $attrHash->{'_content'} = $metavalue;
 
     # Don't want it to wipe out any other pieces of metadata
     $parser->{'parameters'}->{'metamode'} = "done";
-
     }
 
     # raw extended
+    # Someone please write some comments on why adding ':'.$tagname => $attrHash
     return (':'.$tagname => $attrHash, [$tagname => $attrHash]);
 }
@@ -390 +391 @@
 sub dxml_description
 {
-
     my ($tagname, $attrHash, $contextArray, $parentDataArray, $parser) = @_;
-
     my $metamode = $parser->{'parameters'}->{'metamode'};
 
-
+    # Accumulate the metadata
+    # NOTE: This appends new metadata element to all description fields.
+    # If there are multiple Sections/SubSections, the new metadata block will get appended to all of them
     if ($metamode eq "accumulate") {
     # tack a new metadata tag on to the end of the <Metadata>+ block
-
     my $metaname = $parser->{'parameters'}->{'metaname'};
     my $metavalue = $parser->{'parameters'}->{'metavalue'};
     
-
     my $metadata_attr = { '_content' => $metavalue, 
                   'name'     => $metaname, 
@@ -408 +407 @@
 
     my $append_metadata = [ "Metadata" => $metadata_attr ];
-
     my $description_content = $attrHash->{'_content'};
 
     push(@$description_content,"    ", $append_metadata ,"\n        ");
-
     }
 
@@ -419 +416 @@
 }
 
+
 sub edit_xml_file
 {
     my $self = shift @_;
-
     my ($gsdl_cgi, $filename, $rules, $options) = @_;
 
     # use XML::Rules to add it in (read in and out again)
-
     my $parser = XML::Rules->new(rules => $rules, 
                  style => 'filter' );
@@ -435 +431 @@
     }
     else {
+        # Read all the text in
     my $line;
     while (defined ($line=<MIN>)) {
@@ -441 +438 @@
     close(MIN);
     
-    
+    # Matched lines will get handled by the call backs
     my $xml_out = "";
     $parser->filter($xml_in,\$xml_out, $options);
@@ -450 +447 @@
     else {
         print MOUT $xml_out;
-        close(MOUT);
-        
-    }
-    }
-}
-
+        close(MOUT);        
+    }
+    }
+}
 
 
@@ -461 +456 @@
 {
     my $self = shift @_;
-
     my ($gsdl_cgi, $doc_xml_filename, $metaname, $metavalue, $metapos) = @_;
 
     # use XML::Rules to add it in (read in and out again)
-
+    # Set the call back functions
     my @rules = 
     ( _default => 'raw extended',
@@ -471 +465 @@
       'Description' => \&dxml_description );
       
+    # Sets the parameters
     my $options = { 'metaname'  => $metaname,
             'metapos'   => $metapos,
             'metavalue' => $metavalue };
-
     $self->edit_xml_file($gsdl_cgi,$doc_xml_filename,\@rules,$options);
 }
@@ -488 +482 @@
     my $gsdlhome  = $self->{'gsdlhome'};
 
-
     # don't user authenticate for now
     if ($baseaction::authentication_enabled) {
@@ -495 +488 @@
     }
 
+    # Obtain the collect and archive dir    
     my $collect_dir = &util::filename_cat($gsdlhome, "collect");
-#    $gsdl_cgi->checked_chdir($collect_dir);
-
     my $archive_dir = &util::filename_cat($collect_dir,$collect,"archives");
 
@@ -504 +496 @@
 
     # look up additional args
-
     my $docid  = $self->{'d'};
-
     my $metaname   = $self->{'metaname'};
     my $metavalue  = $self->{'metavalue'};
     my $metapos    = $self->{'metapos'};
-
     $metapos = 0 if (!defined $metapos);
-
+    
+    # Obtain the doc.xml path for the specified docID
     my $arcinfo_doc_filename = &dbutil::get_infodb_file_path("gdbm", "archiveinf-doc", $archive_dir);
-
     my $doc_rec_string = &dbutil::read_infodb_entry("gdbm", $arcinfo_doc_filename, $docid);
-    my $doc_rec = &dbutil::convert_infodb_string_to_hash($doc_rec_string);
-
+    my $doc_rec = &dbutil::convert_infodb_string_to_hash($doc_rec_string);    
     my $doc_xml_file = $doc_rec->{'doc-file'}->[0];
-
-    
-    my $archives_dir = &util::filename_cat($collect_dir,$collect,"archives");
-    
+    
+    # The $doc_xml_file is relative to the archives, and now let's get the full path
+    my $archives_dir = &util::filename_cat($collect_dir,$collect,"archives");    
     my $doc_xml_filename = &util::filename_cat($archives_dir,$doc_xml_file);
-
-    
+    
+    # Edit the doc.xml file with the specified metadata name, value and position.
+    # TODO: there is a potential problem here as this edit_doc_xml function 
+    # is assuming the simple doc.xml situation where there is only one Section and no SubSections.
+    # Running import.pl -groupsize will cause this to have multiple sections in one doc.xml
     $self->edit_doc_xml($gsdl_cgi,$doc_xml_filename,
             $metaname,$metavalue,$metapos);
 
-}
-
-
+    # Release the lock once it is done
+    $self->unlock_collection($username, $collect);
+}
 
 
 sub mxml_metadata
 {
-
     my ($tagname, $attrHash, $contextArray, $parentDataArray, $parser) = @_;
-
-
     my $metaname = $parser->{'parameters'}->{'metaname'};
     my $metamode = $parser->{'parameters'}->{'metamode'};
 
+    # Find the right metadata tag and checks if we are going to override it
+    # Note: This over writes the first metadata block it encountered even if it doesn't belong to the source file we specified
     my $name_attr = $attrHash->{'name'};
-
     if (($name_attr eq $metaname) && ($metamode eq "override")) {
-
+        # Get the value and override the current value
     my $metavalue = $parser->{'parameters'}->{'metavalue'};
-
     $attrHash->{'_content'} = $metavalue;
 
     # Don't want it to wipe out any other pieces of metadata
     $parser->{'parameters'}->{'metamode'} = "done";
-
     }
 
     # raw extended
+    # Someone please write some comments on why adding ':'.$tagname => $attrHash
     return (':'.$tagname => $attrHash, [$tagname => $attrHash]);
 }
@@ -563 +550 @@
 sub mxml_description
 {
-
     my ($tagname, $attrHash, $contextArray, $parentDataArray, $parser) = @_;
-
     my $metamode = $parser->{'parameters'}->{'metamode'};
 
-
+    # Accumulate the metadata block to the end of the description block
+    # Note: This adds metadata block to all description blocks, so if there are 
+    # multiple FileSets, it will add to all of them
     if ($metamode eq "accumulate") {
     # tack a new metadata tag on to the end of the <Metadata>+ block
-
     my $metaname = $parser->{'parameters'}->{'metaname'};
     my $metavalue = $parser->{'parameters'}->{'metavalue'};
     
-
     my $metadata_attr = { '_content' => $metavalue, 
                   'name'     => $metaname, 
@@ -581 +566 @@
 
     my $append_metadata = [ "Metadata" => $metadata_attr ];
-
     my $description_content = $attrHash->{'_content'};
 
     push(@$description_content,"    ", $append_metadata ,"\n        ");
-
     }
 
@@ -592 +575 @@
 }
 
+
 sub edit_metadata_xml
 {
     my $self = shift @_;
-
     my ($gsdl_cgi, $metadata_xml_filename, $metaname, $metavalue, $metamode) = @_;
 
-    # use XML::Rules to add it in (read in and out again)
-
+    # Set the call-back functions for the metadata tags
     my @rules = 
     ( _default => 'raw extended',
       'Metadata' => \&mxml_metadata,
       'Description' => \&mxml_description );
-      
-
+
+    # use XML::Rules to add it in (read in and out again)
     my $parser = XML::Rules->new(rules => \@rules, 
                  style => 'filter' );
-
 
     my $xml_in = "";
@@ -615 +596 @@
     }
     else {
+        # Read them in
     my $line;
     while (defined ($line=<MIN>)) {
         $xml_in .= $line;
     }
-    close(MIN);
+    close(MIN); 
     
-    
+        # Filter with the call-back functions
     my $xml_out = "";
     $parser->filter($xml_in,\$xml_out, { metaname => $metaname,
                          metavalue => $metavalue,
-                         metamode => $metamode } );
-    
+                                             metamode => $metamode } );
+        
     if (!open(MOUT,">$metadata_xml_filename")) {
         $gsdl_cgi->generate_error("Unable to write out to $metadata_xml_filename: $!");
@@ -632 +614 @@
     else {
         print MOUT $xml_out;
-        close(MOUT);
-        
+        close(MOUT);        
     }
     }
@@ -642 +623 @@
 {
     my $self = shift @_;
-
+    
     my $username  = $self->{'username'};
     my $collect   = $self->{'collect'};
@@ -648 +629 @@
     my $gsdlhome  = $self->{'gsdlhome'};
 
-
     # don't user authenticate for now
     if ($baseaction::authentication_enabled) {
@@ -655 +635 @@
     }
 
+    # Obtain the collect and archive dir    
     my $collect_dir = &util::filename_cat($gsdlhome, "collect");
-#    $gsdl_cgi->checked_chdir($collect_dir);
-
     my $archive_dir = &util::filename_cat($collect_dir,$collect,"archives");
-
 
     # Make sure the collection isn't locked by someone else
@@ -665 +643 @@
 
     # look up additional args
-
     # want either d= or f=
     my $docid  = $self->{'d'};
     my $import_file  = $self->{'f'};
-
     if ((!defined $docid) && (!defined $import_file)) {
     $gsdl_cgi->generate_error("No docid (d=...) or import file (f=) specified.");
     } 
 
+    # Get the parameters and set default mode to "accumulate"
     my $metaname   = $self->{'metaname'};
     my $metavalue  = $self->{'metavalue'};
     my $metamode   = $self->{'metamode'};
-
     if ((!defined $metamode) || ($metamode =~ m/^\s*$/)) {
     # make "accumulate" the default (less destructive, as won't actually 
     # delete any existing values)
-
     $metamode = "accumulate";
     }
 
+    # Obtain where the metadata.xml is from the archiveinfo-doc.gdb file
+    # If the doc oid is not specified, we assume the metadata.xml is next to the specified "f"
     my $metadata_xml_file;
-
     my $import_filename = undef;
-
     if (defined $docid) {
     my $arcinfo_doc_filename = &dbutil::get_infodb_file_path("gdbm", "archiveinf-doc", $archive_dir);
@@ -698 +673 @@
     }
     else {
-    $import_filename = &util::filename_cat($collect_dir,$collect,$import_file);
-    }
-    
-    # figure out correct metadata.xml file
-
+        $import_filename = &util::filename_cat($collect_dir,$collect,$import_file);
+    }
+    
+    
+    # figure out correct metadata.xml file [?]
+    # Assuming the metadata.xml file is next to the source file
+    # Note: This will not work if it is using the inherited metadata from the parent folder
     my ($import_tailname, $import_dirname) 
     = File::Basename::fileparse($import_filename);
-
     my $metadata_xml_filename = &util::filename_cat($import_dirname,"metadata.xml");
 
+    # Edit the metadata.xml
+    # Note: At moment it doesn't correctly on metadata.xml with multiple FileSets
+    # "accumulate" mode will add new metadata block to all FileSets
+    # "override" mode will over write the first encountered metadata block, even if it doesn't have the right source
     $self->edit_metadata_xml($gsdl_cgi,$metadata_xml_filename,
                  $metaname,$metavalue,$metamode);
 
-}
-
-
-
+    # Release the lock once it is done
+    $self->unlock_collection($username, $collect);
+}
 
 
@@ -726 +705 @@
     my $gsdlhome  = $self->{'gsdlhome'};
 
-
     if ($baseaction::authentication_enabled) {
     # Ensure the user is allowed to edit this collection
@@ -732 +710 @@
     }
 
+    # Obtain the collect dir
     my $collect_dir = &util::filename_cat($gsdlhome, "collect");
-#    $gsdl_cgi->checked_chdir($collect_dir);
-
 
     # Make sure the collection isn't locked by someone else
@@ -740 +717 @@
 
     # look up additional args
-
     my $docid     = $self->{'d'};
+    if ((!defined $docid) || ($docid =~ m/^\s*$/)) {
+      $gsdl_cgi->generate_error("No docid (d=...) specified.");
+    }
+    
+    # Generate the dbkey
     my $metaname  = $self->{'metaname'};
-
     my $dbkey = "$docid.$metaname";
 
+    # To people who know $collect_tail please add some comments
+    # Obtain the live gdbm_db path 
     my $collect_tail = $collect;
     $collect_tail =~ s/^.*[\/\\]//;
-
     my $index_text_directory = &util::filename_cat($collect_dir,$collect,"index","text");
     my $infodb_file_path = &dbutil::get_infodb_file_path("gdbm", "live-$collect_tail", $index_text_directory);
 
+    # Remove the key
     my $cmd = "gdbmdel \"$infodb_file_path\" \"$dbkey\"";
-
     my $status = system($cmd);
-
     if ($status != 0) {
+        # Catch error if gdbmdel failed
     my $mess = "Failed to set metadata key: $dbkey\n";
     
@@ -782 +763 @@
     my $gsdlhome  = $self->{'gsdlhome'};
 
-
     # don't user authenticate for now
     if ($baseaction::authentication_enabled) {
@@ -789 +769 @@
     }
 
+    # Obtain the collect dir
     my $collect_dir = &util::filename_cat($gsdlhome, "collect");
-#    $gsdl_cgi->checked_chdir($collect_dir);
-
 
     # Make sure the collection isn't locked by someone else
@@ -797 +776 @@
 
     # look up additional args
-
     my $docid     = $self->{'d'};
+    if ((!defined $docid) || ($docid =~ m/^\s*$/)) {
+      $gsdl_cgi->generate_error("No docid (d=...) specified.");
+    }
     my $metaname  = $self->{'metaname'};
     my $metapos   = $self->{'metapos'};
 
+    # To people who know $collect_tail please add some comments
+    # Obtain the path to the database
     my $collect_tail = $collect;
     $collect_tail =~ s/^.*[\/\\]//;
-
     my $index_text_directory = &util::filename_cat($collect_dir,$collect,"index","text");
     my $infodb_file_path = &dbutil::get_infodb_file_path("gdbm", $collect_tail, $index_text_directory);
+
+    # Read the docid entry
     my $doc_rec_string = &dbutil::read_infodb_entry("gdbm", $infodb_file_path, $docid);
     my $doc_rec = &dbutil::convert_infodb_string_to_hash($doc_rec_string);
+
+    # Basically loop through and unescape_html the values
     foreach my $k (keys %$doc_rec) {
     my @escaped_v = ();
@@ -823 +809 @@
         }
     }
-
     $doc_rec->{$k} = \@escaped_v;
     }
 
+    # Check to make sure the key does exist
+    if (!defined ($doc_rec->{$metaname})) {
+        $gsdl_cgi->generate_error("No metadata field \"" . $metaname . "\" in the specified document: [" . $docid . "]");
+    }
+
+    # Obtain the specified metadata pos
     $metapos = 0 if (!defined $metapos);
 
     # consider check key is defined before deleting?
-
+    # Loop through the metadata array and ignore the specified position
     my $filtered_metadata = [];
-    my $num_metadata_vals = scalar(@{$doc_rec->{$metaname}});
-
+    my $num_metadata_vals = scalar(@{$doc_rec->{$metaname}});    
     for (my $i=0; $i<$num_metadata_vals; $i++) {
     my $metavalue = shift(@{$doc_rec->{$metaname}});
@@ -841 +831 @@
     }
     }
-
     $doc_rec->{$metaname} = $filtered_metadata;
 
+    # Turn the record back to string
     my $serialized_doc_rec = &dbutil::convert_infodb_hash_to_string($doc_rec);
 
+    # Store it back to the database
     my $cmd = "gdbmset \"$infodb_file_path\" \"$docid\" \"$serialized_doc_rec\"";
-
     my $status = system($cmd);
-
     if ($status != 0) {
     my $mess = "Failed to set metadata key: $docid\n";
@@ -867 +856 @@
     $gsdl_cgi->generate_ok_message($mess);
     }
-
-}
-
-
-
-
+}