infrastructure/blender-buildbot-www
1
0
Fork 2
Code Issues 4 Pull Requests 1 Projects Releases Wiki Activity

Add support for JSON listing #1

Merged
Francesco Siddi merged 7 commits from json-view into develop 2023-11-01 12:08:09 +01:00
Conversation 2 Commits 7 Files Changed 2 +57 -6
Francesco Siddi commented 2023-10-20 16:00:04 +02:00
Owner
Copy Link

By appending ?format=json&v=1 it is now possible to display the list
of builds as JSON object. This makes it easier for updaters or
download scripts to list the builds offered.

Notice that v=1 must be specified for the listing to appear, otherwise
we return code 400 with an error message.

By appending ?format=json&v=1 it is now possible to display the list of builds as JSON object. This makes it easier for updaters or download scripts to list the builds offered. Notice that v=1 must be specified for the listing to appear, otherwise we return code 400 with an error message.
👍 1
Francesco Siddi added 2 commits 2023-10-20 16:00:06 +02:00 
This prevents url params from being included when building a file url
with getFileNameURL(), leading to a url that always works.
By appending ?format=json&v=1 it is now possible to display the list
of builds as JSON object. This makes it easier for updaters or
download scripts to list the builds offered.

Notice that v=1 must be specified for the listing to appear, otherwise
we return code 400 with an error message.
Sergey Sharybin was assigned by Francesco Siddi 2023-10-20 16:00:17 +02:00
Pablo Vazquez requested review from Sergey Sharybin 2023-10-24 00:28:20 +02:00
Sergey Sharybin was unassigned by Pablo Vazquez 2023-10-24 00:28:27 +02:00
Sergey Sharybin requested changes 2023-10-30 13:56:29 +01:00
source/main.php Outdated
@ -31,2 +31,3 @@
// Serve the directory listing.
// Check if we are requesting a JSON formatted view.
if (isset($_GET['format']) && $_GET['format'] == 'json') {
Sergey Sharybin commented 2023-10-30 13:55:06 +01:00
Owner
Copy Link

You do not need to check isset($_GET['format']) when you're comparing to an explicit constant. Doing if ($_GET['format'] == 'json') { should suffice.

You do not need to check `isset($_GET['format'])` when you're comparing to an explicit constant. Doing `if ($_GET['format'] == 'json') {` should suffice.
Francesco Siddi commented 2023-10-31 21:48:02 +01:00
Author
Owner
Copy Link

I tried, but removing the check raises:

Warning: Undefined array key "format" in /var/www/html/source/main.php on line 56

I tried, but removing the check raises: `Warning: Undefined array key "format" in /var/www/html/source/main.php on line 56`
source/main.php Outdated
@ -33,0 +32,4 @@
// Check if we are requesting a JSON formatted view.
if (isset($_GET['format']) && $_GET['format'] == 'json') {
header('Content-Type: application/json; charset=utf-8');
// Require v=1 to be specified
Sergey Sharybin commented 2023-10-30 13:55:18 +01:00
Owner
Copy Link

Move to renderDownloadResponceAsJSON or something similar.

Move to `renderDownloadResponceAsJSON` or something similar.
fsiddi marked this conversation as resolved
source/main.php Outdated
@ -33,0 +45,4 @@
return;
}
// Serve the directory listing as HTML.
Sergey Sharybin commented 2023-10-30 13:48:56 +01:00
Owner
Copy Link

Move to function renderDownloadResponceAsHTML.

Move to `function renderDownloadResponceAsHTML`.
fsiddi marked this conversation as resolved
Francesco Siddi added 1 commit 2023-10-31 23:46:22 +01:00 
Follow the single responsibility priciple.
Francesco Siddi added 1 commit 2023-10-31 23:50:07 +01:00 
Removes Warning of undefined key.
Sergey Sharybin requested changes 2023-11-01 10:12:13 +01:00
source/BuildsRenderer.php Outdated
@ -11,0 +12,4 @@
/* Return a JSON formatted list of builds. */
foreach ($this->builds as $build) {
// Construct a full URL to the downloadable file
$build->url = $this->getFileNameURL($build);
Sergey Sharybin commented 2023-11-01 10:10:24 +01:00
Owner
Copy Link

I didn't realize how it worked before. We should not be modifying builds from the renderer. And we should never inject fields which do not exist in the class.

Unfortunately, PHP does not provide anything to ensure const-correctness. But once the build and list of builds were constructed they should remain non-mutable.

More verbose but more correct is to create an explicit associative array with the fields which you're interested in:

$builds_json = array();

foreach ($this->builds as $build) {
  $builds_json[] = array(
    'url' => $this->getFileNameURL($build),
    'app' => $build->app,
    'version' => $build->version,
    'risk_id' => $build->risk_id,
    'branch' => $build->branch,
    'patch' => $build->patch,
    'hash' => $build->hash,
    'platform' => $build->platform,
    'architecture' => $build->architecture,
    'bitness' => $build->bitness,
    'file_mtime' => $build->file_mtime,
    'file_name' => $build->file_name,
    'file_size' => $build->file_size,
    'file_extension' => $build->file_extension,
    'release_cycle' => $build->release_cycle,
  );
}

echo(json_encode($builds_json, JSON_PRETTY_PRINT | JSON_UNESCAPED_SLASHES));
I didn't realize how it worked before. We should not be modifying builds from the renderer. And we should never inject fields which do not exist in the class. Unfortunately, PHP does not provide anything to ensure const-correctness. But once the build and list of builds were constructed they should remain non-mutable. More verbose but more correct is to create an explicit associative array with the fields which you're interested in: ``` $builds_json = array(); foreach ($this->builds as $build) { $builds_json[] = array( 'url' => $this->getFileNameURL($build), 'app' => $build->app, 'version' => $build->version, 'risk_id' => $build->risk_id, 'branch' => $build->branch, 'patch' => $build->patch, 'hash' => $build->hash, 'platform' => $build->platform, 'architecture' => $build->architecture, 'bitness' => $build->bitness, 'file_mtime' => $build->file_mtime, 'file_name' => $build->file_name, 'file_size' => $build->file_size, 'file_extension' => $build->file_extension, 'release_cycle' => $build->release_cycle, ); } echo(json_encode($builds_json, JSON_PRETTY_PRINT | JSON_UNESCAPED_SLASHES)); ```
fsiddi marked this conversation as resolved
Francesco Siddi added 2 commits 2023-11-01 11:03:12 +01:00 
Once we are done building the response, we should exit. Using return
allows for more content to be added to the response, and we don't
want that.
Do not modify the content of $builds in BuildsRenderer.
Sergey Sharybin commented 2023-11-01 11:50:17 +01:00
Owner
Copy Link

Use exit instead of return

This hides the code flow, without any indication that it is what that will happen. An errorous output after rendering response should not be guarded with an exit somewhere deep in the call hierarchy.

There might be other things happening after render, like performance or statistics gathering, logging, etc.

I guess that is because renderDownloadResponseAsHTML () could have happened after renderDownloadResponseAsJSON(), but the proper fix for it is to:

  if (isset($_GET['format']) && $_GET['format'] == 'json') {
    renderDownloadResponseAsJSON($lister);
    return;
  }

  renderDownloadResponseAsHTML($lister);
> Use exit instead of return This hides the code flow, without any indication that it is what that will happen. An errorous output after rendering response should not be guarded with an `exit` somewhere deep in the call hierarchy. There might be other things happening after render, like performance or statistics gathering, logging, etc. I guess that is because `renderDownloadResponseAsHTML ()` could have happened after `renderDownloadResponseAsJSON()`, but the proper fix for it is to: ``` if (isset($_GET['format']) && $_GET['format'] == 'json') { renderDownloadResponseAsJSON($lister); return; } renderDownloadResponseAsHTML($lister); ```
Francesco Siddi added 1 commit 2023-11-01 11:59:56 +01:00
Francesco Siddi commented 2023-11-01 12:01:32 +01:00
Author
Owner
Copy Link

I indeed missed the return after renderDownloadResponseAsJSON .
Thank you for your patience with this review.

I indeed missed the `return` after `renderDownloadResponseAsJSON `. Thank you for your patience with this review.
Sergey Sharybin approved these changes 2023-11-01 12:04:27 +01:00
Francesco Siddi merged commit 8f2fd36e3b into develop 2023-11-01 12:08:09 +01:00
Francesco Siddi deleted branch json-view 2023-11-01 12:08:09 +01:00
Sign in to join this conversation.
Reviewers
No reviewers
Sergey Sharybin
Labels
Clear labels
No items
No Label
Milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
2 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: infrastructure/blender-buildbot-www#1
No description provided.
Delete Branch "json-view"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?