Mixed scripting language API to determine file upload location and scrape city government website to find corresponding government official

Question

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.

Answer 1

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.