Re: GSoC: Adding a web interface similar to the Hydra web interface

[Danny Milosavljevic <dannym@scratchpost.org>]

[-- Attachment #1: Type: text/plain, Size: 3238 bytes --]

Hi Tatiana,

nice work!

I have a few comments:

db-get-builds looks fine and we could merge this change to master.  But you have other changes in the same commit, so we can't directly cherry-pick it.  (not so bad, but somewhat cumbersome now)

I'd suggest to rename "db-get-evaluations-info" to "db-get-evaluations-build-summary".  Please don't use nonsensical names like "succ" or "que" in public interfaces (f.e. #:que ).

Nitpick: You have a few variables with "_" like "eval_cnt".  This is not customary in Scheme.  Also, having funny abbreviations like "cnt" isn't either.  I suggest "evaluation-count".

For db-get-evaluations-count: Please add a short docstring documenting what it's for (i.e. "Return the number of evaluations of the given specification SPEC").

In src/cuirass/http.scm :

I don't think that HTML5 is XML-compatible.  So if not, please use XHTML preamble and content-type (I think that that is even broken on master - but it's not difficult to fix).  It's also easier on web browsers if they can assume well-formedness once their XML parser is done parsing.

respond-static-file: We should not second-guess the VFS layer.  Checking for ".." gives an illusion of security when in fact random things could be mounted and also the VFS could have funny syntax for who knows what on the filesystem.  Let's rather have a static list of permissible names and allow those (whitelist).  That's the intention of the check anyway, right?

Also, in light of an ever-changing backing store (cuirass continusly evaluates things), the way you are doing pagination is not the correct way to do it because the data set will scroll underneath you and you will miss items (or see duplicate items) as an user.  Also, it's slow and the DBMS can't reuse anything because you are cutting it off and offseting it over and over again and the transaction isolation level is too low for the DBMS to be able to do anything about it[1].

See also https://use-the-index-luke.com/blog/2013-07/pagination-done-the-postgresql-way for a much better way.  Basically, remember the last value and use it as a limit in the WHERE condition by this one (also, sorting always).

That means if you have the (sorted) list:

A
B
C
E
G
H

and for some reason only 3 fit the page, print first page:

A
B
C <--- print, and remember this one for the boundary of the next page

Second page is "after C", so "... WHERE ... > 'C'":

E
G
H <--- print; and remember that no next page exists.

Note that this also works if, in the mean time, "D" appeared in the backing store.

Then second page is "after C", so "... WHERE ... > 'C'":

D
E
G <--- print, and remember this one for the boundary of the next page

This would only cause a problem if items vanished, so we should keep this in mind.  And jumps to random pages by page number would be difficult now (but who needs that in this case?  Much better to be able to jump by actual timestamp, which then would be easily possible).

In the case of evaluations, it would be good to sort by timestamp and then by evaluation id - since no matter what this would be a stable sort criteria.
(maybe revision, too)

[1] We should fix the latter eventually, too.

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]