From mboxrd@z Thu Jan 1 00:00:00 1970 From: =?UTF-8?Q?G=C3=A1bor_Boskovits?= Subject: Re: GSoC: Adding a web interface similar to the Hydra web interface Date: Wed, 13 Jun 2018 08:39:58 +0200 Message-ID: References: <87vac3twbe.fsf@gnu.org> <87o9hog2ye.fsf@elephly.net> <87d0xyn9zs.fsf@elephly.net> <87d0xswvls.fsf@elephly.net> <87r2m4ntk4.fsf@mdc-berlin.de> <87tvqxy4i9.fsf@elephly.net> <87in78hxo2.fsf@elephly.net> <878t7xb58o.fsf@elephly.net> <874lijbqvf.fsf@elephly.net> <20180606200210.7a9c4dd6@scratchpost.org> <20180612183504.2621cefa@scratchpost.org> <8736xrd64y.fsf@elephly.net> Mime-Version: 1.0 Content-Type: multipart/alternative; boundary="0000000000006f7201056e8040af" Return-path: Received: from eggs.gnu.org ([2001:4830:134:3::10]:59934) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1fSzRp-0007wI-UP for guix-devel@gnu.org; Wed, 13 Jun 2018 02:40:03 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1fSzRo-0005lv-F7 for guix-devel@gnu.org; Wed, 13 Jun 2018 02:40:01 -0400 Received: from mail-io0-x230.google.com ([2607:f8b0:4001:c06::230]:34393) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1fSzRo-0005ll-6i for guix-devel@gnu.org; Wed, 13 Jun 2018 02:40:00 -0400 Received: by mail-io0-x230.google.com with SMTP id e15-v6so2303566iog.1 for ; Tue, 12 Jun 2018 23:40:00 -0700 (PDT) In-Reply-To: List-Id: "Development of GNU Guix and the GNU System distribution." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: guix-devel-bounces+gcggd-guix-devel=m.gmane.org@gnu.org Sender: "Guix-devel" To: Tatiana Sholokhova Cc: guix-devel --0000000000006f7201056e8040af Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable 2018-06-13 0:43 GMT+02:00 Tatiana Sholokhova : > Hello, > > Thank you for your reviews! > > I've just fixed codestyle issues and replaced HTML5 preamble with XHTML. > > > 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 t= he >> filesystem. Let's rather have a static list of permissible names and al= low >> those (whitelist). That's the intention of the check anyway, right? >> > > I adopted the static file serving procedure from code shared by Ricardo. > > https://github.com/BIMSBbioinfo/rcas-web/blob/ > 22a114a0f281845117ed0ab105267f132fc525e4/rcas/web/render.scm#L68 > > I am considering the following possible implementation of a whitelist. We > can create association list with allowed file names and their mime types = (to > replace file-mime-type list). What do you think about it? > > Also, in light of an ever-changing backing store (cuirass continusly >> evaluates things), the way you are doing pagination is not the correct w= ay >> to do it because the data set will scroll underneath you and you will mi= ss >> 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 ove= r >> and over again and the transaction isolation level is too low for the DB= MS >> to be able to do anything about it[1]. > > > I understand how to fix the pagination on the database level. However, it > is not completely clear to me how to incorporate the solution in the > web-request handling. Now we have the only one parameter of the request > which is related to pagination. It is the page number. > Actually one parameter is still enough, but it will be the last item on the previous page. This style of pagination supports sequential visiting of pages from the first page. It is later possible to extend on this for example by interpolating on a counter, but on the first run this also needs only one parameter... > Should we add more parameters to the request in order to > request evaluations with specific timestamps and IDs? Or there is some > other way of doing that? I have checked the Hydra pagination request > structure. It uses the same form of the request path for pagination butto= ns > that we have now: "?page=3D". > > It is good practise to make small commits, one for every set of >> logically connected changes. This makes the review simpler and it makes >> it easier to merge some parts while leaving others for later. > > > Okay, I will follow this commit strategy. > > Best regards, > Tatiana > > > > 2018-06-13 0:52 GMT+03:00 Ricardo Wurmus : > >> >> Hi Tatiana, >> >> I have only little to add to Danny=E2=80=99s 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) >> >> It is good practise to make small commits, one for every set of >> logically connected changes. This makes the review simpler and it makes >> it easier to merge some parts while leaving others for later. >> >> As you work on the changes that Danny=E2=80=99s comments imply, please t= ake the >> opportunity to group related changes and commit only those together. It >> is fine and desirable to have many independent small commits. >> >> Thanks again for your excellent work! >> >> -- >> Ricardo >> >> > --0000000000006f7201056e8040af Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
2018= -06-13 0:43 GMT+02:00 Tatiana Sholokhova <tanja201396@gmail.com>= ;:
Hello,
Thank you for your reviews!

I've j= ust fixed codestyle=C2=A0issues and replaced HTML5 preamble with XHTML.


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

I a= dopted the static file serving procedure from code shared by Ricardo.=C2=A0=

https://github= .com/BIMSBbioinfo/rcas-web/blob/22a114a0f281845117ed0ab105267f132fc525e4/rcas/web/render.scm#L68

I am considering the following=C2=A0possible implementation of a whitelist. We can create association= list with allowed file names and their mime types (to replace=C2= =A0file-mime-type= =C2=A0list). What do you think about it?

Also, in light of an ever-changing backing store (cuirass = continusly evaluates things), the way you are doing pagination is not the c= orrect way to do it because the data set will scroll underneath you and you= will miss items (or see duplicate items) as an user.=C2=A0 Also, it's = slow and the DBMS can't reuse anything because you are cutting it off a= nd 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].

I understand how to fix the pagination on the database level. = However, it is not completely clear to me how to incorporate the solution i= n the web-request handling. Now we have the only one parameter=C2=A0of the = request which is related to pagination. It is the page number.
=

Actually one parameter is still enou= gh, but it will be the last item on the previous page. This style of pagina= tion supports sequential visiting of pages from the first
page. I= t is later possible to extend on this for example by interpolating on a cou= nter, but on the first run this also needs only one parameter...=C2=A0
=C2=A0
Should we add more parameters to the request in order= to request=C2=A0evaluations with specific timestamps and IDs? Or there is = some other way of doing that? I have checked the Hydra pagination request s= tructure. It uses the same form of the request path for pagination buttons = that we have now:=C2=A0"?page= =3D<page-id>".=C2=A0

It i= s good practise to make small commi= ts, one for every set of
logical= ly connected changes.=C2=A0 This makes the review simpler and it makes
<= /span>it easier to merge some parts while leaving others= for later.

Okay,=C2= =A0I will follow this commit strategy.=C2=A0=C2=A0

Best regards,
Tatiana
<= div>



2018-06-13 0:52 GMT+03:= 00 Ricardo Wurmus <rekado@elephly.net>:

Hi Tatiana,

I have only little to add to Danny=E2=80=99s 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 directl= y
> cherry-pick it.=C2=A0 (not so bad, but somewhat cumbersome now)

It is good practise to make small commits, one for every set of
logically connected changes.=C2=A0 This makes the review simpler and it mak= es
it easier to merge some parts while leaving others for later.

As you work on the changes that Danny=E2=80=99s comments imply, please take= the
opportunity to group related changes and commit only those together.=C2=A0 = It
is fine and desirable to have many independent small commits.

Thanks again for your excellent work!

--
Ricardo



--0000000000006f7201056e8040af--