| Message ID | 20180601025055.6938-1-afg984@gmail.com |
|---|---|
| State | New |
| Headers | show |
Hi, this is a WIP patch following up the one at March: [PATCH] feat(rpc): return "providers" packages when querying by `name` or `name-desc` (I failed to make git-send-email to reply to the thread) The goal is to allow tools to use the RPC to resolve dependencies reliably, by introducing a new version of the RPC (v6): 1. Support querying by the provides field 2. Support querying with multiple _by_, for example, by[]=name&by[]=provides The design pretty much follows the one discussed (in the thread mentioned above) previously, I can bring up more context if needed. Here's a draft document of how it works: https://github.com/afg984/aurweb/wiki/aurweb-RPC-Interface-(v6-draft) This is a WIP patch, and I'd like to get some feedback before moving on. There are a few things worth noting: 1. commit 1ff40987 implemented search by depends, checkdepends, optdepends. They are currently left out. 2. v5 allowed searching for orphan packages by leaving out the arg= argument. This is left out as well. 3. as search and info is now a similar concept (and a little different than those in v5), I've implemented them in a separate function. I'd like to know if this is the way to go, or I should try to reuse the existing structure. 2018-06-01 10:50 GMT+08:00 Li-Yu Yu <afg984@gmail.com>: > --- > web/lib/aurjson.class.php | 125 ++++++++++++++++++++++++++++++++++++-- > 1 file changed, 120 insertions(+), 5 deletions(-) > > diff --git a/web/lib/aurjson.class.php b/web/lib/aurjson.class.php > index c51e9c2..db11117 100644 > --- a/web/lib/aurjson.class.php > +++ b/web/lib/aurjson.class.php > @@ -20,6 +20,14 @@ class AurJSON { > 'name', 'name-desc', 'maintainer', > 'depends', 'makedepends', 'checkdepends', 'optdepends' > ); > + private static $exposed_fields_v6 = array( > + 'name', 'description', 'maintainer', 'provides', > + ); > + private static $exposed_fields_map_v6 = array( > + 'name' => 'Packages.Name', > + 'description' => 'Packages.Description', > + 'maintainer' => 'Packages.Maintainer', > + ); > private static $exposed_depfields = array( > 'depends', 'makedepends', 'checkdepends', 'optdepends' > ); > @@ -80,7 +88,7 @@ class AurJSON { > if (isset($http_data['v'])) { > $this->version = intval($http_data['v']); > } > - if ($this->version < 1 || $this->version > 5) { > + if ($this->version < 1 || $this->version > 6) { > return $this->json_error('Invalid version specified.'); > } > > @@ -94,8 +102,21 @@ class AurJSON { > if (isset($http_data['search_by']) && !isset($http_data['by'])) { > $http_data['by'] = $http_data['search_by']; > } > - if (isset($http_data['by']) && !in_array($http_data['by'], self::$exposed_fields)) { > - return $this->json_error('Incorrect by field specified.'); > + if (isset($http_data['by'])) { > + if ($this->version < 6) { > + if (!in_array($http_data['by'], self::$exposed_fields)) { > + return $this->json_error('Incorrect by field specified.'); > + } > + } else { > + if (!is_array($http_data['by'])) { > + $http_data['by'] = array($http_data['by']); > + } > + foreach ($http_data['by'] as $by) { > + if (!in_array($by, self::$exposed_fields_v6)) { > + return $this->json_error("Incorrect by field '$by' specified."); > + } > + } > + } > } > > $this->dbh = DB::connect(); > @@ -109,7 +130,11 @@ class AurJSON { > if ($type == 'info' && $this->version >= 5) { > $type = 'multiinfo'; > } > - $json = call_user_func(array(&$this, $type), $http_data); > + if ($this->version < 6) { > + $json = call_user_func(array(&$this, $type), $http_data); > + } else { > + $json = $this->info_search_v6($type, $http_data); > + } > > $etag = md5($json); > header("Etag: \"$etag\""); > @@ -374,6 +399,21 @@ class AurJSON { > } > $result = $this->dbh->query($query); > > + return $this->process_result($type, $result); > + } > + > + > + /* > + * Retrieve package information from a dbh->query result > + * > + * @param $type The request type. > + * @param $result A dbh->query result. > + * > + * @return mixed Returns an array of package matches. > + */ > + private function process_result($type, $result) { > + $max_results = config_get_int('options', 'max_rpc_results'); > + > if ($result) { > $resultcount = 0; > $search_data = array(); > @@ -515,6 +555,82 @@ class AurJSON { > return $this->process_query('search', $where_condition); > } > > + /* > + * Performs a info or search query to the package database. > + * > + * @param $type The request type. > + * @param array $http_data Query parameters. > + * > + * @return mixed Returns an array of package matches. > + */ > + private function info_search_v6($type, $http_data) { > + if (isset($http_data['by'])) { > + $query_by = $http_data['by']; > + if (!is_array($query_by)) { > + $query_by = array($query_by); > + } > + } else { > + if ($type == "multiinfo") { > + $query_by = array('name'); > + } else { // search > + $query_by = array('name', 'description'); > + } > + } > + > + if ($type == "multiinfo") { > + $args = $http_data['arg']; > + if (!is_array($args)) { > + $args = array($args); > + } > + foreach ($args as $i => $arg) { > + $args[$i] = $this->dbh->quote($arg); > + } > + $op_rhs = " IN (" . implode(",", $args) . ")"; > + } else { > + $keyword_string = $http_data['arg']; > + $keyword_string = $this->dbh->quote("%" . addcslashes($keyword_string, '%_') . "%"); > + $op_rhs = " LIKE " . $keyword_string; > + } > + > + $has_provides_query = false; > + $where_condition = ""; > + foreach ($query_by as $index => $by) { > + if ($index != 0) { > + $where_condition .= " OR "; > + } > + if ($by == "provides") { > + $has_provides_query = true; > + $where_condition .= "(RelationTypes.Name = 'provides' AND "; > + $where_condition .= "PackageRelations.RelName $op_rhs)"; > + } else { > + $where_condition .= self::$exposed_fields_map_v6[$by]; > + $where_condition .= $op_rhs; > + } > + } > + > + $max_results = config_get_int('options', 'max_rpc_results'); > + $fields = implode(',', self::$fields_v4); > + $q = "SELECT {$fields} " . > + "FROM Packages LEFT JOIN PackageBases " . > + "ON PackageBases.ID = Packages.PackageBaseID " . > + "LEFT JOIN Users " . > + "ON PackageBases.MaintainerUID = Users.ID "; > + if ($has_provides_query) { > + $q .= "LEFT JOIN PackageRelations ON PackageRelations.PackageID = Packages.ID "; > + $q .= "LEFT JOIN RelationTypes ON RelationTypes.ID = PackageRelations.RelTypeID "; > + } > + $q .= "WHERE ${where_condition} "; > + $q .= "AND PackageBases.PackagerUID IS NOT NULL "; > + if ($has_provides_query) { > + $q .= "GROUP BY Packages.ID "; > + } > + $q .= "LIMIT $max_results"; > + > + $result = $this->dbh->query($q); > + > + return $this->process_result($type, $result); > + } > + > /* > * Returns the info on a specific package. > * > @@ -680,4 +796,3 @@ class AurJSON { > return json_encode($output); > } > } > - > -- > 2.17.1 >
On Fri, 01 Jun 2018 at 05:24:16, 尤立宇 wrote: > Hi, this is a WIP patch following up the one at March: > [PATCH] feat(rpc): return "providers" packages when querying by `name` > or `name-desc` > (I failed to make git-send-email to reply to the thread) > > The goal is to allow tools to use the RPC to resolve dependencies reliably, > by introducing a new version of the RPC (v6): > 1. Support querying by the provides field > 2. Support querying with multiple _by_, for example, by[]=name&by[]=provides > > The design pretty much follows the one discussed (in the thread > mentioned above) previously, > I can bring up more context if needed. > Here's a draft document of how it works: > https://github.com/afg984/aurweb/wiki/aurweb-RPC-Interface-(v6-draft) The draft looks good to me. I only had a quick look at the implementation and did not spot any obvious problems, apart from some whitespace issues. Will have a more detailed look later. Thanks for working on this! Regards, Lukas
On 5/31/18 11:24 PM, 尤立宇 wrote: > Hi, this is a WIP patch following up the one at March: > [PATCH] feat(rpc): return "providers" packages when querying by `name` > or `name-desc` > (I failed to make git-send-email to reply to the thread) > > The goal is to allow tools to use the RPC to resolve dependencies reliably, > by introducing a new version of the RPC (v6): > 1. Support querying by the provides field > 2. Support querying with multiple _by_, for example, by[]=name&by[]=provides > > The design pretty much follows the one discussed (in the thread > mentioned above) previously, > I can bring up more context if needed. > Here's a draft document of how it works: > https://github.com/afg984/aurweb/wiki/aurweb-RPC-Interface-(v6-draft) > > This is a WIP patch, and I'd like to get some feedback before moving on. > > There are a few things worth noting: > 1. commit 1ff40987 implemented search by depends, checkdepends, > optdepends. They are currently left out. > 2. v5 allowed searching for orphan packages by leaving out the arg= > argument. This is left out as well. > 3. as search and info is now a similar concept (and a little different > than those in v5), I've implemented them in a separate function. I'd > like to know if this is the way to go, or I should try to reuse the > existing structure. Can you update doc/rpc.txt to describe the new version? Looks like the github wiki contains partial work towards this, but it's not in asciidoc format.
On 5/31/18 11:24 PM, 尤立宇 wrote: > Hi, this is a WIP patch following up the one at March: > [PATCH] feat(rpc): return "providers" packages when querying by `name` > or `name-desc` > (I failed to make git-send-email to reply to the thread) > > The goal is to allow tools to use the RPC to resolve dependencies reliably, > by introducing a new version of the RPC (v6): > 1. Support querying by the provides field > 2. Support querying with multiple _by_, for example, by[]=name&by[]=provides > > The design pretty much follows the one discussed (in the thread > mentioned above) previously, > I can bring up more context if needed. > Here's a draft document of how it works: > https://github.com/afg984/aurweb/wiki/aurweb-RPC-Interface-(v6-draft) > > This is a WIP patch, and I'd like to get some feedback before moving on. > > There are a few things worth noting: > 1. commit 1ff40987 implemented search by depends, checkdepends, > optdepends. They are currently left out. This means we're in a funny situation where in order to get depfields searches we would need to still recommend the v5 interface. Could we implement this as well? If not, we should document it... > 2. v5 allowed searching for orphan packages by leaving out the arg= > argument. This is left out as well. > 3. as search and info is now a similar concept (and a little different > than those in v5), I've implemented them in a separate function. I'd > like to know if this is the way to go, or I should try to reuse the > existing structure. > > 2018-06-01 10:50 GMT+08:00 Li-Yu Yu <afg984@gmail.com>: >> --- >> web/lib/aurjson.class.php | 125 ++++++++++++++++++++++++++++++++++++-- >> 1 file changed, 120 insertions(+), 5 deletions(-) >> >> diff --git a/web/lib/aurjson.class.php b/web/lib/aurjson.class.php >> index c51e9c2..db11117 100644 >> --- a/web/lib/aurjson.class.php >> +++ b/web/lib/aurjson.class.php >> @@ -20,6 +20,14 @@ class AurJSON { >> 'name', 'name-desc', 'maintainer', >> 'depends', 'makedepends', 'checkdepends', 'optdepends' >> ); >> + private static $exposed_fields_v6 = array( >> + 'name', 'description', 'maintainer', 'provides', >> + ); >> + private static $exposed_fields_map_v6 = array( >> + 'name' => 'Packages.Name', >> + 'description' => 'Packages.Description', >> + 'maintainer' => 'Packages.Maintainer', >> + ); Why is this pulling indexes from a map? It's only ever used once, and I don't think listing search qualifiers here really helps code clarity vs. a case statement. Moreso when we need to drop the special handling of finding NULL maintainers, and don't do provides here because it doesn't fit into the architecture at all.
Eli Schwartz <eschwartz@archlinux.org>: > Can you update doc/rpc.txt to describe the new version? Looks like the > github wiki contains partial work towards this, but it's not in asciidoc > format. Sure. Eli Schwartz <eschwartz@archlinux.org>: > > > > This is a WIP patch, and I'd like to get some feedback before moving on. > > > > There are a few things worth noting: > > 1. commit 1ff40987 implemented search by depends, checkdepends, > > optdepends. They are currently left out. > > This means we're in a funny situation where in order to get depfields > searches we would need to still recommend the v5 interface. Could we > implement this as well? If not, we should document it... Of course, v6 should absolutely support what is possible now in v5. The previous patch is more about asking if we can agree on the interface. I'll work on to bring the v6 patch up to date with the new rpc methods supported in v5. > > 2. v5 allowed searching for orphan packages by leaving out the arg= > > argument. This is left out as well. > > 3. as search and info is now a similar concept (and a little different > > than those in v5), I've implemented them in a separate function. I'd > > like to know if this is the way to go, or I should try to reuse the > > existing structure. > > > > 2018-06-01 10:50 GMT+08:00 Li-Yu Yu <afg984@gmail.com>: > >> --- > >> web/lib/aurjson.class.php | 125 ++++++++++++++++++++++++++++++++++++-- > >> 1 file changed, 120 insertions(+), 5 deletions(-) > >> > >> diff --git a/web/lib/aurjson.class.php b/web/lib/aurjson.class.php > >> index c51e9c2..db11117 100644 > >> --- a/web/lib/aurjson.class.php > >> +++ b/web/lib/aurjson.class.php > >> @@ -20,6 +20,14 @@ class AurJSON { > >> 'name', 'name-desc', 'maintainer', > >> 'depends', 'makedepends', 'checkdepends', 'optdepends' > >> ); > >> + private static $exposed_fields_v6 = array( > >> + 'name', 'description', 'maintainer', 'provides', > >> + ); > >> + private static $exposed_fields_map_v6 = array( > >> + 'name' => 'Packages.Name', > >> + 'description' => 'Packages.Description', > >> + 'maintainer' => 'Packages.Maintainer', > >> + ); > > Why is this pulling indexes from a map? It's only ever used once, and I > don't think listing search qualifiers here really helps code clarity vs. > a case statement. I'll fix it. > Moreso when we need to drop the special handling of > finding NULL maintainers > and don't do provides here because it doesn't > fit into the architecture at all. I don't quite get it. Do you mean support for searching by NULL maintainers should be dropped?
On 11/6/18 2:07 AM, 尤立宇 wrote: > Eli Schwartz <eschwartz@archlinux.org>: >> Can you update doc/rpc.txt to describe the new version? Looks like the >> github wiki contains partial work towards this, but it's not in asciidoc >> format. > Sure. > > Eli Schwartz <eschwartz@archlinux.org>: >>> >>> This is a WIP patch, and I'd like to get some feedback before moving on. >>> >>> There are a few things worth noting: >>> 1. commit 1ff40987 implemented search by depends, checkdepends, >>> optdepends. They are currently left out. >> >> This means we're in a funny situation where in order to get depfields >> searches we would need to still recommend the v5 interface. Could we >> implement this as well? If not, we should document it... > Of course, v6 should absolutely support what is possible now in v5. > The previous patch is more about asking if we can agree on the interface. > I'll work on to bring the v6 patch up to date with the new rpc methods > supported in v5. Great! Because other than that, the new interface looks reasonable to me too, so I'd like to see this go forward as well. >> Why is this pulling indexes from a map? It's only ever used once, and I >> don't think listing search qualifiers here really helps code clarity vs. >> a case statement. > I'll fix it. > >> Moreso when we need to drop the special handling of >> finding NULL maintainers >> and don't do provides here because it doesn't >> fit into the architecture at all. > I don't quite get it. > Do you mean support for searching by NULL maintainers should be dropped? Sorry, that may have been confusing. I mean that keeping everything in a map over here doesn't make sense when in order to properly handle searching for NULL maintainers and searching for provides, we (I think) need a switch later on anyway. This is in addition to my earlier point about my belief that the reading of the logic flow would benefit from making the different cases all appear in a switch rather than some being in an if statement and some being included via a map lookup.
On 11/6/18 2:07 AM, 尤立宇 wrote: > Of course, v6 should absolutely support what is possible now in v5. > The previous patch is more about asking if we can agree on the interface. > I'll work on to bring the v6 patch up to date with the new rpc methods > supported in v5. Any update?
I'm a bit busy these days, but I think I can have a working patch around the weekend Eli Schwartz <eschwartz@archlinux.org> 於 2018年12月11日 週二 下午10:44寫道： > > On 11/6/18 2:07 AM, 尤立宇 wrote: > > Of course, v6 should absolutely support what is possible now in v5. > > The previous patch is more about asking if we can agree on the interface. > > I'll work on to bring the v6 patch up to date with the new rpc methods > > supported in v5. > > Any update? > > -- > Eli Schwartz > Bug Wrangler and Trusted User >
On Wed, 12 Dec 2018 at 17:13, 尤立宇 <afg984@gmail.com> wrote: > > I'm a bit busy these days, but I think I can have a working patch > around the weekend > > Eli Schwartz <eschwartz@archlinux.org> 於 2018年12月11日 週二 下午10:44寫道： > > > > On 11/6/18 2:07 AM, 尤立宇 wrote: > > > Of course, v6 should absolutely support what is possible now in v5. > > > The previous patch is more about asking if we can agree on the interface. > > > I'll work on to bring the v6 patch up to date with the new rpc methods > > > supported in v5. > > > > Any update? > > > > -- > > Eli Schwartz > > Bug Wrangler and Trusted User > > Long weekend huh ;) Any update on this?
Morgan Adamiec <morganamilo@gmail.com> 於 2019年1月24日 週四 上午5:08寫道： > Long weekend huh ;) > > Any update on this? Unfortunately, I don't have time to update the patch, my limited PHP knowledge does not allow me to work on it casually. If anyone wants to, feel free to take them. I think the code would follow whatever license aurweb currently is. Looks like someone is already working on it though. Probably this can become a ticket in the issue tracker (if there isn't already one) so anyone interested can see the progress easier IMO, instead of digging through the mailing list.
diff --git a/web/lib/aurjson.class.php b/web/lib/aurjson.class.php index c51e9c2..db11117 100644 --- a/web/lib/aurjson.class.php +++ b/web/lib/aurjson.class.php @@ -20,6 +20,14 @@ class AurJSON { 'name', 'name-desc', 'maintainer', 'depends', 'makedepends', 'checkdepends', 'optdepends' ); + private static $exposed_fields_v6 = array( + 'name', 'description', 'maintainer', 'provides', + ); + private static $exposed_fields_map_v6 = array( + 'name' => 'Packages.Name', + 'description' => 'Packages.Description', + 'maintainer' => 'Packages.Maintainer', + ); private static $exposed_depfields = array( 'depends', 'makedepends', 'checkdepends', 'optdepends' ); @@ -80,7 +88,7 @@ class AurJSON { if (isset($http_data['v'])) { $this->version = intval($http_data['v']); } - if ($this->version < 1 || $this->version > 5) { + if ($this->version < 1 || $this->version > 6) { return $this->json_error('Invalid version specified.'); } @@ -94,8 +102,21 @@ class AurJSON { if (isset($http_data['search_by']) && !isset($http_data['by'])) { $http_data['by'] = $http_data['search_by']; } - if (isset($http_data['by']) && !in_array($http_data['by'], self::$exposed_fields)) { - return $this->json_error('Incorrect by field specified.'); + if (isset($http_data['by'])) { + if ($this->version < 6) { + if (!in_array($http_data['by'], self::$exposed_fields)) { + return $this->json_error('Incorrect by field specified.'); + } + } else { + if (!is_array($http_data['by'])) { + $http_data['by'] = array($http_data['by']); + } + foreach ($http_data['by'] as $by) { + if (!in_array($by, self::$exposed_fields_v6)) { + return $this->json_error("Incorrect by field '$by' specified."); + } + } + } } $this->dbh = DB::connect(); @@ -109,7 +130,11 @@ class AurJSON { if ($type == 'info' && $this->version >= 5) { $type = 'multiinfo'; } - $json = call_user_func(array(&$this, $type), $http_data); + if ($this->version < 6) { + $json = call_user_func(array(&$this, $type), $http_data); + } else { + $json = $this->info_search_v6($type, $http_data); + } $etag = md5($json); header("Etag: \"$etag\""); @@ -374,6 +399,21 @@ class AurJSON { } $result = $this->dbh->query($query); + return $this->process_result($type, $result); + } + + + /* + * Retrieve package information from a dbh->query result + * + * @param $type The request type. + * @param $result A dbh->query result. + * + * @return mixed Returns an array of package matches. + */ + private function process_result($type, $result) { + $max_results = config_get_int('options', 'max_rpc_results'); + if ($result) { $resultcount = 0; $search_data = array(); @@ -515,6 +555,82 @@ class AurJSON { return $this->process_query('search', $where_condition); } + /* + * Performs a info or search query to the package database. + * + * @param $type The request type. + * @param array $http_data Query parameters. + * + * @return mixed Returns an array of package matches. + */ + private function info_search_v6($type, $http_data) { + if (isset($http_data['by'])) { + $query_by = $http_data['by']; + if (!is_array($query_by)) { + $query_by = array($query_by); + } + } else { + if ($type == "multiinfo") { + $query_by = array('name'); + } else { // search + $query_by = array('name', 'description'); + } + } + + if ($type == "multiinfo") { + $args = $http_data['arg']; + if (!is_array($args)) { + $args = array($args); + } + foreach ($args as $i => $arg) { + $args[$i] = $this->dbh->quote($arg); + } + $op_rhs = " IN (" . implode(",", $args) . ")"; + } else { + $keyword_string = $http_data['arg']; + $keyword_string = $this->dbh->quote("%" . addcslashes($keyword_string, '%_') . "%"); + $op_rhs = " LIKE " . $keyword_string; + } + + $has_provides_query = false; + $where_condition = ""; + foreach ($query_by as $index => $by) { + if ($index != 0) { + $where_condition .= " OR "; + } + if ($by == "provides") { + $has_provides_query = true; + $where_condition .= "(RelationTypes.Name = 'provides' AND "; + $where_condition .= "PackageRelations.RelName $op_rhs)"; + } else { + $where_condition .= self::$exposed_fields_map_v6[$by]; + $where_condition .= $op_rhs; + } + } + + $max_results = config_get_int('options', 'max_rpc_results'); + $fields = implode(',', self::$fields_v4); + $q = "SELECT {$fields} " . + "FROM Packages LEFT JOIN PackageBases " . + "ON PackageBases.ID = Packages.PackageBaseID " . + "LEFT JOIN Users " . + "ON PackageBases.MaintainerUID = Users.ID "; + if ($has_provides_query) { + $q .= "LEFT JOIN PackageRelations ON PackageRelations.PackageID = Packages.ID "; + $q .= "LEFT JOIN RelationTypes ON RelationTypes.ID = PackageRelations.RelTypeID "; + } + $q .= "WHERE ${where_condition} "; + $q .= "AND PackageBases.PackagerUID IS NOT NULL "; + if ($has_provides_query) { + $q .= "GROUP BY Packages.ID "; + } + $q .= "LIMIT $max_results"; + + $result = $this->dbh->query($q); + + return $this->process_result($type, $result); + } + /* * Returns the info on a specific package. * @@ -680,4 +796,3 @@ class AurJSON { return json_encode($output); } } -