From 471e1f1b9b3c50c6fb7b444c58053eb5ab62f4a5 Mon Sep 17 00:00:00 2001 From: Zooko O'Whielacronx Date: Fri, 19 Dec 2008 08:39:24 -0700 Subject: [PATCH] try to tidy up uri-as-string vs. uri-as-object I get confused about whether a given argument or return value is a uri-as-string or uri-as-object. This patch adds a lot of assertions that it is one or the other, and also changes CheckerResults to take objects not strings. In the future, I hope that we generally use Python objects except when importing into or exporting from the Python interpreter e.g. over the wire, the UI, or a stored file. --- src/allmydata/checker_results.py | 4 ++-- src/allmydata/dirnode.py | 11 +++++++++-- src/allmydata/immutable/checker.py | 4 ++-- src/allmydata/interfaces.py | 2 +- src/allmydata/mutable/checker.py | 5 +++-- src/allmydata/test/common.py | 26 ++++++++++++++------------ src/allmydata/test/test_dirnode.py | 26 +++++++++++++------------- src/allmydata/test/test_web.py | 7 +++++-- 8 files changed, 49 insertions(+), 36 deletions(-) diff --git a/src/allmydata/checker_results.py b/src/allmydata/checker_results.py index 7031ff3a..34c588e2 100644 --- a/src/allmydata/checker_results.py +++ b/src/allmydata/checker_results.py @@ -1,14 +1,14 @@ from zope.interface import implements from allmydata.interfaces import ICheckerResults, ICheckAndRepairResults, \ - IDeepCheckResults, IDeepCheckAndRepairResults + IDeepCheckResults, IDeepCheckAndRepairResults, IURI from allmydata.util import base32 class CheckerResults: implements(ICheckerResults) def __init__(self, uri, storage_index): - assert isinstance(uri, str) + assert IURI.providedBy(uri), uri self.uri = uri self.storage_index = storage_index self.problems = [] diff --git a/src/allmydata/dirnode.py b/src/allmydata/dirnode.py index 38a31136..1afa6655 100644 --- a/src/allmydata/dirnode.py +++ b/src/allmydata/dirnode.py @@ -13,6 +13,7 @@ from allmydata.checker_results import DeepCheckResults, \ DeepCheckAndRepairResults from allmydata.monitor import Monitor from allmydata.util import hashutil, mathutil, base32, log +from allmydata.util.assertutil import _assert, precondition from allmydata.util.hashutil import netstring from allmydata.util.limiter import ConcurrencyLimiter from allmydata.util.netstring import split_netstring @@ -60,6 +61,8 @@ class Adder: self.overwrite = overwrite def set_node(self, name, node, metadata): + precondition(isinstance(name, unicode), name) + precondition(IFilesystemNode.providedBy(node), node) self.entries.append( [name, node, metadata] ) def modify(self, old_contents, servermap, first_time): @@ -72,6 +75,7 @@ class Adder: else: assert len(e) == 3 name, child, new_metadata = e + assert _assert(IFilesystemNode.providedBy(child), child) assert isinstance(name, unicode) if name in children: if not self.overwrite: @@ -201,9 +205,9 @@ class NewDirectoryNode: or IDirectoryNode.providedBy(child)), (name,child) assert isinstance(metadata, dict) rwcap = child.get_uri() # might be RO if the child is not writeable + assert isinstance(rwcap, str), rwcap rocap = child.get_readonly_uri() assert isinstance(rocap, str), rocap - assert isinstance(rwcap, str), rwcap entry = "".join([netstring(name.encode("utf-8")), netstring(rocap), netstring(self._encrypt_rwcap(rwcap)), @@ -339,7 +343,8 @@ class NewDirectoryNode: If this directory node is read-only, the Deferred will errback with a NotMutableError.""" - assert isinstance(name, unicode) + precondition(isinstance(name, unicode), name) + precondition(isinstance(child_uri, str), child_uri) child_node = self._create_node(child_uri) d = self.set_node(name, child_node, metadata, overwrite) d.addCallback(lambda res: child_node) @@ -369,6 +374,8 @@ class NewDirectoryNode: If this directory node is read-only, the Deferred will errback with a NotMutableError.""" + precondition(IFilesystemNode.providedBy(child), child) + if self.is_readonly(): return defer.fail(NotMutableError()) assert isinstance(name, unicode) diff --git a/src/allmydata/immutable/checker.py b/src/allmydata/immutable/checker.py index 615e0f0a..8c63f984 100644 --- a/src/allmydata/immutable/checker.py +++ b/src/allmydata/immutable/checker.py @@ -70,7 +70,7 @@ class SimpleCHKFileChecker: pass def _done(self, res): - r = CheckerResults(self.uri.to_string(), self.storage_index) + r = CheckerResults(self.uri, self.storage_index) report = [] healthy = bool(len(self.found_shares) >= self.total_shares) r.set_healthy(healthy) @@ -179,7 +179,7 @@ class SimpleCHKFileVerifier(download.FileDownloader): self._si_s = storage.si_b2a(self._storage_index) self.init_logging() - self._check_results = r = CheckerResults(self._uri.to_string(), self._storage_index) + self._check_results = r = CheckerResults(self._uri, self._storage_index) r.set_data({"count-shares-needed": k, "count-shares-expected": N, }) diff --git a/src/allmydata/interfaces.py b/src/allmydata/interfaces.py index e87050ca..e3484688 100644 --- a/src/allmydata/interfaces.py +++ b/src/allmydata/interfaces.py @@ -372,7 +372,7 @@ class IURI(Interface): """Return a string of printable ASCII characters, suitable for passing into init_from_string.""" -class IVerifierURI(Interface): +class IVerifierURI(Interface, IURI): def init_from_string(uri): """Accept a string (as created by my to_string() method) and populate this instance with its data. I am not normally called directly, diff --git a/src/allmydata/mutable/checker.py b/src/allmydata/mutable/checker.py index 1a64163b..31e8744c 100644 --- a/src/allmydata/mutable/checker.py +++ b/src/allmydata/mutable/checker.py @@ -2,6 +2,7 @@ from twisted.internet import defer from twisted.python import failure from allmydata import hashtree +from allmydata.uri import from_string from allmydata.util import hashutil, base32, idlib, log from allmydata.checker_results import CheckAndRepairResults, CheckerResults @@ -16,7 +17,7 @@ class MutableChecker: self._monitor = monitor self.bad_shares = [] # list of (nodeid,shnum,failure) self._storage_index = self._node.get_storage_index() - self.results = CheckerResults(node.get_uri(), self._storage_index) + self.results = CheckerResults(from_string(node.get_uri()), self._storage_index) self.need_repair = False self.responded = set() # set of (binary) nodeids @@ -297,7 +298,7 @@ class MutableCheckAndRepairer(MutableChecker): d = self._node.repair(self.results) def _repair_finished(repair_results): self.cr_results.repair_successful = True - r = CheckerResults(self._node.get_uri(), self._storage_index) + r = CheckerResults(from_string(self._node.get_uri()), self._storage_index) self.cr_results.post_repair_results = r self._fill_checker_results(repair_results.servermap, r) self.cr_results.repair_results = repair_results # TODO? diff --git a/src/allmydata/test/common.py b/src/allmydata/test/common.py index 1d09f897..e21cccc8 100644 --- a/src/allmydata/test/common.py +++ b/src/allmydata/test/common.py @@ -16,6 +16,7 @@ from allmydata.checker_results import CheckerResults, CheckAndRepairResults, \ from allmydata.mutable.common import CorruptShareError from allmydata.storage import storage_index_to_dir from allmydata.util import log, fileutil, pollmixin +from allmydata.util.assertutil import precondition from allmydata.stats import StatsGathererService from allmydata.key_generator import KeyGeneratorService import common_util as testutil @@ -36,16 +37,17 @@ class FakeCHKFileNode: bad_shares = {} def __init__(self, u, client): + precondition(IURI.providedBy(u), u) self.client = client - self.my_uri = u.to_string() + self.my_uri = u self.storage_index = u.storage_index def get_uri(self): - return self.my_uri + return self.my_uri.to_string() def get_readonly_uri(self): - return self.my_uri + return self.my_uri.to_string() def get_verify_cap(self): - return IURI(self.my_uri).get_verify_cap() + return self.my_uri.get_verify_cap() def get_storage_index(self): return self.storage_index @@ -92,25 +94,25 @@ class FakeCHKFileNode: return True def download(self, target): - if self.my_uri not in self.all_contents: + if self.my_uri.to_string() not in self.all_contents: f = failure.Failure(NotEnoughSharesError()) target.fail(f) return defer.fail(f) - data = self.all_contents[self.my_uri] + data = self.all_contents[self.my_uri.to_string()] target.open(len(data)) target.write(data) target.close() return defer.maybeDeferred(target.finish) def download_to_data(self): - if self.my_uri not in self.all_contents: + if self.my_uri.to_string() not in self.all_contents: return defer.fail(NotEnoughSharesError()) - data = self.all_contents[self.my_uri] + data = self.all_contents[self.my_uri.to_string()] return defer.succeed(data) def get_size(self): try: - data = self.all_contents[self.my_uri] - except KeyError: - raise NotEnoughSharesError() + data = self.all_contents[self.my_uri.to_string()] + except KeyError, le: + raise NotEnoughSharesError(le) return len(data) def read(self, consumer, offset=0, size=None): d = self.download_to_data() @@ -184,7 +186,7 @@ class FakeMutableFileNode: return self.storage_index def check(self, monitor, verify=False): - r = CheckerResults(self.my_uri.to_string(), self.storage_index) + r = CheckerResults(self.my_uri, self.storage_index) is_bad = self.bad_shares.get(self.storage_index, None) data = {} data["count-shares-needed"] = 3 diff --git a/src/allmydata/test/test_dirnode.py b/src/allmydata/test/test_dirnode.py index d29e5da9..e2a4591a 100644 --- a/src/allmydata/test/test_dirnode.py +++ b/src/allmydata/test/test_dirnode.py @@ -42,7 +42,7 @@ class Marker: return self.storage_index def check(self, monitor, verify=False): - r = CheckerResults("", None) + r = CheckerResults(uri.from_string(self.nodeuri), None) r.set_healthy(True) r.set_recoverable(True) return defer.succeed(r) @@ -107,7 +107,7 @@ class Dirnode(unittest.TestCase, d = self.client.create_empty_dirnode() def _created(dn): u = make_mutable_file_uri() - d = dn.set_uri(u"child", u, {}) + d = dn.set_uri(u"child", u.to_string(), {}) d.addCallback(lambda res: dn.list()) def _check1(children): self.failUnless(u"child" in children) @@ -239,7 +239,7 @@ class Dirnode(unittest.TestCase, d = self.client.create_empty_dirnode() def _created(rw_dn): - d2 = rw_dn.set_uri(u"child", fileuri) + d2 = rw_dn.set_uri(u"child", fileuri.to_string()) d2.addCallback(lambda res: rw_dn) return d2 d.addCallback(_created) @@ -251,7 +251,7 @@ class Dirnode(unittest.TestCase, self.failUnless(ro_dn.is_mutable()) self.shouldFail(dirnode.NotMutableError, "set_uri ro", None, - ro_dn.set_uri, u"newchild", fileuri) + ro_dn.set_uri, u"newchild", fileuri.to_string()) self.shouldFail(dirnode.NotMutableError, "set_uri ro", None, ro_dn.set_node, u"newchild", filenode) self.shouldFail(dirnode.NotMutableError, "set_nodes ro", None, @@ -315,11 +315,11 @@ class Dirnode(unittest.TestCase, self.expected_manifest.append( ((u"child",) , m.get_uri()) ) self.expected_verifycaps.add(ffu_v) self.expected_storage_indexes.add(base32.b2a(m.get_storage_index())) - d.addCallback(lambda res: n.set_uri(u"child", fake_file_uri)) + d.addCallback(lambda res: n.set_uri(u"child", fake_file_uri.to_string())) d.addCallback(lambda res: self.shouldFail(ExistingChildError, "set_uri-no", "child 'child' already exists", - n.set_uri, u"child", other_file_uri, + n.set_uri, u"child", other_file_uri.to_string(), overwrite=False)) # / # /child = mutable @@ -445,12 +445,12 @@ class Dirnode(unittest.TestCase, # set_uri + metadata # it should be possible to add a child without any metadata - d.addCallback(lambda res: n.set_uri(u"c2", fake_file_uri, {})) + d.addCallback(lambda res: n.set_uri(u"c2", fake_file_uri.to_string(), {})) d.addCallback(lambda res: n.get_metadata_for(u"c2")) d.addCallback(lambda metadata: self.failUnlessEqual(metadata, {})) # if we don't set any defaults, the child should get timestamps - d.addCallback(lambda res: n.set_uri(u"c3", fake_file_uri)) + d.addCallback(lambda res: n.set_uri(u"c3", fake_file_uri.to_string())) d.addCallback(lambda res: n.get_metadata_for(u"c3")) d.addCallback(lambda metadata: self.failUnlessEqual(sorted(metadata.keys()), @@ -458,7 +458,7 @@ class Dirnode(unittest.TestCase, # or we can add specific metadata at set_uri() time, which # overrides the timestamps - d.addCallback(lambda res: n.set_uri(u"c4", fake_file_uri, + d.addCallback(lambda res: n.set_uri(u"c4", fake_file_uri.to_string(), {"key": "value"})) d.addCallback(lambda res: n.get_metadata_for(u"c4")) d.addCallback(lambda metadata: @@ -500,9 +500,9 @@ class Dirnode(unittest.TestCase, d.addCallback(lambda res: n.delete(u"d4")) # metadata through set_children() - d.addCallback(lambda res: n.set_children([ (u"e1", fake_file_uri), - (u"e2", fake_file_uri, {}), - (u"e3", fake_file_uri, + d.addCallback(lambda res: n.set_children([ (u"e1", fake_file_uri.to_string()), + (u"e2", fake_file_uri.to_string(), {}), + (u"e3", fake_file_uri.to_string(), {"key": "value"}), ])) d.addCallback(lambda res: @@ -700,7 +700,7 @@ class Dirnode(unittest.TestCase, # now make sure that we honor overwrite=False d.addCallback(lambda res: - self.subdir2.set_uri(u"newchild", other_file_uri)) + self.subdir2.set_uri(u"newchild", other_file_uri.to_string())) d.addCallback(lambda res: self.shouldFail(ExistingChildError, "move_child_to-no", diff --git a/src/allmydata/test/test_web.py b/src/allmydata/test/test_web.py index 4a91e361..2711a0e6 100644 --- a/src/allmydata/test/test_web.py +++ b/src/allmydata/test/test_web.py @@ -9,6 +9,7 @@ from allmydata import interfaces, provisioning, uri, webish from allmydata.immutable import upload, download from allmydata.web import status, common from allmydata.util import fileutil, base32 +from allmydata.util.assertutil import precondition from allmydata.test.common import FakeDirectoryNode, FakeCHKFileNode, \ FakeMutableFileNode, create_chk_filenode from allmydata.interfaces import IURI, INewDirectoryURI, \ @@ -56,6 +57,7 @@ class FakeClient(service.MultiService): return [] def create_node_from_uri(self, auri): + precondition(isinstance(auri, str), auri) u = uri.from_string(auri) if (INewDirectoryURI.providedBy(u) or IReadonlyNewDirectoryURI.providedBy(u)): @@ -2274,6 +2276,7 @@ class Web(WebMixin, testutil.StallMixin, unittest.TestCase): file_contents = "New file contents here\n" d = self.PUT("/uri", file_contents) def _check(uri): + assert isinstance(uri, str), uri self.failUnless(uri in FakeCHKFileNode.all_contents) self.failUnlessEqual(FakeCHKFileNode.all_contents[uri], file_contents) @@ -2309,8 +2312,8 @@ class Web(WebMixin, testutil.StallMixin, unittest.TestCase): return d def _check(uri): - self.failUnless(uri in FakeCHKFileNode.all_contents) - self.failUnlessEqual(FakeCHKFileNode.all_contents[uri], + self.failUnless(uri.to_string() in FakeCHKFileNode.all_contents) + self.failUnlessEqual(FakeCHKFileNode.all_contents[uri.to_string()], file_contents) return self.GET("/uri/%s" % uri) d.addCallback(_check) -- 2.45.2