Download files in fetch.txt by kba · Pull Request #119 · LibraryOfCongress/bagit-python

kba · 2018-11-27T14:59:30Z

Adds a new method Bag.fetch_files_to_be_fetched() that fetches files listed in fetch.txt, c.f. #118.

If this is useful for someone, can be further refined (CLI, overrideable fetch implementation, anti-hammering interval).

coveralls · 2018-11-27T15:03:28Z

Coverage decreased (-1.7%) to 81.842% when pulling 1de38c4 on kba:fetch-files into c39b650 on LibraryOfCongress:master.

coveralls · 2018-11-27T15:03:30Z

Coverage increased (+0.09%) to 83.595% when pulling 6fda25c on kba:fetch-files into 8a8263e on LibraryOfCongress:master.

edsu · 2018-12-06T17:37:17Z

Thanks for diving in to add the fetch functionality @kba. I wonder if it might be a bit more readable to rename fetch_files_to_be_fetched() to fetch() and have it take an optional force parameter that would re-download things that are already present?

edsu · 2018-12-06T17:41:21Z

Also, I haven't worked with fetch.txt files much before. But I'm kind of surprised that the test suite considers a bag valid if it has a fetch.txt containing an item that is not present in the payload directory.

See: https://github.com/LibraryOfCongress/bagit-python/blob/master/test.py#L1023

kba · 2018-12-07T12:10:26Z

I wonder if it might be a bit more readable to rename fetch_files_to_be_fetched() to fetch()

Fine with me, I wanted to avoid confusion with fetch_entries.

and have it take an optional force parameter that would re-download things that are already present?

Sure.

I'm kind of surprised that the test suite considers a bag valid if it has a fetch.txt containing an item that is not present in the payload directory.

These tests break the "Every file listed in the fetch file MUST be listed in every payload manifest" rule and it isn't validated. fetch_entries should not just check for unsafe filenames but ensure files is also listed in payload_entries. The validation only checks data on disk and manifest entries. That is a bug.

Since the manifests determines the number and size of files, it could make sense to allow "bag with holes" validation against only the files not mentioned in fetch.txt with a special parameter though, if you don't want to fetch the whole thing. By default,

if it has a fetch.txt containing an item that is not present in the payload directory

should not be valid, you're right.

edsu · 2018-12-07T12:41:19Z

I guess we could consider validation as a separate issue from this PR though.

…gress#118

By default only allow HTTP(S) and (S)FTP URL

acdha · 2019-04-04T13:50:49Z

 self.normalized_manifest_names = {}

 self.algorithms = []
+ self.fetch_url_whitelist = DEFAULT_FETCH_URL_WHITELIST if fetch_url_whitelist is None else fetch_url_whitelist


This could be simplified:
Suggested change
self.fetch_url_whitelist=DEFAULT_FETCH_URL_WHITELISTiffetch_url_whitelistisNoneelsefetch_url_whitelist
self.fetch_url_whitelist=fetch_url_whitelistorDEFAULT_FETCH_URL_WHITELIST

acdha · 2019-04-04T14:19:43Z

+ for url, expected_size, filename in self.fetch_entries():
+ if not fnmatch_any(url, self.fetch_url_whitelist):
+ raise BagError(_("Malformed URL in fetch.txt: %s, matches none of the whitelisted URL patterns %s") % (url, self.fetch_url_whitelist))
+ expected_size = -1 if expected_size == '-' else int(expected_size)


I'm thinking this would be more idiomatic as:
Suggested change
expected_size=-1ifexpected_size=='-'elseint(expected_size)
expected_size=Noneifexpected_size=='-'elseint(expected_size)

I also wanted to check whether fetch_entries() is correctly validating that these values are either “-” or digits.

acdha · 2019-04-04T14:21:47Z

+ for _, _, filename in self.fetch_entries():
 yield filename

+ def fetch(self, force=False):


I'm wondering whether we should support force() or simply have the user handle file operations outside of scope. The main thing which that does is open up the need to think about potential security concerns so perhaps that should just be confirming that we correctly validate file paths not being outside of the bag scope.

acdha · 2019-04-04T14:24:04Z

+ else:
+ content_length = int(headers['content-length'])
+ if content_length != expected_size:
+ raise BagError(_("Inconsistent size of %s: Expected %s but Content-Length is %s") % (filename, expected_size, content_length))


I was wondering whether this should have the URL as well:
Suggested change
raiseBagError(_("Inconsistent size of %s: Expected %s but Content-Length is %s") % (filename, expected_size, content_length))
raiseBagError(_("Inconsistent size for %s: expected %s bytes but %s had a Content-Length of %s") % (filename, expected_size, url, content_length))

acdha · 2019-04-04T14:30:53Z

+ resp = opener.open(req)
+ headers = resp.info()
+ if expected_size >= 0:
+ if "content-length" not in headers:


This would only apply to HTTP requests but the intention of this code is to support other protocols, which suggests that this would need to be a conditional warning since I don't believe the stdlib sets a header like that for other protocols.

acdha · 2019-04-04T14:37:24Z

- parsed_url = urlparse(url)
-
- if not all((parsed_url.scheme, parsed_url.netloc)):
- raise BagError(_("Malformed URL in fetch.txt: %s") % url)


I think we should still have this validation step since it reports some malformed URLs earlier. This would also allow us to remove both of the fnmatch checks by changing the whitelist to be a list of URL schemes and simply adding a second if parsed_url.scheme not in self.fetch_url_schema_whitelist: … check here.

acdha requested changes Nov 27, 2018
View reviewed changes

Comment thread.gitignore Outdated
Comment threadbagit.py Outdated
Comment threadbagit.py Outdated
Comment threadtest.py Outdated

kba force-pushed the fetch-files branch from 5e05700 to bb19dedCompare December 10, 2018 13:14

kba added 7 commits December 10, 2018 14:15

Minimal implementation of fetching entries of fetch.txt,LibraryOfCon…
dae7b40
…gress#118

update locale
aae8cc3

properly set user-agent for py2/py3
6f93786

revert gitignore changes so git clean will do its thing
ac5ea43

replace urlopen with non-global urllib2/urllib.request solution
91f5f4d

Implement a whitelist mechanism to validate fetch entries
cacf9d9
By default only allow HTTP(S) and (S)FTP URL

Rename fetch_files_to_be_fetched -> fetch, force option
6fda25c

kba force-pushed the fetch-files branch from bb19ded to 6fda25cCompare December 10, 2018 13:15

acdha mentioned this pull request Apr 4, 2019
Download files in fetch.txt #118
Open

acdha requested changes Apr 4, 2019
View reviewed changes

	self.fetch_url_whitelist=DEFAULT_FETCH_URL_WHITELISTiffetch_url_whitelistisNoneelsefetch_url_whitelist
	self.fetch_url_whitelist=fetch_url_whitelistorDEFAULT_FETCH_URL_WHITELIST

	expected_size=-1ifexpected_size=='-'elseint(expected_size)
	expected_size=Noneifexpected_size=='-'elseint(expected_size)

	raiseBagError(_("Inconsistent size of %s: Expected %s but Content-Length is %s") % (filename, expected_size, content_length))
	raiseBagError(_("Inconsistent size for %s: expected %s bytes but %s had a Content-Length of %s") % (filename, expected_size, url, content_length))

Uh oh!

Conversation

kba commented Nov 27, 2018

Uh oh!

coveralls commented Nov 27, 2018

Uh oh!

coveralls commented Nov 27, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

edsu commented Dec 6, 2018

Uh oh!

edsu commented Dec 6, 2018

Uh oh!

kba commented Dec 7, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

edsu commented Dec 7, 2018

Uh oh!

acdhaApr 4, 2019

Choose a reason for hiding this comment

Uh oh!

acdhaApr 4, 2019

Choose a reason for hiding this comment

Uh oh!

acdhaApr 4, 2019

Choose a reason for hiding this comment

Uh oh!

acdhaApr 4, 2019

Choose a reason for hiding this comment

Uh oh!

acdhaApr 4, 2019

Choose a reason for hiding this comment

Uh oh!

acdhaApr 4, 2019

Choose a reason for hiding this comment

Uh oh!

acdhaApr 4, 2019

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

coveralls commented Nov 27, 2018•
edited
Loading

kba commented Dec 7, 2018•
edited
Loading