voceconnect / lift-search

Lift Search for WordPress
14 stars 12 forks source link

Maintain api compatibility #75

Closed matgargano closed 9 years ago

matgargano commented 10 years ago
jeffstieler commented 9 years ago

@matstars does the change from uint to int still work on API version 2011-02-01? The docs for 2011-02-01 only mention uint, which is why I ask.

matgargano commented 9 years ago

@jeffstieler the type gets set on schema creation. Any sites still using the old API will continue to work. Any new sites or (per above) adding the Lift plugin for the first time, deactivate the Lift plugin or, clicking on (and confirming) Reset Lift will use the new int.

jeffstieler commented 9 years ago

@matstars cool. I was just taking a skim of the diff, I didn't dive in. :+1:

prettyboymp commented 9 years ago

@johnciacia @kevinlangleyjr since we don't have a way to spin up a new domain with the old api, do we have any options for testing this on an existing domain?

kevinlangleyjr commented 9 years ago

@prettyboymp @johnciacia I actually already have a test running on my local for BTN and will be reporting back.

kevinlangleyjr commented 9 years ago

@matstars Initial testing indicates an issue with applying the access policy. This is on two different sites - I wanted to test pndotcom as well since BTN has Lift already and I wanted to test a fresh install as well.

There was an error while applying the default access policy to the domain.

event_log - (/nfs/bigten/wp-content/themes/vip/plugins/lift-search/wp/domain-manager.php:200)

apply_access_policy
There was an error while applying the default access policy to the domain.

event_log - (/nfs/pndotcom/wp-content/plugins/lift-search/wp/domain-manager.php:200)

apply_access_policy
matgargano commented 9 years ago

@kevinlangleyjr can you provide the steps that you took to recreate these.

kevinlangleyjr commented 9 years ago

@matstars installed plugin, went through setup steps to create the domain through Lift, then Lift attempted to apply the access policy and fails. It also tries on every single page load after that which takes some time since it tries to apply it multiple times per page load.

matgargano commented 9 years ago

When testing this PR, I was creating the access policy manually - I mistakenly thought that was part of the setup process; I didn't realize the plugin sets those.

Investigating how to get this to set the policies with the new API.

Just crossing t's dotting i's -- do not pull this until further notice!

kevinlangleyjr commented 9 years ago

@matstars - I'm now able to get the domain created and it applies an access policy but the access policy is not getting the right IP address applied to it so it is using 0.0.0.0 and therefore is open to save documents and search from any IP.

{
  "Version": "2012-10-17",
  "Statement": [
    {
      "Sid": "",
      "Effect": "Allow",
      "Principal": {
        "AWS": "*"
      },
      "Action": "*",
      "Condition": {
        "IpAddress": {
          "aws:SourceIp": "0.0.0.0/0"
        }
      }
    }
  ]
}

I thought possibly http://ifconfig.me/ip might be down and causing that to happen but it is not. Also, even with it down we cannot just apply that open of an access policy to an account. The user would be oblivious to such open permissions. We should look at what we can do to remove ifconfig as a dependency here as it has been down more than once while testing this further.

Also, upon navigating to the front end of a site and searching, I am seeing the following warning: ( ! ) Notice: Undefined property: stdClass::$messages in /nfs/pndotcom/wp-content/plugins/lift-search/api/cloud-search-api.php on line 71

If you notice, were checking for the existence of the error property and then utilizing the messages property. Also, if you inspect the $response variable there on line 70 you can see that Amazon is returning the following error.

[*Deprecated*: Use the outer message field] Parameter 'bq' is no longer valid. Replace 'bq=query' with 'q.parser=structured&q=query'.

We're going to have to change how we formulate the queries before we send them off. If you look at http://docs.aws.amazon.com/cloudsearch/latest/developerguide/migrating.html you can see all the obsolete and new options and parameters for the API. We especially need to heavily test and verify there aren't any issues with the facet fields there. I believe the current setup uses at the very least the rank field which as you can see is obsolete.

I knew this was going to be a big one.

matgargano commented 9 years ago

in the latest commits, coming later today, I tested the facet_contraint (sic) and facet_top_n by running the following on the instance of Cloud_Search_Query:

$cs_query->add_facet_contraint( 'taxonomy_category_id', array(
        '..5', '6..10', '11..20', '21..'
) );

and

$cs_query->add_facet_top_n( 'taxonomy_category_id', 10 );

respectively, I am passing the data through the API and getting results (and not an error) but I am not sure of a good use case to test they are working as expected.

Also, the facet.FIELD replacement for the *-top-n looks like it wasn't another parameter for sorting (bucket or count) -- I am pretty sure it would be count but just something else to note. reference: http://docs.aws.amazon.com/cloudsearch/latest/developerguide/faceting.html

I also added notes from the API docs wherever I could to explain why I made a change. Those and the above are more notes for later/review than anything.