5
\$\begingroup\$

I have created a project which uploads data from my local storage to my Google Drive account using the Google Drive API.
You can read about how to run it on its Github repository.

The main script upload.py:

#!/usr/bin/env python
from __future__ import print_function

import os
import sys
import json
import subprocess
import hashlib

from apiclient.http import MediaFileUpload
from api_boilerplate import file_service


def file_exists(fileId):
    """
    Checks whether a file exists on the Drive or not.
    :param fileId: The ID of the file to check.
    :type fileId: str
    :returns: bool
    """
    if not fileId:
        return False
    try:
        file_service.get(fileId=fileId, fields="").execute()
        return True
    except:
        return False


def create_file(file_path, parentId=None):
    """
    Creates a new file on the Drive.
    :param file_path: The path of the source file on local storage.
    :type file_path: str
    :param parentId: The ID of the directory in which the file has to be
    created. If it is None, the file will be created in the root directory.
    :type parentId: str or None
    :returns: A dictionary containing the ID of the file created.
    """
    file_name = os.path.basename(file_path)

    mimetype = None
    if not os.path.splitext(file_name)[1]:
        # Required for files with names like '.astylerc'
        mimetype = "text/plain"

    media_body = MediaFileUpload(file_path, mimetype=mimetype)
    body = {'name': file_name}
    if parentId:
        body['parents'] = [parentId]

    results = file_service.create(
        body=body, media_body=media_body, fields="id").execute()

    return results


def update_file(file_path, fileId):
    """
    Modifies an already existing file on the Drive.
    :param file_path: The path of the source file on local storage.
    :type file_path: str
    :param fileId: The ID of the file to be modified.
    :type fileId: str
    :returns: A dictionary containing the ID of the file modified.
    """
    mimetype = None
    if not os.path.splitext(os.path.basename(file_path))[1]:
        # Required for files with names like '.astylerc'
        mimetype = "text/plain"

    media_body = MediaFileUpload(file_path, mimetype)

    results = file_service.update(
        fileId=fileId, media_body=media_body, fields="id").execute()

    return results


def update_or_create_file(input_file):
    """
    Updates the file if it exists already on the Drive, else creates a new one.
    :param input_file: A dictionary containing the details about the file.
    The required details are 'path', 'fileId' and 'parentId'.
    :type input_file: dict
    :returns: A dictionary containing the details about the file.
    """
    file_path = input_file['path']
    fileId = input_file['fileId']
    parentId = input_file['parentId']

    if file_exists(fileId):
        return update_file(file_path, fileId)
    else:
        return create_file(file_path, parentId=parentId)


def is_file_modified(input_file):
    """
    Checks whether a file on the Drive is different from its local counterpart.
    It does this by comparing their hash values.
    :param input_file: A dictionary containing the details about the file.
    The required details are 'path', 'fileId' and 'parentId'.
    :type input_file: dict
    :returns: bool
    """
    file_path = input_file['path']
    fileId = input_file['fileId']

    if not file_exists(fileId):
        return True

    local_file_hash = hashlib.md5(open(file_path, 'rb').read()).hexdigest()

    remote_file_hash = file_service.get(
        fileId=fileId, fields="md5Checksum").execute()['md5Checksum']

    return local_file_hash != remote_file_hash


def archive_directory(dir_path):
    """
    Creates an archive of the directory's contents.
    :param dir_path: The path of the directory on local storage.
    :type dir_path: str
    :returns: str -- The path of the archive created.
    """
    archive_path = dir_path + ".tar.xz"

    # Remove the previous archive
    if os.path.exists(archive_path):
        os.remove(archive_path)

    subprocess.check_call(["tar", "-caf", archive_path, "-C", dir_path, "."])

    return archive_path


def main():
    file_list_file_path = "file_list.json"

    in_file = open(file_list_file_path)
    file_list = json.load(in_file)
    in_file.close()

    for input_file in file_list:
        print(str(input_file))
        sys.stdout.flush()

        file_path = input_file['path']
        if os.path.isdir(file_path):
            file_path = archive_directory(file_path)

        # Creating a backup object to prevent changing
        # 'dir_path' to 'dir_path.tar.xz' in the output json file
        input_file_new = dict(input_file)
        input_file_new['path'] = file_path

        if is_file_modified(input_file_new):
            results = update_or_create_file(input_file_new)
            input_file['fileId'] = results['id']

        # Delete the archive file
        if os.path.isdir(input_file['path']):
            os.remove(file_path)

    # Write the list to the json file again
    # as it may contain new fileId's for some files
    out_file = open(file_list_file_path, "w")
    json.dump(file_list, out_file, indent=4, sort_keys=True)
    out_file.close()

    print("Done!!")


if __name__ == '__main__':
    main()

api_boilerplate.py:
(Uploading this file just for completeness, as it was not written by me.
It was copied from the Google API Documentation.
So, there is no need to review it.)

"""
This module is responsible for doing all the authentication.
Adapted from the Google API Documentation.
"""

from __future__ import print_function
import os
import httplib2

import apiclient
import oauth2client

try:
    import argparse
    flags = argparse.ArgumentParser(
        parents=[oauth2client.tools.argparser]).parse_args()
except ImportError:
    flags = None

SCOPES = 'https://www.googleapis.com/auth/drive'
CLIENT_SECRET_FILE = 'client_secret.json'

# Enter your project name here!!
APPLICATION_NAME = 'API Project'


def get_credentials():
    """Gets valid user credentials from storage.

    If nothing has been stored, or if the stored credentials are invalid,
    the OAuth2 flow is completed to obtain the new credentials.

    Returns:
        Credentials, the obtained credential.
    """
    home_dir = os.path.expanduser('~')
    credential_dir = os.path.join(home_dir, '.credentials')
    if not os.path.exists(credential_dir):
        os.makedirs(credential_dir)
    credential_path = os.path.join(credential_dir,
                                   'drive-credentials.json')

    store = oauth2client.file.Storage(credential_path)
    credentials = store.get()
    if not credentials or credentials.invalid:
        flow = oauth2client.client.flow_from_clientsecrets(
            CLIENT_SECRET_FILE, SCOPES)
        flow.user_agent = APPLICATION_NAME
        if flags:
            credentials = oauth2client.tools.run_flow(flow, store, flags)
        else:  # Needed only for compatibility with Python 2.6
            credentials = oauth2client.tools.run(flow, store)
        print('Storing credentials to ' + credential_path)
    return credentials


credentials = get_credentials()
http = credentials.authorize(httplib2.Http())
file_service = apiclient.discovery.build('drive', 'v3', http=http).files()

file_list.json:

[
    {
        "fileId": "random_alphanumeric_string1",
        "parentId": "random_alphanumeric_string2",
        "path": "/path/to/file/1.txt"
    },
    {
        "fileId": "random_alphanumeric_string3",
        "parentId": "random_alphanumeric_string4",
        "path": "/path/to/folder/2"
    },
    {
        "fileId": "random_alphanumeric_string5",
        "parentId": "random_alphanumeric_string6",
        "path": "/path/to/file/3.txt"
    }
]

I would like to get a general review about anything.

\$\endgroup\$

1 Answer 1

3
\$\begingroup\$

This is a "blind" review in that I'm not testing my changes or running your code.

Don't use #!/usr/bin/env python unless actually version-agnostic; use #!/usr/bin/env python2 or #!/usr/bin/env python3.

This here

if not fileId:
    return False
try:
    file_service.get(fileId=fileId, fields="").execute()
    return True
except:
    return False

has a raw except. I don't like that; replace it with an except catching the specific wanted exception, or Exception if you really don't know which will be raised. This would at least avoid catching BaseException, which is rarely wanted.

Don't use if x to test if something is None, use is None. This is a stricter, faster check and is less likely to cause false positives.

You'd be better off using pathlib than the string and os.path combination you're using now. For example, create_file would be roughly

file_path = PurePosixPath(file_path)

mimetype = None
if not file_path.suffix:
    # Required for files with names like '.astylerc'
    mimetype = "text/plain"

media_body = MediaFileUpload(file_path, mimetype=mimetype)
body = {'name': file_path.name}
...

which I'd shorten to

file_path = PurePosixPath(file_path)

# Required for files with names like '.astylerc'
mimetype = "text/plain" if file_path.suffix else None

media_body = MediaFileUpload(file_path, mimetype=mimetype)
body = {'name': file_path.name}
...

update_file does

if not os.path.splitext(os.path.basename(file_path))[1]:

which seems redundant compared to just

if not os.path.splitext(file_path)[1]:

and still worse than with pathlib.

You do

some_function(name_of_thing=name_of_thing)

quite often. Don't bother duplicating the name, it doesn't help. Only do this if the parameter name is different and adds information.

Your code looks race-y. For example,

if file_exists(fileId):
    return update_file(file_path, fileId)
else:
    return create_file(file_path, parentId=parentId)

would likely break if a file is created or deleted between the check and the action. You should check the provided API for solutions here (although this might not be a problem).

The code

local_file_hash = hashlib.md5(open(file_path, 'rb').read()).hexdigest()

leaks a file handle. Always (almost always) use with for files. Do so in main too.

The flush here isn't needed:

print(str(input_file))
sys.stdout.flush()
\$\endgroup\$
2
  • \$\begingroup\$ 1. Why should I specify the Python version explicitly even though it works on both? 2. By leaking of file handle, do you mean that the file remains open even after that statement is executed? 3. Don't bother duplicating the name, it doesn't help. I am doing this because sometimes the order of the actual parameters are different from that of the formal parameters. \$\endgroup\$ Commented Jan 10, 2016 at 17:22
  • \$\begingroup\$ @AnmolSinghJaggi 1. My bad; didn't realize you were targeting both. 2. Exactly, yeah. with gives exception-safe automatic handling for this stuff. 3. Fair enough I guess, but don't overuse it. \$\endgroup\$
    – Veedrac
    Commented Jan 10, 2016 at 17:38

Your Answer

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

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