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

[Danny Milosavljevic]

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.

pgpxQIFDhd50c.pgp
Description: OpenPGP digital signature