RPC: Allow to search packages by "*depends" fields
diff mbox

Message ID 20180128212734.19088-1-baptiste@bitsofnetworks.org
State Superseded, archived
Headers show

Commit Message

Baptiste Jonglez Jan. 28, 2018, 9:27 p.m. UTC
From: Baptiste Jonglez <git@bitsofnetworks.org>

It is now possible to search for packages that depend on a given package,
for instance:

    /rpc/?v=5&type=search&by=depends&arg=ocaml

It is similarly possible to match on "makedepends", "checkdepends" and
"optdepends".

Signed-off-by: Baptiste Jonglez <git@bitsofnetworks.org>
---
 doc/rpc.txt               |  8 +++++++-
 web/lib/aurjson.class.php | 33 +++++++++++++++++++++++++++++----
 2 files changed, 36 insertions(+), 5 deletions(-)

Comments

Lukas Fleischer Jan. 29, 2018, 6:47 p.m. UTC | #1
On Sun, 28 Jan 2018 at 22:27:34, Baptiste Jonglez wrote:
> From: Baptiste Jonglez <git@bitsofnetworks.org>
> 
> It is now possible to search for packages that depend on a given package,
> for instance:
> 
>     /rpc/?v=5&type=search&by=depends&arg=ocaml
> 
> It is similarly possible to match on "makedepends", "checkdepends" and
> "optdepends".
> 
> Signed-off-by: Baptiste Jonglez <git@bitsofnetworks.org>
> ---
>  doc/rpc.txt               |  8 +++++++-
>  web/lib/aurjson.class.php | 33 +++++++++++++++++++++++++++++----
>  2 files changed, 36 insertions(+), 5 deletions(-)
> 
> diff --git a/doc/rpc.txt b/doc/rpc.txt
> index f353ff0..3148ebe 100644
> --- a/doc/rpc.txt
> +++ b/doc/rpc.txt
> @@ -11,6 +11,10 @@ search argument and _field_ is one of the following values:
>  * `name` (search by package name only)
>  * `name-desc` (search by package name and description)
>  * `maintainer` (search by package maintainer)
> +* `depends` (search for packages that depend on _keywords_)
> +* `makedepends` (search for packages that makedepend on _keywords_)
> +* `optdepends` (search for packages that optdepend on _keywords_)
> +* `checkdepends` (search for packages that checkdepend on _keywords_)

Great!

> [...]
> diff --git a/web/lib/aurjson.class.php b/web/lib/aurjson.class.php
> index 9eeaafd..30bdc89 100644
> --- a/web/lib/aurjson.class.php
> +++ b/web/lib/aurjson.class.php
> @@ -17,7 +17,8 @@ class AurJSON {
>                 'suggest-pkgbase', 'get-comment-form'
>         );
>         private static $exposed_fields = array(
> -               'name', 'name-desc', 'maintainer'
> +               'name', 'name-desc', 'maintainer',
> +               'depends', 'makedepends', 'checkdepends', 'optdepends'

Just another idea: maybe we can put the new fields in a separate array
$exposed_depfields and merge that array into $exposed_fields upon
initialization. Then we would not need to hardcode them again in the
in_array() check below.

>         );
>         private static $fields_v1 = array(
>                 'Packages.ID', 'Packages.Name',
> @@ -243,16 +244,27 @@ class AurJSON {
>  
>         /*
>          * Retrieve package information (used in info, multiinfo, search and
> -        * msearch requests).
> +        * depends requests).
>          *
>          * @param $type The request type.
>          * @param $where_condition An SQL WHERE-condition to filter packages.
> +        * @param $join_depends Whether to add a SQL JOIN on the PackageDepends table.
> +        *        It will produce duplicate packages unless $where_condition filters
> +        *        the result appropriately.
>          *
>          * @return mixed Returns an array of package matches.
>          */
> -       private function process_query($type, $where_condition) {
> +       private function process_query($type, $where_condition, $join_depends=false) {
>                 $max_results = config_get_int('options', 'max_rpc_results');
>  
> +               $additional_joins = "";
> +               if ($join_depends) {
> +                       $additional_joins .= "LEFT JOIN PackageDepends " .
> +                                       "ON Packages.ID = PackageDepends.PackageID " .
> +                                       "LEFT JOIN DependencyTypes " .
> +                                       "ON PackageDepends.DepTypeID = DependencyTypes.ID";
> +               }
> +
>                 if ($this->version == 1) {
>                         $fields = implode(',', self::$fields_v1);
>                         $query = "SELECT {$fields} " .
> @@ -264,6 +276,7 @@ class AurJSON {
>                                 "ON PackageLicenses.PackageID = Packages.ID " .
>                                 "LEFT JOIN Licenses " .
>                                 "ON Licenses.ID = PackageLicenses.LicenseID " .
> +                               "${additional_joins} " .
>                                 "WHERE ${where_condition} " .
>                                 "AND PackageBases.PackagerUID IS NOT NULL " .
>                                 "LIMIT $max_results";
> @@ -278,6 +291,7 @@ class AurJSON {
>                                 "ON PackageBases.ID = Packages.PackageBaseID " .
>                                 "LEFT JOIN Users " .
>                                 "ON PackageBases.MaintainerUID = Users.ID " .
> +                               "${additional_joins} " .
>                                 "WHERE ${where_condition} " .
>                                 "AND PackageBases.PackagerUID IS NOT NULL " .
>                                 "LIMIT $max_results";
> @@ -380,6 +394,7 @@ class AurJSON {
>          * @return mixed Returns an array of package matches.
>          */
>         private function search($http_data) {
> +               $join_depends = false;
>                 $keyword_string = $http_data['arg'];
>  
>                 if (isset($http_data['by'])) {
> @@ -407,9 +422,19 @@ class AurJSON {
>                                 $keyword_string = $this->dbh->quote($keyword_string);
>                                 $where_condition = "Users.Username = $keyword_string ";
>                         }
> +               } else if (in_array($search_by, ['depends', 'makedepends', 'checkdepends', 'optdepends'])) {
> +                       if (empty($keyword_string)) {
> +                               return $this->json_error('Query arg is empty.');
> +                       } else {
> +                               $keyword_string = $this->dbh->quote($keyword_string);
> +                               $search_string = $this->dbh->quote($search_by);
> +                               $where_condition = "PackageDepends.DepName = $keyword_string AND ";
> +                               $where_condition .= "DependencyTypes.Name = $search_string";
> [...]

This is a bit unfortunate, indeed. Can we, instead of doing this, use a
subselect? Something like the following statement might work:

    $keyword_string IN (SELECT PackageDepends.DepName FROM PackageDepends
                        LEFT JOIN DependencyTypes
                        ON PackageDepends.DepTypeID = DependencyTypes.ID
                        WHERE PackageDepends.PackageID = Packages.ID
                        AND DependencyTypes.Name = $search_string)

Note that this is fully untested, though.

Best regards,
Lukas
Baptiste Jonglez Jan. 31, 2018, 8 p.m. UTC | #2
Thanks for the new review!  I have sent a v2 PATCH, comments inline below.

On 29-01-18, Lukas Fleischer wrote:
> > diff --git a/web/lib/aurjson.class.php b/web/lib/aurjson.class.php
> > index 9eeaafd..30bdc89 100644
> > --- a/web/lib/aurjson.class.php
> > +++ b/web/lib/aurjson.class.php
> > @@ -17,7 +17,8 @@ class AurJSON {
> >                 'suggest-pkgbase', 'get-comment-form'
> >         );
> >         private static $exposed_fields = array(
> > -               'name', 'name-desc', 'maintainer'
> > +               'name', 'name-desc', 'maintainer',
> > +               'depends', 'makedepends', 'checkdepends', 'optdepends'
> 
> Just another idea: maybe we can put the new fields in a separate array
> $exposed_depfields and merge that array into $exposed_fields upon
> initialization. Then we would not need to hardcode them again in the
> in_array() check below.

I have moved the new fields to a variable $exposed_depfields, it indeed
makes things slightly easier to read.  However, I think that merging the
two arrays later in the code would be rather confusing for the reader
(because at first glance, the dependency fields would seem to be absent
from $exposed_fields).

> This is a bit unfortunate, indeed. Can we, instead of doing this, use a
> subselect? Something like the following statement might work:
> 
>     $keyword_string IN (SELECT PackageDepends.DepName FROM PackageDepends
>                         LEFT JOIN DependencyTypes
>                         ON PackageDepends.DepTypeID = DependencyTypes.ID
>                         WHERE PackageDepends.PackageID = Packages.ID
>                         AND DependencyTypes.Name = $search_string)

Excellent idea, and it even worked on the first try!  It simplifies the
code a lot, and performance-wise it should be roughly similar to a JOIN,
possibly a bit slower (from what I could read on various stack overflow
posts).

Baptiste

Patch
diff mbox

diff --git a/doc/rpc.txt b/doc/rpc.txt
index f353ff0..3148ebe 100644
--- a/doc/rpc.txt
+++ b/doc/rpc.txt
@@ -11,6 +11,10 @@  search argument and _field_ is one of the following values:
 * `name` (search by package name only)
 * `name-desc` (search by package name and description)
 * `maintainer` (search by package maintainer)
+* `depends` (search for packages that depend on _keywords_)
+* `makedepends` (search for packages that makedepend on _keywords_)
+* `optdepends` (search for packages that optdepend on _keywords_)
+* `checkdepends` (search for packages that checkdepend on _keywords_)
 
 The _by_ parameter can be skipped and defaults to `name-desc`.
 
@@ -30,7 +34,9 @@  Examples
 `search`::
   `/rpc/?v=5&type=search&arg=foobar`
 `search` by maintainer::
-  `/rpc/?v=5&type=search&search_by=maintainer&arg=john`
+  `/rpc/?v=5&type=search&by=maintainer&arg=john`
+`search` packages that have _boost_ as `makedepends`::
+  `/rpc/?v=5&type=search&by=makedepends&arg=boost`
 `search` with callback::
   `/rpc/?v=5&type=search&arg=foobar&callback=jsonp1192244621103`
 `info`::
diff --git a/web/lib/aurjson.class.php b/web/lib/aurjson.class.php
index 9eeaafd..30bdc89 100644
--- a/web/lib/aurjson.class.php
+++ b/web/lib/aurjson.class.php
@@ -17,7 +17,8 @@  class AurJSON {
 		'suggest-pkgbase', 'get-comment-form'
 	);
 	private static $exposed_fields = array(
-		'name', 'name-desc', 'maintainer'
+		'name', 'name-desc', 'maintainer',
+		'depends', 'makedepends', 'checkdepends', 'optdepends'
 	);
 	private static $fields_v1 = array(
 		'Packages.ID', 'Packages.Name',
@@ -243,16 +244,27 @@  class AurJSON {
 
 	/*
 	 * Retrieve package information (used in info, multiinfo, search and
-	 * msearch requests).
+	 * depends requests).
 	 *
 	 * @param $type The request type.
 	 * @param $where_condition An SQL WHERE-condition to filter packages.
+	 * @param $join_depends Whether to add a SQL JOIN on the PackageDepends table.
+	 *        It will produce duplicate packages unless $where_condition filters
+	 *        the result appropriately.
 	 *
 	 * @return mixed Returns an array of package matches.
 	 */
-	private function process_query($type, $where_condition) {
+	private function process_query($type, $where_condition, $join_depends=false) {
 		$max_results = config_get_int('options', 'max_rpc_results');
 
+		$additional_joins = "";
+		if ($join_depends) {
+			$additional_joins .= "LEFT JOIN PackageDepends " .
+					"ON Packages.ID = PackageDepends.PackageID " .
+					"LEFT JOIN DependencyTypes " .
+					"ON PackageDepends.DepTypeID = DependencyTypes.ID";
+		}
+
 		if ($this->version == 1) {
 			$fields = implode(',', self::$fields_v1);
 			$query = "SELECT {$fields} " .
@@ -264,6 +276,7 @@  class AurJSON {
 				"ON PackageLicenses.PackageID = Packages.ID " .
 				"LEFT JOIN Licenses " .
 				"ON Licenses.ID = PackageLicenses.LicenseID " .
+				"${additional_joins} " .
 				"WHERE ${where_condition} " .
 				"AND PackageBases.PackagerUID IS NOT NULL " .
 				"LIMIT $max_results";
@@ -278,6 +291,7 @@  class AurJSON {
 				"ON PackageBases.ID = Packages.PackageBaseID " .
 				"LEFT JOIN Users " .
 				"ON PackageBases.MaintainerUID = Users.ID " .
+				"${additional_joins} " .
 				"WHERE ${where_condition} " .
 				"AND PackageBases.PackagerUID IS NOT NULL " .
 				"LIMIT $max_results";
@@ -380,6 +394,7 @@  class AurJSON {
 	 * @return mixed Returns an array of package matches.
 	 */
 	private function search($http_data) {
+		$join_depends = false;
 		$keyword_string = $http_data['arg'];
 
 		if (isset($http_data['by'])) {
@@ -407,9 +422,19 @@  class AurJSON {
 				$keyword_string = $this->dbh->quote($keyword_string);
 				$where_condition = "Users.Username = $keyword_string ";
 			}
+		} else if (in_array($search_by, ['depends', 'makedepends', 'checkdepends', 'optdepends'])) {
+			if (empty($keyword_string)) {
+				return $this->json_error('Query arg is empty.');
+			} else {
+				$keyword_string = $this->dbh->quote($keyword_string);
+				$search_string = $this->dbh->quote($search_by);
+				$where_condition = "PackageDepends.DepName = $keyword_string AND ";
+				$where_condition .= "DependencyTypes.Name = $search_string";
+				$join_depends = true;
+			}
 		}
 
-		return $this->process_query('search', $where_condition);
+		return $this->process_query('search', $where_condition, $join_depends);
 	}
 
 	/*