Merge pull request #321 from vladimir-v-diaz/address-pr#295-review

Address pr#295 review

Author: vladimir-v-diaz

tests/test_updater.py

@@ -465,18 +465,18 @@ def test_2__import_delegations(self):
 
 
 
-  def test_2__versioninfo_has_changed(self):
+  def test_2__versioninfo_has_been_updated(self):
     # Verify that the method returns 'False' if a versioninfo was not changed.
     snapshot_filepath = os.path.join(self.client_metadata_current, 'snapshot.json')
     snapshot_signable = tuf.util.load_json_file(snapshot_filepath)
     root_versioninfo = snapshot_signable['signed']['meta']['root.json'] 
 
-    self.assertFalse(self.repository_updater._versioninfo_has_changed('root.json',
+    self.assertFalse(self.repository_updater._versioninfo_has_been_updated('root.json',
                                                            root_versioninfo))
 
     # Verify that the method returns 'True' if Root's version number changes.
     root_versioninfo['version'] = 8 
-    self.assertTrue(self.repository_updater._versioninfo_has_changed('root.json',
+    self.assertTrue(self.repository_updater._versioninfo_has_been_updated('root.json',
                                                            root_versioninfo))
 
 

tuf/client/updater.py

@@ -1555,17 +1555,33 @@ def _update_metadata_if_changed(self, metadata_role,
     # Ensure the referenced metadata has been loaded.  The 'root' role may be
     # updated without having 'snapshot' available.  
     if referenced_metadata not in self.metadata['current']:
-      message = 'Cannot update ' + repr(metadata_role) + ' because ' \
-                + referenced_metadata + ' is missing.'
-      raise tuf.RepositoryError(message)
+      raise tuf.RepositoryError('Cannot update ' + repr(metadata_role) +
+        ' because ' + referenced_metadata + ' is missing.')
 
-    # The referenced metadata has been loaded.  Extract the new
-    # versioninfo for 'metadata_role' from it. 
+    # The referenced metadata has been loaded.  Extract the new versioninfo for
+    # 'metadata_role' from it. 
     else:
-      message = repr(metadata_role) + ' referenced in ' +\
-        repr(referenced_metadata)+ '.  ' + repr(metadata_role)+' may be updated.'
-      logger.debug(message)
+      logger.debug(repr(metadata_role) + ' referenced in ' +
+        repr(referenced_metadata)+ '.  ' + repr(metadata_role) +
+        ' may be updated.')
+
+    # Extract the versioninfo of the uncompressed version of 'metadata_role'.
+    expected_versioninfo = self.metadata['current'][referenced_metadata] \
+                                        ['meta'] \
+                                        [uncompressed_metadata_filename]
+
+    # Simply return if the metadata for 'metadata_role' has not been updated,
+    # according to the uncompressed metadata provided by the referenced
+    # metadata.  The metadata is considered updated if its version number is
+    # strictly greater than its currently trusted version number.
+    if not self._versioninfo_has_been_updated(uncompressed_metadata_filename,
+                                         expected_versioninfo):
+      logger.info(repr(uncompressed_metadata_filename) + ' up-to-date.')
+      
+      return
 
+    logger.debug('Metadata ' + repr(uncompressed_metadata_filename) + ' has changed.')
+
     # There might be a compressed version of 'snapshot.json' or Targets
     # metadata available for download.  Check the 'meta' field of
     # 'referenced_metadata' to see if it is listed when 'metadata_role'
@@ -1581,38 +1597,22 @@ def _update_metadata_if_changed(self, metadata_role,
     # decompressing a file that may be invalid or partially intact.
     compression = None
 
-    # Extract the versioninfo of the uncompressed version of 'metadata_role'.
-    expected_versioninfo = self.metadata['current'][referenced_metadata] \
-                                        ['meta'] \
-                                        [uncompressed_metadata_filename]
-
     # Check for the availability of compressed versions of 'snapshot.json',
     # 'targets.json', and delegated Targets (that also start with 'targets').
     # For 'targets.json' and delegated metadata, 'referenced_metata'
     # should always be 'snapshot'.  'snapshot.json' specifies all roles
     # provided by a repository, including their version numbers.
     if metadata_role == 'snapshot' or metadata_role.startswith('targets'):
-      gzip_metadata_filename = uncompressed_metadata_filename + '.gz'
       if 'gzip' in self.metadata['current']['root']['compression_algorithms']:
         compression = 'gzip'
-        
-        logger.debug('Compressed version of ' + \
-                repr(uncompressed_metadata_filename) + ' is available at ' + \
-                repr(gzip_metadata_filename) + '.')
-      else:
-        logger.debug('Compressed version of ' + \
-                     repr(uncompressed_metadata_filename) + ' not available.')
-
-    # Simply return if the file has not changed, according to the metadata
-    # about the uncompressed file provided by the referenced metadata.
-    if not self._versioninfo_has_changed(uncompressed_metadata_filename,
-                                         expected_versioninfo):
-      logger.info(repr(uncompressed_metadata_filename) + ' up-to-date.')
+        gzip_metadata_filename = uncompressed_metadata_filename + '.gz'
+        logger.debug('Compressed version of ' +
+          repr(uncompressed_metadata_filename) + ' is available at ' +
+          repr(gzip_metadata_filename) + '.')
 
-      return
-
-    logger.debug('Metadata ' + repr(uncompressed_metadata_filename) + \
-                 ' has changed.')
+      else:
+        logger.debug('Compressed version of ' +
+          repr(uncompressed_metadata_filename) + ' not available.')
 
     # The file lengths of metadata are unknown, only their version numbers are
     # known.  Set an upper limit for the length of the downloaded file for each
@@ -1659,14 +1659,15 @@ def _update_metadata_if_changed(self, metadata_role,
 
 
 
-  def _versioninfo_has_changed(self, metadata_filename, new_versioninfo):
+  def _versioninfo_has_been_updated(self, metadata_filename, new_versioninfo):
     """
     <Purpose>
       Non-public method that determines whether the current versioninfo of
-      'metadata_filename' differs from 'new_versioninfo'.  The 'new_versioninfo'
-      argument should be extracted from the latest copy of the metadata that
-      references 'metadata_filename'.  Example: 'root.json' would be referenced
-      by 'snapshot.json'.
+      'metadata_filename' is less than 'new_versioninfo' (i.e., the version
+      number has been incremented).  The 'new_versioninfo' argument should be
+      extracted from the latest copy of the metadata that references
+      'metadata_filename'.  Example: 'root.json' would be referenced by
+      'snapshot.json'.
         
       'new_versioninfo' should only be 'None' if this is for updating
       'root.json' without having 'snapshot.json' available.
@@ -1689,11 +1690,11 @@ def _versioninfo_has_changed(self, metadata_filename, new_versioninfo):
       None.
 
     <Side Effects>
-      If there is no versioninfo currently loaded for 'metada_filename',
-      try to load it.
+      If there is no versioninfo currently loaded for 'metadata_filename', try
+      to load it.
 
     <Returns>
-      Boolean.  True if the versioninfo has changed, false otherwise.
+      Boolean.  True if the versioninfo has changed, False otherwise.
     """
 
     # If there is no versioninfo currently stored for 'metadata_filename',
@@ -1763,9 +1764,10 @@ def _update_versioninfo(self, metadata_filename):
         self.metadata['current']['timestamp']['version']
 
     # When updating snapshot.json, the client either (1) has a copy of
-    # snapshot.json, or (2) in the process of obtaining it by first downloading
-    # timestamp.json.  Note: Clients may have only root.json and perform a
-    # refresh of top-level metadata to obtain the remaining roles.
+    # snapshot.json, or (2) is in the process of obtaining it by first
+    # downloading timestamp.json.  Note: Clients are allowed to have only
+    # root.json initially, and perform a refresh of top-level metadata to
+    # obtain the remaining roles.
     elif metadata_filename == 'snapshot.json':
 
       # Verify the version number of the currently trusted snapshot.json in
@@ -2791,8 +2793,9 @@ def download_target(self, target, destination_directory):
     <Purpose>
       Download 'target' and verify it is trusted.
         
-      This will only store the file at 'destination_directory' if the downloaded
-      file matches the description of the file in the trusted metadata.
+      This will only store the file at 'destination_directory' if the
+      downloaded file matches the description of the file in the trusted
+      metadata.
     
     <Arguments>
       target:
@@ -2809,6 +2812,10 @@ def download_target(self, target, destination_directory):
       tuf.NoWorkingMirrorError:
         If a target could not be downloaded from any of the mirrors.
 
+        Although expected to be rare, there might be OSError exceptions (except
+        errno.EEXIST) raised when creating the destination directory (if it
+        doesn't exist). 
+
     <Side Effects>
       A target file is saved to the local system.
 
@@ -2834,15 +2841,20 @@ def download_target(self, target, destination_directory):
     target_file_object = self._get_target_file(target_filepath, trusted_length,
                                                trusted_hashes)
 
-    # We acquired a target file object from a mirror.  Move the file into
-    # place (i.e., locally to 'destination_directory').  Note: join() discards
-    # 'destination_directory' if 'target_path' contains a leading path separator
-    # (i.e., is treated as an absolute path).
+    # We acquired a target file object from a mirror.  Move the file into place
+    # (i.e., locally to 'destination_directory').  Note: join() discards
+    # 'destination_directory' if 'target_path' contains a leading path
+    # separator (i.e., is treated as an absolute path).
     destination = os.path.join(destination_directory,
                                target_filepath.lstrip(os.sep))
     destination = os.path.abspath(destination)
     target_dirpath = os.path.dirname(destination)
-    
+   
+    # When attempting to create the leaf directory of 'target_dirpath', ignore
+    # any exceptions raised if the root directory already exists.  All other
+    # exceptions potentially thrown by os.makedirs() are re-raised.
+    # Note: os.makedirs can raise OSError if the leaf directory already exists
+    # or cannot be created.
     try:
       os.makedirs(target_dirpath)
 

tuf/formats.py

@@ -1037,13 +1037,11 @@ def make_versioninfo(version_number):
       in Snapshot metadata.
 
   <Exceptions>
-    tuf.FormatError, if the 'VERSIONINFO_SCHEMA' to be returned
-    does not have the correct format.
+    tuf.FormatError, if the dict to be returned does not have the correct
+    format (i.e., VERSIONINFO_SCHEMA).
 
   <Side Effects>
-    If any of the arguments are incorrectly formatted, the dict
-    returned will be checked for formatting errors, and if found,
-    will raise a 'tuf.FormatError' exception.
+    None.
 
   <Returns>
     A dictionary conformant to 'VERSIONINFO_SCHEMA', containing the version
@@ -1053,9 +1051,14 @@ def make_versioninfo(version_number):
   versioninfo = {'version' : version_number}
 
   # Raise 'tuf.FormatError' if 'versioninfo' is improperly formatted.
-  VERSIONINFO_SCHEMA.check_match(versioninfo)
-
-  return versioninfo
+  try: 
+    VERSIONINFO_SCHEMA.check_match(versioninfo)
+  
+  except:
+    raise
+  
+  else:
+    return versioninfo
 
 
 

tuf/repository_lib.py

@@ -451,9 +451,9 @@ def _delete_obsolete_metadata(metadata_directory, snapshot_metadata,
         # metadata might co-exist if write() and
         # write(consistent_snapshot=True) are mixed, so ensure only
         # '<version_number>.filename' metadata is stripped.
-        embeded_version_number = None
+        embedded_version_number = None
         if metadata_name not in snapshot_metadata['meta']: 
-          metadata_name, embeded_version_number = \
+          metadata_name, embedded_version_number = \
             _strip_consistent_snapshot_version_number(metadata_name, consistent_snapshot)
 
         # Strip filename extensions.  The role database does not include the
@@ -474,10 +474,10 @@ def _delete_obsolete_metadata(metadata_directory, snapshot_metadata,
         # file extension of roles.  TODO: Should we leave it up to integrators
         # to remove outdated consistent snapshots?
         """ 
-        if consistent_snapshot and embeded_version_number is not None:
+        if consistent_snapshot and embedded_version_number is not None:
           file_hashes = list(snapshot_metadata['meta'][metadata_name_extension] \
                                         ['hashes'].values())
-          if embeded_digest not in file_hashes:
+          if embedded_digest not in file_hashes:
             logger.info('Removing outdated metadata: ' + repr(metadata_path))
             os.remove(metadata_path)
         """
@@ -508,24 +508,24 @@ def _strip_consistent_snapshot_version_number(metadata_filename,
   Strip from 'metadata_filename' any version data (in the expected
   '{dirname}/version_number.filename' format) that it may contain, and return
   the stripped filename and its version number as a tuple.
+  'consistent_snapshot' is a boolean indicating if 'metadata_filename' contains
+  prepended version number.
   """
 
-  embeded_version_number = ''
+  embedded_version_number = ''
 
   # Strip the version number if 'consistent_snapshot' is True.
   # Example:  'targets/unclaimed/10.django.json'  -->
   # 'targets/unclaimed/django.json'
   if consistent_snapshot:
     dirname, basename = os.path.split(metadata_filename)
-    embeded_version_number = basename[:basename.find('.')]
+    embedded_version_number, basename = basename.split('.', 1)
+    stripped_metadata_filename = os.path.join(dirname, basename)
 
-    # Ensure the version number, including the period, is stripped.
-    basename = basename[basename.find('.') + 1:]
-    
-    metadata_filename = os.path.join(dirname, basename)
+    return stripped_metadata_filename, embedded_version_number
 
-
-  return metadata_filename, embeded_version_number
+  else:
+    return metadata_filename, ''