[webkit-reviews] review denied: [Bug 191557] Extend commits table to contain testability warning information. : [Attachment 357273] Patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has denied dewei_zhu at apple.com's request for
review:
Bug 191557: Extend commits table to contain testability warning information.
https://bugs.webkit.org/show_bug.cgi?id=191557

Attachment 357273: Patch

https://bugs.webkit.org/attachment.cgi?id=357273&action=review




--- Comment #10 from Ryosuke Niwa <rniwa at webkit.org> ---
Comment on attachment 357273
  --> https://bugs.webkit.org/attachment.cgi?id=357273
Patch

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

> Websites/perf.webkit.org/public/api/report-commits.php:7
> +    list($db, $commits) = connect_database_and_sanitize_commits($post_data);

We shouldn't do two unrelated things at once like this. Keep new Database here
and pass it to this function.
Also, instead of sanitize call it validate since you're not doing any kind of
sanitization / normalization
in this function but rather checking that certain conditions are met.

> Websites/perf.webkit.org/public/api/update-commits.php:14
> +	       $db->rollback_transaction();
> +	       exit_with_error('InvalidRepository', array('commit' =>
$commit_info));

We shouldn't start a transaction until all these validations are done. r-
because of this.

In general, we want to do all validations upfront as much as possible, and
execute all queries at once without any checks.
Having an open transaction incurs a significant database-side cost.
e.g. see
http://blog.lerner.co.il/in-postgresql-as-in-life-dont-wait-too-long-to-commit/

> Websites/perf.webkit.org/public/api/update-commits.php:19
> +	   foreach($commit_info as $key => $value) {

I'd rather have a bunch of if's instead of looping over keys and checking
conditions for each as in:
if ($commit_info['time'])
    $commit_update['time'] = $commit_info['time'];

> Websites/perf.webkit.org/public/api/update-commits.php:21
> +	       switch ($key) {
> +		   case "repository":

Nit: Wrong indentation. It should look like:
    switch (~) {
    case "~":
	~
	break;
    }

> Websites/perf.webkit.org/public/include/json-header.php:208
> +    $db = new Database;
> +    if (!$db->connect())
> +	   exit_with_error('DatabaseConnectionFailure');
> +

This shouldn't be done in a helper function like this.

> Websites/perf.webkit.org/public/include/json-header.php:211
> +    $report = json_decode($post_data, true);
> +
> +    verify_slave($db, $report);

Neither are these. With these code being baked in this function, 
we won't be able to use this validation logic in any other backend code which
may want to validate commit information
which process other non-commit information, or API where commits appear within
some other data structures.

> Websites/perf.webkit.org/public/include/json-header.php:232
> +    $committer_data = $committer_query;

The fact this is making a copy of $committer_query is rather subtle.
I think it's better we just constructed these arrays where we call
update_or_insert_row instead (as in duplicate the code).

> Websites/perf.webkit.org/public/include/json-header.php:239
> +	   $db->rollback_transaction();
> +	   exit_with_error('FailedToInsertCommitter', array('committer' =>
$committer_data));

A helper function shouldn't be rolling back a transaction it didn't start like
this.
The caller should be doing this work instead.

Alternatively, we could create a class which knows how to add or update a
commit.
That class then start & manage its own transaction.

> Websites/perf.webkit.org/public/include/json-header.php:249
> +	   $db->rollback_transaction();
> +	   exit_with_error('FailedToFindPreviousCommit', array('commit' =>
$commit_info));

Ditto.

> Websites/perf.webkit.org/tools/js/os-build-fetcher.js:34
> +	   const splittedArrayList = [];
> +	   for (let startIndex = 0; startIndex < array.length; startIndex +=
maxCount)

Why is this function defined here? It's making the diff impossible to read.
Also, we probably don't need this function. We can simply keep calling
commitsToPost.splice(0, maxCount) until commitsToPost is empty.

> Websites/perf.webkit.org/tools/js/os-build-fetcher.js:53
> +	   const repository = config['name'];

It's important to clarify whether this variable stores a Repository object, or
its name.
So this rename is a regression as far as I'm concerned.

> Websites/perf.webkit.org/tools/js/os-build-fetcher.js:100
> +	   const output = await this._subprocess.execute(command);
> +	   return JSON.parse(output);

This change would make the syncing script to be incompatible with the old
script.
We shouldn't make that kind of backwards incompatible change at the same
modifying the database schema.
Make this work with both old & new scripts, or split this part into its own
patch.
r- because of this.

In general, we should always make these changes backwards compatible so that
we can isolate the deployment of new version of dashboards from deployment of
new helper scripts, etc...