504699 - Write a new Search API as part of addons services!!!

Status: VERIFIED FIXED

(addons.mozilla.org Graveyard :: API, defect)

Description

[Dave Dash [:davedash, :dd] (assign all bugs to mbrandt)]

I discussed this with Jeff.

I'm going to create a new controller "Api13Controller" which will handle all versioned API search traffic marked as 1.3.

This way I can work on the search engine without fear of breaking Firefoxen.

This API will handle "natural" queries:
e.g.

firebug platform:linux lang:en

and convert it into the sphinx query language.

this will ultimately power the Frontend web search as well.

All the heavy lifting that needs to be done before sending it to Sphinx should be done in this controller -or- libraries that get created.

Will support JSON and XML formats.

Comment 1

[Dave Dash [:davedash, :dd] (assign all bugs to mbrandt)]

Attachment: v1

Hi,

You'll need to get the sphinx engine setup before testing this and you'll need to run the bin/upgrade_versions.py script.

addonSearch is the heart of the search engine, and will be the same class I use in the frontend.

addonSearch has a wierd way of getting to the ->query method of appModel... not sure what the proper cake way is, but I'd like to follow that rather than write maverick code.

Please be strict in this review.

Comment 2

[Jeff Balogh (:jbalogh)]

Comment on attachment 392793 [details] [diff] [review]
v1

diff --git a/bin/upgrade_versions.py b/bin/upgrade_versions.py
@@ -0,0 +1,91 @@
+#  Use this to upgrade the versions table
+#  query the current database for versions
+#  create a new versions column as an INT
+
+import os
+import sys
+import re
+sys.path.append(os.path.join(os.path.dirname(__file__),'schematic'))
+
+from schematic import VARIABLES, get_settings, say

I like to import the module, not specific pieces, so that I can see where names
come from, e.g. `schematic.say(...)`.

+SETTINGS_DIR = os.path.realpath(os.path.join(os.path.dirname(__file__),os.path.pardir, "site/app/config/migrations"))    
+settings = get_settings(SETTINGS_DIR)
+
+if __name__ == '__main__':
+    # say(settings.db, "ALTER TABLE appversions ADD COLUMN version_int  BIGINT UNSIGNED AFTER version;")

Shouldn't be commented out.

+    id_versions = say(settings.db, "SELECT id, version FROM appversions WHERE version_int >  3060000001000 ")

The rest of that file is a big scary mess.  Please don't hurt my eyes.

diff --git a/site/app/controllers/api15_controller.php b/site/app/controllers/api15_controller.php
@@ -0,0 +1,67 @@
+    public function search($term) {
+    	$this->layout = 'rest'; 

Mixing tabs and spaces.

+    	$as = new AddonSearch($this->Addon);
+    	list($matches, $total_results) = $as->query($term);
+    	
+        $this->_getAddons($matches);
+        
+        // var_dump($matches);
+        // var_dump($this->viewVars['addonsdata']);

debug stuff

+        $this->publish('api_version', $this->api_version); 
+        $this->publish('guids', array_flip($this->Application->getGUIDList()));
+        $this->publish('app_names', $app_names = $this->Application->getIDList()); 
+        $this->publish('total_results', $total_results); 
+        $this->publish('os_translation', $this->os_translation);   
+        $this->publish('addonsdata', $this->viewVars['addonsdata']);
+    }
+}
diff --git a/site/app/controllers/api_controller.php b/site/app/controllers/api_controller.php
@@ -63,7 +63,7 @@ class ApiController extends AppController
     var $securityLevel = 'low';
 
     function beforeFilter() {
-
+        DboAmoMysql::$disable_log = true;

Is this necessary?  You can turn off logging by setting debug < 2.

diff --git a/site/app/models/dbo/dbo_amo_mysql.php b/site/app/models/dbo/dbo_amo_mysql.php
index 7dc1fef..ff91702 100644
--- a/site/app/models/dbo/dbo_amo_mysql.php
+++ b/site/app/models/dbo/dbo_amo_mysql.php
@@ -61,6 +61,8 @@ class DboAmoMysql extends DboMysql {
      *        0 => (translation data)
      *      )
      */
+     
+     public static $disable_log = false;
     function __filterResults(&$results, &$model, $filtered = array()) {
         // if there's no translations, we don't change anything
         // (also: if there's no results for the queried model, this is
@@ -172,6 +174,10 @@ class DboAmoMysql extends DboMysql {
      * Outputs a pretty version of the Cake query log that is hidden by default.
      */
     function showLog($sorted = false) {
+        if (self::$disable_log)
+        {
+            return;
+        }

Our style guide says that opening brackets don't deserve their own line.

diff --git a/site/app/views/api15/search.thtml b/site/app/views/api15/search.thtml
+    if (isset($error)) { ?>
+      <error><?php echo ($error); ?></error>   
+<?php
+    } else {
+      $http = (isset($_SERVER['HTTPS']) ? 'https': 'http') . '://';
+?>
+<searchresults <?php echo "total_results = '$total_results'"; ?> >
+<?php     
+     foreach ($addonsdata as $addon) {
+         // testing for dummies is an optimization hack for b3
+         // so search can get the numbers right
+         //  this will not occur in api_versions > 0
+         if ($addon != 'dummy') {
+             include dirname(__FILE__).'/../api/api_addon.thtml';

Is this an acceptable way to do this?  I don't think I've seen it in amo before.

+         } else {
+             ?><addon /><?php

Does not parse in my brain.  I think that's a good thing.

diff --git a/site/vendors/sphinx/addonsSearch.php b/site/vendors/sphinx/addonsSearch.php
@@ -0,0 +1,232 @@
+<?php
+
+/**
+* AddonSearch
+*/
+class AddonSearch
+{

Bracket style, again.

+    /**
+     *  Actually preform the search
+     */
+    public function query($term) {
+        // summon the sphinx api
+        $sphinx = $this->sphinx;
+        $sphinx->SetSelect("addon_id, app");
+        $sphinx->SetFieldWeights(array('name'=> 4));
+        $sphinx->SetSortMode ( SPH_SORT_EXPR, "@weight + IF(status=1, 0, 100)" );

Weird paren spaces there.

+        $sphinx->SetLimits(0, 60);
+        $sphinx->SetFilter('inactive', array(0));
+        
+        // filter based on the app we're looking for e.g is this /firefox/ or /seamonkey/ etc
+        $sphinx->SetFilter('app', array(APP_ID));
+        
+        // version filter
+        // convert version to int
+        // convert into to a thing to serach for

s/serach/search/

+        // date filter
+        if (preg_match("{\bafter:([0-9-]+)\b}", $term, $matches)) {
+            
+            $term      = str_replace($matches[0], '', $term);

You're doing that weird spacing thing.  All the other lines look fine without
it.

+    /**
+     *  Takes a string and converts it to a category id.
+     *  e.g. 'alerts' should return category 72
+     */
+    public function convert_category($str) {

I don't think we use public anywhere else.  It's not terrible to have it here,
but I'd rather not start.  (I don't believe in public vs. private, btw.)

+        // right now we're just using a simple reverse lookup query that only searches using the english locale
+        // query is safe since $str has to match \w+
+        
+        // prepared statements aren't working
+            
+        $q  = "SELECT categories.id FROM categories, translations t "
+            . "WHERE name = t.id and application_id = %s AND locale='en-US' AND localized_string LIKE '%s%%'";

Only en-US?

+    /**
+     * Takes a string and converts it to a platform id
+     *  e.g. platform:linux => 2
+     */
+    public function convert_platform($str) {
+        switch($str) {
+            case 'all':
+                return PLATFORM_ALL;
+            case 'linux':
+                return PLATFORM_LINUX;
+            case 'mac':
+                return PLATFORM_MAC;
+            case 'bsd':
+                return PLATFORM_BSD;
+            case 'win':
+                return PLATFORM_WIN;
+            case 'sun':
+                return PLATFORM_SUN;            
+        }
+    }

We don't have this anywhere else already?

+    
+    public function convert_version($ver) {
+        $ver = str_replace('.x', '.99', $ver);
+        $ver = str_replace('.*', '.99', $ver);
+        
+        if (preg_match('/(\d+)\+/', $ver, $matches)) {
+            $pre = int($matches[1]) + 1;
+            $ver = str_replace($matches[0], $pre, $ver);
+        }
+        
+        if (preg_match('/(\d+)\.(\d+)\.?(\d+)?\.?(\d+)?([a|b]?)(\d*)(pre)?(\d)?()?/',$ver,$matches)) {
+            list($full,$major,$minor1,$minor2,$minor3,$alpha,$alpha_n,$pre,$pre_n) = $matches;

epic.

Comment 3

[Dave Dash [:davedash, :dd] (assign all bugs to mbrandt)]

All dependencies are fixed - closing.