Code review comment for lp://staging/~stevenk/launchpad/db-add-bprc

82 + <class
83 + class="lp.soyuz.model.binarypackagepath.BinaryPackagePath">
84 + <allow
85 + interface="lp.soyuz.interfaces.binarypackagepath.IBinaryPackagePathSource"/>
86 + </class>

No need for this. You already grant it to the securedutility.

104 + <class
105 + class="lp.soyuz.model.binarypackagereleasecontents.BinaryPackageReleaseContents">
106 + <allow
107 + interface="lp.soyuz.interfaces.binarypackagereleasecontents.IBinaryPackageReleaseContentsSet"/>
108 + </class>

Same here.

You also have one Source and one Set. Please make them the same.

+ path = TextLine(title=_('Full path name'), required=True, readonly=True)

s/Full path name/Absolute path/, perhaps?

158 + def getOrCreate(path):
159 + """Fetch the ID of the given path name, or create it.
160 +
161 + :param: path: The full path name to query or create.
162 +
163 + :return: An ID.
164 + """

Why an ID? Why not the object itself?

191 + """The contents of a binary package release.
192 +
193 + A binary package release is a representation of a binary package in
194 + one or more releases. This class models the files contained within
195 + the binary package.
196 + """

In one or more releases? I think replacing all this with a one-liner would be good. "A file contained by an `IBinaryPackageRelease`." or so.

250 + def getOrCreate(self, path):
251 + """See `IBinaryPackagePathSource`."""
252 + store = IMasterStore(BinaryPackagePath)
253 + bpp = store.find(BinaryPackagePath, BinaryPackagePath.path == path)
254 + if bpp.count():
255 + return bpp[0]
256 + else:
257 + bpp = BinaryPackagePath()
258 + bpp.path = path
259 + return store.add(bpp)

Try something like this:

    bpp = store.find(BinaryPackagePath, BinaryPackagePath.path == path).one()
    if bpp is None:
        bpp = BinaryPackagePath(path)
        store.add(bpp)
    return bpp

314 + with TempDir() as tmp_dir:

Can you use a TemporaryFile instead? Or even a NamedTemporaryFile, if that doesn't work. Or does DebPackage really want a filename, and care about what it is? Also, might as well break out of the with block as soon as you're done with DebPackage.

324 + bpp = getUtility(IBinaryPackagePathSource).getOrCreate(
325 + unicode(filename))

Objection! unicode() on potentially non-ASCII data == wrong. Probably best to treat it as UTF-8, probably skipping if it's undecodable.

Also, this is going to be pretty slow. You may need to think about optimising it down to a single query eventually.

334 + results = store.find(
335 + BinaryPackageReleaseContents,
336 + BinaryPackageReleaseContents.binarypackagerelease == bpr.id)
337 + for bprc in results:
338 + store.remove(bprc)

You can't just call remove() on the resultset?

362 + def test_get_or_create(self):

Method name style should be preserved here. So, test_getOrCreate.

405 + deb = open(datadir('pmount_0.9.7-2ubuntu2_amd64.deb'), 'r')
406 + lfa = self.factory.makeLibraryFileAlias(
407 + filename='pmount_0.9.7-2ubuntu2_amd64.deb', content=deb.read())
408 + deb.close()

with looks handy here.

420 + self.assertEqual(13, results.count())

This is a bit too opaque for my liking.