Re: [PATCH] Support conjunctive keyword search in RPC interface
Okay, I was totally mistaken - `$keyword_string` is only ever sanitized in
the right place (name/name-desc) in master. Yeah, I can rework this to be a
bit cleaner as you recommended -- no problem. ++ on this, will be included
in next rev.
On Sun, Jul 5, 2020 at 11:51 AM Kevin Morris wrote:
> Do name and name-desc now behave exactly the same (see below)? If so,
>> this change in behavior should be documented at least. I would have
>> expected some more refactoring in construct_keyword_search() and one
>> additional parameter being passed here though.
>>
>> Should we include pkgfuncs.inc.php from aurjson.class.php now? How does
>> this currently get imported?
>>
>> Also, since the code for both name and name-desc are now very similar,
>> can we refactor and share most of the code instead of copy pasting? To
>> this end, it might be easier to switch the blocks (i.e. check for API
>> version first, then check for request type). That would allow us to
>> reuse the same assignment to $keyword_string as before and possibly the
>> same construct_keyword_search() invocation too.
>>
>
> I noticed an issue with pre-sanitizing $keyword_string as per `master` --
> when searching by maintainer in rpc, a WHERE Username = $keyword_string is
> used without wildcards (%...%). As far as I know, wildcards should only be
> used in a LIKE expression? Could be wrong, but that's what I thought when
> originally modifying `aurjson.inc.php`'s `function search`. Thoughts?
>
> On Sun, Jul 5, 2020 at 6:33 AM Lukas Fleischer
> wrote:
>
>> On Sun, 05 Jul 2020 at 01:13:25, Kevin Morris wrote:
>> > Alright, patch sent; I used `-v1` this time with `git send-email`... I
>> > can't find documentation for that switch yet. I've tested basic search
>> > through both paths; the only refactoring that needed to be done was to
>> > remove the extra "AND (" and ")" from the generic function, and add it
>> > where we need it in `pkg_search_page`. Then, we can reuse the same
>> > `construct_keyword_search` in rpc.
>>
>> Thanks for the revision! I added some comments inline.
>>
>> It's common to not use any versioning for the first revision of a patch
>> (e.g. send without -v1) but then start using (-v2, -v3, ...) for
>> followup revisions. The -v argument is documented in the
>> git-format-patch(1) man page, if you are using git-format-patch and
>> git-send-email separately, you need to specify the argument when
>> formatting the patch (however, in most cases, you can also run `git
>> format patch` directly).
>>
>
>
> --
> Kevin Morris
> Software Developer
>
> Personal Inquiries: kevr.gt...@gmail.com
> Personal Phone: (415) 583-9687
>
> Technologies: C, C++, Python, Django, Ruby, Rails, ReactJS, jQuery,
> Javascript, SQL, Redux
>
--
Kevin Morris
Software Developer
Personal Inquiries: kevr.gt...@gmail.com
Personal Phone: (415) 583-9687
Technologies: C, C++, Python, Django, Ruby, Rails, ReactJS, jQuery,
Javascript, SQL, Redux
Re: [PATCH] Support conjunctive keyword search in RPC interface
>
> Same here, do we need the space before the closing parentheses? More
> importantly though, do we want to add a space after the parenthesis
> instead to match the previous behavior of SQL strings always ending with
> a space, so we can immediately append without a space?
>
This one can't hurt at all, the only thing it'd do is make
construct_keyword_search more flexible. ++ on this, change will be in the
next rev.
On Sun, Jul 5, 2020 at 11:51 AM Kevin Morris wrote:
> Do name and name-desc now behave exactly the same (see below)? If so,
>> this change in behavior should be documented at least. I would have
>> expected some more refactoring in construct_keyword_search() and one
>> additional parameter being passed here though.
>>
>> Should we include pkgfuncs.inc.php from aurjson.class.php now? How does
>> this currently get imported?
>>
>> Also, since the code for both name and name-desc are now very similar,
>> can we refactor and share most of the code instead of copy pasting? To
>> this end, it might be easier to switch the blocks (i.e. check for API
>> version first, then check for request type). That would allow us to
>> reuse the same assignment to $keyword_string as before and possibly the
>> same construct_keyword_search() invocation too.
>>
>
> I noticed an issue with pre-sanitizing $keyword_string as per `master` --
> when searching by maintainer in rpc, a WHERE Username = $keyword_string is
> used without wildcards (%...%). As far as I know, wildcards should only be
> used in a LIKE expression? Could be wrong, but that's what I thought when
> originally modifying `aurjson.inc.php`'s `function search`. Thoughts?
>
> On Sun, Jul 5, 2020 at 6:33 AM Lukas Fleischer
> wrote:
>
>> On Sun, 05 Jul 2020 at 01:13:25, Kevin Morris wrote:
>> > Alright, patch sent; I used `-v1` this time with `git send-email`... I
>> > can't find documentation for that switch yet. I've tested basic search
>> > through both paths; the only refactoring that needed to be done was to
>> > remove the extra "AND (" and ")" from the generic function, and add it
>> > where we need it in `pkg_search_page`. Then, we can reuse the same
>> > `construct_keyword_search` in rpc.
>>
>> Thanks for the revision! I added some comments inline.
>>
>> It's common to not use any versioning for the first revision of a patch
>> (e.g. send without -v1) but then start using (-v2, -v3, ...) for
>> followup revisions. The -v argument is documented in the
>> git-format-patch(1) man page, if you are using git-format-patch and
>> git-send-email separately, you need to specify the argument when
>> formatting the patch (however, in most cases, you can also run `git
>> format patch` directly).
>>
>
>
> --
> Kevin Morris
> Software Developer
>
> Personal Inquiries: kevr.gt...@gmail.com
> Personal Phone: (415) 583-9687
>
> Technologies: C, C++, Python, Django, Ruby, Rails, ReactJS, jQuery,
> Javascript, SQL, Redux
>
--
Kevin Morris
Software Developer
Personal Inquiries: kevr.gt...@gmail.com
Personal Phone: (415) 583-9687
Technologies: C, C++, Python, Django, Ruby, Rails, ReactJS, jQuery,
Javascript, SQL, Redux
Re: [PATCH] Support conjunctive keyword search in RPC interface
>
> Do name and name-desc now behave exactly the same (see below)? If so,
> this change in behavior should be documented at least. I would have
> expected some more refactoring in construct_keyword_search() and one
> additional parameter being passed here though.
>
> Should we include pkgfuncs.inc.php from aurjson.class.php now? How does
> this currently get imported?
>
> Also, since the code for both name and name-desc are now very similar,
> can we refactor and share most of the code instead of copy pasting? To
> this end, it might be easier to switch the blocks (i.e. check for API
> version first, then check for request type). That would allow us to
> reuse the same assignment to $keyword_string as before and possibly the
> same construct_keyword_search() invocation too.
>
I noticed an issue with pre-sanitizing $keyword_string as per `master` --
when searching by maintainer in rpc, a WHERE Username = $keyword_string is
used without wildcards (%...%). As far as I know, wildcards should only be
used in a LIKE expression? Could be wrong, but that's what I thought when
originally modifying `aurjson.inc.php`'s `function search`. Thoughts?
On Sun, Jul 5, 2020 at 6:33 AM Lukas Fleischer
wrote:
> On Sun, 05 Jul 2020 at 01:13:25, Kevin Morris wrote:
> > Alright, patch sent; I used `-v1` this time with `git send-email`... I
> > can't find documentation for that switch yet. I've tested basic search
> > through both paths; the only refactoring that needed to be done was to
> > remove the extra "AND (" and ")" from the generic function, and add it
> > where we need it in `pkg_search_page`. Then, we can reuse the same
> > `construct_keyword_search` in rpc.
>
> Thanks for the revision! I added some comments inline.
>
> It's common to not use any versioning for the first revision of a patch
> (e.g. send without -v1) but then start using (-v2, -v3, ...) for
> followup revisions. The -v argument is documented in the
> git-format-patch(1) man page, if you are using git-format-patch and
> git-send-email separately, you need to specify the argument when
> formatting the patch (however, in most cases, you can also run `git
> format patch` directly).
>
--
Kevin Morris
Software Developer
Personal Inquiries: kevr.gt...@gmail.com
Personal Phone: (415) 583-9687
Technologies: C, C++, Python, Django, Ruby, Rails, ReactJS, jQuery,
Javascript, SQL, Redux
Re: [PATCH] Support conjunctive keyword search in RPC interface
On Sun, 05 Jul 2020 at 01:13:25, Kevin Morris wrote:
> Alright, patch sent; I used `-v1` this time with `git send-email`... I
> can't find documentation for that switch yet. I've tested basic search
> through both paths; the only refactoring that needed to be done was to
> remove the extra "AND (" and ")" from the generic function, and add it
> where we need it in `pkg_search_page`. Then, we can reuse the same
> `construct_keyword_search` in rpc.
Thanks for the revision! I added some comments inline.
It's common to not use any versioning for the first revision of a patch
(e.g. send without -v1) but then start using (-v2, -v3, ...) for
followup revisions. The -v argument is documented in the
git-format-patch(1) man page, if you are using git-format-patch and
git-send-email separately, you need to specify the argument when
formatting the patch (however, in most cases, you can also run `git
format patch` directly).
Re: [PATCH] Support conjunctive keyword search in RPC interface
On Sun, 05 Jul 2020 at 01:10:07, Kevin Morris wrote:
> Newly supported API Version 6 modifies `type=search` functionality; it
> now behaves the same as `name` or `name-desc` search through the
> https://aur.archlinux.org/packages/ search page.
>
> Search for packages containing the literal keyword `blah blah` AND `haha`:
> https://aur.archlinux.org/rpc/?v=6=search="blah blah"%20haha
>
> Search for packages containing the literal keyword `abc 123`:
> https://aur.archlinux.org/rpc/?v=6=search="abc 123"
>
> The following example searches for packages that contain `blah` AND `abc`:
> https://aur.archlinux.org/rpc/?v=6=search=blah%20abc
>
> The legacy method still searches for packages that contain `blah abc`:
> https://aur.archlinux.org/rpc/?v=5=search=blah%20abc
> https://aur.archlinux.org/rpc/?v=5=search=blah%20abc
>
> API Version 6 is currently only considered during a `search` of `name` or
> `name-desc`.
>
> Note: This change was written as a solution to
> https://bugs.archlinux.org/task/49133.
>
> PS: + Some spacing issues fixed in comments.
>
> Signed-off-by: Kevin Morris
> ---
> doc/rpc.txt | 4
> web/lib/aurjson.class.php | 29 +
> web/lib/pkgfuncs.inc.php | 10 +-
> 3 files changed, 30 insertions(+), 13 deletions(-)
> [...]
> @@ -492,13 +492,26 @@ class AurJSON {
> if (strlen($keyword_string) < 2) {
> return $this->json_error('Query arg too
> small.');
> }
> - $keyword_string = $this->dbh->quote("%" .
> addcslashes($keyword_string, '%_') . "%");
>
> if ($search_by === 'name') {
> - $where_condition = "(Packages.Name LIKE
> $keyword_string)";
> + if ($this->version >= 6) {
> + $where_condition =
> construct_keyword_search($this->dbh,
> + $keyword_string, false);
Do name and name-desc now behave exactly the same (see below)? If so,
this change in behavior should be documented at least. I would have
expected some more refactoring in construct_keyword_search() and one
additional parameter being passed here though.
Should we include pkgfuncs.inc.php from aurjson.class.php now? How does
this currently get imported?
Also, since the code for both name and name-desc are now very similar,
can we refactor and share most of the code instead of copy pasting? To
this end, it might be easier to switch the blocks (i.e. check for API
version first, then check for request type). That would allow us to
reuse the same assignment to $keyword_string as before and possibly the
same construct_keyword_search() invocation too.
> + } else {
> + $keyword_string = $this->dbh->quote(
> + "%" .
> addcslashes($keyword_string, '%_') . "%");
> + $where_condition = "(Packages.Name
> LIKE $keyword_string)";
> + }
> } else if ($search_by === 'name-desc') {
> - $where_condition = "(Packages.Name LIKE
> $keyword_string OR ";
> - $where_condition .= "Description LIKE
> $keyword_string)";
> + if ($this->version >= 6) {
> + $where_condition =
> construct_keyword_search($this->dbh,
> + $keyword_string, true);
See above.
> + } else {
> + $keyword_string = $this->dbh->quote(
> + "%" .
> addcslashes($keyword_string, '%_') . "%");
> + $where_condition = "(Packages.Name
> LIKE $keyword_string ";
> + $where_condition .= "OR Description
> LIKE $keyword_string)";
> + }
> }
> } else if ($search_by === 'maintainer') {
> if (empty($keyword_string)) {
> diff --git a/web/lib/pkgfuncs.inc.php b/web/lib/pkgfuncs.inc.php
> index 8c915711..f6108e5a 100644
> --- a/web/lib/pkgfuncs.inc.php
> +++ b/web/lib/pkgfuncs.inc.php
> @@ -697,7 +697,9 @@ function pkg_search_page($params, $show_headers=true,
> $SID="") {
> }
> elseif (isset($params["SeB"]) && $params["SeB"] == "k") {
> /* Search by keywords. */
> + $q_where .= " AND ( ";
Do we need the space at the end?
> $q_where .= construct_keyword_search($dbh,
> $params['K'], false);
> + $q_where .= " )";
Same here, do we need the space before the closing parentheses? More
importantly
Re: [PATCH] Support conjunctive keyword search in RPC interface
Alright, patch sent; I used `-v1` this time with `git send-email`... I
can't find documentation for that switch yet. I've tested basic search
through both paths; the only refactoring that needed to be done was to
remove the extra "AND (" and ")" from the generic function, and add it
where we need it in `pkg_search_page`. Then, we can reuse the same
`construct_keyword_search` in rpc.
