Bug 182266

Summary: Should fetch owner commits in build-requests-fetcher.
Product: WebKit Reporter: dewei_zhu
Component: New BugsAssignee: Nobody <webkit-unassigned>
Status: NEW ---    
Severity: Normal CC: dewei_zhu, rniwa
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch rniwa: review+

Description dewei_zhu 2018-01-29 15:27:00 PST
Should fetch owner commits in build-requests-fetcher.
Comment 1 dewei_zhu 2018-01-29 15:36:25 PST
Created attachment 332590 [details]
Patch
Comment 2 Ryosuke Niwa 2018-01-29 16:55:01 PST
Comment on attachment 332590 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=332590&action=review

> Websites/perf.webkit.org/public/include/build-requests-fetcher.php:135
> +            if ($commit_owner_id && !array_key_exists($commit_owner_id, $this->commits_by_id))
> +                $commit_owners[$commit_owner_id] = TRUE;

Rather than manually finding owner commits like this, we should simply query the database as so:
SELECT * FROM commits, repositories WHERE commit_repository = repository_id AND commit_id
    IN (SELECT commitset_commit FROM commit_set_items WHERE commitset_set = $1);
Comment 3 dewei_zhu 2018-01-29 17:19:26 PST
Created attachment 332607 [details]
Patch
Comment 4 Ryosuke Niwa 2018-01-30 00:26:58 PST
Comment on attachment 332607 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=332607&action=review

> Websites/perf.webkit.org/public/include/build-requests-fetcher.php:149
> +        $owner_commits_rows = $this->db->query_and_fetch_all('SELECT * FROM commits WHERE commit_id IN
> +            (SELECT DISTINCT(commitset_commit_owner) FROM commit_set_items WHERE commitset_set = $1
> +            AND commitset_commit_owner IS NOT NULL)', array($commit_set_id));

I would have indented this query as:
SELECT * FROM commits WHERE commit_id
    IN (SELECT DISTINCT(commitset_commit_owner) FROM commit_set_items
        WHERE commitset_set = $1 AND commitset_commit_owner IS NOT NULL)

> Websites/perf.webkit.org/public/include/build-requests-fetcher.php:158
> +                'repository' => $row['commit_repository'],

Do: $resolve_ids ? $row['repository_name'] : $row['repository_id'];

> Websites/perf.webkit.org/public/include/build-requests-fetcher.php:159
> +                'commitOwner' => NULL,

We should assert that $row['commitset_commit_owner'] is NULL.

> Websites/perf.webkit.org/server-tests/api-build-requests-tests.js:289
> +        return MockData.addTestGroupWithOwnerCommitNotInCommitSet(TestServer.database()).then(() => {
> +            return Manifest.fetch();
> +        }).then(() => {
> +            return BuildRequest.fetchForTriggerable('build-webkit');
> +        }).then((buildRequests) => {

Use await!
await MockData.addTestGroupWithOwnerCommitNotInCommitSet(TestServer.database());
await Manifest.fetch();
const buildRequests = await BuildRequest.fetchForTriggerable('build-webkit');

> Websites/perf.webkit.org/server-tests/api-build-requests-tests.js:292
> +            let test = Test.findById(200);

const.

> Websites/perf.webkit.org/server-tests/api-build-requests-tests.js:295
> +            let platform = Platform.findById(65);

const.
Comment 5 dewei_zhu 2018-01-30 01:54:21 PST
Comment on attachment 332607 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=332607&action=review

>> Websites/perf.webkit.org/public/include/build-requests-fetcher.php:158
>> +                'repository' => $row['commit_repository'],
> 
> Do: $resolve_ids ? $row['repository_name'] : $row['repository_id'];

Nice catch.

>> Websites/perf.webkit.org/public/include/build-requests-fetcher.php:159
>> +                'commitOwner' => NULL,
> 
> We should assert that $row['commitset_commit_owner'] is NULL.

$owner_commits_rows only contains information from commits table which does not contains 'commitset_commit_owner'
Comment 6 Ryosuke Niwa 2018-01-30 13:57:58 PST
Comment on attachment 332607 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=332607&action=review

>>> Websites/perf.webkit.org/public/include/build-requests-fetcher.php:159
>>> +                'commitOwner' => NULL,
>> 
>> We should assert that $row['commitset_commit_owner'] is NULL.
> 
> $owner_commits_rows only contains information from commits table which does not contains 'commitset_commit_owner'

That just happens to be true for our database today. We always should be asserting those conditions in the case they change.
What's the chance you'd remember to fix this code five years from now when that condition changes?