Code review comment for lp:~benji/launchpad/bug-781600

On Tue, Jul 12, 2011 at 3:34 PM, Francis J. Lacoste
<email address hidden> wrote:
> Review: Needs Fixing

Thanks very much for the review. I'm certainly not knowledgeable of
this area of LP yet and can use the help.

>> === modified file 'lib/lp/registry/interfaces/distribution.py'
>
>>      @operation_parameters(
>> +        since=Datetime(
>> +            title=_("Time of last change"),
>> +            description=_(
>> +                "Return branches that have new tips since this timestamp."),
>> +            required=False))
>> +    @export_operation_as(name="getBranchTips")
>> +    @export_read_operation()
>> +    @operation_for_version('devel')
>> +    def getBranchTips(since):
>> +        """Return a collection of branches which have new tips since a date.
>> +        """
>
> I'd suggest improving the docstring. Something along the lines of:

Looks good. Done.

> You probably need to make since=None since it's not a required parameter.

It seems odd to me to put the default in the interface in addition to in
the implementation. It has no effect and I worry that the DRY violation
could result in a loss of synchronization with the real default and
result in confusion. Also, the "required=False" bit is what indicates
that "since" is optional, the interface reader shouldn't care what
internal value we use to signify that it wasn't provided.

That being said, I'm only -0 so I'll add it given a bit of a push.

>> === modified file 'lib/lp/registry/model/distribution.py'
>
>>          """See `IBugTarget`."""
>>          return get_bug_tags("BugTask.distribution = %s" % sqlvalues(self))
>>
>> +    def getBranchTips(self, since=None):
>> +        """See `IDistribution`."""
>> +        query = """
>> +            SELECT unique_name, last_scanned_id,
>> +                SeriesSourcePackageBranch.distroseries FROM Branch
>> +            JOIN DistroSeries ON Branch.distroseries = DistroSeries.id
>> +            JOIN Distribution ON DistroSeries.distribution = Distribution.id
>> +            JOIN SeriesSourcePackageBranch ON
>> +                Branch.id = SeriesSourcePackageBranch.branch
>> +            WHERE Distribution.name = %s""" % sqlvalues(self.name)
>
> Not all branches will be official, so you want to use a LEFT OUTER JOIN on
> both SeriesSourcePackageBranch and DistroSeries.

I fixed that and added a test as mentioned below.

> Distro series id is pretty meangingless, we should return their name.
> DistroSeries.name instead of SeriesSourcePackageBranch.distroseries

Done. I had to tweak the query more than just substituting
DistroSeries.name but not too much.

>> +
>> +        if since is not None:
>> +            query += (
>> +                ' AND branch.last_scanned > %s' % sqlvalues(since))
>> +
>> +        query += ' ORDER BY unique_name, last_scanned_id;'
>> +
>> +        data = list(Store.of(self).execute(query))
>> +
>> +        # Group on location (unique_name) and revision (last_scanned_id).
>> +        results = []
>> +        for key, group in itertools.groupby(data, itemgetter(0, 1)):
>> +            results.append(list(key))
>> +            # Pull out all the official series IDs and append them as a list
>> +            # to the end of the current record.
>> +            results[-1].append(map(itemgetter(-1), group))
>> +        return results
>> +
>
> That last grouping will need to be modified to handle the NULL case.

Yep. Done.

> It's kind of a shame that we have to materialize the whole results set here,
> since for huge size, it will be batched anyway by the web service code. Would
> it make sense to use an generator here? Maybe not, since we'd still retrieve
> all results for later batches.

Given that we use ORDER BY, I suspect the only savings would be not
transmitting the non-batched results from the DB server to the app
server. However, making the method a generator might still be a win
because even though they will likely come back to get the rest, that
request may well hit a different app server. (I don't think we do any
sort of request affinity but would like to know if we do.)

Later... Nope, that didn't work. lazr.restful exploded because it
couldn't adapt the generator to IFiniteSequence. I would have expected
returning a generator to work, but I can't find any other instances in
the code base. I've left it as a non-generator for the time being but
would appreciate any knowledge that can be shared on the topic.

>> === modified file 'lib/lp/registry/tests/test_distro_webservice.py'
>
> Personally, I would have tested the implementation directly and then just
> added one test to show that it's available over the webservice (no need to go
> over the overhead of launchpadlib for every test). But heh, since the main
> point of this is to be available of the web service, I guess that's fine.

Yeah, I could have gone either way there.

> I'd like to see an additional showing what happens with branches that are not
> official. (They are not returned with the current implementation.)

Done (mentioned above).

>> +class TestGetBranchTips(TestCaseWithFactory):
>> +    """Test the getBranchTips method and it's exposure to the web service."""
>> +
>> +    layer = DatabaseFunctionalLayer
>> +
>> +    def setUp(self):
>> +        super(TestGetBranchTips, self).setUp()
>> +        self.distro = self.factory.makeDistribution()
>> +        series_1 = self.series_1 = self.factory.makeDistroRelease(self.distro)
>> +        series_2 = self.series_2 = self.factory.makeDistroRelease(self.distro)
>> +        source_package = self.factory.makeSourcePackage(distroseries=series_1)
>> +        self.branch = self.factory.makeBranch(sourcepackage=source_package)
>> +        registrant = self.factory.makePerson()
>> +        now = datetime.now(pytz.UTC)
>> +        sourcepackagename = self.factory.makeSourcePackageName()
>> +        SeriesSourcePackageBranchSet.new(
>> +            series_1, PackagePublishingPocket.RELEASE, sourcepackagename,
>> +            self.branch, registrant, now)
>> +        SeriesSourcePackageBranchSet.new(
>> +            series_2, PackagePublishingPocket.RELEASE, sourcepackagename,
>> +            self.branch, registrant, now)
>
> You can use makeRelatedBranches(reference_branch=self.branch) to make it an
> official source package. Or use
> makeRelatedBranchesForSourcePackage(source_package) to create the branch and
> make it official in one call.

I tried to use makeRelatedBranchesForSourcePackage and
makeRelatedBranches but the need to have two different distro series
releases (series_1 and series_2 in the original code) kept me from
getting it to work. If I'm misunderstanding something, please help me.

>> +        self.factory.makeRevisionsForBranch(self.branch)
>> +        endInteraction()
>> +
>> +        self.lp = launchpadlib_for("anonymous-access")
>> +        self.lp_distro = [d for d in self.lp.distributions
>> +            if d.name == self.distro.name][0]
>
> Couldn't you use self.lp.distributions[d.name] directly?

I could have, if I'd thought of it. Fixed now.

>> +
>> +    def test_structure(self):
>> +        """The structure of the results is what we expect."""
>> +        # The results should be structured as a list of
>> +        # (location, tip revision ID, [official series, official series, ...])
>> +        item = self.lp_distro.getBranchTips()[0]
>> +        self.assertTrue(item[0].startswith('~person-name-'))
> Can't you compare with the actual expected value?

I can. I'll treat this as a request to do so. ;) Done.

> self.assertEquals(self.branch_unique_name, item[0])?
>
>> +        self.assertTrue(item[1].startswith('revision-id-'))
>> +        self.assertEqual(sorted(item[2]), [14, 15])
>
> Wow, talk about magic numbers! Comparing with the created distro series name
> probably makes more sense.

Indeed. I fixed another instance and didn't realize I'd left those in.
Fixed.
--
Benji York