From 4da8a30f200a0dcb25e5257f2acf5fe05e63df60 Mon Sep 17 00:00:00 2001 From: Scott Sanderson Date: Sat, 27 Dec 2014 00:10:20 -0500 Subject: [PATCH 01/13] DEV: Refactor checkpoint logic from FileContentsManager. - Add a `CheckpointManager` base class and infrastructure for creating a `checkpoint_manager` instance attribute on `ContentsManager`. - Provide default implementations of `delete` and `rename` in the base `ContentsManager` class. `ContentsManager` subclasses are now required to implement `delete_file` and `rename_file`. These methods no longer need to manage checkpoints. - Move checkpoint-related functionality from `FileContentsManager` to a dedicated `FileCheckpointManager` subclass. - Move shared filesystem interaction logic into `FileManagerMixin` used by both `FileContentsManager` and `FileCheckpointManager`. - Minor tweaks to ContentsManager tests to get methods from the right object. The purpose of this change is to provide an API for users to replace just the checkpoint logic associated with a particular `ContentsManager`. In particular, this change makes it possible to easily support remote storage of checkpoints while otherwise retaining normal filesystem interactions. --- IPython/html/services/contents/filemanager.py | 519 ++++++++++-------- IPython/html/services/contents/manager.py | 118 +++- .../services/contents/tests/test_manager.py | 13 +- 3 files changed, 389 insertions(+), 261 deletions(-) diff --git a/IPython/html/services/contents/filemanager.py b/IPython/html/services/contents/filemanager.py index 9a1ab9f27..047e6b0e9 100644 --- a/IPython/html/services/contents/filemanager.py +++ b/IPython/html/services/contents/filemanager.py @@ -4,24 +4,31 @@ # Distributed under the terms of the Modified BSD License. import base64 +from contextlib import contextmanager import errno import io import os import shutil -from contextlib import contextmanager import mimetypes from tornado import web -from .manager import ContentsManager +from .manager import ( + CheckpointManager, + ContentsManager, +) from IPython import nbformat from IPython.utils.io import atomic_writing from IPython.utils.importstring import import_item from IPython.utils.path import ensure_dir_exists from IPython.utils.traitlets import Any, Unicode, Bool, TraitError -from IPython.utils.py3compat import getcwd, str_to_unicode, string_types +from IPython.utils.py3compat import getcwd, string_types, str_to_unicode from IPython.utils import tz -from IPython.html.utils import is_hidden, to_os_path, to_api_path +from IPython.html.utils import ( + is_hidden, + to_api_path, + to_os_path, +) _script_exporter = None @@ -48,19 +55,40 @@ def _post_save_script(model, os_path, contents_manager, **kwargs): with io.open(script_fname, 'w', encoding='utf-8') as f: f.write(script) -class FileContentsManager(ContentsManager): - root_dir = Unicode(config=True) +class FileManagerMixin(object): + """ + Mixin for ContentsAPI classes that interact with the filesystem. + + Shared by both FileContentsManager and FileCheckpointManager. + + Note + ---- + Classes using this mixin must provide the following attributes: + + root_dir : unicode + A directory against against which API-style paths are to be resolved. + + log : logging.Logger + """ + + @contextmanager + def open(self, os_path, *args, **kwargs): + """wrapper around io.open that turns permission errors into 403""" + with self.perm_to_403(os_path): + with io.open(os_path, *args, **kwargs) as f: + yield f + + @contextmanager + def atomic_writing(self, os_path, *args, **kwargs): + """wrapper around atomic_writing that turns permission errors into 403""" + with self.perm_to_403(os_path): + with atomic_writing(os_path, *args, **kwargs) as f: + yield f - def _root_dir_default(self): - try: - return self.parent.notebook_dir - except AttributeError: - return getcwd() - @contextmanager def perm_to_403(self, os_path=''): - """context manager for turning permission errors into 403""" + """context manager for turning permission errors into 403.""" try: yield except OSError as e: @@ -70,92 +98,10 @@ class FileContentsManager(ContentsManager): # but nobody should be doing that anyway. if not os_path: os_path = str_to_unicode(e.filename or 'unknown file') - path = to_api_path(os_path, self.root_dir) + path = to_api_path(os_path, root=self.root_dir) raise web.HTTPError(403, u'Permission denied: %s' % path) else: raise - - @contextmanager - def open(self, os_path, *args, **kwargs): - """wrapper around io.open that turns permission errors into 403""" - with self.perm_to_403(os_path): - with io.open(os_path, *args, **kwargs) as f: - yield f - - @contextmanager - def atomic_writing(self, os_path, *args, **kwargs): - """wrapper around atomic_writing that turns permission errors into 403""" - with self.perm_to_403(os_path): - with atomic_writing(os_path, *args, **kwargs) as f: - yield f - - save_script = Bool(False, config=True, help='DEPRECATED, use post_save_hook') - def _save_script_changed(self): - self.log.warn(""" - `--script` is deprecated. You can trigger nbconvert via pre- or post-save hooks: - - ContentsManager.pre_save_hook - FileContentsManager.post_save_hook - - A post-save hook has been registered that calls: - - ipython nbconvert --to script [notebook] - - which behaves similarly to `--script`. - """) - - self.post_save_hook = _post_save_script - - post_save_hook = Any(None, config=True, - help="""Python callable or importstring thereof - - to be called on the path of a file just saved. - - This can be used to process the file on disk, - such as converting the notebook to a script or HTML via nbconvert. - - It will be called as (all arguments passed by keyword): - - hook(os_path=os_path, model=model, contents_manager=instance) - - path: the filesystem path to the file just written - model: the model representing the file - contents_manager: this ContentsManager instance - """ - ) - def _post_save_hook_changed(self, name, old, new): - if new and isinstance(new, string_types): - self.post_save_hook = import_item(self.post_save_hook) - elif new: - if not callable(new): - raise TraitError("post_save_hook must be callable") - - def run_post_save_hook(self, model, os_path): - """Run the post-save hook if defined, and log errors""" - if self.post_save_hook: - try: - self.log.debug("Running post-save hook on %s", os_path) - self.post_save_hook(os_path=os_path, model=model, contents_manager=self) - except Exception: - self.log.error("Post-save hook failed on %s", os_path, exc_info=True) - - def _root_dir_changed(self, name, old, new): - """Do a bit of validation of the root_dir.""" - if not os.path.isabs(new): - # If we receive a non-absolute path, make it absolute. - self.root_dir = os.path.abspath(new) - return - if not os.path.isdir(new): - raise TraitError("%r is not a directory" % new) - - checkpoint_dir = Unicode('.ipynb_checkpoints', config=True, - help="""The directory name in which to keep file checkpoints - - This is a path relative to the file's own directory. - - By default, it is .ipynb_checkpoints - """ - ) def _copy(self, src, dest): """copy src to dest @@ -183,26 +129,6 @@ class FileContentsManager(ContentsManager): """ return to_os_path(path, self.root_dir) - def dir_exists(self, path): - """Does the API-style path refer to an extant directory? - - API-style wrapper for os.path.isdir - - Parameters - ---------- - path : string - The path to check. This is an API path (`/` separated, - relative to root_dir). - - Returns - ------- - exists : bool - Whether the path is indeed a directory. - """ - path = path.strip('/') - os_path = self._get_os_path(path=path) - return os.path.isdir(os_path) - def is_hidden(self, path): """Does the API style path correspond to a hidden directory or file? @@ -240,6 +166,26 @@ class FileContentsManager(ContentsManager): os_path = self._get_os_path(path) return os.path.isfile(os_path) + def dir_exists(self, path): + """Does the API-style path refer to an extant directory? + + API-style wrapper for os.path.isdir + + Parameters + ---------- + path : string + The path to check. This is an API path (`/` separated, + relative to root_dir). + + Returns + ------- + exists : bool + Whether the path is indeed a directory. + """ + path = path.strip('/') + os_path = self._get_os_path(path=path) + return os.path.isdir(os_path) + def exists(self, path): """Returns True if the path exists, else returns False. @@ -259,6 +205,214 @@ class FileContentsManager(ContentsManager): os_path = self._get_os_path(path=path) return os.path.exists(os_path) + +class FileCheckpointManager(FileManagerMixin, CheckpointManager): + """ + A CheckpointManager that caches checkpoints for files in adjacent + directories. + """ + + checkpoint_dir = Unicode( + '.ipynb_checkpoints', + config=True, + help="""The directory name in which to keep file checkpoints + + This is a path relative to the file's own directory. + + By default, it is .ipynb_checkpoints + """, + ) + + @property + def root_dir(self): + try: + return self.parent.root_dir + except AttributeError: + return getcwd() + + # public checkpoint API + def create_checkpoint(self, path): + """Create a checkpoint from the current state of a file""" + path = path.strip('/') + if not self.file_exists(path): + raise web.HTTPError(404) + src_path = self._get_os_path(path) + # only the one checkpoint ID: + checkpoint_id = u"checkpoint" + cp_path = self.get_checkpoint_path(checkpoint_id, path) + self.log.debug("creating checkpoint for %s", path) + with self.perm_to_403(): + self._copy(src_path, cp_path) + + # return the checkpoint info + return self.get_checkpoint_model(checkpoint_id, path) + + def rename_checkpoint(self, checkpoint_id, old_path, new_path): + """Rename a checkpoint from old_path to new_path.""" + old_cp_path = self.get_checkpoint_path(checkpoint_id, old_path) + new_cp_path = self.get_checkpoint_path(checkpoint_id, new_path) + if os.path.isfile(old_cp_path): + self.log.debug( + "Renaming checkpoint %s -> %s", + old_cp_path, + new_cp_path, + ) + with self.perm_to_403(): + shutil.move(old_cp_path, new_cp_path) + + def list_checkpoints(self, path): + """list the checkpoints for a given file + + This contents manager currently only supports one checkpoint per file. + """ + path = path.strip('/') + checkpoint_id = "checkpoint" + os_path = self.get_checkpoint_path(checkpoint_id, path) + if not os.path.exists(os_path): + return [] + else: + return [self.get_checkpoint_model(checkpoint_id, path)] + + def restore_checkpoint(self, checkpoint_id, path): + """restore a file to a checkpointed state""" + path = path.strip('/') + self.log.info("restoring %s from checkpoint %s", path, checkpoint_id) + nb_path = self._get_os_path(path) + cp_path = self.get_checkpoint_path(checkpoint_id, path) + if not os.path.isfile(cp_path): + self.log.debug("checkpoint file does not exist: %s", cp_path) + self.no_such_checkpoint(path, checkpoint_id) + + # ensure notebook is readable (never restore from unreadable notebook) + if cp_path.endswith('.ipynb'): + with self.open(cp_path, 'r', encoding='utf-8') as f: + nbformat.read(f, as_version=4) + self.log.debug("copying %s -> %s", cp_path, nb_path) + with self.perm_to_403(): + self._copy(cp_path, nb_path) + + def delete_checkpoint(self, checkpoint_id, path): + """delete a file's checkpoint""" + path = path.strip('/') + cp_path = self.get_checkpoint_path(checkpoint_id, path) + if not os.path.isfile(cp_path): + self.no_such_checkpoint(path, checkpoint_id) + + self.log.debug("unlinking %s", cp_path) + with self.perm_to_403(): + os.unlink(cp_path) + + # Checkpoint-related utilities + def get_checkpoint_path(self, checkpoint_id, path): + """find the path to a checkpoint""" + path = path.strip('/') + parent, name = ('/' + path).rsplit('/', 1) + parent = parent.strip('/') + basename, ext = os.path.splitext(name) + filename = u"{name}-{checkpoint_id}{ext}".format( + name=basename, + checkpoint_id=checkpoint_id, + ext=ext, + ) + os_path = self._get_os_path(path=parent) + cp_dir = os.path.join(os_path, self.checkpoint_dir) + with self.perm_to_403(): + ensure_dir_exists(cp_dir) + cp_path = os.path.join(cp_dir, filename) + return cp_path + + def get_checkpoint_model(self, checkpoint_id, path): + """construct the info dict for a given checkpoint""" + path = path.strip('/') + cp_path = self.get_checkpoint_path(checkpoint_id, path) + stats = os.stat(cp_path) + last_modified = tz.utcfromtimestamp(stats.st_mtime) + info = dict( + id=checkpoint_id, + last_modified=last_modified, + ) + return info + + # Error Handling + def no_such_checkpoint(self, path, checkpoint_id): + raise web.HTTPError( + 404, + u'Checkpoint does not exist: %s@%s' % (path, checkpoint_id) + ) + + +class FileContentsManager(FileManagerMixin, ContentsManager): + + root_dir = Unicode(config=True) + + def _root_dir_default(self): + try: + return self.parent.notebook_dir + except AttributeError: + return getcwd() + + save_script = Bool(False, config=True, help='DEPRECATED, use post_save_hook') + def _save_script_changed(self): + self.log.warn(""" + `--script` is deprecated. You can trigger nbconvert via pre- or post-save hooks: + + ContentsManager.pre_save_hook + FileContentsManager.post_save_hook + + A post-save hook has been registered that calls: + + ipython nbconvert --to script [notebook] + + which behaves similarly to `--script`. + """) + + self.post_save_hook = _post_save_script + + post_save_hook = Any(None, config=True, + help="""Python callable or importstring thereof + + to be called on the path of a file just saved. + + This can be used to process the file on disk, + such as converting the notebook to a script or HTML via nbconvert. + + It will be called as (all arguments passed by keyword): + + hook(os_path=os_path, model=model, contents_manager=instance) + + path: the filesystem path to the file just written + model: the model representing the file + contents_manager: this ContentsManager instance + """ + ) + def _post_save_hook_changed(self, name, old, new): + if new and isinstance(new, string_types): + self.post_save_hook = import_item(self.post_save_hook) + elif new: + if not callable(new): + raise TraitError("post_save_hook must be callable") + + def run_post_save_hook(self, model, os_path): + """Run the post-save hook if defined, and log errors""" + if self.post_save_hook: + try: + self.log.debug("Running post-save hook on %s", os_path) + self.post_save_hook(os_path=os_path, model=model, contents_manager=self) + except Exception: + self.log.error("Post-save hook failed on %s", os_path, exc_info=True) + + def _root_dir_changed(self, name, old, new): + """Do a bit of validation of the root_dir.""" + if not os.path.isabs(new): + # If we receive a non-absolute path, make it absolute. + self.root_dir = os.path.abspath(new) + return + if not os.path.isdir(new): + raise TraitError("%r is not a directory" % new) + + def _checkpoint_manager_class_default(self): + return FileCheckpointManager + def _base_model(self, path): """Build the common base of a contents model""" os_path = self._get_os_path(path) @@ -478,9 +632,10 @@ class FileContentsManager(ContentsManager): self.run_pre_save_hook(model=model, path=path) + cp_mgr = self.checkpoint_manager # One checkpoint should always exist - if self.file_exists(path) and not self.list_checkpoints(path): - self.create_checkpoint(path) + if self.file_exists(path) and not cp_mgr.list_checkpoints(path): + cp_mgr.create_checkpoint(path) os_path = self._get_os_path(path) self.log.debug("Saving %s", os_path) @@ -512,28 +667,23 @@ class FileContentsManager(ContentsManager): return model - def delete(self, path): + def delete_file(self, path): """Delete file at path.""" path = path.strip('/') os_path = self._get_os_path(path) rm = os.unlink if os.path.isdir(os_path): listing = os.listdir(os_path) - # don't delete non-empty directories (checkpoints dir doesn't count) - if listing and listing != [self.checkpoint_dir]: - raise web.HTTPError(400, u'Directory %s not empty' % os_path) + # Don't delete non-empty directories. + # A directory containing only leftover checkpoints is + # considered empty. + cp_dir = getattr(self.checkpoint_manager, 'checkpoint_dir', None) + for entry in listing: + if entry != cp_dir: + raise web.HTTPError(400, u'Directory %s not empty' % os_path) elif not os.path.isfile(os_path): raise web.HTTPError(404, u'File does not exist: %s' % os_path) - # clear checkpoints - for checkpoint in self.list_checkpoints(path): - checkpoint_id = checkpoint['id'] - cp_path = self.get_checkpoint_path(checkpoint_id, path) - if os.path.isfile(cp_path): - self.log.debug("Unlinking checkpoint %s", cp_path) - with self.perm_to_403(): - rm(cp_path) - if os.path.isdir(os_path): self.log.debug("Removing directory %s", os_path) with self.perm_to_403(): @@ -543,7 +693,7 @@ class FileContentsManager(ContentsManager): with self.perm_to_403(): rm(os_path) - def rename(self, old_path, new_path): + def rename_file(self, old_path, new_path): """Rename a file.""" old_path = old_path.strip('/') new_path = new_path.strip('/') @@ -566,111 +716,6 @@ class FileContentsManager(ContentsManager): except Exception as e: raise web.HTTPError(500, u'Unknown error renaming file: %s %s' % (old_path, e)) - # Move the checkpoints - old_checkpoints = self.list_checkpoints(old_path) - for cp in old_checkpoints: - checkpoint_id = cp['id'] - old_cp_path = self.get_checkpoint_path(checkpoint_id, old_path) - new_cp_path = self.get_checkpoint_path(checkpoint_id, new_path) - if os.path.isfile(old_cp_path): - self.log.debug("Renaming checkpoint %s -> %s", old_cp_path, new_cp_path) - with self.perm_to_403(): - shutil.move(old_cp_path, new_cp_path) - - # Checkpoint-related utilities - - def get_checkpoint_path(self, checkpoint_id, path): - """find the path to a checkpoint""" - path = path.strip('/') - parent, name = ('/' + path).rsplit('/', 1) - parent = parent.strip('/') - basename, ext = os.path.splitext(name) - filename = u"{name}-{checkpoint_id}{ext}".format( - name=basename, - checkpoint_id=checkpoint_id, - ext=ext, - ) - os_path = self._get_os_path(path=parent) - cp_dir = os.path.join(os_path, self.checkpoint_dir) - with self.perm_to_403(): - ensure_dir_exists(cp_dir) - cp_path = os.path.join(cp_dir, filename) - return cp_path - - def get_checkpoint_model(self, checkpoint_id, path): - """construct the info dict for a given checkpoint""" - path = path.strip('/') - cp_path = self.get_checkpoint_path(checkpoint_id, path) - stats = os.stat(cp_path) - last_modified = tz.utcfromtimestamp(stats.st_mtime) - info = dict( - id = checkpoint_id, - last_modified = last_modified, - ) - return info - - # public checkpoint API - - def create_checkpoint(self, path): - """Create a checkpoint from the current state of a file""" - path = path.strip('/') - if not self.file_exists(path): - raise web.HTTPError(404) - src_path = self._get_os_path(path) - # only the one checkpoint ID: - checkpoint_id = u"checkpoint" - cp_path = self.get_checkpoint_path(checkpoint_id, path) - self.log.debug("creating checkpoint for %s", path) - with self.perm_to_403(): - self._copy(src_path, cp_path) - - # return the checkpoint info - return self.get_checkpoint_model(checkpoint_id, path) - - def list_checkpoints(self, path): - """list the checkpoints for a given file - - This contents manager currently only supports one checkpoint per file. - """ - path = path.strip('/') - checkpoint_id = "checkpoint" - os_path = self.get_checkpoint_path(checkpoint_id, path) - if not os.path.exists(os_path): - return [] - else: - return [self.get_checkpoint_model(checkpoint_id, path)] - - - def restore_checkpoint(self, checkpoint_id, path): - """restore a file to a checkpointed state""" - path = path.strip('/') - self.log.info("restoring %s from checkpoint %s", path, checkpoint_id) - nb_path = self._get_os_path(path) - cp_path = self.get_checkpoint_path(checkpoint_id, path) - if not os.path.isfile(cp_path): - self.log.debug("checkpoint file does not exist: %s", cp_path) - raise web.HTTPError(404, - u'checkpoint does not exist: %s@%s' % (path, checkpoint_id) - ) - # ensure notebook is readable (never restore from an unreadable notebook) - if cp_path.endswith('.ipynb'): - with self.open(cp_path, 'r', encoding='utf-8') as f: - nbformat.read(f, as_version=4) - self.log.debug("copying %s -> %s", cp_path, nb_path) - with self.perm_to_403(): - self._copy(cp_path, nb_path) - - def delete_checkpoint(self, checkpoint_id, path): - """delete a file's checkpoint""" - path = path.strip('/') - cp_path = self.get_checkpoint_path(checkpoint_id, path) - if not os.path.isfile(cp_path): - raise web.HTTPError(404, - u'Checkpoint does not exist: %s@%s' % (path, checkpoint_id) - ) - self.log.debug("unlinking %s", cp_path) - os.unlink(cp_path) - def info_string(self): return "Serving notebooks from local directory: %s" % self.root_dir diff --git a/IPython/html/services/contents/manager.py b/IPython/html/services/contents/manager.py index d876c0944..5ec49ee37 100644 --- a/IPython/html/services/contents/manager.py +++ b/IPython/html/services/contents/manager.py @@ -15,11 +15,60 @@ from IPython.config.configurable import LoggingConfigurable from IPython.nbformat import sign, validate, ValidationError from IPython.nbformat.v4 import new_notebook from IPython.utils.importstring import import_item -from IPython.utils.traitlets import Instance, Unicode, List, Any, TraitError +from IPython.utils.traitlets import ( + Any, + Dict, + Instance, + List, + TraitError, + Type, + Unicode, +) from IPython.utils.py3compat import string_types copy_pat = re.compile(r'\-Copy\d*\.') + +class CheckpointManager(LoggingConfigurable): + """ + Base class for managing checkpoints for a ContentsManager. + """ + + def create_checkpoint(self, path): + """Create a checkpoint of the current state of a file + + Returns a checkpoint_id for the new checkpoint. + """ + raise NotImplementedError("must be implemented in a subclass") + + def rename_checkpoint(self, checkpoint_id, old_path, new_path): + """Rename a checkpoint from old_path to new_path.""" + raise NotImplementedError("must be implemented in a subclass") + + def delete_checkpoint(self, checkpoint_id, path): + """delete a checkpoint for a file""" + raise NotImplementedError("must be implemented in a subclass") + + def restore_checkpoint(self, checkpoint_id, path): + """Restore a file from one of its checkpoints""" + raise NotImplementedError("must be implemented in a subclass") + + def list_checkpoints(self, path): + """Return a list of checkpoints for a given file""" + raise NotImplementedError("must be implemented in a subclass") + + def rename_all_checkpoints(self, old_path, new_path): + """Rename all checkpoints for old_path to new_path.""" + old_checkpoints = self.list_checkpoints(old_path) + for cp in old_checkpoints: + self.rename_checkpoint(cp['id'], old_path, new_path) + + def delete_all_checkpoints(self, path): + """Delete all checkpoints for the given path.""" + for checkpoint in self.list_checkpoints(path): + self.delete_checkpoint(checkpoint['id'], path) + + class ContentsManager(LoggingConfigurable): """Base class for serving files and directories. @@ -97,6 +146,19 @@ class ContentsManager(LoggingConfigurable): except Exception: self.log.error("Pre-save hook failed on %s", path, exc_info=True) + checkpoint_manager_class = Type(CheckpointManager, config=True) + checkpoint_manager = Instance(CheckpointManager) + checkpoint_manager_kwargs = Dict(allow_none=False) + + def _checkpoint_manager_default(self): + return self.checkpoint_manager_class(**self.checkpoint_manager_kwargs) + + def _checkpoint_manager_kwargs_default(self): + return dict( + parent=self, + log=self.log, + ) + # ContentsManager API part 1: methods that must be # implemented in subclasses. @@ -186,32 +248,27 @@ class ContentsManager(LoggingConfigurable): """ raise NotImplementedError('must be implemented in a subclass') - def delete(self, path): + def delete_file(self, path): """Delete file or directory by path.""" raise NotImplementedError('must be implemented in a subclass') - def create_checkpoint(self, path): - """Create a checkpoint of the current state of a file - - Returns a checkpoint_id for the new checkpoint. - """ - raise NotImplementedError("must be implemented in a subclass") - - def list_checkpoints(self, path): - """Return a list of checkpoints for a given file""" - return [] - - def restore_checkpoint(self, checkpoint_id, path): - """Restore a file from one of its checkpoints""" - raise NotImplementedError("must be implemented in a subclass") - - def delete_checkpoint(self, checkpoint_id, path): - """delete a checkpoint for a file""" - raise NotImplementedError("must be implemented in a subclass") + def rename_file(self, old_path, new_path): + """Rename a file.""" + raise NotImplementedError('must be implemented in a subclass') # ContentsManager API part 2: methods that have useable default # implementations, but can be overridden in subclasses. + def delete(self, path): + """Delete a file/directory and any associated checkpoints.""" + self.delete_file(path) + self.checkpoint_manager.delete_all_checkpoints(path) + + def rename(self, old_path, new_path): + """Rename a file and any checkpoints associated with that file.""" + self.rename_file(old_path, new_path) + self.checkpoint_manager.rename_all_checkpoints(old_path, new_path) + def update(self, model, path): """Update the file's path @@ -431,3 +488,24 @@ class ContentsManager(LoggingConfigurable): def should_list(self, name): """Should this file/directory name be displayed in a listing?""" return not any(fnmatch(name, glob) for glob in self.hide_globs) + + # Part 3: Checkpoints API + # By default, all methods are forwarded to our CheckpointManager instance. + def create_checkpoint(self, path): + return self.checkpoint_manager.create_checkpoint(path) + + def rename_checkpoint(self, checkpoint_id, old_path, new_path): + return self.checkpoint_manager.rename_checkpoint( + checkpoint_id, + old_path, + new_path, + ) + + def list_checkpoints(self, path): + return self.checkpoint_manager.list_checkpoints(path) + + def restore_checkpoint(self, checkpoint_id, path): + return self.checkpoint_manager.restore_checkpoint(checkpoint_id, path) + + def delete_checkpoint(self, checkpoint_id, path): + return self.checkpoint_manager.delete_checkpoint(checkpoint_id, path) diff --git a/IPython/html/services/contents/tests/test_manager.py b/IPython/html/services/contents/tests/test_manager.py index f531742f5..2a81e622e 100644 --- a/IPython/html/services/contents/tests/test_manager.py +++ b/IPython/html/services/contents/tests/test_manager.py @@ -84,11 +84,16 @@ class TestFileContentsManager(TestCase): root = td os.mkdir(os.path.join(td, subd)) fm = FileContentsManager(root_dir=root) - cp_dir = fm.get_checkpoint_path('cp', 'test.ipynb') - cp_subdir = fm.get_checkpoint_path('cp', '/%s/test.ipynb' % subd) + cpm = fm.checkpoint_manager + cp_dir = cpm.get_checkpoint_path( + 'cp', 'test.ipynb' + ) + cp_subdir = cpm.get_checkpoint_path( + 'cp', '/%s/test.ipynb' % subd + ) self.assertNotEqual(cp_dir, cp_subdir) - self.assertEqual(cp_dir, os.path.join(root, fm.checkpoint_dir, cp_name)) - self.assertEqual(cp_subdir, os.path.join(root, subd, fm.checkpoint_dir, cp_name)) + self.assertEqual(cp_dir, os.path.join(root, cpm.checkpoint_dir, cp_name)) + self.assertEqual(cp_subdir, os.path.join(root, subd, cpm.checkpoint_dir, cp_name)) @dec.skip_win32 def test_bad_symlink(self): From dc295f67bdf920aa85800cc49281dd2d33a8a4b3 Mon Sep 17 00:00:00 2001 From: Scott Sanderson Date: Sat, 27 Dec 2014 00:48:57 -0500 Subject: [PATCH 02/13] MAINT: Add missing `config=True`s. --- IPython/html/services/contents/manager.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/IPython/html/services/contents/manager.py b/IPython/html/services/contents/manager.py index 5ec49ee37..195cbf59b 100644 --- a/IPython/html/services/contents/manager.py +++ b/IPython/html/services/contents/manager.py @@ -147,8 +147,8 @@ class ContentsManager(LoggingConfigurable): self.log.error("Pre-save hook failed on %s", path, exc_info=True) checkpoint_manager_class = Type(CheckpointManager, config=True) - checkpoint_manager = Instance(CheckpointManager) - checkpoint_manager_kwargs = Dict(allow_none=False) + checkpoint_manager = Instance(CheckpointManager, config=True) + checkpoint_manager_kwargs = Dict(allow_none=False, config=True) def _checkpoint_manager_default(self): return self.checkpoint_manager_class(**self.checkpoint_manager_kwargs) From f71f216880893ff092132b2d54f2dbb0fd086849 Mon Sep 17 00:00:00 2001 From: Scott Sanderson Date: Mon, 29 Dec 2014 04:39:55 -0500 Subject: [PATCH 03/13] DEV: More checkpoint API refactoring. Changed the public checkpoint API to: - `create_checkpoint(self, nb, path)` - `get_checkpoint_content(self, checkpoint_id, path)` - `rename_checkpoint(self, checkpoint_id, old_path, new_path)` - `delete_checkpoint(self, checkpoint_id, path)` - `list_checkpoints(self, path)` All paths in the above are API-style paths, and the `nb` argument to `create_checkpoint` is a dictionary suitable for passing to `nbformat.write`. The new `get_checkpoint_content` method returns an unvalidated notebook content dictionary. It is used by `ContentManager.restore_checkpoint` to load content to be written via `save`. --- IPython/html/services/contents/filemanager.py | 257 +++++++++--------- IPython/html/services/contents/manager.py | 50 ++-- 2 files changed, 161 insertions(+), 146 deletions(-) diff --git a/IPython/html/services/contents/filemanager.py b/IPython/html/services/contents/filemanager.py index 047e6b0e9..74feafc67 100644 --- a/IPython/html/services/contents/filemanager.py +++ b/IPython/html/services/contents/filemanager.py @@ -111,9 +111,20 @@ class FileManagerMixin(object): shutil.copyfile(src, dest) try: shutil.copystat(src, dest) - except OSError as e: + except OSError: self.log.debug("copystat on %s failed", dest, exc_info=True) + def _read_notebook(self, os_path, as_version=4): + """Read a notebook from an os path.""" + with self.open(os_path, 'r', encoding='utf-8') as f: + try: + return nbformat.read(f, as_version=as_version) + except Exception as e: + raise web.HTTPError( + 400, + u"Unreadable Notebook: %s %r" % (os_path, e), + ) + def _get_os_path(self, path): """Given an API path, return its file system path. @@ -129,82 +140,6 @@ class FileManagerMixin(object): """ return to_os_path(path, self.root_dir) - def is_hidden(self, path): - """Does the API style path correspond to a hidden directory or file? - - Parameters - ---------- - path : string - The path to check. This is an API path (`/` separated, - relative to root_dir). - - Returns - ------- - hidden : bool - Whether the path exists and is hidden. - """ - path = path.strip('/') - os_path = self._get_os_path(path=path) - return is_hidden(os_path, self.root_dir) - - def file_exists(self, path): - """Returns True if the file exists, else returns False. - - API-style wrapper for os.path.isfile - - Parameters - ---------- - path : string - The relative path to the file (with '/' as separator) - - Returns - ------- - exists : bool - Whether the file exists. - """ - path = path.strip('/') - os_path = self._get_os_path(path) - return os.path.isfile(os_path) - - def dir_exists(self, path): - """Does the API-style path refer to an extant directory? - - API-style wrapper for os.path.isdir - - Parameters - ---------- - path : string - The path to check. This is an API path (`/` separated, - relative to root_dir). - - Returns - ------- - exists : bool - Whether the path is indeed a directory. - """ - path = path.strip('/') - os_path = self._get_os_path(path=path) - return os.path.isdir(os_path) - - def exists(self, path): - """Returns True if the path exists, else returns False. - - API-style wrapper for os.path.exists - - Parameters - ---------- - path : string - The API path to the file (with '/' as separator) - - Returns - ------- - exists : bool - Whether the target exists. - """ - path = path.strip('/') - os_path = self._get_os_path(path=path) - return os.path.exists(os_path) - class FileCheckpointManager(FileManagerMixin, CheckpointManager): """ @@ -223,30 +158,44 @@ class FileCheckpointManager(FileManagerMixin, CheckpointManager): """, ) - @property - def root_dir(self): + root_dir = Unicode(config=True) + + def _root_dir_default(self): try: return self.parent.root_dir except AttributeError: return getcwd() # public checkpoint API - def create_checkpoint(self, path): - """Create a checkpoint from the current state of a file""" + def create_checkpoint(self, nb, path): + """Create a checkpoint from the current content of a notebook.""" path = path.strip('/') - if not self.file_exists(path): - raise web.HTTPError(404) - src_path = self._get_os_path(path) # only the one checkpoint ID: checkpoint_id = u"checkpoint" - cp_path = self.get_checkpoint_path(checkpoint_id, path) + os_checkpoint_path = self.get_checkpoint_path(checkpoint_id, path) self.log.debug("creating checkpoint for %s", path) with self.perm_to_403(): - self._copy(src_path, cp_path) + self._save_notebook(os_checkpoint_path, nb) # return the checkpoint info return self.get_checkpoint_model(checkpoint_id, path) + def get_checkpoint_content(self, checkpoint_id, path): + """Get the content of a checkpoint. + + Returns an unvalidated model with the same structure as + the return value of ContentsManager.get + """ + path = path.strip('/') + self.log.info("restoring %s from checkpoint %s", path, checkpoint_id) + os_checkpoint_path = self.get_checkpoint_path(checkpoint_id, path) + return self._read_notebook(os_checkpoint_path, as_version=4) + + def _save_notebook(self, os_path, nb): + """Save a notebook file.""" + with self.atomic_writing(os_path, encoding='utf-8') as f: + nbformat.write(nb, f, version=nbformat.NO_CONVERT) + def rename_checkpoint(self, checkpoint_id, old_path, new_path): """Rename a checkpoint from old_path to new_path.""" old_cp_path = self.get_checkpoint_path(checkpoint_id, old_path) @@ -260,6 +209,17 @@ class FileCheckpointManager(FileManagerMixin, CheckpointManager): with self.perm_to_403(): shutil.move(old_cp_path, new_cp_path) + def delete_checkpoint(self, checkpoint_id, path): + """delete a file's checkpoint""" + path = path.strip('/') + cp_path = self.get_checkpoint_path(checkpoint_id, path) + if not os.path.isfile(cp_path): + self.no_such_checkpoint(path, checkpoint_id) + + self.log.debug("unlinking %s", cp_path) + with self.perm_to_403(): + os.unlink(cp_path) + def list_checkpoints(self, path): """list the checkpoints for a given file @@ -273,35 +233,6 @@ class FileCheckpointManager(FileManagerMixin, CheckpointManager): else: return [self.get_checkpoint_model(checkpoint_id, path)] - def restore_checkpoint(self, checkpoint_id, path): - """restore a file to a checkpointed state""" - path = path.strip('/') - self.log.info("restoring %s from checkpoint %s", path, checkpoint_id) - nb_path = self._get_os_path(path) - cp_path = self.get_checkpoint_path(checkpoint_id, path) - if not os.path.isfile(cp_path): - self.log.debug("checkpoint file does not exist: %s", cp_path) - self.no_such_checkpoint(path, checkpoint_id) - - # ensure notebook is readable (never restore from unreadable notebook) - if cp_path.endswith('.ipynb'): - with self.open(cp_path, 'r', encoding='utf-8') as f: - nbformat.read(f, as_version=4) - self.log.debug("copying %s -> %s", cp_path, nb_path) - with self.perm_to_403(): - self._copy(cp_path, nb_path) - - def delete_checkpoint(self, checkpoint_id, path): - """delete a file's checkpoint""" - path = path.strip('/') - cp_path = self.get_checkpoint_path(checkpoint_id, path) - if not os.path.isfile(cp_path): - self.no_such_checkpoint(path, checkpoint_id) - - self.log.debug("unlinking %s", cp_path) - with self.perm_to_403(): - os.unlink(cp_path) - # Checkpoint-related utilities def get_checkpoint_path(self, checkpoint_id, path): """find the path to a checkpoint""" @@ -413,6 +344,82 @@ class FileContentsManager(FileManagerMixin, ContentsManager): def _checkpoint_manager_class_default(self): return FileCheckpointManager + def is_hidden(self, path): + """Does the API style path correspond to a hidden directory or file? + + Parameters + ---------- + path : string + The path to check. This is an API path (`/` separated, + relative to root_dir). + + Returns + ------- + hidden : bool + Whether the path exists and is hidden. + """ + path = path.strip('/') + os_path = self._get_os_path(path=path) + return is_hidden(os_path, self.root_dir) + + def file_exists(self, path): + """Returns True if the file exists, else returns False. + + API-style wrapper for os.path.isfile + + Parameters + ---------- + path : string + The relative path to the file (with '/' as separator) + + Returns + ------- + exists : bool + Whether the file exists. + """ + path = path.strip('/') + os_path = self._get_os_path(path) + return os.path.isfile(os_path) + + def dir_exists(self, path): + """Does the API-style path refer to an extant directory? + + API-style wrapper for os.path.isdir + + Parameters + ---------- + path : string + The path to check. This is an API path (`/` separated, + relative to root_dir). + + Returns + ------- + exists : bool + Whether the path is indeed a directory. + """ + path = path.strip('/') + os_path = self._get_os_path(path=path) + return os.path.isdir(os_path) + + def exists(self, path): + """Returns True if the path exists, else returns False. + + API-style wrapper for os.path.exists + + Parameters + ---------- + path : string + The API path to the file (with '/' as separator) + + Returns + ------- + exists : bool + Whether the target exists. + """ + path = path.strip('/') + os_path = self._get_os_path(path=path) + return os.path.exists(os_path) + def _base_model(self, path): """Build the common base of a contents model""" os_path = self._get_os_path(path) @@ -518,7 +525,6 @@ class FileContentsManager(FileManagerMixin, ContentsManager): return model - def _notebook_model(self, path, content=True): """Build a notebook model @@ -529,11 +535,7 @@ class FileContentsManager(FileManagerMixin, ContentsManager): model['type'] = 'notebook' if content: os_path = self._get_os_path(path) - with self.open(os_path, 'r', encoding='utf-8') as f: - try: - nb = nbformat.read(f, as_version=4) - except Exception as e: - raise web.HTTPError(400, u"Unreadable Notebook: %s %r" % (os_path, e)) + nb = self._read_notebook(os_path, as_version=4) self.mark_trusted_cells(nb, path) model['content'] = nb model['format'] = 'json' @@ -582,13 +584,15 @@ class FileContentsManager(FileManagerMixin, ContentsManager): model = self._file_model(path, content=content, format=format) return model - def _save_notebook(self, os_path, model, path=''): + def _save_notebook(self, os_path, model, path): """save a notebook file""" - # Save the notebook file nb = nbformat.from_dict(model['content']) - self.check_and_sign(nb, path) + # One checkpoint should always exist for notebooks. + if not self.checkpoint_manager.list_checkpoints(path): + self.checkpoint_manager.create_checkpoint(nb, path) + with self.atomic_writing(os_path, encoding='utf-8') as f: nbformat.write(nb, f, version=nbformat.NO_CONVERT) @@ -632,11 +636,6 @@ class FileContentsManager(FileManagerMixin, ContentsManager): self.run_pre_save_hook(model=model, path=path) - cp_mgr = self.checkpoint_manager - # One checkpoint should always exist - if self.file_exists(path) and not cp_mgr.list_checkpoints(path): - cp_mgr.create_checkpoint(path) - os_path = self._get_os_path(path) self.log.debug("Saving %s", os_path) try: diff --git a/IPython/html/services/contents/manager.py b/IPython/html/services/contents/manager.py index 195cbf59b..10eeda56e 100644 --- a/IPython/html/services/contents/manager.py +++ b/IPython/html/services/contents/manager.py @@ -11,6 +11,7 @@ import re from tornado.web import HTTPError +from IPython import nbformat from IPython.config.configurable import LoggingConfigurable from IPython.nbformat import sign, validate, ValidationError from IPython.nbformat.v4 import new_notebook @@ -34,33 +35,36 @@ class CheckpointManager(LoggingConfigurable): Base class for managing checkpoints for a ContentsManager. """ - def create_checkpoint(self, path): + def create_checkpoint(self, nb, path): """Create a checkpoint of the current state of a file Returns a checkpoint_id for the new checkpoint. """ raise NotImplementedError("must be implemented in a subclass") + def get_checkpoint_content(self, checkpoint_id, path): + """Get the content of a checkpoint. + + Returns an unvalidated model with the same structure as + the return value of ContentsManager.get + """ + raise NotImplementedError("must be implemented in a subclass") + def rename_checkpoint(self, checkpoint_id, old_path, new_path): - """Rename a checkpoint from old_path to new_path.""" + """Rename a single checkpoint from old_path to new_path.""" raise NotImplementedError("must be implemented in a subclass") def delete_checkpoint(self, checkpoint_id, path): """delete a checkpoint for a file""" raise NotImplementedError("must be implemented in a subclass") - def restore_checkpoint(self, checkpoint_id, path): - """Restore a file from one of its checkpoints""" - raise NotImplementedError("must be implemented in a subclass") - def list_checkpoints(self, path): """Return a list of checkpoints for a given file""" raise NotImplementedError("must be implemented in a subclass") def rename_all_checkpoints(self, old_path, new_path): """Rename all checkpoints for old_path to new_path.""" - old_checkpoints = self.list_checkpoints(old_path) - for cp in old_checkpoints: + for cp in self.list_checkpoints(old_path): self.rename_checkpoint(cp['id'], old_path, new_path) def delete_all_checkpoints(self, path): @@ -490,22 +494,34 @@ class ContentsManager(LoggingConfigurable): return not any(fnmatch(name, glob) for glob in self.hide_globs) # Part 3: Checkpoints API - # By default, all methods are forwarded to our CheckpointManager instance. def create_checkpoint(self, path): - return self.checkpoint_manager.create_checkpoint(path) + """Create a checkpoint.""" - def rename_checkpoint(self, checkpoint_id, old_path, new_path): - return self.checkpoint_manager.rename_checkpoint( - checkpoint_id, - old_path, - new_path, - ) + nb = nbformat.from_dict(self.get(path, content=True)['content']) + self.check_and_sign(nb, path) + return self.checkpoint_manager.create_checkpoint(nb, path) def list_checkpoints(self, path): return self.checkpoint_manager.list_checkpoints(path) def restore_checkpoint(self, checkpoint_id, path): - return self.checkpoint_manager.restore_checkpoint(checkpoint_id, path) + """ + Restore a checkpoint. + """ + nb = self.checkpoint_manager.get_checkpoint_content( + checkpoint_id, + path, + ) + + self.mark_trusted_cells(nb, path) + + model = { + 'content': nb, + 'type': 'notebook', + } + + self.validate_notebook_model(model) + return self.save(model, path) def delete_checkpoint(self, checkpoint_id, path): return self.checkpoint_manager.delete_checkpoint(checkpoint_id, path) From 631a68d83e35e12b495e2509d68af826837ab765 Mon Sep 17 00:00:00 2001 From: Scott Sanderson Date: Mon, 29 Dec 2014 07:01:39 -0500 Subject: [PATCH 04/13] TEST: Test separate roots for Contents and Checkpoints. --- .../contents/tests/test_contents_api.py | 28 ++++++++++++++++++- 1 file changed, 27 insertions(+), 1 deletion(-) diff --git a/IPython/html/services/contents/tests/test_contents_api.py b/IPython/html/services/contents/tests/test_contents_api.py index b37e51dea..12a92feda 100644 --- a/IPython/html/services/contents/tests/test_contents_api.py +++ b/IPython/html/services/contents/tests/test_contents_api.py @@ -2,6 +2,7 @@ """Test the contents webservice API.""" import base64 +from contextlib import contextmanager import io import json import os @@ -21,6 +22,7 @@ from IPython.nbformat.v4 import ( from IPython.nbformat import v2 from IPython.utils import py3compat from IPython.utils.data import uniq_stable +from IPython.utils.tempdir import TemporaryDirectory def notebooks_only(dir_model): @@ -502,7 +504,6 @@ class APITest(NotebookTestBase): self.assertEqual(newnb.cells[0].source, u'Created by test ³') - def test_checkpoints(self): resp = self.api.read('foo/a.ipynb') r = self.api.new_checkpoint('foo/a.ipynb') @@ -540,3 +541,28 @@ class APITest(NotebookTestBase): self.assertEqual(r.status_code, 204) cps = self.api.get_checkpoints('foo/a.ipynb').json() self.assertEqual(cps, []) + + @contextmanager + def patch_cp_root(self, dirname): + """ + Temporarily patch the root dir of our checkpoint manager. + """ + cpm = self.notebook.contents_manager.checkpoint_manager + old_dirname = cpm.root_dir + cpm.root_dir = dirname + try: + yield + finally: + cpm.root_dir = old_dirname + + def test_checkpoints_separate_root(self): + """ + Test that FileCheckpointManager functions correctly even when it's + using a different root dir from FileContentsManager. This also keeps + the implementation honest for use with ContentsManagers that don't map + models to the filesystem + """ + + with TemporaryDirectory() as td: + with self.patch_cp_root(td): + self.test_checkpoints() From 23837e9ad48bcb4337ba2234f124c83820cd8bc8 Mon Sep 17 00:00:00 2001 From: Scott Sanderson Date: Thu, 1 Jan 2015 20:49:35 -0500 Subject: [PATCH 05/13] DEV: Remove unnecessary notary calls. --- IPython/html/services/contents/manager.py | 3 --- 1 file changed, 3 deletions(-) diff --git a/IPython/html/services/contents/manager.py b/IPython/html/services/contents/manager.py index 10eeda56e..f8170bea5 100644 --- a/IPython/html/services/contents/manager.py +++ b/IPython/html/services/contents/manager.py @@ -498,7 +498,6 @@ class ContentsManager(LoggingConfigurable): """Create a checkpoint.""" nb = nbformat.from_dict(self.get(path, content=True)['content']) - self.check_and_sign(nb, path) return self.checkpoint_manager.create_checkpoint(nb, path) def list_checkpoints(self, path): @@ -513,8 +512,6 @@ class ContentsManager(LoggingConfigurable): path, ) - self.mark_trusted_cells(nb, path) - model = { 'content': nb, 'type': 'notebook', From 7030a8717ab217cd67e43fb07285767db8affe7e Mon Sep 17 00:00:00 2001 From: Scott Sanderson Date: Thu, 1 Jan 2015 20:48:13 -0500 Subject: [PATCH 06/13] DEV: Add full support for non-notebook checkpoints. --- IPython/html/services/contents/filemanager.py | 214 ++++++++++-------- IPython/html/services/contents/manager.py | 39 +++- .../contents/tests/test_contents_api.py | 50 +++- .../services/contents/tests/test_manager.py | 4 +- 4 files changed, 203 insertions(+), 104 deletions(-) diff --git a/IPython/html/services/contents/filemanager.py b/IPython/html/services/contents/filemanager.py index 74feafc67..ef23d00dd 100644 --- a/IPython/html/services/contents/filemanager.py +++ b/IPython/html/services/contents/filemanager.py @@ -60,7 +60,10 @@ class FileManagerMixin(object): """ Mixin for ContentsAPI classes that interact with the filesystem. - Shared by both FileContentsManager and FileCheckpointManager. + Provides facilities for reading, writing, and copying both notebooks and + generic files. + + Shared by FileContentsManager and FileCheckpointManager. Note ---- @@ -114,17 +117,6 @@ class FileManagerMixin(object): except OSError: self.log.debug("copystat on %s failed", dest, exc_info=True) - def _read_notebook(self, os_path, as_version=4): - """Read a notebook from an os path.""" - with self.open(os_path, 'r', encoding='utf-8') as f: - try: - return nbformat.read(f, as_version=as_version) - except Exception as e: - raise web.HTTPError( - 400, - u"Unreadable Notebook: %s %r" % (os_path, e), - ) - def _get_os_path(self, path): """Given an API path, return its file system path. @@ -140,6 +132,70 @@ class FileManagerMixin(object): """ return to_os_path(path, self.root_dir) + def _read_notebook(self, os_path, as_version=4): + """Read a notebook from an os path.""" + with self.open(os_path, 'r', encoding='utf-8') as f: + try: + return nbformat.read(f, as_version=as_version) + except Exception as e: + raise web.HTTPError( + 400, + u"Unreadable Notebook: %s %r" % (os_path, e), + ) + + def _save_notebook(self, os_path, nb): + """Save a notebook to an os_path.""" + with self.atomic_writing(os_path, encoding='utf-8') as f: + nbformat.write(nb, f, version=nbformat.NO_CONVERT) + + def _read_file(self, os_path, format): + """Read a non-notebook file. + + os_path: The path to be read. + format: + If 'text', the contents will be decoded as UTF-8. + If 'base64', the raw bytes contents will be encoded as base64. + If not specified, try to decode as UTF-8, and fall back to base64 + """ + if not os.path.isfile(os_path): + raise web.HTTPError(400, "Cannot read non-file %s" % os_path) + + with self.open(os_path, 'rb') as f: + bcontent = f.read() + + if format is None or format == 'text': + # Try to interpret as unicode if format is unknown or if unicode + # was explicitly requested. + try: + return bcontent.decode('utf8'), 'text' + except UnicodeError as e: + if format == 'text': + raise web.HTTPError( + 400, + "%s is not UTF-8 encoded" % os_path, + reason='bad format', + ) + return base64.encodestring(bcontent).decode('ascii'), 'base64' + + def _save_file(self, os_path, content, format): + """Save content of a generic file.""" + if format not in {'text', 'base64'}: + raise web.HTTPError( + 400, + "Must specify format of file contents as 'text' or 'base64'", + ) + try: + if format == 'text': + bcontent = content.encode('utf8') + else: + b64_bytes = content.encode('ascii') + bcontent = base64.decodestring(b64_bytes) + except Exception as e: + raise web.HTTPError(400, u'Encoding error saving %s: %s' % (os_path, e)) + + with self.atomic_writing(os_path, text=False) as f: + f.write(bcontent) + class FileCheckpointManager(FileManagerMixin, CheckpointManager): """ @@ -167,39 +223,51 @@ class FileCheckpointManager(FileManagerMixin, CheckpointManager): return getcwd() # public checkpoint API - def create_checkpoint(self, nb, path): + def create_file_checkpoint(self, content, format, path): """Create a checkpoint from the current content of a notebook.""" path = path.strip('/') # only the one checkpoint ID: checkpoint_id = u"checkpoint" - os_checkpoint_path = self.get_checkpoint_path(checkpoint_id, path) + os_checkpoint_path = self.checkpoint_path(checkpoint_id, path) + self.log.debug("creating checkpoint for %s", path) + with self.perm_to_403(): + self._save_file(os_checkpoint_path, content, format=format) + + # return the checkpoint info + return self.checkpoint_model(checkpoint_id, os_checkpoint_path) + + def create_notebook_checkpoint(self, nb, path): + """Create a checkpoint from the current content of a notebook.""" + path = path.strip('/') + # only the one checkpoint ID: + checkpoint_id = u"checkpoint" + os_checkpoint_path = self.checkpoint_path(checkpoint_id, path) self.log.debug("creating checkpoint for %s", path) with self.perm_to_403(): self._save_notebook(os_checkpoint_path, nb) # return the checkpoint info - return self.get_checkpoint_model(checkpoint_id, path) + return self.checkpoint_model(checkpoint_id, os_checkpoint_path) - def get_checkpoint_content(self, checkpoint_id, path): + def get_checkpoint(self, checkpoint_id, path, type): """Get the content of a checkpoint. - Returns an unvalidated model with the same structure as - the return value of ContentsManager.get + Returns a pair of (content, type). """ path = path.strip('/') self.log.info("restoring %s from checkpoint %s", path, checkpoint_id) - os_checkpoint_path = self.get_checkpoint_path(checkpoint_id, path) - return self._read_notebook(os_checkpoint_path, as_version=4) - - def _save_notebook(self, os_path, nb): - """Save a notebook file.""" - with self.atomic_writing(os_path, encoding='utf-8') as f: - nbformat.write(nb, f, version=nbformat.NO_CONVERT) + os_checkpoint_path = self.checkpoint_path(checkpoint_id, path) + if not os.path.isfile(os_checkpoint_path): + self.no_such_checkpoint(path, checkpoint_id) + if type == 'notebook': + return self._read_notebook(os_checkpoint_path, as_version=4), None + else: + return self._read_file(os_checkpoint_path, format=None) def rename_checkpoint(self, checkpoint_id, old_path, new_path): """Rename a checkpoint from old_path to new_path.""" - old_cp_path = self.get_checkpoint_path(checkpoint_id, old_path) - new_cp_path = self.get_checkpoint_path(checkpoint_id, new_path) + old_cp_path = self.checkpoint_path(checkpoint_id, old_path) + new_cp_path = self.checkpoint_path(checkpoint_id, new_path) if os.path.isfile(old_cp_path): self.log.debug( "Renaming checkpoint %s -> %s", @@ -212,7 +280,7 @@ class FileCheckpointManager(FileManagerMixin, CheckpointManager): def delete_checkpoint(self, checkpoint_id, path): """delete a file's checkpoint""" path = path.strip('/') - cp_path = self.get_checkpoint_path(checkpoint_id, path) + cp_path = self.checkpoint_path(checkpoint_id, path) if not os.path.isfile(cp_path): self.no_such_checkpoint(path, checkpoint_id) @@ -227,14 +295,14 @@ class FileCheckpointManager(FileManagerMixin, CheckpointManager): """ path = path.strip('/') checkpoint_id = "checkpoint" - os_path = self.get_checkpoint_path(checkpoint_id, path) - if not os.path.exists(os_path): + os_path = self.checkpoint_path(checkpoint_id, path) + if not os.path.isfile(os_path): return [] else: - return [self.get_checkpoint_model(checkpoint_id, path)] + return [self.checkpoint_model(checkpoint_id, os_path)] # Checkpoint-related utilities - def get_checkpoint_path(self, checkpoint_id, path): + def checkpoint_path(self, checkpoint_id, path): """find the path to a checkpoint""" path = path.strip('/') parent, name = ('/' + path).rsplit('/', 1) @@ -252,11 +320,9 @@ class FileCheckpointManager(FileManagerMixin, CheckpointManager): cp_path = os.path.join(cp_dir, filename) return cp_path - def get_checkpoint_model(self, checkpoint_id, path): + def checkpoint_model(self, checkpoint_id, os_path): """construct the info dict for a given checkpoint""" - path = path.strip('/') - cp_path = self.get_checkpoint_path(checkpoint_id, path) - stats = os.stat(cp_path) + stats = os.stat(os_path) last_modified = tz.utcfromtimestamp(stats.st_mtime) info = dict( id=checkpoint_id, @@ -499,29 +565,17 @@ class FileContentsManager(FileManagerMixin, ContentsManager): os_path = self._get_os_path(path) if content: - if not os.path.isfile(os_path): - # could be FIFO - raise web.HTTPError(400, "Cannot get content of non-file %s" % os_path) - with self.open(os_path, 'rb') as f: - bcontent = f.read() - - if format != 'base64': - try: - model['content'] = bcontent.decode('utf8') - except UnicodeError as e: - if format == 'text': - raise web.HTTPError(400, "%s is not UTF-8 encoded" % path, reason='bad format') - else: - model['format'] = 'text' - default_mime = 'text/plain' - - if model['content'] is None: - model['content'] = base64.encodestring(bcontent).decode('ascii') - model['format'] = 'base64' - if model['format'] == 'base64': - default_mime = 'application/octet-stream' - - model['mimetype'] = mimetypes.guess_type(os_path)[0] or default_mime + content, format = self._read_file(os_path, format) + default_mime = { + 'text': 'text/plain', + 'base64': 'application/octet-stream' + }[format] + + model.update( + content=content, + format=format, + mimetype=mimetypes.guess_type(os_path)[0] or default_mime, + ) return model @@ -584,35 +638,6 @@ class FileContentsManager(FileManagerMixin, ContentsManager): model = self._file_model(path, content=content, format=format) return model - def _save_notebook(self, os_path, model, path): - """save a notebook file""" - nb = nbformat.from_dict(model['content']) - self.check_and_sign(nb, path) - - # One checkpoint should always exist for notebooks. - if not self.checkpoint_manager.list_checkpoints(path): - self.checkpoint_manager.create_checkpoint(nb, path) - - with self.atomic_writing(os_path, encoding='utf-8') as f: - nbformat.write(nb, f, version=nbformat.NO_CONVERT) - - def _save_file(self, os_path, model, path=''): - """save a non-notebook file""" - fmt = model.get('format', None) - if fmt not in {'text', 'base64'}: - raise web.HTTPError(400, "Must specify format of file contents as 'text' or 'base64'") - try: - content = model['content'] - if fmt == 'text': - bcontent = content.encode('utf8') - else: - b64_bytes = content.encode('ascii') - bcontent = base64.decodestring(b64_bytes) - except Exception as e: - raise web.HTTPError(400, u'Encoding error saving %s: %s' % (os_path, e)) - with self.atomic_writing(os_path, text=False) as f: - f.write(bcontent) - def _save_directory(self, os_path, model, path=''): """create a directory""" if is_hidden(os_path, self.root_dir): @@ -640,9 +665,18 @@ class FileContentsManager(FileManagerMixin, ContentsManager): self.log.debug("Saving %s", os_path) try: if model['type'] == 'notebook': - self._save_notebook(os_path, model, path) + nb = nbformat.from_dict(model['content']) + self.check_and_sign(nb, path) + self._save_notebook(os_path, nb) + # One checkpoint should always exist for notebooks. + if not self.checkpoint_manager.list_checkpoints(path): + self.checkpoint_manager.create_notebook_checkpoint( + nb, + path, + ) elif model['type'] == 'file': - self._save_file(os_path, model, path) + # Missing format will be handled internally by _save_file. + self._save_file(os_path, model['content'], model.get('format')) elif model['type'] == 'directory': self._save_directory(os_path, model, path) else: diff --git a/IPython/html/services/contents/manager.py b/IPython/html/services/contents/manager.py index f8170bea5..1a7d29c7c 100644 --- a/IPython/html/services/contents/manager.py +++ b/IPython/html/services/contents/manager.py @@ -34,15 +34,21 @@ class CheckpointManager(LoggingConfigurable): """ Base class for managing checkpoints for a ContentsManager. """ + def create_file_checkpoint(self, content, format, path): + """Create a checkpoint of the current state of a file + + Returns a checkpoint model for the new checkpoint. + """ + raise NotImplementedError("must be implemented in a subclass") - def create_checkpoint(self, nb, path): + def create_notebook_checkpoint(self, nb, path): """Create a checkpoint of the current state of a file - Returns a checkpoint_id for the new checkpoint. + Returns a checkpoint model for the new checkpoint. """ raise NotImplementedError("must be implemented in a subclass") - def get_checkpoint_content(self, checkpoint_id, path): + def get_checkpoint(self, checkpoint_id, path, type): """Get the content of a checkpoint. Returns an unvalidated model with the same structure as @@ -496,9 +502,19 @@ class ContentsManager(LoggingConfigurable): # Part 3: Checkpoints API def create_checkpoint(self, path): """Create a checkpoint.""" - - nb = nbformat.from_dict(self.get(path, content=True)['content']) - return self.checkpoint_manager.create_checkpoint(nb, path) + model = self.get(path, content=True) + type = model['type'] + if type == 'notebook': + return self.checkpoint_manager.create_notebook_checkpoint( + model['content'], + path, + ) + elif type == 'file': + return self.checkpoint_manager.create_file_checkpoint( + model['content'], + model['format'], + path, + ) def list_checkpoints(self, path): return self.checkpoint_manager.list_checkpoints(path) @@ -507,17 +523,18 @@ class ContentsManager(LoggingConfigurable): """ Restore a checkpoint. """ - nb = self.checkpoint_manager.get_checkpoint_content( + type = self.get(path, content=False)['type'] + content, format = self.checkpoint_manager.get_checkpoint( checkpoint_id, path, + type, ) model = { - 'content': nb, - 'type': 'notebook', + 'type': type, + 'content': content, + 'format': format, } - - self.validate_notebook_model(model) return self.save(model, path) def delete_checkpoint(self, checkpoint_id, path): diff --git a/IPython/html/services/contents/tests/test_contents_api.py b/IPython/html/services/contents/tests/test_contents_api.py index 12a92feda..3c0d33aa0 100644 --- a/IPython/html/services/contents/tests/test_contents_api.py +++ b/IPython/html/services/contents/tests/test_contents_api.py @@ -542,6 +542,49 @@ class APITest(NotebookTestBase): cps = self.api.get_checkpoints('foo/a.ipynb').json() self.assertEqual(cps, []) + def test_file_checkpoints(self): + """ + Test checkpointing of non-notebook files. + """ + filename = 'foo/a.txt' + resp = self.api.read(filename) + orig_content = json.loads(resp.text)['content'] + + # Create a checkpoint. + r = self.api.new_checkpoint(filename) + self.assertEqual(r.status_code, 201) + cp1 = r.json() + self.assertEqual(set(cp1), {'id', 'last_modified'}) + self.assertEqual(r.headers['Location'].split('/')[-1], cp1['id']) + + # Modify the file and save. + new_content = orig_content + '\nsecond line' + model = { + 'content': new_content, + 'type': 'file', + 'format': 'text', + } + resp = self.api.save(filename, body=json.dumps(model)) + + # List checkpoints + cps = self.api.get_checkpoints(filename).json() + self.assertEqual(cps, [cp1]) + + content = self.api.read(filename).json()['content'] + self.assertEqual(content, new_content) + + # Restore cp1 + r = self.api.restore_checkpoint(filename, cp1['id']) + self.assertEqual(r.status_code, 204) + restored_content = self.api.read(filename).json()['content'] + self.assertEqual(restored_content, orig_content) + + # Delete cp1 + r = self.api.delete_checkpoint(filename, cp1['id']) + self.assertEqual(r.status_code, 204) + cps = self.api.get_checkpoints(filename).json() + self.assertEqual(cps, []) + @contextmanager def patch_cp_root(self, dirname): """ @@ -561,8 +604,13 @@ class APITest(NotebookTestBase): using a different root dir from FileContentsManager. This also keeps the implementation honest for use with ContentsManagers that don't map models to the filesystem - """ + Override this method to a no-op when testing other managers. + """ with TemporaryDirectory() as td: with self.patch_cp_root(td): self.test_checkpoints() + + with TemporaryDirectory() as td: + with self.patch_cp_root(td): + self.test_file_checkpoints() diff --git a/IPython/html/services/contents/tests/test_manager.py b/IPython/html/services/contents/tests/test_manager.py index 2a81e622e..2cade002e 100644 --- a/IPython/html/services/contents/tests/test_manager.py +++ b/IPython/html/services/contents/tests/test_manager.py @@ -85,10 +85,10 @@ class TestFileContentsManager(TestCase): os.mkdir(os.path.join(td, subd)) fm = FileContentsManager(root_dir=root) cpm = fm.checkpoint_manager - cp_dir = cpm.get_checkpoint_path( + cp_dir = cpm.checkpoint_path( 'cp', 'test.ipynb' ) - cp_subdir = cpm.get_checkpoint_path( + cp_subdir = cpm.checkpoint_path( 'cp', '/%s/test.ipynb' % subd ) self.assertNotEqual(cp_dir, cp_subdir) From 1e2e86dcca598948b4ebbdbb312c7f1f87a3624c Mon Sep 17 00:00:00 2001 From: Scott Sanderson Date: Tue, 6 Jan 2015 14:28:59 -0500 Subject: [PATCH 07/13] MAINT: Return dicts from CheckpointManager.get_checkpoint. The output is going to get converted to a dict anyway, and this makes it easier to pipe output from a CheckpointManager directly to a ContentsManager. --- IPython/html/services/contents/filemanager.py | 17 ++++++++++++++--- IPython/html/services/contents/manager.py | 19 +++++++------------ 2 files changed, 21 insertions(+), 15 deletions(-) diff --git a/IPython/html/services/contents/filemanager.py b/IPython/html/services/contents/filemanager.py index ef23d00dd..7fe4ee84d 100644 --- a/IPython/html/services/contents/filemanager.py +++ b/IPython/html/services/contents/filemanager.py @@ -252,7 +252,7 @@ class FileCheckpointManager(FileManagerMixin, CheckpointManager): def get_checkpoint(self, checkpoint_id, path, type): """Get the content of a checkpoint. - Returns a pair of (content, type). + Returns a model suitable for passing to ContentsManager.save. """ path = path.strip('/') self.log.info("restoring %s from checkpoint %s", path, checkpoint_id) @@ -260,9 +260,20 @@ class FileCheckpointManager(FileManagerMixin, CheckpointManager): if not os.path.isfile(os_checkpoint_path): self.no_such_checkpoint(path, checkpoint_id) if type == 'notebook': - return self._read_notebook(os_checkpoint_path, as_version=4), None + return { + 'type': type, + 'content': self._read_notebook( + os_checkpoint_path, + as_version=4, + ), + } else: - return self._read_file(os_checkpoint_path, format=None) + content, format = self._read_file(os_checkpoint_path, format=None) + return { + 'type': type, + 'content': content, + 'format': format, + } def rename_checkpoint(self, checkpoint_id, old_path, new_path): """Rename a checkpoint from old_path to new_path.""" diff --git a/IPython/html/services/contents/manager.py b/IPython/html/services/contents/manager.py index 1a7d29c7c..ec2f7db7c 100644 --- a/IPython/html/services/contents/manager.py +++ b/IPython/html/services/contents/manager.py @@ -523,19 +523,14 @@ class ContentsManager(LoggingConfigurable): """ Restore a checkpoint. """ - type = self.get(path, content=False)['type'] - content, format = self.checkpoint_manager.get_checkpoint( - checkpoint_id, - path, - type, + return self.save( + model=self.checkpoint_manager.get_checkpoint( + checkpoint_id, + path, + self.get(path, content=False)['type'] + ), + path=path, ) - model = { - 'type': type, - 'content': content, - 'format': format, - } - return self.save(model, path) - def delete_checkpoint(self, checkpoint_id, path): return self.checkpoint_manager.delete_checkpoint(checkpoint_id, path) From 2b73f1e62045414463edaff07f44df71352d2a67 Mon Sep 17 00:00:00 2001 From: Scott Sanderson Date: Tue, 6 Jan 2015 23:59:31 -0500 Subject: [PATCH 08/13] DEV: Implement os.walk analog for ContentsManagers. --- IPython/html/services/contents/manager.py | 40 ++++++++++ .../contents/tests/test_contents_api.py | 79 +++++++++++++++++++ 2 files changed, 119 insertions(+) diff --git a/IPython/html/services/contents/manager.py b/IPython/html/services/contents/manager.py index ec2f7db7c..f8aeec2c5 100644 --- a/IPython/html/services/contents/manager.py +++ b/IPython/html/services/contents/manager.py @@ -30,6 +30,21 @@ from IPython.utils.py3compat import string_types copy_pat = re.compile(r'\-Copy\d*\.') +def _separate_dirs_files(models): + """ + Split an iterable of models into a list of file paths and a list of + directory paths. + """ + dirs = [] + files = [] + for model in models: + if model['type'] == 'directory': + dirs.append(model['path']) + else: + files.append(model['path']) + return dirs, files + + class CheckpointManager(LoggingConfigurable): """ Base class for managing checkpoints for a ContentsManager. @@ -499,6 +514,31 @@ class ContentsManager(LoggingConfigurable): """Should this file/directory name be displayed in a listing?""" return not any(fnmatch(name, glob) for glob in self.hide_globs) + def walk(self): + """ + Like os.walk, but written in terms of the ContentsAPI. + + Returns a generator of tuples of the form: + (directory name, [subdirectories], [files in directory]) + """ + return self._walk(['']) + + def _walk(self, dirs): + """ + Recursive helper for walk. + """ + for directory in dirs: + children = self.get( + directory, + content=True, + type='directory', + )['content'] + dirs, files = map(sorted, _separate_dirs_files(children)) + yield (directory, dirs, files) + if dirs: + for entry in self._walk(dirs): + yield(entry) + # Part 3: Checkpoints API def create_checkpoint(self, path): """Create a checkpoint.""" diff --git a/IPython/html/services/contents/tests/test_contents_api.py b/IPython/html/services/contents/tests/test_contents_api.py index 3c0d33aa0..6fa2ec9ea 100644 --- a/IPython/html/services/contents/tests/test_contents_api.py +++ b/IPython/html/services/contents/tests/test_contents_api.py @@ -614,3 +614,82 @@ class APITest(NotebookTestBase): with TemporaryDirectory() as td: with self.patch_cp_root(td): self.test_file_checkpoints() + + def test_walk(self): + """ + Test ContentsManager.walk. + """ + results = list(self.notebook.contents_manager.walk()) + expected = [ + ( + '', + [ + 'Directory with spaces in', + 'foo', + 'ordering', + u'unicodé', + u'å b', + ], + ['inroot.blob', 'inroot.ipynb', 'inroot.txt'], + ), + ( + 'Directory with spaces in', + [], + ['inspace.blob', 'inspace.ipynb', 'inspace.txt'], + ), + ( + 'foo', + ['bar'], + [ + 'a.blob', 'a.ipynb', 'a.txt', + 'b.blob', 'b.ipynb', 'b.txt', + 'name with spaces.blob', + 'name with spaces.ipynb', + 'name with spaces.txt', + u'unicodé.blob', u'unicodé.ipynb', u'unicodé.txt' + ] + ), + ( + 'foo/bar', + [], + ['baz.blob', 'baz.ipynb', 'baz.txt'], + ), + ( + 'ordering', + [], + [ + 'A.blob', 'A.ipynb', 'A.txt', + 'C.blob', 'C.ipynb', 'C.txt', + 'b.blob', 'b.ipynb', 'b.txt', + ], + ), + ( + u'unicodé', + [], + ['innonascii.blob', 'innonascii.ipynb', 'innonascii.txt'], + ), + ( + u'å b', + [], + [u'ç d.blob', u'ç d.ipynb', u'ç d.txt'], + ), + ] + + for idx, (dname, subdirs, files) in enumerate(expected): + result_dname, result_subdirs, result_files = results[idx] + if dname == '': + sep = '' + else: + sep = '/' + self.assertEqual( + dname, + result_dname, + ) + self.assertEqual( + [sep.join([dname, sub]) for sub in subdirs], + result_subdirs, + ) + self.assertEqual( + [sep.join([dname, fname]) for fname in files], + result_files, + ) From 55d4e20aaf5a38ee5ec31ea90fb187cc7701905d Mon Sep 17 00:00:00 2001 From: Scott Sanderson Date: Wed, 7 Jan 2015 16:13:42 -0500 Subject: [PATCH 09/13] DEV: Remove ContentsManager.walk. --- IPython/html/services/contents/manager.py | 40 ---------- .../contents/tests/test_contents_api.py | 79 ------------------- 2 files changed, 119 deletions(-) diff --git a/IPython/html/services/contents/manager.py b/IPython/html/services/contents/manager.py index f8aeec2c5..ec2f7db7c 100644 --- a/IPython/html/services/contents/manager.py +++ b/IPython/html/services/contents/manager.py @@ -30,21 +30,6 @@ from IPython.utils.py3compat import string_types copy_pat = re.compile(r'\-Copy\d*\.') -def _separate_dirs_files(models): - """ - Split an iterable of models into a list of file paths and a list of - directory paths. - """ - dirs = [] - files = [] - for model in models: - if model['type'] == 'directory': - dirs.append(model['path']) - else: - files.append(model['path']) - return dirs, files - - class CheckpointManager(LoggingConfigurable): """ Base class for managing checkpoints for a ContentsManager. @@ -514,31 +499,6 @@ class ContentsManager(LoggingConfigurable): """Should this file/directory name be displayed in a listing?""" return not any(fnmatch(name, glob) for glob in self.hide_globs) - def walk(self): - """ - Like os.walk, but written in terms of the ContentsAPI. - - Returns a generator of tuples of the form: - (directory name, [subdirectories], [files in directory]) - """ - return self._walk(['']) - - def _walk(self, dirs): - """ - Recursive helper for walk. - """ - for directory in dirs: - children = self.get( - directory, - content=True, - type='directory', - )['content'] - dirs, files = map(sorted, _separate_dirs_files(children)) - yield (directory, dirs, files) - if dirs: - for entry in self._walk(dirs): - yield(entry) - # Part 3: Checkpoints API def create_checkpoint(self, path): """Create a checkpoint.""" diff --git a/IPython/html/services/contents/tests/test_contents_api.py b/IPython/html/services/contents/tests/test_contents_api.py index 6fa2ec9ea..3c0d33aa0 100644 --- a/IPython/html/services/contents/tests/test_contents_api.py +++ b/IPython/html/services/contents/tests/test_contents_api.py @@ -614,82 +614,3 @@ class APITest(NotebookTestBase): with TemporaryDirectory() as td: with self.patch_cp_root(td): self.test_file_checkpoints() - - def test_walk(self): - """ - Test ContentsManager.walk. - """ - results = list(self.notebook.contents_manager.walk()) - expected = [ - ( - '', - [ - 'Directory with spaces in', - 'foo', - 'ordering', - u'unicodé', - u'å b', - ], - ['inroot.blob', 'inroot.ipynb', 'inroot.txt'], - ), - ( - 'Directory with spaces in', - [], - ['inspace.blob', 'inspace.ipynb', 'inspace.txt'], - ), - ( - 'foo', - ['bar'], - [ - 'a.blob', 'a.ipynb', 'a.txt', - 'b.blob', 'b.ipynb', 'b.txt', - 'name with spaces.blob', - 'name with spaces.ipynb', - 'name with spaces.txt', - u'unicodé.blob', u'unicodé.ipynb', u'unicodé.txt' - ] - ), - ( - 'foo/bar', - [], - ['baz.blob', 'baz.ipynb', 'baz.txt'], - ), - ( - 'ordering', - [], - [ - 'A.blob', 'A.ipynb', 'A.txt', - 'C.blob', 'C.ipynb', 'C.txt', - 'b.blob', 'b.ipynb', 'b.txt', - ], - ), - ( - u'unicodé', - [], - ['innonascii.blob', 'innonascii.ipynb', 'innonascii.txt'], - ), - ( - u'å b', - [], - [u'ç d.blob', u'ç d.ipynb', u'ç d.txt'], - ), - ] - - for idx, (dname, subdirs, files) in enumerate(expected): - result_dname, result_subdirs, result_files = results[idx] - if dname == '': - sep = '' - else: - sep = '/' - self.assertEqual( - dname, - result_dname, - ) - self.assertEqual( - [sep.join([dname, sub]) for sub in subdirs], - result_subdirs, - ) - self.assertEqual( - [sep.join([dname, fname]) for fname in files], - result_files, - ) From 12fe97e2af7cda431610b35ab6ba22bfd37ae609 Mon Sep 17 00:00:00 2001 From: Scott Sanderson Date: Wed, 7 Jan 2015 21:10:38 -0500 Subject: [PATCH 10/13] DEV: Allow CheckpointManagers to optimize for shared backends. On `create_checkpoint` and `restore_checkpoint`, pass a path and a `ContentsManager` to `CheckpointManager` instead of an already-loaded model. The `CheckpointManager` base class provides a correct implementation of these methods that's generic across any ContentsManager, but subclasses are free to specialize when the storage backend of `ContentsManager` is shared. --- IPython/html/services/contents/filemanager.py | 149 ++++++++++++------ IPython/html/services/contents/manager.py | 53 ++++--- .../contents/tests/test_contents_api.py | 22 +++ 3 files changed, 148 insertions(+), 76 deletions(-) diff --git a/IPython/html/services/contents/filemanager.py b/IPython/html/services/contents/filemanager.py index 7fe4ee84d..81b64b22c 100644 --- a/IPython/html/services/contents/filemanager.py +++ b/IPython/html/services/contents/filemanager.py @@ -222,59 +222,48 @@ class FileCheckpointManager(FileManagerMixin, CheckpointManager): except AttributeError: return getcwd() - # public checkpoint API - def create_file_checkpoint(self, content, format, path): - """Create a checkpoint from the current content of a notebook.""" - path = path.strip('/') - # only the one checkpoint ID: - checkpoint_id = u"checkpoint" - os_checkpoint_path = self.checkpoint_path(checkpoint_id, path) - self.log.debug("creating checkpoint for %s", path) - with self.perm_to_403(): - self._save_file(os_checkpoint_path, content, format=format) - - # return the checkpoint info - return self.checkpoint_model(checkpoint_id, os_checkpoint_path) - - def create_notebook_checkpoint(self, nb, path): - """Create a checkpoint from the current content of a notebook.""" - path = path.strip('/') - # only the one checkpoint ID: - checkpoint_id = u"checkpoint" - os_checkpoint_path = self.checkpoint_path(checkpoint_id, path) - self.log.debug("creating checkpoint for %s", path) - with self.perm_to_403(): - self._save_notebook(os_checkpoint_path, nb) + # ContentsManager-dependent checkpoint API + def create_checkpoint(self, contents_mgr, path): + """ + Create a checkpoint. - # return the checkpoint info - return self.checkpoint_model(checkpoint_id, os_checkpoint_path) + If contents_mgr is backed by the local filesystem, just copy the + appropriate file to the checkpoint directory. Otherwise, ask the + ContentsManager for a model and write it ourselves. + """ + if contents_mgr.backend == 'local_file': + # We know that the file is in the local filesystem, so just copy + # from the base location to our location. + checkpoint_id = u'checkpoint' + src_path = contents_mgr._get_os_path(path) + dest_path = self.checkpoint_path(checkpoint_id, path) + self._copy(src_path, dest_path) + return self.checkpoint_model(checkpoint_id, dest_path) + else: + return super(FileCheckpointManager, self).create_checkpoint( + contents_mgr, path, + ) - def get_checkpoint(self, checkpoint_id, path, type): - """Get the content of a checkpoint. + def restore_checkpoint(self, contents_mgr, checkpoint_id, path): + """ + Restore a checkpoint. - Returns a model suitable for passing to ContentsManager.save. + If contents_mgr is backed by the local filesystem, just copy the + appropriate file from the checkpoint directory. Otherwise, load the + model and pass it to ContentsManager.save. """ - path = path.strip('/') - self.log.info("restoring %s from checkpoint %s", path, checkpoint_id) - os_checkpoint_path = self.checkpoint_path(checkpoint_id, path) - if not os.path.isfile(os_checkpoint_path): - self.no_such_checkpoint(path, checkpoint_id) - if type == 'notebook': - return { - 'type': type, - 'content': self._read_notebook( - os_checkpoint_path, - as_version=4, - ), - } + if contents_mgr.backend == 'local_file': + # We know that the file is in the local filesystem, so just copy + # from our base location to the location expected by content + src_path = self.checkpoint_path(checkpoint_id, path) + dest_path = contents_mgr._get_os_path(path) + self._copy(src_path, dest_path) else: - content, format = self._read_file(os_checkpoint_path, format=None) - return { - 'type': type, - 'content': content, - 'format': format, - } + super(FileCheckpointManager, self).restore_checkpoint( + contents_mgr, checkpoint_id, path + ) + # ContentsManager-independent checkpoint API def rename_checkpoint(self, checkpoint_id, old_path, new_path): """Rename a checkpoint from old_path to new_path.""" old_cp_path = self.checkpoint_path(checkpoint_id, old_path) @@ -341,6 +330,64 @@ class FileCheckpointManager(FileManagerMixin, CheckpointManager): ) return info + def create_file_checkpoint(self, content, format, path): + """Create a checkpoint from the current content of a notebook.""" + path = path.strip('/') + # only the one checkpoint ID: + checkpoint_id = u"checkpoint" + os_checkpoint_path = self.checkpoint_path(checkpoint_id, path) + self.log.debug("creating checkpoint for %s", path) + with self.perm_to_403(): + self._save_file(os_checkpoint_path, content, format=format) + + # return the checkpoint info + return self.checkpoint_model(checkpoint_id, os_checkpoint_path) + + def create_notebook_checkpoint(self, nb, path): + """Create a checkpoint from the current content of a notebook.""" + path = path.strip('/') + # only the one checkpoint ID: + checkpoint_id = u"checkpoint" + os_checkpoint_path = self.checkpoint_path(checkpoint_id, path) + self.log.debug("creating checkpoint for %s", path) + with self.perm_to_403(): + self._save_notebook(os_checkpoint_path, nb) + + # return the checkpoint info + return self.checkpoint_model(checkpoint_id, os_checkpoint_path) + + def get_checkpoint(self, checkpoint_id, path, type): + """Get the content of a checkpoint. + + Returns a model suitable for passing to ContentsManager.save. + """ + path = path.strip('/') + self.log.info("restoring %s from checkpoint %s", path, checkpoint_id) + os_checkpoint_path = self.checkpoint_path(checkpoint_id, path) + if not os.path.isfile(os_checkpoint_path): + self.no_such_checkpoint(path, checkpoint_id) + + if type == 'notebook': + return { + 'type': type, + 'content': self._read_notebook( + os_checkpoint_path, + as_version=4, + ), + } + elif type == 'file': + content, format = self._read_file(os_checkpoint_path, format=None) + return { + 'type': type, + 'content': content, + 'format': format, + } + else: + raise web.HTTPError( + 500, + u'Unexpected type %s' % type + ) + # Error Handling def no_such_checkpoint(self, path, checkpoint_id): raise web.HTTPError( @@ -421,6 +468,9 @@ class FileContentsManager(FileManagerMixin, ContentsManager): def _checkpoint_manager_class_default(self): return FileCheckpointManager + def _backend_default(self): + return 'local_file' + def is_hidden(self, path): """Does the API style path correspond to a hidden directory or file? @@ -681,10 +731,7 @@ class FileContentsManager(FileManagerMixin, ContentsManager): self._save_notebook(os_path, nb) # One checkpoint should always exist for notebooks. if not self.checkpoint_manager.list_checkpoints(path): - self.checkpoint_manager.create_notebook_checkpoint( - nb, - path, - ) + self.create_checkpoint(path) elif model['type'] == 'file': # Missing format will be handled internally by _save_file. self._save_file(os_path, model['content'], model.get('format')) diff --git a/IPython/html/services/contents/manager.py b/IPython/html/services/contents/manager.py index ec2f7db7c..6b25dc118 100644 --- a/IPython/html/services/contents/manager.py +++ b/IPython/html/services/contents/manager.py @@ -11,7 +11,6 @@ import re from tornado.web import HTTPError -from IPython import nbformat from IPython.config.configurable import LoggingConfigurable from IPython.nbformat import sign, validate, ValidationError from IPython.nbformat.v4 import new_notebook @@ -34,6 +33,28 @@ class CheckpointManager(LoggingConfigurable): """ Base class for managing checkpoints for a ContentsManager. """ + + def create_checkpoint(self, contents_mgr, path): + model = contents_mgr.get(path, content=True) + type = model['type'] + if type == 'notebook': + return self.create_notebook_checkpoint( + model['content'], + path, + ) + elif type == 'file': + return self.create_file_checkpoint( + model['content'], + model['format'], + path, + ) + + def restore_checkpoint(self, contents_mgr, checkpoint_id, path): + """Restore a checkpoint.""" + type = contents_mgr.get(path, content=False)['type'] + model = self.get_checkpoint(checkpoint_id, path, type) + contents_mgr.save(model, path) + def create_file_checkpoint(self, content, format, path): """Create a checkpoint of the current state of a file @@ -159,6 +180,7 @@ class ContentsManager(LoggingConfigurable): checkpoint_manager_class = Type(CheckpointManager, config=True) checkpoint_manager = Instance(CheckpointManager, config=True) checkpoint_manager_kwargs = Dict(allow_none=False, config=True) + backend = Unicode(default_value="") def _checkpoint_manager_default(self): return self.checkpoint_manager_class(**self.checkpoint_manager_kwargs) @@ -502,35 +524,16 @@ class ContentsManager(LoggingConfigurable): # Part 3: Checkpoints API def create_checkpoint(self, path): """Create a checkpoint.""" - model = self.get(path, content=True) - type = model['type'] - if type == 'notebook': - return self.checkpoint_manager.create_notebook_checkpoint( - model['content'], - path, - ) - elif type == 'file': - return self.checkpoint_manager.create_file_checkpoint( - model['content'], - model['format'], - path, - ) - - def list_checkpoints(self, path): - return self.checkpoint_manager.list_checkpoints(path) + return self.checkpoint_manager.create_checkpoint(self, path) def restore_checkpoint(self, checkpoint_id, path): """ Restore a checkpoint. """ - return self.save( - model=self.checkpoint_manager.get_checkpoint( - checkpoint_id, - path, - self.get(path, content=False)['type'] - ), - path=path, - ) + self.checkpoint_manager.restore_checkpoint(self, checkpoint_id, path) + + def list_checkpoints(self, path): + return self.checkpoint_manager.list_checkpoints(path) def delete_checkpoint(self, checkpoint_id, path): return self.checkpoint_manager.delete_checkpoint(checkpoint_id, path) diff --git a/IPython/html/services/contents/tests/test_contents_api.py b/IPython/html/services/contents/tests/test_contents_api.py index 3c0d33aa0..b19a8faed 100644 --- a/IPython/html/services/contents/tests/test_contents_api.py +++ b/IPython/html/services/contents/tests/test_contents_api.py @@ -614,3 +614,25 @@ class APITest(NotebookTestBase): with TemporaryDirectory() as td: with self.patch_cp_root(td): self.test_file_checkpoints() + + @contextmanager + def patch_cm_backend(self): + """ + Temporarily patch our ContentsManager to present a different backend. + """ + mgr = self.notebook.contents_manager + old_backend = mgr.backend + mgr.backend = "" + try: + yield + finally: + mgr.backend = old_backend + + def test_checkpoints_empty_backend(self): + with self.patch_cm_backend(): + self.test_checkpoints() + + with self.patch_cm_backend(): + self.test_file_checkpoints() + + From 021e2da495d21c55e8166474460a3b71a9ac37f1 Mon Sep 17 00:00:00 2001 From: Scott Sanderson Date: Thu, 8 Jan 2015 13:49:58 -0500 Subject: [PATCH 11/13] DEV: Separate FileCheckpointManager and GenericFileCheckpointManager. - Adds a `GenericCheckpointMixin` as a helper for implementing the two boundary-traversing Checkpoint API methods, `create_checkpoint` and `restore_checkpoint`. - `GenericFileCheckpointManager` is implemented as a subclass of `FileCheckpointManager` using `GenericCheckpointMixin`. Note that this is the safe subtyping relationship because of method signature *contra*variance: `FileCheckpointManager` accepts `FileContentsManager` in its method signatures type, whereas `GenericFileCheckpointManager` accepts any `ContentsManager`. - Moved Checkpoint-related classes to their own files. --- IPython/html/services/contents/checkpoints.py | 112 ++++++ .../html/services/contents/filecheckpoints.py | 198 ++++++++++ IPython/html/services/contents/fileio.py | 166 ++++++++ IPython/html/services/contents/filemanager.py | 361 +----------------- IPython/html/services/contents/manager.py | 73 +--- .../contents/tests/test_contents_api.py | 29 +- 6 files changed, 494 insertions(+), 445 deletions(-) create mode 100644 IPython/html/services/contents/checkpoints.py create mode 100644 IPython/html/services/contents/filecheckpoints.py create mode 100644 IPython/html/services/contents/fileio.py diff --git a/IPython/html/services/contents/checkpoints.py b/IPython/html/services/contents/checkpoints.py new file mode 100644 index 000000000..0ba546ff0 --- /dev/null +++ b/IPython/html/services/contents/checkpoints.py @@ -0,0 +1,112 @@ +""" +Classes for managing Checkpoints. +""" + +# Copyright (c) IPython Development Team. +# Distributed under the terms of the Modified BSD License. + +from IPython.config.configurable import LoggingConfigurable + + +class CheckpointManager(LoggingConfigurable): + """ + Base class for managing checkpoints for a ContentsManager. + + Subclasses are required to implement: + + create_checkpoint(self, contents_mgr, path) + restore_checkpoint(self, contents_mgr, checkpoint_id, path) + rename_checkpoint(self, checkpoint_id, old_path, new_path) + delete_checkpoint(self, checkpoint_id, path) + list_checkpoints(self, path) + """ + def create_checkpoint(self, contents_mgr, path): + """Create a checkpoint.""" + raise NotImplementedError("must be implemented in a subclass") + + def restore_checkpoint(self, contents_mgr, checkpoint_id, path): + """Restore a checkpoint""" + raise NotImplementedError("must be implemented in a subclass") + + def rename_checkpoint(self, checkpoint_id, old_path, new_path): + """Rename a single checkpoint from old_path to new_path.""" + raise NotImplementedError("must be implemented in a subclass") + + def delete_checkpoint(self, checkpoint_id, path): + """delete a checkpoint for a file""" + raise NotImplementedError("must be implemented in a subclass") + + def list_checkpoints(self, path): + """Return a list of checkpoints for a given file""" + raise NotImplementedError("must be implemented in a subclass") + + def rename_all_checkpoints(self, old_path, new_path): + """Rename all checkpoints for old_path to new_path.""" + for cp in self.list_checkpoints(old_path): + self.rename_checkpoint(cp['id'], old_path, new_path) + + def delete_all_checkpoints(self, path): + """Delete all checkpoints for the given path.""" + for checkpoint in self.list_checkpoints(path): + self.delete_checkpoint(checkpoint['id'], path) + + +class GenericCheckpointMixin(object): + """ + Helper for creating CheckpointManagers that can be used with any + ContentsManager. + + Provides an implementation of `create_checkpoint` and `restore_checkpoint` + in terms of the following operations: + + create_file_checkpoint(self, content, format, path) + create_notebook_checkpoint(self, nb, path) + get_checkpoint(self, checkpoint_id, path, type) + + **Any** valid CheckpointManager implementation should also be valid when + this mixin is applied. + """ + + def create_checkpoint(self, contents_mgr, path): + model = contents_mgr.get(path, content=True) + type = model['type'] + if type == 'notebook': + return self.create_notebook_checkpoint( + model['content'], + path, + ) + elif type == 'file': + return self.create_file_checkpoint( + model['content'], + model['format'], + path, + ) + + def restore_checkpoint(self, contents_mgr, checkpoint_id, path): + """Restore a checkpoint.""" + type = contents_mgr.get(path, content=False)['type'] + model = self.get_checkpoint(checkpoint_id, path, type) + contents_mgr.save(model, path) + + # Required Methods + def create_file_checkpoint(self, content, format, path): + """Create a checkpoint of the current state of a file + + Returns a checkpoint model for the new checkpoint. + """ + raise NotImplementedError("must be implemented in a subclass") + + def create_notebook_checkpoint(self, nb, path): + """Create a checkpoint of the current state of a file + + Returns a checkpoint model for the new checkpoint. + """ + raise NotImplementedError("must be implemented in a subclass") + + def get_checkpoint(self, checkpoint_id, path, type): + """Get the content of a checkpoint. + + Returns an unvalidated model with the same structure as + the return value of ContentsManager.get + """ + raise NotImplementedError("must be implemented in a subclass") diff --git a/IPython/html/services/contents/filecheckpoints.py b/IPython/html/services/contents/filecheckpoints.py new file mode 100644 index 000000000..e1e4f8c60 --- /dev/null +++ b/IPython/html/services/contents/filecheckpoints.py @@ -0,0 +1,198 @@ +""" +File-based CheckpointManagers. +""" +import os +import shutil + +from tornado.web import HTTPError + +from .checkpoints import ( + CheckpointManager, + GenericCheckpointMixin, +) +from .fileio import FileManagerMixin + +from IPython.utils import tz +from IPython.utils.path import ensure_dir_exists +from IPython.utils.py3compat import getcwd +from IPython.utils.traitlets import Unicode + + +class FileCheckpointManager(FileManagerMixin, CheckpointManager): + """ + A CheckpointManager that caches checkpoints for files in adjacent + directories. + + Only works with FileContentsManager. Use GenericFileCheckpointManager if + you want file-based checkpoints with another ContentsManager. + """ + + checkpoint_dir = Unicode( + '.ipynb_checkpoints', + config=True, + help="""The directory name in which to keep file checkpoints + + This is a path relative to the file's own directory. + + By default, it is .ipynb_checkpoints + """, + ) + + root_dir = Unicode(config=True) + + def _root_dir_default(self): + try: + return self.parent.root_dir + except AttributeError: + return getcwd() + + # ContentsManager-dependent checkpoint API + def create_checkpoint(self, contents_mgr, path): + """Create a checkpoint.""" + checkpoint_id = u'checkpoint' + src_path = contents_mgr._get_os_path(path) + dest_path = self.checkpoint_path(checkpoint_id, path) + self._copy(src_path, dest_path) + return self.checkpoint_model(checkpoint_id, dest_path) + + def restore_checkpoint(self, contents_mgr, checkpoint_id, path): + """Restore a checkpoint.""" + src_path = self.checkpoint_path(checkpoint_id, path) + dest_path = contents_mgr._get_os_path(path) + self._copy(src_path, dest_path) + + # ContentsManager-independent checkpoint API + def rename_checkpoint(self, checkpoint_id, old_path, new_path): + """Rename a checkpoint from old_path to new_path.""" + old_cp_path = self.checkpoint_path(checkpoint_id, old_path) + new_cp_path = self.checkpoint_path(checkpoint_id, new_path) + if os.path.isfile(old_cp_path): + self.log.debug( + "Renaming checkpoint %s -> %s", + old_cp_path, + new_cp_path, + ) + with self.perm_to_403(): + shutil.move(old_cp_path, new_cp_path) + + def delete_checkpoint(self, checkpoint_id, path): + """delete a file's checkpoint""" + path = path.strip('/') + cp_path = self.checkpoint_path(checkpoint_id, path) + if not os.path.isfile(cp_path): + self.no_such_checkpoint(path, checkpoint_id) + + self.log.debug("unlinking %s", cp_path) + with self.perm_to_403(): + os.unlink(cp_path) + + def list_checkpoints(self, path): + """list the checkpoints for a given file + + This contents manager currently only supports one checkpoint per file. + """ + path = path.strip('/') + checkpoint_id = "checkpoint" + os_path = self.checkpoint_path(checkpoint_id, path) + if not os.path.isfile(os_path): + return [] + else: + return [self.checkpoint_model(checkpoint_id, os_path)] + + # Checkpoint-related utilities + def checkpoint_path(self, checkpoint_id, path): + """find the path to a checkpoint""" + path = path.strip('/') + parent, name = ('/' + path).rsplit('/', 1) + parent = parent.strip('/') + basename, ext = os.path.splitext(name) + filename = u"{name}-{checkpoint_id}{ext}".format( + name=basename, + checkpoint_id=checkpoint_id, + ext=ext, + ) + os_path = self._get_os_path(path=parent) + cp_dir = os.path.join(os_path, self.checkpoint_dir) + with self.perm_to_403(): + ensure_dir_exists(cp_dir) + cp_path = os.path.join(cp_dir, filename) + return cp_path + + def checkpoint_model(self, checkpoint_id, os_path): + """construct the info dict for a given checkpoint""" + stats = os.stat(os_path) + last_modified = tz.utcfromtimestamp(stats.st_mtime) + info = dict( + id=checkpoint_id, + last_modified=last_modified, + ) + return info + + # Error Handling + def no_such_checkpoint(self, path, checkpoint_id): + raise HTTPError( + 404, + u'Checkpoint does not exist: %s@%s' % (path, checkpoint_id) + ) + + +class GenericFileCheckpointManager(GenericCheckpointMixin, + FileCheckpointManager): + """ + Local filesystem CheckpointManager that works with any conforming + ContentsManager. + """ + def create_file_checkpoint(self, content, format, path): + """Create a checkpoint from the current content of a notebook.""" + path = path.strip('/') + # only the one checkpoint ID: + checkpoint_id = u"checkpoint" + os_checkpoint_path = self.checkpoint_path(checkpoint_id, path) + self.log.debug("creating checkpoint for %s", path) + with self.perm_to_403(): + self._save_file(os_checkpoint_path, content, format=format) + + # return the checkpoint info + return self.checkpoint_model(checkpoint_id, os_checkpoint_path) + + def create_notebook_checkpoint(self, nb, path): + """Create a checkpoint from the current content of a notebook.""" + path = path.strip('/') + # only the one checkpoint ID: + checkpoint_id = u"checkpoint" + os_checkpoint_path = self.checkpoint_path(checkpoint_id, path) + self.log.debug("creating checkpoint for %s", path) + with self.perm_to_403(): + self._save_notebook(os_checkpoint_path, nb) + + # return the checkpoint info + return self.checkpoint_model(checkpoint_id, os_checkpoint_path) + + def get_checkpoint(self, checkpoint_id, path, type): + """Get the content of a checkpoint. + + Returns a model suitable for passing to ContentsManager.save. + """ + path = path.strip('/') + self.log.info("restoring %s from checkpoint %s", path, checkpoint_id) + os_checkpoint_path = self.checkpoint_path(checkpoint_id, path) + if not os.path.isfile(os_checkpoint_path): + self.no_such_checkpoint(path, checkpoint_id) + + if type == 'notebook': + return { + 'type': type, + 'content': self._read_notebook( + os_checkpoint_path, + as_version=4, + ), + } + elif type == 'file': + content, format = self._read_file(os_checkpoint_path, format=None) + return { + 'type': type, + 'content': content, + 'format': format, + } + else: + raise HTTPError(500, u'Unexpected type %s' % type) diff --git a/IPython/html/services/contents/fileio.py b/IPython/html/services/contents/fileio.py new file mode 100644 index 000000000..3b646aa14 --- /dev/null +++ b/IPython/html/services/contents/fileio.py @@ -0,0 +1,166 @@ +""" +Utilities for file-based Contents/Checkpoints managers. +""" + +# Copyright (c) IPython Development Team. +# Distributed under the terms of the Modified BSD License. + +import base64 +from contextlib import contextmanager +import errno +import io +import os +import shutil + +from tornado.web import HTTPError + +from IPython.html.utils import ( + to_api_path, + to_os_path, +) +from IPython import nbformat +from IPython.utils.io import atomic_writing +from IPython.utils.py3compat import str_to_unicode + + +class FileManagerMixin(object): + """ + Mixin for ContentsAPI classes that interact with the filesystem. + + Provides facilities for reading, writing, and copying both notebooks and + generic files. + + Shared by FileContentsManager and FileCheckpointManager. + + Note + ---- + Classes using this mixin must provide the following attributes: + + root_dir : unicode + A directory against against which API-style paths are to be resolved. + + log : logging.Logger + """ + + @contextmanager + def open(self, os_path, *args, **kwargs): + """wrapper around io.open that turns permission errors into 403""" + with self.perm_to_403(os_path): + with io.open(os_path, *args, **kwargs) as f: + yield f + + @contextmanager + def atomic_writing(self, os_path, *args, **kwargs): + """wrapper around atomic_writing that turns permission errors to 403""" + with self.perm_to_403(os_path): + with atomic_writing(os_path, *args, **kwargs) as f: + yield f + + @contextmanager + def perm_to_403(self, os_path=''): + """context manager for turning permission errors into 403.""" + try: + yield + except OSError as e: + if e.errno in {errno.EPERM, errno.EACCES}: + # make 403 error message without root prefix + # this may not work perfectly on unicode paths on Python 2, + # but nobody should be doing that anyway. + if not os_path: + os_path = str_to_unicode(e.filename or 'unknown file') + path = to_api_path(os_path, root=self.root_dir) + raise HTTPError(403, u'Permission denied: %s' % path) + else: + raise + + def _copy(self, src, dest): + """copy src to dest + + like shutil.copy2, but log errors in copystat + """ + shutil.copyfile(src, dest) + try: + shutil.copystat(src, dest) + except OSError: + self.log.debug("copystat on %s failed", dest, exc_info=True) + + def _get_os_path(self, path): + """Given an API path, return its file system path. + + Parameters + ---------- + path : string + The relative API path to the named file. + + Returns + ------- + path : string + Native, absolute OS path to for a file. + """ + return to_os_path(path, self.root_dir) + + def _read_notebook(self, os_path, as_version=4): + """Read a notebook from an os path.""" + with self.open(os_path, 'r', encoding='utf-8') as f: + try: + return nbformat.read(f, as_version=as_version) + except Exception as e: + raise HTTPError( + 400, + u"Unreadable Notebook: %s %r" % (os_path, e), + ) + + def _save_notebook(self, os_path, nb): + """Save a notebook to an os_path.""" + with self.atomic_writing(os_path, encoding='utf-8') as f: + nbformat.write(nb, f, version=nbformat.NO_CONVERT) + + def _read_file(self, os_path, format): + """Read a non-notebook file. + + os_path: The path to be read. + format: + If 'text', the contents will be decoded as UTF-8. + If 'base64', the raw bytes contents will be encoded as base64. + If not specified, try to decode as UTF-8, and fall back to base64 + """ + if not os.path.isfile(os_path): + raise HTTPError(400, "Cannot read non-file %s" % os_path) + + with self.open(os_path, 'rb') as f: + bcontent = f.read() + + if format is None or format == 'text': + # Try to interpret as unicode if format is unknown or if unicode + # was explicitly requested. + try: + return bcontent.decode('utf8'), 'text' + except UnicodeError: + if format == 'text': + raise HTTPError( + 400, + "%s is not UTF-8 encoded" % os_path, + reason='bad format', + ) + return base64.encodestring(bcontent).decode('ascii'), 'base64' + + def _save_file(self, os_path, content, format): + """Save content of a generic file.""" + if format not in {'text', 'base64'}: + raise HTTPError( + 400, + "Must specify format of file contents as 'text' or 'base64'", + ) + try: + if format == 'text': + bcontent = content.encode('utf8') + else: + b64_bytes = content.encode('ascii') + bcontent = base64.decodestring(b64_bytes) + except Exception as e: + raise HTTPError( + 400, u'Encoding error saving %s: %s' % (os_path, e) + ) + + with self.atomic_writing(os_path, text=False) as f: + f.write(bcontent) diff --git a/IPython/html/services/contents/filemanager.py b/IPython/html/services/contents/filemanager.py index 81b64b22c..561c4a771 100644 --- a/IPython/html/services/contents/filemanager.py +++ b/IPython/html/services/contents/filemanager.py @@ -3,9 +3,7 @@ # Copyright (c) IPython Development Team. # Distributed under the terms of the Modified BSD License. -import base64 -from contextlib import contextmanager -import errno + import io import os import shutil @@ -13,25 +11,23 @@ import mimetypes from tornado import web -from .manager import ( - CheckpointManager, - ContentsManager, -) +from .filecheckpoints import FileCheckpointManager +from .fileio import FileManagerMixin +from .manager import ContentsManager + from IPython import nbformat -from IPython.utils.io import atomic_writing from IPython.utils.importstring import import_item -from IPython.utils.path import ensure_dir_exists from IPython.utils.traitlets import Any, Unicode, Bool, TraitError -from IPython.utils.py3compat import getcwd, string_types, str_to_unicode +from IPython.utils.py3compat import getcwd, string_types from IPython.utils import tz from IPython.html.utils import ( is_hidden, to_api_path, - to_os_path, ) _script_exporter = None + def _post_save_script(model, os_path, contents_manager, **kwargs): """convert notebooks to Python script after save with nbconvert @@ -56,346 +52,6 @@ def _post_save_script(model, os_path, contents_manager, **kwargs): f.write(script) -class FileManagerMixin(object): - """ - Mixin for ContentsAPI classes that interact with the filesystem. - - Provides facilities for reading, writing, and copying both notebooks and - generic files. - - Shared by FileContentsManager and FileCheckpointManager. - - Note - ---- - Classes using this mixin must provide the following attributes: - - root_dir : unicode - A directory against against which API-style paths are to be resolved. - - log : logging.Logger - """ - - @contextmanager - def open(self, os_path, *args, **kwargs): - """wrapper around io.open that turns permission errors into 403""" - with self.perm_to_403(os_path): - with io.open(os_path, *args, **kwargs) as f: - yield f - - @contextmanager - def atomic_writing(self, os_path, *args, **kwargs): - """wrapper around atomic_writing that turns permission errors into 403""" - with self.perm_to_403(os_path): - with atomic_writing(os_path, *args, **kwargs) as f: - yield f - - @contextmanager - def perm_to_403(self, os_path=''): - """context manager for turning permission errors into 403.""" - try: - yield - except OSError as e: - if e.errno in {errno.EPERM, errno.EACCES}: - # make 403 error message without root prefix - # this may not work perfectly on unicode paths on Python 2, - # but nobody should be doing that anyway. - if not os_path: - os_path = str_to_unicode(e.filename or 'unknown file') - path = to_api_path(os_path, root=self.root_dir) - raise web.HTTPError(403, u'Permission denied: %s' % path) - else: - raise - - def _copy(self, src, dest): - """copy src to dest - - like shutil.copy2, but log errors in copystat - """ - shutil.copyfile(src, dest) - try: - shutil.copystat(src, dest) - except OSError: - self.log.debug("copystat on %s failed", dest, exc_info=True) - - def _get_os_path(self, path): - """Given an API path, return its file system path. - - Parameters - ---------- - path : string - The relative API path to the named file. - - Returns - ------- - path : string - Native, absolute OS path to for a file. - """ - return to_os_path(path, self.root_dir) - - def _read_notebook(self, os_path, as_version=4): - """Read a notebook from an os path.""" - with self.open(os_path, 'r', encoding='utf-8') as f: - try: - return nbformat.read(f, as_version=as_version) - except Exception as e: - raise web.HTTPError( - 400, - u"Unreadable Notebook: %s %r" % (os_path, e), - ) - - def _save_notebook(self, os_path, nb): - """Save a notebook to an os_path.""" - with self.atomic_writing(os_path, encoding='utf-8') as f: - nbformat.write(nb, f, version=nbformat.NO_CONVERT) - - def _read_file(self, os_path, format): - """Read a non-notebook file. - - os_path: The path to be read. - format: - If 'text', the contents will be decoded as UTF-8. - If 'base64', the raw bytes contents will be encoded as base64. - If not specified, try to decode as UTF-8, and fall back to base64 - """ - if not os.path.isfile(os_path): - raise web.HTTPError(400, "Cannot read non-file %s" % os_path) - - with self.open(os_path, 'rb') as f: - bcontent = f.read() - - if format is None or format == 'text': - # Try to interpret as unicode if format is unknown or if unicode - # was explicitly requested. - try: - return bcontent.decode('utf8'), 'text' - except UnicodeError as e: - if format == 'text': - raise web.HTTPError( - 400, - "%s is not UTF-8 encoded" % os_path, - reason='bad format', - ) - return base64.encodestring(bcontent).decode('ascii'), 'base64' - - def _save_file(self, os_path, content, format): - """Save content of a generic file.""" - if format not in {'text', 'base64'}: - raise web.HTTPError( - 400, - "Must specify format of file contents as 'text' or 'base64'", - ) - try: - if format == 'text': - bcontent = content.encode('utf8') - else: - b64_bytes = content.encode('ascii') - bcontent = base64.decodestring(b64_bytes) - except Exception as e: - raise web.HTTPError(400, u'Encoding error saving %s: %s' % (os_path, e)) - - with self.atomic_writing(os_path, text=False) as f: - f.write(bcontent) - - -class FileCheckpointManager(FileManagerMixin, CheckpointManager): - """ - A CheckpointManager that caches checkpoints for files in adjacent - directories. - """ - - checkpoint_dir = Unicode( - '.ipynb_checkpoints', - config=True, - help="""The directory name in which to keep file checkpoints - - This is a path relative to the file's own directory. - - By default, it is .ipynb_checkpoints - """, - ) - - root_dir = Unicode(config=True) - - def _root_dir_default(self): - try: - return self.parent.root_dir - except AttributeError: - return getcwd() - - # ContentsManager-dependent checkpoint API - def create_checkpoint(self, contents_mgr, path): - """ - Create a checkpoint. - - If contents_mgr is backed by the local filesystem, just copy the - appropriate file to the checkpoint directory. Otherwise, ask the - ContentsManager for a model and write it ourselves. - """ - if contents_mgr.backend == 'local_file': - # We know that the file is in the local filesystem, so just copy - # from the base location to our location. - checkpoint_id = u'checkpoint' - src_path = contents_mgr._get_os_path(path) - dest_path = self.checkpoint_path(checkpoint_id, path) - self._copy(src_path, dest_path) - return self.checkpoint_model(checkpoint_id, dest_path) - else: - return super(FileCheckpointManager, self).create_checkpoint( - contents_mgr, path, - ) - - def restore_checkpoint(self, contents_mgr, checkpoint_id, path): - """ - Restore a checkpoint. - - If contents_mgr is backed by the local filesystem, just copy the - appropriate file from the checkpoint directory. Otherwise, load the - model and pass it to ContentsManager.save. - """ - if contents_mgr.backend == 'local_file': - # We know that the file is in the local filesystem, so just copy - # from our base location to the location expected by content - src_path = self.checkpoint_path(checkpoint_id, path) - dest_path = contents_mgr._get_os_path(path) - self._copy(src_path, dest_path) - else: - super(FileCheckpointManager, self).restore_checkpoint( - contents_mgr, checkpoint_id, path - ) - - # ContentsManager-independent checkpoint API - def rename_checkpoint(self, checkpoint_id, old_path, new_path): - """Rename a checkpoint from old_path to new_path.""" - old_cp_path = self.checkpoint_path(checkpoint_id, old_path) - new_cp_path = self.checkpoint_path(checkpoint_id, new_path) - if os.path.isfile(old_cp_path): - self.log.debug( - "Renaming checkpoint %s -> %s", - old_cp_path, - new_cp_path, - ) - with self.perm_to_403(): - shutil.move(old_cp_path, new_cp_path) - - def delete_checkpoint(self, checkpoint_id, path): - """delete a file's checkpoint""" - path = path.strip('/') - cp_path = self.checkpoint_path(checkpoint_id, path) - if not os.path.isfile(cp_path): - self.no_such_checkpoint(path, checkpoint_id) - - self.log.debug("unlinking %s", cp_path) - with self.perm_to_403(): - os.unlink(cp_path) - - def list_checkpoints(self, path): - """list the checkpoints for a given file - - This contents manager currently only supports one checkpoint per file. - """ - path = path.strip('/') - checkpoint_id = "checkpoint" - os_path = self.checkpoint_path(checkpoint_id, path) - if not os.path.isfile(os_path): - return [] - else: - return [self.checkpoint_model(checkpoint_id, os_path)] - - # Checkpoint-related utilities - def checkpoint_path(self, checkpoint_id, path): - """find the path to a checkpoint""" - path = path.strip('/') - parent, name = ('/' + path).rsplit('/', 1) - parent = parent.strip('/') - basename, ext = os.path.splitext(name) - filename = u"{name}-{checkpoint_id}{ext}".format( - name=basename, - checkpoint_id=checkpoint_id, - ext=ext, - ) - os_path = self._get_os_path(path=parent) - cp_dir = os.path.join(os_path, self.checkpoint_dir) - with self.perm_to_403(): - ensure_dir_exists(cp_dir) - cp_path = os.path.join(cp_dir, filename) - return cp_path - - def checkpoint_model(self, checkpoint_id, os_path): - """construct the info dict for a given checkpoint""" - stats = os.stat(os_path) - last_modified = tz.utcfromtimestamp(stats.st_mtime) - info = dict( - id=checkpoint_id, - last_modified=last_modified, - ) - return info - - def create_file_checkpoint(self, content, format, path): - """Create a checkpoint from the current content of a notebook.""" - path = path.strip('/') - # only the one checkpoint ID: - checkpoint_id = u"checkpoint" - os_checkpoint_path = self.checkpoint_path(checkpoint_id, path) - self.log.debug("creating checkpoint for %s", path) - with self.perm_to_403(): - self._save_file(os_checkpoint_path, content, format=format) - - # return the checkpoint info - return self.checkpoint_model(checkpoint_id, os_checkpoint_path) - - def create_notebook_checkpoint(self, nb, path): - """Create a checkpoint from the current content of a notebook.""" - path = path.strip('/') - # only the one checkpoint ID: - checkpoint_id = u"checkpoint" - os_checkpoint_path = self.checkpoint_path(checkpoint_id, path) - self.log.debug("creating checkpoint for %s", path) - with self.perm_to_403(): - self._save_notebook(os_checkpoint_path, nb) - - # return the checkpoint info - return self.checkpoint_model(checkpoint_id, os_checkpoint_path) - - def get_checkpoint(self, checkpoint_id, path, type): - """Get the content of a checkpoint. - - Returns a model suitable for passing to ContentsManager.save. - """ - path = path.strip('/') - self.log.info("restoring %s from checkpoint %s", path, checkpoint_id) - os_checkpoint_path = self.checkpoint_path(checkpoint_id, path) - if not os.path.isfile(os_checkpoint_path): - self.no_such_checkpoint(path, checkpoint_id) - - if type == 'notebook': - return { - 'type': type, - 'content': self._read_notebook( - os_checkpoint_path, - as_version=4, - ), - } - elif type == 'file': - content, format = self._read_file(os_checkpoint_path, format=None) - return { - 'type': type, - 'content': content, - 'format': format, - } - else: - raise web.HTTPError( - 500, - u'Unexpected type %s' % type - ) - - # Error Handling - def no_such_checkpoint(self, path, checkpoint_id): - raise web.HTTPError( - 404, - u'Checkpoint does not exist: %s@%s' % (path, checkpoint_id) - ) - - class FileContentsManager(FileManagerMixin, ContentsManager): root_dir = Unicode(config=True) @@ -468,9 +124,6 @@ class FileContentsManager(FileManagerMixin, ContentsManager): def _checkpoint_manager_class_default(self): return FileCheckpointManager - def _backend_default(self): - return 'local_file' - def is_hidden(self, path): """Does the API style path correspond to a hidden directory or file? diff --git a/IPython/html/services/contents/manager.py b/IPython/html/services/contents/manager.py index 6b25dc118..6d2f229bd 100644 --- a/IPython/html/services/contents/manager.py +++ b/IPython/html/services/contents/manager.py @@ -11,6 +11,7 @@ import re from tornado.web import HTTPError +from .checkpoints import CheckpointManager from IPython.config.configurable import LoggingConfigurable from IPython.nbformat import sign, validate, ValidationError from IPython.nbformat.v4 import new_notebook @@ -29,77 +30,6 @@ from IPython.utils.py3compat import string_types copy_pat = re.compile(r'\-Copy\d*\.') -class CheckpointManager(LoggingConfigurable): - """ - Base class for managing checkpoints for a ContentsManager. - """ - - def create_checkpoint(self, contents_mgr, path): - model = contents_mgr.get(path, content=True) - type = model['type'] - if type == 'notebook': - return self.create_notebook_checkpoint( - model['content'], - path, - ) - elif type == 'file': - return self.create_file_checkpoint( - model['content'], - model['format'], - path, - ) - - def restore_checkpoint(self, contents_mgr, checkpoint_id, path): - """Restore a checkpoint.""" - type = contents_mgr.get(path, content=False)['type'] - model = self.get_checkpoint(checkpoint_id, path, type) - contents_mgr.save(model, path) - - def create_file_checkpoint(self, content, format, path): - """Create a checkpoint of the current state of a file - - Returns a checkpoint model for the new checkpoint. - """ - raise NotImplementedError("must be implemented in a subclass") - - def create_notebook_checkpoint(self, nb, path): - """Create a checkpoint of the current state of a file - - Returns a checkpoint model for the new checkpoint. - """ - raise NotImplementedError("must be implemented in a subclass") - - def get_checkpoint(self, checkpoint_id, path, type): - """Get the content of a checkpoint. - - Returns an unvalidated model with the same structure as - the return value of ContentsManager.get - """ - raise NotImplementedError("must be implemented in a subclass") - - def rename_checkpoint(self, checkpoint_id, old_path, new_path): - """Rename a single checkpoint from old_path to new_path.""" - raise NotImplementedError("must be implemented in a subclass") - - def delete_checkpoint(self, checkpoint_id, path): - """delete a checkpoint for a file""" - raise NotImplementedError("must be implemented in a subclass") - - def list_checkpoints(self, path): - """Return a list of checkpoints for a given file""" - raise NotImplementedError("must be implemented in a subclass") - - def rename_all_checkpoints(self, old_path, new_path): - """Rename all checkpoints for old_path to new_path.""" - for cp in self.list_checkpoints(old_path): - self.rename_checkpoint(cp['id'], old_path, new_path) - - def delete_all_checkpoints(self, path): - """Delete all checkpoints for the given path.""" - for checkpoint in self.list_checkpoints(path): - self.delete_checkpoint(checkpoint['id'], path) - - class ContentsManager(LoggingConfigurable): """Base class for serving files and directories. @@ -180,7 +110,6 @@ class ContentsManager(LoggingConfigurable): checkpoint_manager_class = Type(CheckpointManager, config=True) checkpoint_manager = Instance(CheckpointManager, config=True) checkpoint_manager_kwargs = Dict(allow_none=False, config=True) - backend = Unicode(default_value="") def _checkpoint_manager_default(self): return self.checkpoint_manager_class(**self.checkpoint_manager_kwargs) diff --git a/IPython/html/services/contents/tests/test_contents_api.py b/IPython/html/services/contents/tests/test_contents_api.py index b19a8faed..d45f83d11 100644 --- a/IPython/html/services/contents/tests/test_contents_api.py +++ b/IPython/html/services/contents/tests/test_contents_api.py @@ -13,6 +13,9 @@ pjoin = os.path.join import requests +from ..filecheckpoints import GenericFileCheckpointManager + +from IPython.config import Config from IPython.html.utils import url_path_join, url_escape, to_os_path from IPython.html.tests.launchnotebook import NotebookTestBase, assert_http_error from IPython.nbformat import read, write, from_dict @@ -615,24 +618,12 @@ class APITest(NotebookTestBase): with self.patch_cp_root(td): self.test_file_checkpoints() - @contextmanager - def patch_cm_backend(self): - """ - Temporarily patch our ContentsManager to present a different backend. - """ - mgr = self.notebook.contents_manager - old_backend = mgr.backend - mgr.backend = "" - try: - yield - finally: - mgr.backend = old_backend - - def test_checkpoints_empty_backend(self): - with self.patch_cm_backend(): - self.test_checkpoints() - - with self.patch_cm_backend(): - self.test_file_checkpoints() +class GenericFileCheckpointsAPITest(APITest): + """ + Run the tests from APITest with GenericFileCheckpointManager. + """ + config = Config() + config.FileContentsManager.checkpoint_manager_class = \ + GenericFileCheckpointManager From 2569c3069dadb128208e41d1ad41fe08553ab33d Mon Sep 17 00:00:00 2001 From: Scott Sanderson Date: Thu, 8 Jan 2015 14:41:46 -0500 Subject: [PATCH 12/13] STY: s/CheckpointManager/Checkpoints It's easy to confuse CheckpointManager and ContentsManager. --- IPython/html/services/contents/checkpoints.py | 19 +++++++++----- .../html/services/contents/filecheckpoints.py | 17 ++++++------ IPython/html/services/contents/fileio.py | 2 +- IPython/html/services/contents/filemanager.py | 10 +++---- IPython/html/services/contents/manager.py | 26 +++++++++---------- .../contents/tests/test_contents_api.py | 21 ++++++++++----- .../services/contents/tests/test_manager.py | 2 +- 7 files changed, 54 insertions(+), 43 deletions(-) diff --git a/IPython/html/services/contents/checkpoints.py b/IPython/html/services/contents/checkpoints.py index 0ba546ff0..6cea13c1d 100644 --- a/IPython/html/services/contents/checkpoints.py +++ b/IPython/html/services/contents/checkpoints.py @@ -8,7 +8,7 @@ Classes for managing Checkpoints. from IPython.config.configurable import LoggingConfigurable -class CheckpointManager(LoggingConfigurable): +class Checkpoints(LoggingConfigurable): """ Base class for managing checkpoints for a ContentsManager. @@ -51,20 +51,25 @@ class CheckpointManager(LoggingConfigurable): self.delete_checkpoint(checkpoint['id'], path) -class GenericCheckpointMixin(object): +class GenericCheckpointsMixin(object): """ - Helper for creating CheckpointManagers that can be used with any + Helper for creating Checkpoints subclasses that can be used with any ContentsManager. - Provides an implementation of `create_checkpoint` and `restore_checkpoint` - in terms of the following operations: + Provides a ContentsManager-agnostic implementation of `create_checkpoint` + and `restore_checkpoint` in terms of the following operations: create_file_checkpoint(self, content, format, path) create_notebook_checkpoint(self, nb, path) get_checkpoint(self, checkpoint_id, path, type) - **Any** valid CheckpointManager implementation should also be valid when - this mixin is applied. + To create a generic CheckpointManager, add this mixin to a class that + implement the above three methods plus the remaining Checkpoints API + methods: + + delete_checkpoint(self, checkpoint_id, path) + list_checkpoints(self, path) + rename_checkpoint(self, checkpoint_id, old_path, new_path) """ def create_checkpoint(self, contents_mgr, path): diff --git a/IPython/html/services/contents/filecheckpoints.py b/IPython/html/services/contents/filecheckpoints.py index e1e4f8c60..17774836e 100644 --- a/IPython/html/services/contents/filecheckpoints.py +++ b/IPython/html/services/contents/filecheckpoints.py @@ -1,5 +1,5 @@ """ -File-based CheckpointManagers. +File-based Checkpoints implementations. """ import os import shutil @@ -7,8 +7,8 @@ import shutil from tornado.web import HTTPError from .checkpoints import ( - CheckpointManager, - GenericCheckpointMixin, + Checkpoints, + GenericCheckpointsMixin, ) from .fileio import FileManagerMixin @@ -18,12 +18,12 @@ from IPython.utils.py3compat import getcwd from IPython.utils.traitlets import Unicode -class FileCheckpointManager(FileManagerMixin, CheckpointManager): +class FileCheckpoints(FileManagerMixin, Checkpoints): """ - A CheckpointManager that caches checkpoints for files in adjacent + A Checkpoints that caches checkpoints for files in adjacent directories. - Only works with FileContentsManager. Use GenericFileCheckpointManager if + Only works with FileContentsManager. Use GenericFileCheckpoints if you want file-based checkpoints with another ContentsManager. """ @@ -136,10 +136,9 @@ class FileCheckpointManager(FileManagerMixin, CheckpointManager): ) -class GenericFileCheckpointManager(GenericCheckpointMixin, - FileCheckpointManager): +class GenericFileCheckpoints(GenericCheckpointsMixin, FileCheckpoints): """ - Local filesystem CheckpointManager that works with any conforming + Local filesystem Checkpoints that works with any conforming ContentsManager. """ def create_file_checkpoint(self, content, format, path): diff --git a/IPython/html/services/contents/fileio.py b/IPython/html/services/contents/fileio.py index 3b646aa14..fcd78ad4d 100644 --- a/IPython/html/services/contents/fileio.py +++ b/IPython/html/services/contents/fileio.py @@ -30,7 +30,7 @@ class FileManagerMixin(object): Provides facilities for reading, writing, and copying both notebooks and generic files. - Shared by FileContentsManager and FileCheckpointManager. + Shared by FileContentsManager and FileCheckpoints. Note ---- diff --git a/IPython/html/services/contents/filemanager.py b/IPython/html/services/contents/filemanager.py index 561c4a771..29ddfde5f 100644 --- a/IPython/html/services/contents/filemanager.py +++ b/IPython/html/services/contents/filemanager.py @@ -11,7 +11,7 @@ import mimetypes from tornado import web -from .filecheckpoints import FileCheckpointManager +from .filecheckpoints import FileCheckpoints from .fileio import FileManagerMixin from .manager import ContentsManager @@ -121,8 +121,8 @@ class FileContentsManager(FileManagerMixin, ContentsManager): if not os.path.isdir(new): raise TraitError("%r is not a directory" % new) - def _checkpoint_manager_class_default(self): - return FileCheckpointManager + def _checkpoints_class_default(self): + return FileCheckpoints def is_hidden(self, path): """Does the API style path correspond to a hidden directory or file? @@ -383,7 +383,7 @@ class FileContentsManager(FileManagerMixin, ContentsManager): self.check_and_sign(nb, path) self._save_notebook(os_path, nb) # One checkpoint should always exist for notebooks. - if not self.checkpoint_manager.list_checkpoints(path): + if not self.checkpoints.list_checkpoints(path): self.create_checkpoint(path) elif model['type'] == 'file': # Missing format will be handled internally by _save_file. @@ -421,7 +421,7 @@ class FileContentsManager(FileManagerMixin, ContentsManager): # Don't delete non-empty directories. # A directory containing only leftover checkpoints is # considered empty. - cp_dir = getattr(self.checkpoint_manager, 'checkpoint_dir', None) + cp_dir = getattr(self.checkpoints, 'checkpoint_dir', None) for entry in listing: if entry != cp_dir: raise web.HTTPError(400, u'Directory %s not empty' % os_path) diff --git a/IPython/html/services/contents/manager.py b/IPython/html/services/contents/manager.py index 6d2f229bd..7012a2a44 100644 --- a/IPython/html/services/contents/manager.py +++ b/IPython/html/services/contents/manager.py @@ -11,7 +11,7 @@ import re from tornado.web import HTTPError -from .checkpoints import CheckpointManager +from .checkpoints import Checkpoints from IPython.config.configurable import LoggingConfigurable from IPython.nbformat import sign, validate, ValidationError from IPython.nbformat.v4 import new_notebook @@ -107,14 +107,14 @@ class ContentsManager(LoggingConfigurable): except Exception: self.log.error("Pre-save hook failed on %s", path, exc_info=True) - checkpoint_manager_class = Type(CheckpointManager, config=True) - checkpoint_manager = Instance(CheckpointManager, config=True) - checkpoint_manager_kwargs = Dict(allow_none=False, config=True) + checkpoints_class = Type(Checkpoints, config=True) + checkpoints = Instance(Checkpoints, config=True) + checkpoints_kwargs = Dict(allow_none=False, config=True) - def _checkpoint_manager_default(self): - return self.checkpoint_manager_class(**self.checkpoint_manager_kwargs) + def _checkpoints_default(self): + return self.checkpoints_class(**self.checkpoints_kwargs) - def _checkpoint_manager_kwargs_default(self): + def _checkpoints_kwargs_default(self): return dict( parent=self, log=self.log, @@ -223,12 +223,12 @@ class ContentsManager(LoggingConfigurable): def delete(self, path): """Delete a file/directory and any associated checkpoints.""" self.delete_file(path) - self.checkpoint_manager.delete_all_checkpoints(path) + self.checkpoints.delete_all_checkpoints(path) def rename(self, old_path, new_path): """Rename a file and any checkpoints associated with that file.""" self.rename_file(old_path, new_path) - self.checkpoint_manager.rename_all_checkpoints(old_path, new_path) + self.checkpoints.rename_all_checkpoints(old_path, new_path) def update(self, model, path): """Update the file's path @@ -453,16 +453,16 @@ class ContentsManager(LoggingConfigurable): # Part 3: Checkpoints API def create_checkpoint(self, path): """Create a checkpoint.""" - return self.checkpoint_manager.create_checkpoint(self, path) + return self.checkpoints.create_checkpoint(self, path) def restore_checkpoint(self, checkpoint_id, path): """ Restore a checkpoint. """ - self.checkpoint_manager.restore_checkpoint(self, checkpoint_id, path) + self.checkpoints.restore_checkpoint(self, checkpoint_id, path) def list_checkpoints(self, path): - return self.checkpoint_manager.list_checkpoints(path) + return self.checkpoints.list_checkpoints(path) def delete_checkpoint(self, checkpoint_id, path): - return self.checkpoint_manager.delete_checkpoint(checkpoint_id, path) + return self.checkpoints.delete_checkpoint(checkpoint_id, path) diff --git a/IPython/html/services/contents/tests/test_contents_api.py b/IPython/html/services/contents/tests/test_contents_api.py index d45f83d11..db46cb65a 100644 --- a/IPython/html/services/contents/tests/test_contents_api.py +++ b/IPython/html/services/contents/tests/test_contents_api.py @@ -13,7 +13,7 @@ pjoin = os.path.join import requests -from ..filecheckpoints import GenericFileCheckpointManager +from ..filecheckpoints import GenericFileCheckpoints from IPython.config import Config from IPython.html.utils import url_path_join, url_escape, to_os_path @@ -593,7 +593,7 @@ class APITest(NotebookTestBase): """ Temporarily patch the root dir of our checkpoint manager. """ - cpm = self.notebook.contents_manager.checkpoint_manager + cpm = self.notebook.contents_manager.checkpoints old_dirname = cpm.root_dir cpm.root_dir = dirname try: @@ -603,7 +603,7 @@ class APITest(NotebookTestBase): def test_checkpoints_separate_root(self): """ - Test that FileCheckpointManager functions correctly even when it's + Test that FileCheckpoints functions correctly even when it's using a different root dir from FileContentsManager. This also keeps the implementation honest for use with ContentsManagers that don't map models to the filesystem @@ -621,9 +621,16 @@ class APITest(NotebookTestBase): class GenericFileCheckpointsAPITest(APITest): """ - Run the tests from APITest with GenericFileCheckpointManager. + Run the tests from APITest with GenericFileCheckpoints. """ - config = Config() - config.FileContentsManager.checkpoint_manager_class = \ - GenericFileCheckpointManager + config.FileContentsManager.checkpoints_class = GenericFileCheckpoints + + def test_config_did_something(self): + + self.assertIsInstance( + self.notebook.contents_manager.checkpoints, + GenericFileCheckpoints, + ) + + diff --git a/IPython/html/services/contents/tests/test_manager.py b/IPython/html/services/contents/tests/test_manager.py index 2cade002e..06a664557 100644 --- a/IPython/html/services/contents/tests/test_manager.py +++ b/IPython/html/services/contents/tests/test_manager.py @@ -84,7 +84,7 @@ class TestFileContentsManager(TestCase): root = td os.mkdir(os.path.join(td, subd)) fm = FileContentsManager(root_dir=root) - cpm = fm.checkpoint_manager + cpm = fm.checkpoints cp_dir = cpm.checkpoint_path( 'cp', 'test.ipynb' ) From 64e5c496080e475bea307ce83541bd2aa05aec00 Mon Sep 17 00:00:00 2001 From: Scott Sanderson Date: Thu, 8 Jan 2015 17:00:53 -0500 Subject: [PATCH 13/13] DEV: Break get_checkpoint into separate methods. One for notebooks and one for checkpoints. --- IPython/html/services/contents/checkpoints.py | 24 +++++++--- .../html/services/contents/filecheckpoints.py | 45 ++++++++++--------- 2 files changed, 41 insertions(+), 28 deletions(-) diff --git a/IPython/html/services/contents/checkpoints.py b/IPython/html/services/contents/checkpoints.py index 6cea13c1d..d87b7cc95 100644 --- a/IPython/html/services/contents/checkpoints.py +++ b/IPython/html/services/contents/checkpoints.py @@ -5,6 +5,8 @@ Classes for managing Checkpoints. # Copyright (c) IPython Development Team. # Distributed under the terms of the Modified BSD License. +from tornado.web import HTTPError + from IPython.config.configurable import LoggingConfigurable @@ -59,17 +61,18 @@ class GenericCheckpointsMixin(object): Provides a ContentsManager-agnostic implementation of `create_checkpoint` and `restore_checkpoint` in terms of the following operations: - create_file_checkpoint(self, content, format, path) - create_notebook_checkpoint(self, nb, path) - get_checkpoint(self, checkpoint_id, path, type) + - create_file_checkpoint(self, content, format, path) + - create_notebook_checkpoint(self, nb, path) + - get_file_checkpoint(self, checkpoint_id, path) + - get_notebook_checkpoint(self, checkpoint_id, path) To create a generic CheckpointManager, add this mixin to a class that implement the above three methods plus the remaining Checkpoints API methods: - delete_checkpoint(self, checkpoint_id, path) - list_checkpoints(self, path) - rename_checkpoint(self, checkpoint_id, old_path, new_path) + - delete_checkpoint(self, checkpoint_id, path) + - list_checkpoints(self, path) + - rename_checkpoint(self, checkpoint_id, old_path, new_path) """ def create_checkpoint(self, contents_mgr, path): @@ -86,11 +89,18 @@ class GenericCheckpointsMixin(object): model['format'], path, ) + else: + raise HTTPError(500, u'Unexpected type %s' % type) def restore_checkpoint(self, contents_mgr, checkpoint_id, path): """Restore a checkpoint.""" type = contents_mgr.get(path, content=False)['type'] - model = self.get_checkpoint(checkpoint_id, path, type) + if type == 'notebook': + model = self.get_notebook_checkpoint(checkpoint_id, path) + elif type == 'file': + model = self.get_file_checkpoint(checkpoint_id, path) + else: + raise HTTPError(500, u'Unexpected type %s' % type) contents_mgr.save(model, path) # Required Methods diff --git a/IPython/html/services/contents/filecheckpoints.py b/IPython/html/services/contents/filecheckpoints.py index 17774836e..425bae359 100644 --- a/IPython/html/services/contents/filecheckpoints.py +++ b/IPython/html/services/contents/filecheckpoints.py @@ -167,31 +167,34 @@ class GenericFileCheckpoints(GenericCheckpointsMixin, FileCheckpoints): # return the checkpoint info return self.checkpoint_model(checkpoint_id, os_checkpoint_path) - def get_checkpoint(self, checkpoint_id, path, type): - """Get the content of a checkpoint. + def get_notebook_checkpoint(self, checkpoint_id, path): - Returns a model suitable for passing to ContentsManager.save. - """ path = path.strip('/') self.log.info("restoring %s from checkpoint %s", path, checkpoint_id) os_checkpoint_path = self.checkpoint_path(checkpoint_id, path) + if not os.path.isfile(os_checkpoint_path): self.no_such_checkpoint(path, checkpoint_id) - if type == 'notebook': - return { - 'type': type, - 'content': self._read_notebook( - os_checkpoint_path, - as_version=4, - ), - } - elif type == 'file': - content, format = self._read_file(os_checkpoint_path, format=None) - return { - 'type': type, - 'content': content, - 'format': format, - } - else: - raise HTTPError(500, u'Unexpected type %s' % type) + return { + 'type': 'notebook', + 'content': self._read_notebook( + os_checkpoint_path, + as_version=4, + ), + } + + def get_file_checkpoint(self, checkpoint_id, path): + path = path.strip('/') + self.log.info("restoring %s from checkpoint %s", path, checkpoint_id) + os_checkpoint_path = self.checkpoint_path(checkpoint_id, path) + + if not os.path.isfile(os_checkpoint_path): + self.no_such_checkpoint(path, checkpoint_id) + + content, format = self._read_file(os_checkpoint_path, format=None) + return { + 'type': 'file', + 'content': content, + 'format': format, + }