5
\$\begingroup\$

I wrote this script as an NYC-specific-API for file upload for a mobile app. Users upload a video file and also their geographic coordinates.

I then use an external API to get the corresponding borough in NYC (since the iOS reverse geocoding SDK labels everything as New York, NY regardless of user location within NYC) before scraping the NYC Council homepage to find the appropriate council member based on geographic location. I don't have any experience with this sort of thing, so I really welcome all comments.

Here's the overall structure:

  1. PHP file receives the upload, processes the upload, and calls an R script which calls NYT API to match latitude, longitude to NYC borough.
  2. Once the correct borough is returned from the R script, the original PHP script scalls Scrapy crawler.
  3. Once the Scrapy crawler returns, MongoDB is updated and appropriate emails are sent out.

fileupload.php

// generalized server email functions + email functions specific to this file uploading API
require_once("email.php");
require_once("upload_mail_functions.php");

// MongoDB record constants
$SCRAPY_FAIL = "scrapyFail";
$NYT_FAIL = "nytFail";
$SUCCESS = "success";

// constant location parameters
$TARGET_DIR = "/var/www/html/uploads/";
$SCRAPY_DIR = "~/scrape/nyc_council";
$ADMIN_EMAIL = "[email protected]";

// move uploaded file to appropriate location, file name is made unique before upload
$target_dir = $TARGET_DIR;
$target_file = $target_dir . $_FILES["upload"]["name"];

$success = move_uploaded_file($_FILES["upload"]["tmp_name"], $target_file);
if($success) mail($ADMIN_EMAIL, "file uploaded", $_POST['latitude']);
else  fail_email($mail, ' ', $email, $address, $short_file." FAILED TO UPlOAD" );


// I am using MongoDB so my understanding is that SQL injection isn't such a scary thing.

// mailing address
$address = $_POST['address'];
$email = $_POST['email'];
$address_components = explode(",", $address);
$street_address = $address_components[0];

// actual location
$latitude = $_POST['latitude'];
$longitude = $_POST['longitude'];

// other relevant parameters
$license = $_POST['license'];
$time = time();

// this becomes the unique record identifier since it is unique to the time and place of the video
// (coupled with a random number generated at time file was saved, just to be safe
$filename = $target_file;
$short_file = $_FILES["upload"]["name"];

// connect to Mongo and insert basic data
$m = new MongoClient();
$db = $m->selectDB("idling");
$collection = $db->videos;
$collection->insert(array("email" => $email, "address" => $address, "latitude" => $latitude, "longitude" => $longitude, "license" => $license, "time" => $time, "file" => $filename));

// Now on to getting details so we can contact the relevant NYC councilperson
// now first get borough with R script using littler
exec("r get_borough.R '$latitude' '$longitude'", $resultVar);

//then if borough is not nonsense (response is null if API call fails), feed into Scrapy
if(strlen($resultVar[0]) > 1) $borough_abbrev = substr($resultVar[0], 0, 4);

         $borough = 1;
         switch($borough_abbrev){
                case "Manh":
                $borough = 1;
                break;

                case "Bron":
                $borough = 2;
                break;

                case "Broo":
                $borough = 3;
                break;

                case "Quee":
                $borough = 4;
                break;

                default:
                $borough = 5;
        }


        exec("cd ".$SCRAPY_DIR." && scrapy crawl dmoz -a address='$street_address' -a borough='$borough'", $resultVar);

        // got back informative feedback from Scrapy with a contact email address for city councilperson
        if(strlen($resultVar[0])>3){
                // send an email to appropriate NYC councilperson and success email to the admin
                $council_mail = str_replace("mailto:", "", $resultVar[0]);
                success_email($mail, $council_mail, $email, $address, $short_file );
                $collection->update(array("file" => $filename), array("email_sucess" => $SUCCESS));

        }
        // API call failed, send email to the admin who can look up the contact info manually
        else{
                 fail_email($mail, ' ', $email, $address, $short_file );
                $collection->update(array("file" => $filename), array("email_sucess" => $SCRAPY_FAIL));

        }
}
// never got a meaningful response from NYT API so no need to proceed further
// notify admin that api query failed
else{
        fail_email($mail, ' ', $email, $address, $short_file." NYT API query failed" );
        $collection->update(array("file" => $filename), array("email_sucess" => $NYT_FAIL));
}

quick R script to grab borough from NYT API

# set options to silence warnings/errors
library(RJSONIO)
options(warn=-1)

# data needed to generate API URL
api_key_district = API_KEY
lat = argv[1] 
lon = argv[2] 
prep_url = paste0("http://api.nytimes.com/svc/politics/v2/districts.json?lat=", lat, "&lng=", lon, "&api-key=", api_key_district)

# see if NYT API returns any useful data about location
getData <- function(url){
  raw.data <- readLines(url)
  rd  <- fromJSON(raw.data)
  df = rd$results
  length_result = length(df)
  for(i in 1:length_result){
    if(df[[i]]["level"] == "Borough"){
      borough = unname(df[[i]]["district"])
      write(borough, stdout())
      break
    }
  }
}

getData(prep_url)

Scrapy spider

from scrapy.http import FormRequest
from scrapy.spider import BaseSpider
from scrapy.http import Request
from bs4 import BeautifulSoup
import re
import sys

'''to call use scrapy crawl dmoz -a address={ADDRESS} -a borough={BOROUGH #}
boroughs are: 1 = Manhattan, 2 = Bronx, 3 = Brooklyn, 4 = Queens, 5 = Staten Island'''

class DmozSpider(BaseSpider):
    name = "dmoz"
    start_urls = ["http://council.nyc.gov/html/members/members.shtml"]

    def __init__(self, address='', borough = 0):
        super(BaseSpider, self).__init__()
        self.address = address
        self.borough = borough

    def parse(self, response):
        return self.login(response)

    def login(self, response):
        return [FormRequest.from_response(response,
                                          formdata={'lookup_address': self.address, 'lookup_borough' : self.borough},
                                          formnumber = 1, callback=self.parse_evalPage)]

    def parse_evalPage(self, response):
        soup = BeautifulSoup(response.body)
        anchors =  [td.find('a') for td in soup.findAll('td', {"class":"nav_text"})]\
        for a in anchors:
            with open("~/recordedEmails.txt", 'a') as f:
                link = a['href']
                mailto_remove = re.compile(re.escape('mailto:'), re.IGNORECASE)
                link = mailto_remove.sub('', link)
                f.write(link)
                sys.stdout.write(a['href'])

I chose the languages for each section based on what I thought would be easiest/most direct. For example, the NYT API returns many fields that can easily be read into a data structure and naturally searched in R, whereas doing the same in PHP would feel like a drag of loops and loops. And I only know Scrapy for scraping, and it does the job so well, why would I use something else? I have the feeling feedback will likely be in the form of why'd the heck did you do that, and I guess partly I'd want to know (seriously) what's objectionable about that.

As I said before, I'd welcome comments at all levels of specificity. If you just want to comment on one language, please feel free to do so.

\$\endgroup\$
3
  • \$\begingroup\$ Without wanting to sound patronizing (but I'm afraid I'm going to): there's nothing wrong with choosing the right tool for the job. It's very common to see a mixed language stack these days (for example: PHP + JS + Python). However, how you link them all together is a different matter. Your use of exec to get R and PHP to play together is a worry. As for the patronizing bit: Looking at your PHP code, I'm guessing it's not the language you're most comfortable with, maybe that's why you feel that half of the things would be cumbersome to write in it? (I genuinely don't mean that in a bad way) \$\endgroup\$ Oct 16, 2015 at 10:22
  • \$\begingroup\$ But you seem to handle PHP as though it's an extended, web oriented version of the bastard love-child of Bash and Perl. It isn't. There are some similarities, but they are fundamentally different. PHP3 borrowed quite a lot from Perl, but it's gone off in a completely different direction since then. I'm not saying you should spend a couple of weeks learning the language, but instead you look to me to be more comfortable using Python, and should perhaps consider replacing PHP with Python (Django?) instead. Just because you prefer it, not because it's better or worse... \$\endgroup\$ Oct 16, 2015 at 10:24
  • \$\begingroup\$ @EliasVanOotegem You don't sound patronizing to me, but can you tell me what a better PHP version would look like, apart from these worrying calls to exec. And how would you call another script with arguments if not with exec? Is the problem that I didn't use an object based approach? To me that seemed like overkill given that this API is not going to grow and doesn't have similar uses where I could have multipurpose code. What would be a true PHP-styled version of this script in terms of the major features? \$\endgroup\$
    – sunny
    Oct 16, 2015 at 21:33

1 Answer 1

2
\$\begingroup\$

First; You shouldn't be mixing languages to solve this. PHP can get website/api content using curl or file_get_contents, using Python and R is a little bit beyond extraneous.


Reviewing your PHP

  • You have double indentation; that is incorrect, use a single level of indentation (four spaces).
  • In the following code sample, there's no reason to assign $target_dir:
$target_dir = $TARGET_DIR;
$target_file = $target_dir . $_FILES["upload"]["name"];
  • Your large if statement is incorrectly formatted, you leave out the opening bracket, and still provide the closing brackets. Don't leave out brackets, it makes your code harder to maintain.

  • array("email_sucess": you misspelled success.

  • You initialise a bunch of pointless variables, the idea that they're magic keywords is a little extraneous, you don't need to initialise them:

$SCRAPY_FAIL = "scrapyFail";
$NYT_FAIL = "nytFail";
$SUCCESS = "success";
  • $address_components: you don't use this more than once, forget initialising it at all. $address_components = explode(",", $address); $street_address = $address_components[0]; => $street_address = explode(",", $address)[0];

  • $borough_abbrev: normally I'd suggest against using abbreviations as it makes your code harder to maintain, but in this case, it should be fine. However, you don't need to process it with a switch; use an array instead.

$boroughAbbreviations = ["Manh" => 1, "Bron" => 2, "Broo" => 3, "Quee" => 4];
$borough = array_key_exists($abbreviation, $abbreviationList)
    ? $boroughAbbreviations[$abbreviation]
    : 5;

Injection Threat:

You directly use the $_POST variables without any moderation. This could potentially be harmful. For example, let's say, instead of my latitude I put in || rm -rf;, your system would crash. And that's the least of it, pointing out the possibilities for viruses, trojans and all kinds of matter coming in.

exec("r get_borough.R '$latitude' '$longitude'", $resultVar);

Consider checking your script for special characters, using a regex or direct character match.


Reviewing your Python:

Here, your code violates a few PEP8 points, such as too long lines, extraneous whitespace. Try using pep8online.com to test that.

dmoz: the magical keyword

class DmozSpider(BaseSpider):
    name = "dmoz"

I don't know whether dmoz is your username/nickname, but its presence is explained and probably unnecessary. The name variable is not even used.

  • start_urls: is completely unnecessary to initialise as an array, when it contains a single string.
  • parse is extraneous, consider removing it altogether.
  • After anchor is defined you have extraneous whitespace, and at the end of the line, an extraneous \:
anchors =  [td.find('a') for td in soup.findAll('td', {"class":"nav_text"})]\
  • The following block can be simplified.
    link = mailto_remove.sub('', link)
    f.write(link)
    sys.stdout.write(a['href'])

Into the following:

    f.write(mailto_remove.sub('', link))
    sys.stdout.write(link)

I don't know R very much, but here's a little:

  • length_result is extraneous, you don't need to initialise it:
length_result = length(df)
for(i in 1:length_result){

Instead;

for(i in 1:length(df)){
  • rd <- fromJSON(raw.data): in this line, you have an extraneous space after rd

  • lat & lon: you don't really need to initialise these, you can just put the argv[0] and argv[1] definitions in the parameters directly.

\$\endgroup\$
2
  • \$\begingroup\$ Thanks for your comments. Are you familiar with the Scrapy framework? My understanding (a limited one) is that the class variable name and the parse function are both integral to a scrapy crawler. Also I definitely disagree about not defining all my "constants" up top, but this makes me realize I should have used a straight up define statement, which I'd forgotten about. \$\endgroup\$
    – sunny
    Oct 16, 2015 at 21:37
  • \$\begingroup\$ Ah, if that's the case then okay, I am truly unfamiliar with Scrapy. \$\endgroup\$
    – Quill
    Oct 16, 2015 at 22:45

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.