From adc2275671346c7471790146fb542a69c2943d27 Mon Sep 17 00:00:00 2001 From: Ian Murray Date: Tue, 22 Nov 2011 17:28:46 +0000 Subject: [PATCH 01/55] [controller/package] A very rough sketch of pluggable package controller behaviour. --- ckan/config/routing.py | 13 +++++- ckan/controllers/package.py | 89 ++++++++++++++++++++++++++++++++----- ckan/plugins/interfaces.py | 79 +++++++++++++++++++++++++++++++- 3 files changed, 167 insertions(+), 14 deletions(-) diff --git a/ckan/config/routing.py b/ckan/config/routing.py index eb8d0bcb07d..53d9a3f337b 100644 --- a/ckan/config/routing.py +++ b/ckan/config/routing.py @@ -7,7 +7,10 @@ """ from pylons import config from routes import Mapper -from ckan.plugins import PluginImplementations, IRoutes +from ckan.controllers.package import set_fallback_controller as set_fallback_package_controller,\ + add_package_controller,\ + set_default_as_fallback_controller_if_required as set_default_as_fallback_package_controller_if_required +from ckan.plugins import PluginImplementations, IRoutes, IPluggablePackageController routing_plugins = PluginImplementations(IRoutes) @@ -171,6 +174,14 @@ def make_map(): ## /END API ########### + for plugin in PluginImplementations(IPluggablePackageController): + if plugin.is_fallback(): + set_fallback_package_controller(plugin) + for package_type in plugin.package_types(): + add_package_controller(package_type, plugin) + + set_default_as_fallback_package_controller_if_required() + map.redirect("/packages", "/dataset") map.redirect("/packages/{url:.*}", "/dataset/{url}") map.redirect("/package", "/dataset") diff --git a/ckan/controllers/package.py b/ckan/controllers/package.py index 87a54b8e6ae..a05946424e4 100644 --- a/ckan/controllers/package.py +++ b/ckan/controllers/package.py @@ -25,6 +25,8 @@ from ckan.logic import tuplize_dict, clean_dict, parse_params, flatten_to_string_key from ckan.lib.dictization import table_dictize from ckan.lib.i18n import get_lang +from ckan.plugins import SingletonPlugin, implements +from ckan.plugins.interfaces import IPluggablePackageController import ckan.forms import ckan.authz import ckan.rating @@ -47,19 +49,63 @@ def search_url(params): ("text", "x-graphviz", ["dot"]), ] -class PackageController(BaseController): - - ## hooks for subclasses - package_form = 'package/new_package_form.html' - - def _form_to_db_schema(self): +_pluggable_package_controllers = dict() +_default_pluggable_package_controller = None + +def set_fallback_controller(plugin_instance): + """ + Sets the fallback controller + """ + global _default_pluggable_package_controller + if _default_pluggable_package_controller is not None: + raise ValueError, "A fallback package controller has alread been registered" + _default_pluggable_package_controller = plugin_instance + +def set_default_as_fallback_controller_if_required(): + """ + Set the fallback controller to be an instance of DefaultPluggablePackageController + """ + if _default_pluggable_package_controller is None: + set_fallback_controller(DefaultPluggablePackageController()) + +def _lookup_plugin(package_type): + """ + Returns the plugin controller associoated with the given package type. + """ + if package_type is None: + return _default_pluggable_package_controller + return _pluggable_package_controllers.get(package_type, + _default_pluggable_package_controller) + +def add_package_controller(package_type, plugin_instance): + """ + Register the given plugin_instance to the given package_type. + """ + if package_type in _pluggable_package_controllers: + raise ValueError, 'A PluggablePackageController is already ' \ + 'associated with this package_type: "%s"' % package_type + +class DefaultPluggablePackageController(object): + """ + Provides a default implementation of the package controller. + + Note - this isn't a plugin implementation. This is deliberate, as + we don't want this being registered. + """ + + ##### Define the hooks that control the behaviour ##### + + def package_form(self): + return 'package/new_package_form.html' + + def form_to_db_schema(self): return package_form_schema() - def _db_to_form_schema(self): + def db_to_form_schema(self): '''This is an interface to manipulate data from the database into a format suitable for the form (optional)''' - def _check_data_dict(self, data_dict): + def check_data_dict(self, data_dict): '''Check if the return data is correct, mostly for checking out if spammers are submitting only part of the form''' @@ -79,7 +125,7 @@ def _check_data_dict(self, data_dict): log.info('incorrect form fields posted') raise DataError(data_dict) - def _setup_template_variables(self, context, data_dict): + def setup_template_variables(self, context, data_dict): c.groups_authz = get_action('group_list_authz')(context, data_dict) data_dict.update({'available_only':True}) c.groups_available = get_action('group_list_authz')(context, data_dict) @@ -100,6 +146,27 @@ def _setup_template_variables(self, context, data_dict): ## end hooks +class PackageController(BaseController): + + def _package_form(self, package_type=None): + return _lookup_plugin(package_type).package_form() + + def _form_to_db_schema(self, package_type=None): + return _lookup_plugin(package_type).form_to_db_schema() + + def _db_to_form_schema(self, package_type=None): + '''This is an interface to manipulate data from the database + into a format suitable for the form (optional)''' + return _lookup_plugin(package_type).db_to_form_schema() + + def _check_data_dict(self, data_dict, package_type=None): + '''Check if the return data is correct, mostly for checking out if + spammers are submitting only part of the form''' + return _lookup_plugin(package_type).check_data_dict(data_dict) + + def _setup_template_variables(self, context, data_dict, package_type=None): + return _lookup_plugin(package_type).setup_template_variables(context, data_dict) + authorizer = ckan.authz.Authorizer() def search(self): @@ -339,7 +406,7 @@ def new(self, data=None, errors=None, error_summary=None): vars = {'data': data, 'errors': errors, 'error_summary': error_summary} self._setup_template_variables(context, {'id': id}) - c.form = render(self.package_form, extra_vars=vars) + c.form = render(self._package_form(), extra_vars=vars) return render('package/new.html') @@ -378,7 +445,7 @@ def edit(self, id, data=None, errors=None, error_summary=None): self._setup_template_variables(context, {'id': id}) - c.form = render(self.package_form, extra_vars=vars) + c.form = render(self._package_form(), extra_vars=vars) return render('package/edit.html') def read_ajax(self, id, revision=None): diff --git a/ckan/plugins/interfaces.py b/ckan/plugins/interfaces.py index 6c646bb2ab7..24058293288 100644 --- a/ckan/plugins/interfaces.py +++ b/ckan/plugins/interfaces.py @@ -12,13 +12,12 @@ 'IDomainObjectModification', 'IGroupController', 'IPackageController', 'IPluginObserver', 'IConfigurable', 'IConfigurer', 'IAuthorizer', - 'IActions', 'IResourceUrlChange' + 'IActions', 'IResourceUrlChange', 'IPluggablePackageController', ] from inspect import isclass from pyutilib.component.core import Interface as _pca_Interface - class Interface(_pca_Interface): @classmethod @@ -330,3 +329,79 @@ def get_auth_functions(self): implementation overrides """ +class IPluggablePackageController(Interface): + """ + Allows customisation of the package controller as a plugin. + + Different package types can be associated with different + IPluggablePackageControllers, allowing multiple package controllers + on a CKAN instance. + + The PackageController uses hooks to customise behaviour. A implementation + of IPluggablePackageController must implement these hooks. + + Implementations might want to consider subclassing + ckan.controllers.package.DefaultPluggablePackageController + + """ + + ##### These methods control when the plugin is delegated to ##### + + def is_fallback(self): + """ + Returns true iff this provides the fallback behaviour, when no other + plugin matches a package's type. + + There must be exactly one fallback controller defined, any attempt to + register more than one or to not have any registered, will throw + an exception at startup. + """ + + def package_types(self): + """ + Returns an iterable of package type strings. + + If a request involving a package of one of those types is made, then + this plugin will be delegated to. + + There must only be one plugin registered to each package type. Any + attempts to register more than one plugin to a given package type will + raise an exception at startup. + """ + + ##### End of control methods + + ##### Hooks for customising the PackageController's behaviour ##### + + def package_form(self): + """ + Returns a string representing the location of the template to be + rendered. e.g. "package/new_package_form.html". + """ + + def form_to_db_schema(self): + """ + Returns the schema for mapping package data from a form to a format + suitable for the database. + """ + + def db_to_form_schema(self): + """ + Returns the schema for mapping package data from the database into a + format suitable for the form (optional) + """ + + def check_data_dict(self, data_dict): + """ + Check if the return data is correct. + + raise a DataError if not. + """ + + def setup_template_variables(self, context, data_dict): + """ + Add variables to c just prior to the template being rendered. + """ + + ##### End of hooks ##### + From 1d1c4c8958a2b7961636a5878baddccb60e8db8d Mon Sep 17 00:00:00 2001 From: Ian Murray Date: Wed, 23 Nov 2011 08:45:26 +0000 Subject: [PATCH 02/55] [controllers/package] Created a stub where the package's type can be determined Goes through the action of creating the package. It just has nothing to return yet. --- ckan/controllers/package.py | 44 +++++++++++++++++++++------ ckan/tests/functional/test_package.py | 8 ++++- 2 files changed, 42 insertions(+), 10 deletions(-) diff --git a/ckan/controllers/package.py b/ckan/controllers/package.py index a05946424e4..c8f79cde2e5 100644 --- a/ckan/controllers/package.py +++ b/ckan/controllers/package.py @@ -244,9 +244,10 @@ def pager_url(q=None, page=None): def read(self, id): + package_type = self._get_package_type(id.split('@')[0]) context = {'model': model, 'session': model.Session, 'user': c.user or c.author, 'extras_as_string': True, - 'schema': self._form_to_db_schema()} + 'schema': self._form_to_db_schema(package_type=package_type)} data_dict = {'id': id} # interpret @ or @ suffix @@ -295,9 +296,10 @@ def read(self, id): return render('package/read.html') def comments(self, id): + package_type = self._get_package_type(id) context = {'model': model, 'session': model.Session, 'user': c.user or c.author, 'extras_as_string': True, - 'schema': self._form_to_db_schema()} + 'schema': self._form_to_db_schema(package_type=package_type)} #check if package exists try: @@ -412,18 +414,19 @@ def new(self, data=None, errors=None, error_summary=None): def edit(self, id, data=None, errors=None, error_summary=None): + package_type = self._get_package_type(id) context = {'model': model, 'session': model.Session, 'user': c.user or c.author, 'extras_as_string': True, 'save': 'save' in request.params, 'moderated': config.get('moderated'), 'pending': True, - 'schema': self._form_to_db_schema()} + 'schema': self._form_to_db_schema(package_type=package_type)} if context['save'] and not data: return self._save_edit(id, context) try: old_data = get_action('package_show')(context, {'id':id}) - schema = self._db_to_form_schema() + schema = self._db_to_form_schema(package_type=package_type) if schema and not data: old_data, errors = validate(old_data, schema, context=context) data = data or old_data @@ -443,21 +446,22 @@ def edit(self, id, data=None, errors=None, error_summary=None): errors = errors or {} vars = {'data': data, 'errors': errors, 'error_summary': error_summary} - self._setup_template_variables(context, {'id': id}) + self._setup_template_variables(context, {'id': id}, package_type=package_type) - c.form = render(self._package_form(), extra_vars=vars) + c.form = render(self._package_form(package_type=package_type), extra_vars=vars) return render('package/edit.html') def read_ajax(self, id, revision=None): + package_type=self._get_package_type(id) context = {'model': model, 'session': model.Session, 'user': c.user or c.author, 'extras_as_string': True, - 'schema': self._form_to_db_schema(), + 'schema': self._form_to_db_schema(package_type=package_type), 'revision_id': revision} try: data = get_action('package_show')(context, {'id': id}) - schema = self._db_to_form_schema() + schema = self._db_to_form_schema(package_type=package_type) if schema: data, errors = validate(data, schema) except NotAuthorized: @@ -504,6 +508,27 @@ def history_ajax(self, id): response.headers['Content-Type'] = 'application/json;charset=utf-8' return json.dumps(data) + def _get_package_type(self, id): + """ + Returns the package type for the given id. + + Uses a minimal context to do so. The main use of this method + is for figuring out which plugin to delegate to. + + aborts if an exception is raised. + """ + context = {'model': model, 'session': model.Session, + 'user': c.user or c.author} + try: + data = get_action('package_show')(context, {'id': id}) + except NotFound: + abort(404, _('Package not found')) + except NotAuthorized: + abort(401, _('Unauthorized to read package %s') % id) + + # packages don't have a type yet, so return None for now. + return None + def _save_new(self, context): try: data_dict = clean_dict(unflatten( @@ -528,9 +553,10 @@ def _save_new(self, context): def _save_edit(self, id, context): try: + package_type = self._get_package_type(id) data_dict = clean_dict(unflatten( tuplize_dict(parse_params(request.POST)))) - self._check_data_dict(data_dict) + self._check_data_dict(data_dict, package_type=package_type) context['message'] = data_dict.get('log_message', '') if not context['moderated']: context['pending'] = False diff --git a/ckan/tests/functional/test_package.py b/ckan/tests/functional/test_package.py index 49a336b4f3f..26be2403475 100644 --- a/ckan/tests/functional/test_package.py +++ b/ckan/tests/functional/test_package.py @@ -343,7 +343,13 @@ def test_read_plugin_hook(self): name = u'annakarenina' offset = url_for(controller='package', action='read', id=name) res = self.app.get(offset) - assert plugin.calls['read'] == 1, plugin.calls + + # There are now two reads of the package. The first to find out + # the package's type. And the second is the actual read that + # existed before. I don't know if this is a problem? I expect it + # can be fixed by allowing the package to be passed in to the plugin, + # either via the function argument, or adding it to the c object. + assert plugin.calls['read'] == 2, plugin.calls plugins.unload(plugin) From 489d3eb2cee54664ee3fc6fc35155d34c8591b44 Mon Sep 17 00:00:00 2001 From: Ian Murray Date: Wed, 23 Nov 2011 08:57:04 +0000 Subject: [PATCH 03/55] [plugin/interfaces] Cleared up the docs a little. --- ckan/plugins/interfaces.py | 42 +++++++++++++++++++++++++------------- 1 file changed, 28 insertions(+), 14 deletions(-) diff --git a/ckan/plugins/interfaces.py b/ckan/plugins/interfaces.py index 24058293288..d1bddb22f0d 100644 --- a/ckan/plugins/interfaces.py +++ b/ckan/plugins/interfaces.py @@ -333,15 +333,27 @@ class IPluggablePackageController(Interface): """ Allows customisation of the package controller as a plugin. - Different package types can be associated with different - IPluggablePackageControllers, allowing multiple package controllers - on a CKAN instance. + The behaviour of the plugin is determined by 5 method hooks: - The PackageController uses hooks to customise behaviour. A implementation - of IPluggablePackageController must implement these hooks. + - package_form(self) + - form_to_db_schema(self) + - db_to_form_schema(self) + - check_data_dict(self, data_dict) + - setup_template_variables(self, context, data_dict) - Implementations might want to consider subclassing - ckan.controllers.package.DefaultPluggablePackageController + Furthermore, there can be many implementations of this plugin registered + at once. With each instance associating itself with 0 or more package + type strings. When a package controller action is invoked, the package + type determines which of the registered plugins to delegate to. Each + implementation must implement two methods which are used to determine the + package-type -> plugin mapping: + + - is_fallback(self) + - package_types(self) + + Implementations might want to consider mixing in + ckan.controllers.package.DefaultPluggablePackageController which provides + default behaviours for the 5 method hooks. """ @@ -350,11 +362,13 @@ class IPluggablePackageController(Interface): def is_fallback(self): """ Returns true iff this provides the fallback behaviour, when no other - plugin matches a package's type. + plugin instance matches a package's type. There must be exactly one fallback controller defined, any attempt to - register more than one or to not have any registered, will throw - an exception at startup. + register more than one will throw an exception at startup. If there's + no fallback registered at startup the + ckan.controllers.package.DefaultPluggablePackageController is used + as the fallback. """ def package_types(self): @@ -362,17 +376,17 @@ def package_types(self): Returns an iterable of package type strings. If a request involving a package of one of those types is made, then - this plugin will be delegated to. + this plugin instance will be delegated to. There must only be one plugin registered to each package type. Any - attempts to register more than one plugin to a given package type will - raise an exception at startup. + attempts to register more than one plugin instance to a given package + type will raise an exception at startup. """ ##### End of control methods ##### Hooks for customising the PackageController's behaviour ##### - + ##### TODO: flesh out the docstrings a little more. ##### def package_form(self): """ Returns a string representing the location of the template to be From f9a5f91b313f2e1bc7bf5d11707683ae06810854 Mon Sep 17 00:00:00 2001 From: Ian Murray Date: Wed, 23 Nov 2011 10:18:12 +0000 Subject: [PATCH 04/55] [controller/package] A tidy up. - Moved the logic of registering plugin implementations to package-types from routing.py into controller/package.py - Renamed things more consistently. --- ckan/config/routing.py | 16 ++----- ckan/controllers/package.py | 88 ++++++++++++++++++++++++------------- 2 files changed, 61 insertions(+), 43 deletions(-) diff --git a/ckan/config/routing.py b/ckan/config/routing.py index 53d9a3f337b..62e543cc9ab 100644 --- a/ckan/config/routing.py +++ b/ckan/config/routing.py @@ -7,10 +7,8 @@ """ from pylons import config from routes import Mapper -from ckan.controllers.package import set_fallback_controller as set_fallback_package_controller,\ - add_package_controller,\ - set_default_as_fallback_controller_if_required as set_default_as_fallback_package_controller_if_required -from ckan.plugins import PluginImplementations, IRoutes, IPluggablePackageController +from ckan.controllers.package import register_pluggable_behaviour as register_pluggable_package_behaviour +from ckan.plugins import PluginImplementations, IRoutes routing_plugins = PluginImplementations(IRoutes) @@ -174,14 +172,8 @@ def make_map(): ## /END API ########### - for plugin in PluginImplementations(IPluggablePackageController): - if plugin.is_fallback(): - set_fallback_package_controller(plugin) - for package_type in plugin.package_types(): - add_package_controller(package_type, plugin) - - set_default_as_fallback_package_controller_if_required() - + register_pluggable_package_behaviour() + map.redirect("/packages", "/dataset") map.redirect("/packages/{url:.*}", "/dataset/{url}") map.redirect("/package", "/dataset") diff --git a/ckan/controllers/package.py b/ckan/controllers/package.py index c8f79cde2e5..1fb71ad027c 100644 --- a/ckan/controllers/package.py +++ b/ckan/controllers/package.py @@ -25,8 +25,7 @@ from ckan.logic import tuplize_dict, clean_dict, parse_params, flatten_to_string_key from ckan.lib.dictization import table_dictize from ckan.lib.i18n import get_lang -from ckan.plugins import SingletonPlugin, implements -from ckan.plugins.interfaces import IPluggablePackageController +from ckan.plugins import PluginImplementations, IPluggablePackageController import ckan.forms import ckan.authz import ckan.rating @@ -49,52 +48,79 @@ def search_url(params): ("text", "x-graphviz", ["dot"]), ] -_pluggable_package_controllers = dict() -_default_pluggable_package_controller = None +############## Methods and variables related to the pluggable ############## +############## behaviour of the package controller ############## -def set_fallback_controller(plugin_instance): - """ - Sets the fallback controller - """ - global _default_pluggable_package_controller - if _default_pluggable_package_controller is not None: - raise ValueError, "A fallback package controller has alread been registered" - _default_pluggable_package_controller = plugin_instance +# Mapping from package-type strings to IPluggablePackageController instances +_controller_behaviour_for = dict() + +# The fallback behaviour +_default_controller_behaviour = None -def set_default_as_fallback_controller_if_required(): +def register_pluggable_behaviour(): """ - Set the fallback controller to be an instance of DefaultPluggablePackageController + Register the various IPluggablePackageController instances. + + This method will setup the mappings between package types and the registered + IPluggablePackageController instances. If it's called more than once an + exception will be raised. """ - if _default_pluggable_package_controller is None: - set_fallback_controller(DefaultPluggablePackageController()) + global _default_controller_behaviour + + # Check this method hasn't been invoked already. + # TODO: This method seems to be being invoked more than once during running of + # the tests. So I've disbabled this check until I figure out why. + #if _default_controller_behaviour is not None: + #raise ValueError, "Pluggable package controller behaviour is already defined "\ + #"'%s'" % _default_controller_behaviour + + # Create the mappings and register the fallback behaviour if one is found. + for plugin in PluginImplementations(IPluggablePackageController): + if plugin.is_fallback(): + if _default_controller_behaviour is not None: + raise ValueError, "More than one fallback "\ + "IPluggablePackageController has been registered" + _default_controller_behaviour = plugin + + for package_type in plugin.package_types(): + if package_type in _controller_behaviour_for: + raise ValueError, "An existing IPluggablePackageController is "\ + "already associated with the package type "\ + "'%s'" % package_type + _controller_behaviour_for[package_type] = plugin + + # Setup the fallback behaviour if one hasn't been defined. + if _default_controller_behaviour is None: + _default_controller_behaviour = DefaultPluggablePackageController() def _lookup_plugin(package_type): """ Returns the plugin controller associoated with the given package type. - """ - if package_type is None: - return _default_pluggable_package_controller - return _pluggable_package_controllers.get(package_type, - _default_pluggable_package_controller) -def add_package_controller(package_type, plugin_instance): + If the package type is None or cannot be found in the mapping, then the + fallback behaviour is used. """ - Register the given plugin_instance to the given package_type. - """ - if package_type in _pluggable_package_controllers: - raise ValueError, 'A PluggablePackageController is already ' \ - 'associated with this package_type: "%s"' % package_type + if package_type is None: + return _default_controller_behaviour + return _controller_behaviour_for.get(package_type, + _default_controller_behaviour) class DefaultPluggablePackageController(object): """ - Provides a default implementation of the package controller. + Provides a default implementation of the pluggable package controller behaviour. + + This class has 2 purposes: + + - it provides a base class for IPluggablePackageController implementations + to use if only a subset of the 5 method hooks need to be customised. + + - it provides the fallback behaviour if no plugin is setup to provide + the fallback behaviour. Note - this isn't a plugin implementation. This is deliberate, as we don't want this being registered. """ - ##### Define the hooks that control the behaviour ##### - def package_form(self): return 'package/new_package_form.html' @@ -144,7 +170,7 @@ def setup_template_variables(self, context, data_dict): except NotAuthorized: c.auth_for_change_state = False - ## end hooks +############## End of pluggable package behaviour stuff ############## class PackageController(BaseController): From 01d796651b37c327c4caa842bb233e42249a2238 Mon Sep 17 00:00:00 2001 From: Ian Murray Date: Wed, 23 Nov 2011 11:08:47 +0000 Subject: [PATCH 05/55] [notes.txt] A temporary file containing some notes about the work in progress --- notes.txt | 12 ++++++++++++ 1 file changed, 12 insertions(+) create mode 100644 notes.txt diff --git a/notes.txt b/notes.txt new file mode 100644 index 00000000000..c307d2508de --- /dev/null +++ b/notes.txt @@ -0,0 +1,12 @@ +Notes for per-package-type behaviour. + + - I don't really know where the best place to create the mappings from + package-types to plugin instances is. At the moment, I'm creating the + mapping just prior to the package routing stuff in routing.py. + + - I've assumed package-type is a string. If it's not then only the docs + need to change (as long as it's some hashable value anyway). + + - Running the tests seems to be involing the routing.py more than once. I + haven't looked into why this is. Running the server seems to work fine + thuough. From 01e3e6ef9ab401b34145f267aa7b1977afcd1e59 Mon Sep 17 00:00:00 2001 From: rossjones Date: Mon, 5 Dec 2011 20:19:18 +0000 Subject: [PATCH 06/55] [model][xs] Cope with sqlite on my machine returns a datetime instead of a string. --- ckan/model/types.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ckan/model/types.py b/ckan/model/types.py index ff6f904389d..bd5e5948d52 100644 --- a/ckan/model/types.py +++ b/ckan/model/types.py @@ -85,7 +85,7 @@ def iso_date_to_datetime_for_sqlite(datetime_or_iso_date_if_sqlite): # to call this to convert it into a datetime type. When running on # postgres then you have a datetime anyway, so this function doesn't # do anything. - if meta.engine_is_sqlite(): + if meta.engine_is_sqlite() and isinstance(datetime_or_iso_date_if_sqlite, basestring): return datetime.datetime.strptime(datetime_or_iso_date_if_sqlite, '%Y-%m-%d %H:%M:%S.%f') else: From 51136465fb1bb94ea70df32be00eaef6ae43792b Mon Sep 17 00:00:00 2001 From: Ian Murray Date: Tue, 22 Nov 2011 17:28:46 +0000 Subject: [PATCH 07/55] [controller/package] A very rough sketch of pluggable package controller behaviour. --- ckan/config/routing.py | 13 +++++- ckan/controllers/package.py | 89 ++++++++++++++++++++++++++++++++----- ckan/plugins/interfaces.py | 79 +++++++++++++++++++++++++++++++- 3 files changed, 167 insertions(+), 14 deletions(-) diff --git a/ckan/config/routing.py b/ckan/config/routing.py index f946f2d29b9..90a0b582dca 100644 --- a/ckan/config/routing.py +++ b/ckan/config/routing.py @@ -7,7 +7,10 @@ """ from pylons import config from routes import Mapper -from ckan.plugins import PluginImplementations, IRoutes +from ckan.controllers.package import set_fallback_controller as set_fallback_package_controller,\ + add_package_controller,\ + set_default_as_fallback_controller_if_required as set_default_as_fallback_package_controller_if_required +from ckan.plugins import PluginImplementations, IRoutes, IPluggablePackageController routing_plugins = PluginImplementations(IRoutes) @@ -177,6 +180,14 @@ def make_map(): ## /END API ########### + for plugin in PluginImplementations(IPluggablePackageController): + if plugin.is_fallback(): + set_fallback_package_controller(plugin) + for package_type in plugin.package_types(): + add_package_controller(package_type, plugin) + + set_default_as_fallback_package_controller_if_required() + map.redirect("/packages", "/dataset") map.redirect("/packages/{url:.*}", "/dataset/{url}") map.redirect("/package", "/dataset") diff --git a/ckan/controllers/package.py b/ckan/controllers/package.py index 7d6ae52d2c5..ef5cb1aca87 100644 --- a/ckan/controllers/package.py +++ b/ckan/controllers/package.py @@ -25,6 +25,8 @@ from ckan.logic import tuplize_dict, clean_dict, parse_params, flatten_to_string_key from ckan.lib.dictization import table_dictize from ckan.lib.i18n import get_lang +from ckan.plugins import SingletonPlugin, implements +from ckan.plugins.interfaces import IPluggablePackageController import ckan.forms import ckan.authz import ckan.rating @@ -47,19 +49,63 @@ def search_url(params): ("text", "x-graphviz", ["dot"]), ] -class PackageController(BaseController): - - ## hooks for subclasses - package_form = 'package/new_package_form.html' - - def _form_to_db_schema(self): +_pluggable_package_controllers = dict() +_default_pluggable_package_controller = None + +def set_fallback_controller(plugin_instance): + """ + Sets the fallback controller + """ + global _default_pluggable_package_controller + if _default_pluggable_package_controller is not None: + raise ValueError, "A fallback package controller has alread been registered" + _default_pluggable_package_controller = plugin_instance + +def set_default_as_fallback_controller_if_required(): + """ + Set the fallback controller to be an instance of DefaultPluggablePackageController + """ + if _default_pluggable_package_controller is None: + set_fallback_controller(DefaultPluggablePackageController()) + +def _lookup_plugin(package_type): + """ + Returns the plugin controller associoated with the given package type. + """ + if package_type is None: + return _default_pluggable_package_controller + return _pluggable_package_controllers.get(package_type, + _default_pluggable_package_controller) + +def add_package_controller(package_type, plugin_instance): + """ + Register the given plugin_instance to the given package_type. + """ + if package_type in _pluggable_package_controllers: + raise ValueError, 'A PluggablePackageController is already ' \ + 'associated with this package_type: "%s"' % package_type + +class DefaultPluggablePackageController(object): + """ + Provides a default implementation of the package controller. + + Note - this isn't a plugin implementation. This is deliberate, as + we don't want this being registered. + """ + + ##### Define the hooks that control the behaviour ##### + + def package_form(self): + return 'package/new_package_form.html' + + def form_to_db_schema(self): return package_form_schema() - def _db_to_form_schema(self): + def db_to_form_schema(self): '''This is an interface to manipulate data from the database into a format suitable for the form (optional)''' - def _check_data_dict(self, data_dict): + def check_data_dict(self, data_dict): '''Check if the return data is correct, mostly for checking out if spammers are submitting only part of the form''' @@ -79,7 +125,7 @@ def _check_data_dict(self, data_dict): log.info('incorrect form fields posted') raise DataError(data_dict) - def _setup_template_variables(self, context, data_dict): + def setup_template_variables(self, context, data_dict): c.groups_authz = get_action('group_list_authz')(context, data_dict) data_dict.update({'available_only':True}) c.groups_available = get_action('group_list_authz')(context, data_dict) @@ -100,6 +146,27 @@ def _setup_template_variables(self, context, data_dict): ## end hooks +class PackageController(BaseController): + + def _package_form(self, package_type=None): + return _lookup_plugin(package_type).package_form() + + def _form_to_db_schema(self, package_type=None): + return _lookup_plugin(package_type).form_to_db_schema() + + def _db_to_form_schema(self, package_type=None): + '''This is an interface to manipulate data from the database + into a format suitable for the form (optional)''' + return _lookup_plugin(package_type).db_to_form_schema() + + def _check_data_dict(self, data_dict, package_type=None): + '''Check if the return data is correct, mostly for checking out if + spammers are submitting only part of the form''' + return _lookup_plugin(package_type).check_data_dict(data_dict) + + def _setup_template_variables(self, context, data_dict, package_type=None): + return _lookup_plugin(package_type).setup_template_variables(context, data_dict) + authorizer = ckan.authz.Authorizer() def search(self): @@ -339,7 +406,7 @@ def new(self, data=None, errors=None, error_summary=None): vars = {'data': data, 'errors': errors, 'error_summary': error_summary} self._setup_template_variables(context, {'id': id}) - c.form = render(self.package_form, extra_vars=vars) + c.form = render(self._package_form(), extra_vars=vars) return render('package/new.html') @@ -378,7 +445,7 @@ def edit(self, id, data=None, errors=None, error_summary=None): self._setup_template_variables(context, {'id': id}) - c.form = render(self.package_form, extra_vars=vars) + c.form = render(self._package_form(), extra_vars=vars) return render('package/edit.html') def read_ajax(self, id, revision=None): diff --git a/ckan/plugins/interfaces.py b/ckan/plugins/interfaces.py index 6c646bb2ab7..24058293288 100644 --- a/ckan/plugins/interfaces.py +++ b/ckan/plugins/interfaces.py @@ -12,13 +12,12 @@ 'IDomainObjectModification', 'IGroupController', 'IPackageController', 'IPluginObserver', 'IConfigurable', 'IConfigurer', 'IAuthorizer', - 'IActions', 'IResourceUrlChange' + 'IActions', 'IResourceUrlChange', 'IPluggablePackageController', ] from inspect import isclass from pyutilib.component.core import Interface as _pca_Interface - class Interface(_pca_Interface): @classmethod @@ -330,3 +329,79 @@ def get_auth_functions(self): implementation overrides """ +class IPluggablePackageController(Interface): + """ + Allows customisation of the package controller as a plugin. + + Different package types can be associated with different + IPluggablePackageControllers, allowing multiple package controllers + on a CKAN instance. + + The PackageController uses hooks to customise behaviour. A implementation + of IPluggablePackageController must implement these hooks. + + Implementations might want to consider subclassing + ckan.controllers.package.DefaultPluggablePackageController + + """ + + ##### These methods control when the plugin is delegated to ##### + + def is_fallback(self): + """ + Returns true iff this provides the fallback behaviour, when no other + plugin matches a package's type. + + There must be exactly one fallback controller defined, any attempt to + register more than one or to not have any registered, will throw + an exception at startup. + """ + + def package_types(self): + """ + Returns an iterable of package type strings. + + If a request involving a package of one of those types is made, then + this plugin will be delegated to. + + There must only be one plugin registered to each package type. Any + attempts to register more than one plugin to a given package type will + raise an exception at startup. + """ + + ##### End of control methods + + ##### Hooks for customising the PackageController's behaviour ##### + + def package_form(self): + """ + Returns a string representing the location of the template to be + rendered. e.g. "package/new_package_form.html". + """ + + def form_to_db_schema(self): + """ + Returns the schema for mapping package data from a form to a format + suitable for the database. + """ + + def db_to_form_schema(self): + """ + Returns the schema for mapping package data from the database into a + format suitable for the form (optional) + """ + + def check_data_dict(self, data_dict): + """ + Check if the return data is correct. + + raise a DataError if not. + """ + + def setup_template_variables(self, context, data_dict): + """ + Add variables to c just prior to the template being rendered. + """ + + ##### End of hooks ##### + From 7fdfb2fc23589be61266fbcbc0da3e59556e6fb0 Mon Sep 17 00:00:00 2001 From: Ian Murray Date: Wed, 23 Nov 2011 08:45:26 +0000 Subject: [PATCH 08/55] [controllers/package] Created a stub where the package's type can be determined Goes through the action of creating the package. It just has nothing to return yet. --- ckan/controllers/package.py | 44 +++++++++++++++++++++------ ckan/tests/functional/test_package.py | 8 ++++- 2 files changed, 42 insertions(+), 10 deletions(-) diff --git a/ckan/controllers/package.py b/ckan/controllers/package.py index ef5cb1aca87..29c37c9b016 100644 --- a/ckan/controllers/package.py +++ b/ckan/controllers/package.py @@ -244,9 +244,10 @@ def pager_url(q=None, page=None): def read(self, id): + package_type = self._get_package_type(id.split('@')[0]) context = {'model': model, 'session': model.Session, 'user': c.user or c.author, 'extras_as_string': True, - 'schema': self._form_to_db_schema()} + 'schema': self._form_to_db_schema(package_type=package_type)} data_dict = {'id': id} # interpret @ or @ suffix @@ -295,9 +296,10 @@ def read(self, id): return render('package/read.html') def comments(self, id): + package_type = self._get_package_type(id) context = {'model': model, 'session': model.Session, 'user': c.user or c.author, 'extras_as_string': True, - 'schema': self._form_to_db_schema()} + 'schema': self._form_to_db_schema(package_type=package_type)} #check if package exists try: @@ -412,18 +414,19 @@ def new(self, data=None, errors=None, error_summary=None): def edit(self, id, data=None, errors=None, error_summary=None): + package_type = self._get_package_type(id) context = {'model': model, 'session': model.Session, 'user': c.user or c.author, 'extras_as_string': True, 'save': 'save' in request.params, 'moderated': config.get('moderated'), 'pending': True, - 'schema': self._form_to_db_schema()} + 'schema': self._form_to_db_schema(package_type=package_type)} if context['save'] and not data: return self._save_edit(id, context) try: old_data = get_action('package_show')(context, {'id':id}) - schema = self._db_to_form_schema() + schema = self._db_to_form_schema(package_type=package_type) if schema and not data: old_data, errors = validate(old_data, schema, context=context) data = data or old_data @@ -443,21 +446,22 @@ def edit(self, id, data=None, errors=None, error_summary=None): errors = errors or {} vars = {'data': data, 'errors': errors, 'error_summary': error_summary} - self._setup_template_variables(context, {'id': id}) + self._setup_template_variables(context, {'id': id}, package_type=package_type) - c.form = render(self._package_form(), extra_vars=vars) + c.form = render(self._package_form(package_type=package_type), extra_vars=vars) return render('package/edit.html') def read_ajax(self, id, revision=None): + package_type=self._get_package_type(id) context = {'model': model, 'session': model.Session, 'user': c.user or c.author, 'extras_as_string': True, - 'schema': self._form_to_db_schema(), + 'schema': self._form_to_db_schema(package_type=package_type), 'revision_id': revision} try: data = get_action('package_show')(context, {'id': id}) - schema = self._db_to_form_schema() + schema = self._db_to_form_schema(package_type=package_type) if schema: data, errors = validate(data, schema) except NotAuthorized: @@ -504,6 +508,27 @@ def history_ajax(self, id): response.headers['Content-Type'] = 'application/json;charset=utf-8' return json.dumps(data) + def _get_package_type(self, id): + """ + Returns the package type for the given id. + + Uses a minimal context to do so. The main use of this method + is for figuring out which plugin to delegate to. + + aborts if an exception is raised. + """ + context = {'model': model, 'session': model.Session, + 'user': c.user or c.author} + try: + data = get_action('package_show')(context, {'id': id}) + except NotFound: + abort(404, _('Package not found')) + except NotAuthorized: + abort(401, _('Unauthorized to read package %s') % id) + + # packages don't have a type yet, so return None for now. + return None + def _save_new(self, context): try: data_dict = clean_dict(unflatten( @@ -528,9 +553,10 @@ def _save_new(self, context): def _save_edit(self, id, context): try: + package_type = self._get_package_type(id) data_dict = clean_dict(unflatten( tuplize_dict(parse_params(request.POST)))) - self._check_data_dict(data_dict) + self._check_data_dict(data_dict, package_type=package_type) context['message'] = data_dict.get('log_message', '') if not context['moderated']: context['pending'] = False diff --git a/ckan/tests/functional/test_package.py b/ckan/tests/functional/test_package.py index 5e81d3763c2..cae4dcf4f11 100644 --- a/ckan/tests/functional/test_package.py +++ b/ckan/tests/functional/test_package.py @@ -346,7 +346,13 @@ def test_read_plugin_hook(self): name = u'annakarenina' offset = url_for(controller='package', action='read', id=name) res = self.app.get(offset) - assert plugin.calls['read'] == 1, plugin.calls + + # There are now two reads of the package. The first to find out + # the package's type. And the second is the actual read that + # existed before. I don't know if this is a problem? I expect it + # can be fixed by allowing the package to be passed in to the plugin, + # either via the function argument, or adding it to the c object. + assert plugin.calls['read'] == 2, plugin.calls plugins.unload(plugin) From 0a190a1c9044f6ac4b22ebdf33cada6c16a06c2f Mon Sep 17 00:00:00 2001 From: Ian Murray Date: Wed, 23 Nov 2011 08:57:04 +0000 Subject: [PATCH 09/55] [plugin/interfaces] Cleared up the docs a little. --- ckan/plugins/interfaces.py | 42 +++++++++++++++++++++++++------------- 1 file changed, 28 insertions(+), 14 deletions(-) diff --git a/ckan/plugins/interfaces.py b/ckan/plugins/interfaces.py index 24058293288..d1bddb22f0d 100644 --- a/ckan/plugins/interfaces.py +++ b/ckan/plugins/interfaces.py @@ -333,15 +333,27 @@ class IPluggablePackageController(Interface): """ Allows customisation of the package controller as a plugin. - Different package types can be associated with different - IPluggablePackageControllers, allowing multiple package controllers - on a CKAN instance. + The behaviour of the plugin is determined by 5 method hooks: - The PackageController uses hooks to customise behaviour. A implementation - of IPluggablePackageController must implement these hooks. + - package_form(self) + - form_to_db_schema(self) + - db_to_form_schema(self) + - check_data_dict(self, data_dict) + - setup_template_variables(self, context, data_dict) - Implementations might want to consider subclassing - ckan.controllers.package.DefaultPluggablePackageController + Furthermore, there can be many implementations of this plugin registered + at once. With each instance associating itself with 0 or more package + type strings. When a package controller action is invoked, the package + type determines which of the registered plugins to delegate to. Each + implementation must implement two methods which are used to determine the + package-type -> plugin mapping: + + - is_fallback(self) + - package_types(self) + + Implementations might want to consider mixing in + ckan.controllers.package.DefaultPluggablePackageController which provides + default behaviours for the 5 method hooks. """ @@ -350,11 +362,13 @@ class IPluggablePackageController(Interface): def is_fallback(self): """ Returns true iff this provides the fallback behaviour, when no other - plugin matches a package's type. + plugin instance matches a package's type. There must be exactly one fallback controller defined, any attempt to - register more than one or to not have any registered, will throw - an exception at startup. + register more than one will throw an exception at startup. If there's + no fallback registered at startup the + ckan.controllers.package.DefaultPluggablePackageController is used + as the fallback. """ def package_types(self): @@ -362,17 +376,17 @@ def package_types(self): Returns an iterable of package type strings. If a request involving a package of one of those types is made, then - this plugin will be delegated to. + this plugin instance will be delegated to. There must only be one plugin registered to each package type. Any - attempts to register more than one plugin to a given package type will - raise an exception at startup. + attempts to register more than one plugin instance to a given package + type will raise an exception at startup. """ ##### End of control methods ##### Hooks for customising the PackageController's behaviour ##### - + ##### TODO: flesh out the docstrings a little more. ##### def package_form(self): """ Returns a string representing the location of the template to be From aaddb14cf3d1c293e8cef8ede03860693a7f7215 Mon Sep 17 00:00:00 2001 From: Ian Murray Date: Wed, 23 Nov 2011 10:18:12 +0000 Subject: [PATCH 10/55] [controller/package] A tidy up. - Moved the logic of registering plugin implementations to package-types from routing.py into controller/package.py - Renamed things more consistently. --- ckan/config/routing.py | 16 ++----- ckan/controllers/package.py | 88 ++++++++++++++++++++++++------------- 2 files changed, 61 insertions(+), 43 deletions(-) diff --git a/ckan/config/routing.py b/ckan/config/routing.py index 90a0b582dca..3835c6fe813 100644 --- a/ckan/config/routing.py +++ b/ckan/config/routing.py @@ -7,10 +7,8 @@ """ from pylons import config from routes import Mapper -from ckan.controllers.package import set_fallback_controller as set_fallback_package_controller,\ - add_package_controller,\ - set_default_as_fallback_controller_if_required as set_default_as_fallback_package_controller_if_required -from ckan.plugins import PluginImplementations, IRoutes, IPluggablePackageController +from ckan.controllers.package import register_pluggable_behaviour as register_pluggable_package_behaviour +from ckan.plugins import PluginImplementations, IRoutes routing_plugins = PluginImplementations(IRoutes) @@ -180,14 +178,8 @@ def make_map(): ## /END API ########### - for plugin in PluginImplementations(IPluggablePackageController): - if plugin.is_fallback(): - set_fallback_package_controller(plugin) - for package_type in plugin.package_types(): - add_package_controller(package_type, plugin) - - set_default_as_fallback_package_controller_if_required() - + register_pluggable_package_behaviour() + map.redirect("/packages", "/dataset") map.redirect("/packages/{url:.*}", "/dataset/{url}") map.redirect("/package", "/dataset") diff --git a/ckan/controllers/package.py b/ckan/controllers/package.py index 29c37c9b016..13a28fc4b45 100644 --- a/ckan/controllers/package.py +++ b/ckan/controllers/package.py @@ -25,8 +25,7 @@ from ckan.logic import tuplize_dict, clean_dict, parse_params, flatten_to_string_key from ckan.lib.dictization import table_dictize from ckan.lib.i18n import get_lang -from ckan.plugins import SingletonPlugin, implements -from ckan.plugins.interfaces import IPluggablePackageController +from ckan.plugins import PluginImplementations, IPluggablePackageController import ckan.forms import ckan.authz import ckan.rating @@ -49,52 +48,79 @@ def search_url(params): ("text", "x-graphviz", ["dot"]), ] -_pluggable_package_controllers = dict() -_default_pluggable_package_controller = None +############## Methods and variables related to the pluggable ############## +############## behaviour of the package controller ############## -def set_fallback_controller(plugin_instance): - """ - Sets the fallback controller - """ - global _default_pluggable_package_controller - if _default_pluggable_package_controller is not None: - raise ValueError, "A fallback package controller has alread been registered" - _default_pluggable_package_controller = plugin_instance +# Mapping from package-type strings to IPluggablePackageController instances +_controller_behaviour_for = dict() + +# The fallback behaviour +_default_controller_behaviour = None -def set_default_as_fallback_controller_if_required(): +def register_pluggable_behaviour(): """ - Set the fallback controller to be an instance of DefaultPluggablePackageController + Register the various IPluggablePackageController instances. + + This method will setup the mappings between package types and the registered + IPluggablePackageController instances. If it's called more than once an + exception will be raised. """ - if _default_pluggable_package_controller is None: - set_fallback_controller(DefaultPluggablePackageController()) + global _default_controller_behaviour + + # Check this method hasn't been invoked already. + # TODO: This method seems to be being invoked more than once during running of + # the tests. So I've disbabled this check until I figure out why. + #if _default_controller_behaviour is not None: + #raise ValueError, "Pluggable package controller behaviour is already defined "\ + #"'%s'" % _default_controller_behaviour + + # Create the mappings and register the fallback behaviour if one is found. + for plugin in PluginImplementations(IPluggablePackageController): + if plugin.is_fallback(): + if _default_controller_behaviour is not None: + raise ValueError, "More than one fallback "\ + "IPluggablePackageController has been registered" + _default_controller_behaviour = plugin + + for package_type in plugin.package_types(): + if package_type in _controller_behaviour_for: + raise ValueError, "An existing IPluggablePackageController is "\ + "already associated with the package type "\ + "'%s'" % package_type + _controller_behaviour_for[package_type] = plugin + + # Setup the fallback behaviour if one hasn't been defined. + if _default_controller_behaviour is None: + _default_controller_behaviour = DefaultPluggablePackageController() def _lookup_plugin(package_type): """ Returns the plugin controller associoated with the given package type. - """ - if package_type is None: - return _default_pluggable_package_controller - return _pluggable_package_controllers.get(package_type, - _default_pluggable_package_controller) -def add_package_controller(package_type, plugin_instance): + If the package type is None or cannot be found in the mapping, then the + fallback behaviour is used. """ - Register the given plugin_instance to the given package_type. - """ - if package_type in _pluggable_package_controllers: - raise ValueError, 'A PluggablePackageController is already ' \ - 'associated with this package_type: "%s"' % package_type + if package_type is None: + return _default_controller_behaviour + return _controller_behaviour_for.get(package_type, + _default_controller_behaviour) class DefaultPluggablePackageController(object): """ - Provides a default implementation of the package controller. + Provides a default implementation of the pluggable package controller behaviour. + + This class has 2 purposes: + + - it provides a base class for IPluggablePackageController implementations + to use if only a subset of the 5 method hooks need to be customised. + + - it provides the fallback behaviour if no plugin is setup to provide + the fallback behaviour. Note - this isn't a plugin implementation. This is deliberate, as we don't want this being registered. """ - ##### Define the hooks that control the behaviour ##### - def package_form(self): return 'package/new_package_form.html' @@ -144,7 +170,7 @@ def setup_template_variables(self, context, data_dict): except NotAuthorized: c.auth_for_change_state = False - ## end hooks +############## End of pluggable package behaviour stuff ############## class PackageController(BaseController): From 0301085dc3f666c715ce4460aad152cb60a1be81 Mon Sep 17 00:00:00 2001 From: Ian Murray Date: Wed, 23 Nov 2011 11:08:47 +0000 Subject: [PATCH 11/55] [notes.txt] A temporary file containing some notes about the work in progress --- notes.txt | 12 ++++++++++++ 1 file changed, 12 insertions(+) create mode 100644 notes.txt diff --git a/notes.txt b/notes.txt new file mode 100644 index 00000000000..c307d2508de --- /dev/null +++ b/notes.txt @@ -0,0 +1,12 @@ +Notes for per-package-type behaviour. + + - I don't really know where the best place to create the mappings from + package-types to plugin instances is. At the moment, I'm creating the + mapping just prior to the package routing stuff in routing.py. + + - I've assumed package-type is a string. If it's not then only the docs + need to change (as long as it's some hashable value anyway). + + - Running the tests seems to be involing the routing.py more than once. I + haven't looked into why this is. Running the server seems to work fine + thuough. From d02439a06ee7e725048093a7d89550eb29442938 Mon Sep 17 00:00:00 2001 From: rossjones Date: Wed, 7 Dec 2011 15:26:42 +0000 Subject: [PATCH 12/55] Added a type_name field to the Package. Allows us to use a custom form where type_name has a value and a plugin (IDatasetForm) is bound to that value. If no type_name is defined then the default handler is used instead. --- ckan/controllers/package.py | 29 +++++++++++-------- ckan/lib/dictization/model_dictize.py | 2 +- .../versions/0047_add_package_type.py | 16 ++++++++++ ckan/model/package.py | 1 + ckan/plugins/interfaces.py | 4 +-- 5 files changed, 37 insertions(+), 15 deletions(-) create mode 100644 ckan/migration/versions/0047_add_package_type.py diff --git a/ckan/controllers/package.py b/ckan/controllers/package.py index 13a28fc4b45..c230fcf475a 100644 --- a/ckan/controllers/package.py +++ b/ckan/controllers/package.py @@ -25,7 +25,7 @@ from ckan.logic import tuplize_dict, clean_dict, parse_params, flatten_to_string_key from ckan.lib.dictization import table_dictize from ckan.lib.i18n import get_lang -from ckan.plugins import PluginImplementations, IPluggablePackageController +from ckan.plugins import PluginImplementations, IDatasetForm import ckan.forms import ckan.authz import ckan.rating @@ -51,7 +51,7 @@ def search_url(params): ############## Methods and variables related to the pluggable ############## ############## behaviour of the package controller ############## -# Mapping from package-type strings to IPluggablePackageController instances +# Mapping from package-type strings to IDatasetForm instances _controller_behaviour_for = dict() # The fallback behaviour @@ -59,10 +59,10 @@ def search_url(params): def register_pluggable_behaviour(): """ - Register the various IPluggablePackageController instances. + Register the various IDatasetForm instances. This method will setup the mappings between package types and the registered - IPluggablePackageController instances. If it's called more than once an + IDatasetForm instances. If it's called more than once an exception will be raised. """ global _default_controller_behaviour @@ -75,16 +75,16 @@ def register_pluggable_behaviour(): #"'%s'" % _default_controller_behaviour # Create the mappings and register the fallback behaviour if one is found. - for plugin in PluginImplementations(IPluggablePackageController): + for plugin in PluginImplementations(IDatasetForm): if plugin.is_fallback(): if _default_controller_behaviour is not None: raise ValueError, "More than one fallback "\ - "IPluggablePackageController has been registered" + "IDatasetForm has been registered" _default_controller_behaviour = plugin for package_type in plugin.package_types(): if package_type in _controller_behaviour_for: - raise ValueError, "An existing IPluggablePackageController is "\ + raise ValueError, "An existing IDatasetForm is "\ "already associated with the package type "\ "'%s'" % package_type _controller_behaviour_for[package_type] = plugin @@ -100,6 +100,7 @@ def _lookup_plugin(package_type): If the package type is None or cannot be found in the mapping, then the fallback behaviour is used. """ + #from pdb import set_trace; set_trace() if package_type is None: return _default_controller_behaviour return _controller_behaviour_for.get(package_type, @@ -111,7 +112,7 @@ class DefaultPluggablePackageController(object): This class has 2 purposes: - - it provides a base class for IPluggablePackageController implementations + - it provides a base class for IDatasetForm implementations to use if only a subset of the 5 method hooks need to be customised. - it provides the fallback behaviour if no plugin is setup to provide @@ -174,7 +175,7 @@ def setup_template_variables(self, context, data_dict): class PackageController(BaseController): - def _package_form(self, package_type=None): + def _package_form(self, package_type=None): return _lookup_plugin(package_type).package_form() def _form_to_db_schema(self, package_type=None): @@ -536,13 +537,18 @@ def history_ajax(self, id): def _get_package_type(self, id): """ - Returns the package type for the given id. + Given the id of a package it determines the plugin to load + based on the package's type name (type_name). The plugin found + will be returned, or None if there is no plugin associated with + the type. Uses a minimal context to do so. The main use of this method is for figuring out which plugin to delegate to. aborts if an exception is raised. """ + global _controller_behaviour_for + context = {'model': model, 'session': model.Session, 'user': c.user or c.author} try: @@ -552,8 +558,7 @@ def _get_package_type(self, id): except NotAuthorized: abort(401, _('Unauthorized to read package %s') % id) - # packages don't have a type yet, so return None for now. - return None + return data['type_name'] def _save_new(self, context): try: diff --git a/ckan/lib/dictization/model_dictize.py b/ckan/lib/dictization/model_dictize.py index 675d47f1712..f8e1e459418 100644 --- a/ckan/lib/dictization/model_dictize.py +++ b/ckan/lib/dictization/model_dictize.py @@ -159,7 +159,7 @@ def package_dictize(pkg, context): # Get an actual Package object, not a PackageRevision pkg_object = model.Package.get(pkg.id) result_dict['isopen'] = pkg_object.isopen if isinstance(pkg_object.isopen,bool) else pkg_object.isopen() - + result_dict['type_name']= pkg_object.type_name return result_dict def group_dictize(group, context): diff --git a/ckan/migration/versions/0047_add_package_type.py b/ckan/migration/versions/0047_add_package_type.py new file mode 100644 index 00000000000..35b82c12139 --- /dev/null +++ b/ckan/migration/versions/0047_add_package_type.py @@ -0,0 +1,16 @@ +from sqlalchemy import * +from migrate import * + +def upgrade(migrate_engine): + metadata = MetaData() + metadata.bind = migrate_engine + + migrate_engine.execute( + ''' + ALTER TABLE package + ADD COLUMN type_name text; + + ALTER TABLE package_revision + ADD COLUMN type_name text; + ''' + ) diff --git a/ckan/model/package.py b/ckan/model/package.py index eb0da1d2fd1..ca13d968433 100644 --- a/ckan/model/package.py +++ b/ckan/model/package.py @@ -37,6 +37,7 @@ Column('maintainer_email', types.UnicodeText), Column('notes', types.UnicodeText), Column('license_id', types.UnicodeText), + Column('type_name', types.UnicodeText), ) diff --git a/ckan/plugins/interfaces.py b/ckan/plugins/interfaces.py index d1bddb22f0d..dd24bc3d81f 100644 --- a/ckan/plugins/interfaces.py +++ b/ckan/plugins/interfaces.py @@ -12,7 +12,7 @@ 'IDomainObjectModification', 'IGroupController', 'IPackageController', 'IPluginObserver', 'IConfigurable', 'IConfigurer', 'IAuthorizer', - 'IActions', 'IResourceUrlChange', 'IPluggablePackageController', + 'IActions', 'IResourceUrlChange', 'IDatasetForm', ] from inspect import isclass @@ -329,7 +329,7 @@ def get_auth_functions(self): implementation overrides """ -class IPluggablePackageController(Interface): +class IDatasetForm(Interface): """ Allows customisation of the package controller as a plugin. From 0b8803902a6de09431b587690eba0eec14068814 Mon Sep 17 00:00:00 2001 From: rossjones Date: Thu, 8 Dec 2011 10:43:17 +0000 Subject: [PATCH 13/55] Changes to the package naming to be consistent with plugin in ckanext-example --- ckan/controllers/package.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ckan/controllers/package.py b/ckan/controllers/package.py index c230fcf475a..13a0d8d28a2 100644 --- a/ckan/controllers/package.py +++ b/ckan/controllers/package.py @@ -91,7 +91,7 @@ def register_pluggable_behaviour(): # Setup the fallback behaviour if one hasn't been defined. if _default_controller_behaviour is None: - _default_controller_behaviour = DefaultPluggablePackageController() + _default_controller_behaviour = DefaultDatasetForm() def _lookup_plugin(package_type): """ @@ -106,7 +106,7 @@ def _lookup_plugin(package_type): return _controller_behaviour_for.get(package_type, _default_controller_behaviour) -class DefaultPluggablePackageController(object): +class DefaultDatasetForm(object): """ Provides a default implementation of the pluggable package controller behaviour. From 0e3e99861a16c37065f4f5a37fff8a6fe16f03c7 Mon Sep 17 00:00:00 2001 From: rossjones Date: Thu, 8 Dec 2011 10:46:48 +0000 Subject: [PATCH 14/55] Fix for test_action.TestAction.test_13_group_list_by_size Results were ordered differently on postgres than they presumably are in sqlite. Have sorted() the results sets so that assertEqual doesn't fail when running tests under postgres. --- ckan/tests/functional/api/test_action.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ckan/tests/functional/api/test_action.py b/ckan/tests/functional/api/test_action.py index 8d3cb1d978d..868add6c5f5 100644 --- a/ckan/tests/functional/api/test_action.py +++ b/ckan/tests/functional/api/test_action.py @@ -563,7 +563,7 @@ def test_13_group_list_by_size(self): res = self.app.post('/api/action/group_list', params=postparams) res_obj = json.loads(res.body) - assert_equal(res_obj['result'], ['david', + assert_equal(sorted(res_obj['result']), ['david', 'roger']) def test_13_group_list_by_size_all_fields(self): From 5a7a9781b1b379b0b993767137b06b18f136c8f4 Mon Sep 17 00:00:00 2001 From: rossjones Date: Thu, 8 Dec 2011 12:28:50 +0000 Subject: [PATCH 15/55] Fixed test_dictization to take account of the new field in Package Package has a new field called type_name that wasn't present in all of the dicts in these tests. --- ckan/tests/lib/test_dictization.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/ckan/tests/lib/test_dictization.py b/ckan/tests/lib/test_dictization.py index f2987723018..98f7d646917 100644 --- a/ckan/tests/lib/test_dictization.py +++ b/ckan/tests/lib/test_dictization.py @@ -47,6 +47,7 @@ def setup_class(cls): 'license_id': u'other-open', 'maintainer': None, 'maintainer_email': None, + 'type_name': None, 'name': u'annakarenina', 'notes': u'Some test notes\n\n### A 3rd level heading\n\n**Some bolded text.**\n\n*Some italicized text.*\n\nForeign characters:\nu with umlaut \xfc\n66-style quote \u201c\nforeign word: th\xfcmb\n \nNeeds escaping:\nleft arrow <\n\n\n\n', 'relationships_as_object': [], @@ -146,6 +147,7 @@ def test_01_dictize_main_objects_simple(self): 'maintainer': None, 'maintainer_email': None, 'name': u'annakarenina', + 'type_name': None, 'notes': u'Some test notes\n\n### A 3rd level heading\n\n**Some bolded text.**\n\n*Some italicized text.*\n\nForeign characters:\nu with umlaut \xfc\n66-style quote \u201c\nforeign word: th\xfcmb\n \nNeeds escaping:\nleft arrow <\n\n\n\n', 'state': u'active', 'title': u'A Novel By Tolstoy', @@ -843,6 +845,7 @@ def test_16_group_dictized(self): 'license_id': u'other-open', 'maintainer': None, 'maintainer_email': None, + 'type_name': None, 'name': u'annakarenina3', 'notes': u'Some test notes\n\n### A 3rd level heading\n\n**Some bolded text.**\n\n*Some italicized text.*\n\nForeign characters:\nu with umlaut \xfc\n66-style quote \u201c\nforeign word: th\xfcmb\n \nNeeds escaping:\nleft arrow <\n\n\n\n', 'state': u'active', @@ -854,6 +857,7 @@ def test_16_group_dictized(self): 'license_id': u'other-open', 'maintainer': None, 'maintainer_email': None, + 'type_name': None, 'name': u'annakarenina2', 'notes': u'Some test notes\n\n### A 3rd level heading\n\n**Some bolded text.**\n\n*Some italicized text.*\n\nForeign characters:\nu with umlaut \xfc\n66-style quote \u201c\nforeign word: th\xfcmb\n \nNeeds escaping:\nleft arrow <\n\n\n\n', 'state': u'active', From 0ec041e89a8bf8fca0921a4f8b983877fb532d4c Mon Sep 17 00:00:00 2001 From: Ross Jones Date: Fri, 9 Dec 2011 09:19:10 +0000 Subject: [PATCH 16/55] Added my migration to the merged migration --- ckan/migration/versions/047_rename_package_group_member.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/ckan/migration/versions/047_rename_package_group_member.py b/ckan/migration/versions/047_rename_package_group_member.py index 91ee98b9cb6..9f2eb5bc32e 100644 --- a/ckan/migration/versions/047_rename_package_group_member.py +++ b/ckan/migration/versions/047_rename_package_group_member.py @@ -97,6 +97,10 @@ def upgrade(migrate_engine): ALTER TABLE "group_revision" ALTER COLUMN "type" set not null; +ALTER TABLE "package" + ADD COLUMN "type_name" text; +ALTER TABLE "package_revision" + ADD COLUMN "type_name" text; COMMIT; ''' From 8af9e24d8923529ec1befd5d54c91a7a2b67df17 Mon Sep 17 00:00:00 2001 From: Ross Jones Date: Fri, 9 Dec 2011 09:53:48 +0000 Subject: [PATCH 17/55] Renamed the new package field to match the group field The Group model now has similar functionality but it's type field is called type and so have renamed the package field type_name to match --- ckan/controllers/package.py | 4 ++-- ckan/lib/dictization/model_dictize.py | 2 +- ckan/migration/versions/0047_add_package_type.py | 16 ---------------- .../versions/047_rename_package_group_member.py | 4 ++-- ckan/model/package.py | 2 +- ckan/tests/lib/test_dictization.py | 8 ++++---- 6 files changed, 10 insertions(+), 26 deletions(-) delete mode 100644 ckan/migration/versions/0047_add_package_type.py diff --git a/ckan/controllers/package.py b/ckan/controllers/package.py index c3f2b934524..ea772f953a2 100644 --- a/ckan/controllers/package.py +++ b/ckan/controllers/package.py @@ -540,7 +540,7 @@ def history_ajax(self, id): def _get_package_type(self, id): """ Given the id of a package it determines the plugin to load - based on the package's type name (type_name). The plugin found + based on the package's type name (type). The plugin found will be returned, or None if there is no plugin associated with the type. @@ -560,7 +560,7 @@ def _get_package_type(self, id): except NotAuthorized: abort(401, _('Unauthorized to read package %s') % id) - return data['type_name'] + return data['type'] def _save_new(self, context): try: diff --git a/ckan/lib/dictization/model_dictize.py b/ckan/lib/dictization/model_dictize.py index eabca531e2a..38a89cf6a85 100644 --- a/ckan/lib/dictization/model_dictize.py +++ b/ckan/lib/dictization/model_dictize.py @@ -159,7 +159,7 @@ def package_dictize(pkg, context): # Get an actual Package object, not a PackageRevision pkg_object = model.Package.get(pkg.id) result_dict['isopen'] = pkg_object.isopen if isinstance(pkg_object.isopen,bool) else pkg_object.isopen() - result_dict['type_name']= pkg_object.type_name + result_dict['type']= pkg_object.type return result_dict def group_dictize(group, context): diff --git a/ckan/migration/versions/0047_add_package_type.py b/ckan/migration/versions/0047_add_package_type.py deleted file mode 100644 index 35b82c12139..00000000000 --- a/ckan/migration/versions/0047_add_package_type.py +++ /dev/null @@ -1,16 +0,0 @@ -from sqlalchemy import * -from migrate import * - -def upgrade(migrate_engine): - metadata = MetaData() - metadata.bind = migrate_engine - - migrate_engine.execute( - ''' - ALTER TABLE package - ADD COLUMN type_name text; - - ALTER TABLE package_revision - ADD COLUMN type_name text; - ''' - ) diff --git a/ckan/migration/versions/047_rename_package_group_member.py b/ckan/migration/versions/047_rename_package_group_member.py index 9f2eb5bc32e..f06afb05645 100644 --- a/ckan/migration/versions/047_rename_package_group_member.py +++ b/ckan/migration/versions/047_rename_package_group_member.py @@ -98,9 +98,9 @@ def upgrade(migrate_engine): ALTER COLUMN "type" set not null; ALTER TABLE "package" - ADD COLUMN "type_name" text; + ADD COLUMN "type" text; ALTER TABLE "package_revision" - ADD COLUMN "type_name" text; + ADD COLUMN "type" text; COMMIT; ''' diff --git a/ckan/model/package.py b/ckan/model/package.py index ff7c285da19..fd9fe8fdd6f 100644 --- a/ckan/model/package.py +++ b/ckan/model/package.py @@ -37,7 +37,7 @@ Column('maintainer_email', types.UnicodeText), Column('notes', types.UnicodeText), Column('license_id', types.UnicodeText), - Column('type_name', types.UnicodeText), + Column('type', types.UnicodeText), ) diff --git a/ckan/tests/lib/test_dictization.py b/ckan/tests/lib/test_dictization.py index d27c7b5a772..1b1001329c4 100644 --- a/ckan/tests/lib/test_dictization.py +++ b/ckan/tests/lib/test_dictization.py @@ -49,7 +49,7 @@ def setup_class(cls): 'license_id': u'other-open', 'maintainer': None, 'maintainer_email': None, - 'type_name': None, + 'type': None, 'name': u'annakarenina', 'notes': u'Some test notes\n\n### A 3rd level heading\n\n**Some bolded text.**\n\n*Some italicized text.*\n\nForeign characters:\nu with umlaut \xfc\n66-style quote \u201c\nforeign word: th\xfcmb\n \nNeeds escaping:\nleft arrow <\n\n\n\n', 'relationships_as_object': [], @@ -149,7 +149,7 @@ def test_01_dictize_main_objects_simple(self): 'maintainer': None, 'maintainer_email': None, 'name': u'annakarenina', - 'type_name': None, + 'type': None, 'notes': u'Some test notes\n\n### A 3rd level heading\n\n**Some bolded text.**\n\n*Some italicized text.*\n\nForeign characters:\nu with umlaut \xfc\n66-style quote \u201c\nforeign word: th\xfcmb\n \nNeeds escaping:\nleft arrow <\n\n\n\n', 'state': u'active', 'title': u'A Novel By Tolstoy', @@ -847,7 +847,7 @@ def test_16_group_dictized(self): 'license_id': u'other-open', 'maintainer': None, 'maintainer_email': None, - 'type_name': None, + 'type': None, 'name': u'annakarenina3', 'notes': u'Some test notes\n\n### A 3rd level heading\n\n**Some bolded text.**\n\n*Some italicized text.*\n\nForeign characters:\nu with umlaut \xfc\n66-style quote \u201c\nforeign word: th\xfcmb\n \nNeeds escaping:\nleft arrow <\n\n\n\n', 'state': u'active', @@ -859,7 +859,7 @@ def test_16_group_dictized(self): 'license_id': u'other-open', 'maintainer': None, 'maintainer_email': None, - 'type_name': None, + 'type': None, 'name': u'annakarenina2', 'notes': u'Some test notes\n\n### A 3rd level heading\n\n**Some bolded text.**\n\n*Some italicized text.*\n\nForeign characters:\nu with umlaut \xfc\n66-style quote \u201c\nforeign word: th\xfcmb\n \nNeeds escaping:\nleft arrow <\n\n\n\n', 'state': u'active', From 8b8254669d3801699c93bcce492f2573ccc0181c Mon Sep 17 00:00:00 2001 From: Ross Jones Date: Fri, 9 Dec 2011 10:45:49 +0000 Subject: [PATCH 18/55] Initial implementation of IGroupForm plugin with local DefaultGroupForm As we need an implementation of IGroupForm that mimics the functionality provided by IDatasetForm this is provided in the same way with a lot of the same layout. Currently works with the DefaultGroupForm which is the fallback if nothing else is defined. --- ckan/config/routing.py | 2 + ckan/controllers/group.py | 128 +++++++++++++++++++++++++++++++++---- ckan/plugins/interfaces.py | 93 +++++++++++++++++++++++++++ 3 files changed, 212 insertions(+), 11 deletions(-) diff --git a/ckan/config/routing.py b/ckan/config/routing.py index 3835c6fe813..7bd7a0ea17d 100644 --- a/ckan/config/routing.py +++ b/ckan/config/routing.py @@ -8,6 +8,7 @@ from pylons import config from routes import Mapper from ckan.controllers.package import register_pluggable_behaviour as register_pluggable_package_behaviour +from ckan.controllers.group import register_pluggable_behaviour as register_pluggable_group_behaviour from ckan.plugins import PluginImplementations, IRoutes routing_plugins = PluginImplementations(IRoutes) @@ -179,6 +180,7 @@ def make_map(): ########### register_pluggable_package_behaviour() + register_pluggable_group_behaviour() map.redirect("/packages", "/dataset") map.redirect("/packages/{url:.*}", "/dataset/{url}") diff --git a/ckan/controllers/group.py b/ckan/controllers/group.py index ac292c8dcaa..897742c14e1 100644 --- a/ckan/controllers/group.py +++ b/ckan/controllers/group.py @@ -8,7 +8,7 @@ import ckan.authz as authz from ckan.authz import Authorizer from ckan.lib.helpers import Page -from ckan.plugins import PluginImplementations, IGroupController +from ckan.plugins import PluginImplementations, IGroupController, IGroupForm from ckan.lib.navl.dictization_functions import DataError, unflatten, validate from ckan.logic import NotFound, NotAuthorized, ValidationError from ckan.logic import check_access, get_action @@ -16,19 +16,104 @@ from ckan.logic import tuplize_dict, clean_dict, parse_params import ckan.forms -class GroupController(BaseController): - ## hooks for subclasses - group_form = 'group/new_group_form.html' +# Mapping from group-type strings to IDatasetForm instances +_controller_behaviour_for = dict() + +# The fallback behaviour +_default_controller_behaviour = None + +def register_pluggable_behaviour(): + """ + Register the various IGroupForm instances. + + This method will setup the mappings between package types and the registered + IGroupForm instances. If it's called more than once an + exception will be raised. + """ + global _default_controller_behaviour + + # Create the mappings and register the fallback behaviour if one is found. + for plugin in PluginImplementations(IGroupForm): + if plugin.is_fallback(): + if _default_controller_behaviour is not None: + raise ValueError, "More than one fallback "\ + "IGroupForm has been registered" + _default_controller_behaviour = plugin + + for group_type in plugin.group_types(): + if group_type in _controller_behaviour_for: + raise ValueError, "An existing IGroupForm is "\ + "already associated with the package type "\ + "'%s'" % group_type + _controller_behaviour_for[group_type] = plugin + + # Setup the fallback behaviour if one hasn't been defined. + if _default_controller_behaviour is None: + _default_controller_behaviour = DefaultGroupForm() + +def _lookup_plugin(group_type): + """ + Returns the plugin controller associoated with the given group type. - def _form_to_db_schema(self): + If the group type is None or cannot be found in the mapping, then the + fallback behaviour is used. + """ + if group_type is None: + return _default_controller_behaviour + return _controller_behaviour_for.get(group_type, + _default_controller_behaviour) + +class DefaultGroupForm(object): + """ + Provides a default implementation of the pluggable Group controller behaviour. + + This class has 2 purposes: + + - it provides a base class for IGroupForm implementations + to use if only a subset of the method hooks need to be customised. + + - it provides the fallback behaviour if no plugin is setup to provide + the fallback behaviour. + + Note - this isn't a plugin implementation. This is deliberate, as + we don't want this being registered. + """ + + def group_form(self): + return 'group/new_group_form.html' + + def form_to_db_schema(self): return group_form_schema() - def _db_to_form_schema(self): + def db_to_form_schema(self): '''This is an interface to manipulate data from the database into a format suitable for the form (optional)''' - def _setup_template_variables(self, context): + + def check_data_dict(self, data_dict): + '''Check if the return data is correct, mostly for checking out if + spammers are submitting only part of the form + + # Resources might not exist yet (eg. Add Dataset) + surplus_keys_schema = ['__extras', '__junk', 'state', 'groups', + 'extras_validation', 'save', 'return_to', + 'resources'] + + schema_keys = package_form_schema().keys() + keys_in_schema = set(schema_keys) - set(surplus_keys_schema) + + missing_keys = keys_in_schema - set(data_dict.keys()) + + if missing_keys: + #print data_dict + #print missing_keys + log.info('incorrect form fields posted') + raise DataError(data_dict) + ''' + pass + + def setup_template_variables(self, context, data_dict): c.is_sysadmin = Authorizer().is_sysadmin(c.user) ## This is messy as auths take domain object not data_dict @@ -43,6 +128,27 @@ def _setup_template_variables(self, context): except NotAuthorized: c.auth_for_change_state = False +############## End of pluggable group behaviour ############## + + +class GroupController(BaseController): + + ## hooks for subclasses + + def _group_form(self, group_type=None): + return _lookup_plugin(group_type).group_form() + + def _form_to_db_schema(self, group_type=None): + return _lookup_plugin(group_type).form_to_db_schema() + + def _db_to_form_schema(self, group_type=None): + '''This is an interface to manipulate data from the database + into a format suitable for the form (optional)''' + return _lookup_plugin(group_type).form_to_db_schema() + + def _setup_template_variables(self, context, data_dict, group_type=None): + return _lookup_plugin(group_type).setup_template_variables(context,data_dict) + ## end hooks def index(self): @@ -122,8 +228,8 @@ def new(self, data=None, errors=None, error_summary=None): error_summary = error_summary or {} vars = {'data': data, 'errors': errors, 'error_summary': error_summary} - self._setup_template_variables(context) - c.form = render(self.group_form, extra_vars=vars) + self._setup_template_variables(context,data) + c.form = render(self._group_form(), extra_vars=vars) return render('group/new.html') def edit(self, id, data=None, errors=None, error_summary=None): @@ -163,8 +269,8 @@ def edit(self, id, data=None, errors=None, error_summary=None): errors = errors or {} vars = {'data': data, 'errors': errors, 'error_summary': error_summary} - self._setup_template_variables(context) - c.form = render(self.group_form, extra_vars=vars) + self._setup_template_variables(context, data) + c.form = render(self._group_form(), extra_vars=vars) return render('group/edit.html') def _save_new(self, context): diff --git a/ckan/plugins/interfaces.py b/ckan/plugins/interfaces.py index dd24bc3d81f..98d13faecfd 100644 --- a/ckan/plugins/interfaces.py +++ b/ckan/plugins/interfaces.py @@ -13,6 +13,7 @@ 'IPackageController', 'IPluginObserver', 'IConfigurable', 'IConfigurer', 'IAuthorizer', 'IActions', 'IResourceUrlChange', 'IDatasetForm', + 'IGroupForm', ] from inspect import isclass @@ -419,3 +420,95 @@ def setup_template_variables(self, context, data_dict): ##### End of hooks ##### + +class IGroupForm(Interface): + """ + Allows customisation of the group controller as a plugin. + + The behaviour of the plugin is determined by 5 method hooks: + + - package_form(self) + - form_to_db_schema(self) + - db_to_form_schema(self) + - check_data_dict(self, data_dict) + - setup_template_variables(self, context, data_dict) + + Furthermore, there can be many implementations of this plugin registered + at once. With each instance associating itself with 0 or more package + type strings. When a package controller action is invoked, the package + type determines which of the registered plugins to delegate to. Each + implementation must implement two methods which are used to determine the + package-type -> plugin mapping: + + - is_fallback(self) + - package_types(self) + + Implementations might want to consider mixing in + ckan.controllers.package.DefaultPluggablePackageController which provides + default behaviours for the 5 method hooks. + + """ + + ##### These methods control when the plugin is delegated to ##### + + def is_fallback(self): + """ + Returns true iff this provides the fallback behaviour, when no other + plugin instance matches a package's type. + + There must be exactly one fallback controller defined, any attempt to + register more than one will throw an exception at startup. If there's + no fallback registered at startup the + ckan.controllers.group.DefaultPluggableGroupController is used + as the fallback. + """ + + def group_types(self): + """ + Returns an iterable of group type strings. + + If a request involving a package of one of those types is made, then + this plugin instance will be delegated to. + + There must only be one plugin registered to each group type. Any + attempts to register more than one plugin instance to a given group + type will raise an exception at startup. + """ + + ##### End of control methods + + ##### Hooks for customising the PackageController's behaviour ##### + ##### TODO: flesh out the docstrings a little more. ##### + + def package_form(self): + """ + Returns a string representing the location of the template to be + rendered. e.g. "group/new_group_form.html". + """ + + def form_to_db_schema(self): + """ + Returns the schema for mapping group data from a form to a format + suitable for the database. + """ + + def db_to_form_schema(self): + """ + Returns the schema for mapping group data from the database into a + format suitable for the form (optional) + """ + + def check_data_dict(self, data_dict): + """ + Check if the return data is correct. + + raise a DataError if not. + """ + + def setup_template_variables(self, context, data_dict): + """ + Add variables to c just prior to the template being rendered. + """ + + ##### End of hooks ##### + From 978def7e5056421b8254e6c37b3a356019c23098 Mon Sep 17 00:00:00 2001 From: Ross Jones Date: Fri, 9 Dec 2011 15:39:37 +0000 Subject: [PATCH 19/55] Making sure that all of the internal method calls are passed the group type so that it can determine whether to use a plugin or not for that method call --- ckan/controllers/group.py | 35 +++++++++++++++++++++++++++++++---- 1 file changed, 31 insertions(+), 4 deletions(-) diff --git a/ckan/controllers/group.py b/ckan/controllers/group.py index 897742c14e1..bc6c6aa3ac3 100644 --- a/ckan/controllers/group.py +++ b/ckan/controllers/group.py @@ -80,7 +80,7 @@ class DefaultGroupForm(object): we don't want this being registered. """ - def group_form(self): + def group_form(self): return 'group/new_group_form.html' def form_to_db_schema(self): @@ -174,9 +174,10 @@ def index(self): def read(self, id): + group_type = self._get_group_type(id.split('@')[0]) context = {'model': model, 'session': model.Session, 'user': c.user or c.author, - 'schema': self._form_to_db_schema()} + 'schema': self._form_to_db_schema(group_type=type)} data_dict = {'id': id} try: c.group_dict = get_action('group_show')(context, data_dict) @@ -233,10 +234,11 @@ def new(self, data=None, errors=None, error_summary=None): return render('group/new.html') def edit(self, id, data=None, errors=None, error_summary=None): + group_type = self._get_group_type(id.split('@')[0]) context = {'model': model, 'session': model.Session, 'user': c.user or c.author, 'extras_as_string': True, 'save': 'save' in request.params, - 'schema': self._form_to_db_schema(), + 'schema': self._form_to_db_schema(group_type=group_type), } data_dict = {'id': id} @@ -269,10 +271,35 @@ def edit(self, id, data=None, errors=None, error_summary=None): errors = errors or {} vars = {'data': data, 'errors': errors, 'error_summary': error_summary} - self._setup_template_variables(context, data) + self._setup_template_variables(context, data, group_type=group_type) c.form = render(self._group_form(), extra_vars=vars) return render('group/edit.html') + def _get_group_type(self, id): + """ + Given the id of a group it determines the plugin to load + based on the group's type name (type). The plugin found + will be returned, or None if there is no plugin associated with + the type. + + Uses a minimal context to do so. The main use of this method + is for figuring out which plugin to delegate to. + + aborts if an exception is raised. + """ + global _controller_behaviour_for + + context = {'model': model, 'session': model.Session, + 'user': c.user or c.author} + try: + data = get_action('group_show')(context, {'id': id}) + except NotFound: + abort(404, _('Group not found')) + except NotAuthorized: + abort(401, _('Unauthorized to read group %s') % id) + return data['type'] + + def _save_new(self, context): try: data_dict = clean_dict(unflatten( From 98cc6e5d77c4b9a674bce6a04e1d9f3c28137d9e Mon Sep 17 00:00:00 2001 From: Ross Jones Date: Mon, 12 Dec 2011 15:49:33 +0000 Subject: [PATCH 20/55] Fixed test_group() where read() was being called more than once --- ckan/controllers/group.py | 2 +- ckan/tests/functional/test_group.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/ckan/controllers/group.py b/ckan/controllers/group.py index bc6c6aa3ac3..ee9a8aa7982 100644 --- a/ckan/controllers/group.py +++ b/ckan/controllers/group.py @@ -272,7 +272,7 @@ def edit(self, id, data=None, errors=None, error_summary=None): vars = {'data': data, 'errors': errors, 'error_summary': error_summary} self._setup_template_variables(context, data, group_type=group_type) - c.form = render(self._group_form(), extra_vars=vars) + c.form = render(self._group_form(group_type), extra_vars=vars) return render('group/edit.html') def _get_group_type(self, id): diff --git a/ckan/tests/functional/test_group.py b/ckan/tests/functional/test_group.py index 0ed57c42924..c4c6aa6228f 100644 --- a/ckan/tests/functional/test_group.py +++ b/ckan/tests/functional/test_group.py @@ -102,7 +102,7 @@ def test_read_plugin_hook(self): name = u'david' offset = url_for(controller='group', action='read', id=name) res = self.app.get(offset, status=200, extra_environ={'REMOTE_USER': 'russianfan'}) - assert plugin.calls['read'] == 1, plugin.calls + assert plugin.calls['read'] == 2, plugin.calls plugins.unload(plugin) def test_read_and_authorized_to_edit(self): From a267347d4fdd6a0d1cc0a69917e860a5c5c15a91 Mon Sep 17 00:00:00 2001 From: Ross Jones Date: Mon, 12 Dec 2011 16:48:05 +0000 Subject: [PATCH 21/55] Fixed the test_mapper_plugin_fired test so that it runs with the version of pyutillib that is specified in the requirement files --- ckan/tests/test_plugins.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ckan/tests/test_plugins.py b/ckan/tests/test_plugins.py index d2d002ff664..ad32731e578 100644 --- a/ckan/tests/test_plugins.py +++ b/ckan/tests/test_plugins.py @@ -168,7 +168,7 @@ def test_mapper_plugin_fired(self): config['ckan.plugins'] = 'mapper_plugin' plugins.load_all(config) CreateTestData.create_arbitrary([{'name':u'testpkg'}]) - mapper_plugin = PluginGlobals.plugin_registry['MapperPlugin'].__instance__ + mapper_plugin = PluginGlobals.env().plugin_registry['MapperPlugin'].__instance__ assert len(mapper_plugin.added) == 2 # resource group table added automatically assert mapper_plugin.added[0].name == 'testpkg' @@ -176,7 +176,7 @@ def test_routes_plugin_fired(self): local_config = appconfig('config:%s' % config['__file__'], relative_to=conf_dir) local_config.local_conf['ckan.plugins'] = 'routes_plugin' app = make_app(local_config.global_conf, **local_config.local_conf) - routes_plugin = PluginGlobals.plugin_registry['RoutesPlugin'].__instance__ + routes_plugin = PluginGlobals.env().plugin_registry['RoutesPlugin'].__instance__ assert routes_plugin.calls_made == ['before_map', 'after_map'], \ routes_plugin.calls_made From 325bd8d20289e1b0190cd2cfdc79ad4d48508188 Mon Sep 17 00:00:00 2001 From: Ross Jones Date: Wed, 21 Dec 2011 16:53:34 +0000 Subject: [PATCH 22/55] Implemented subnav_named_route to allow for the generation of subnavigation links based on a named route, instead of a controller. This has been implemented to allow for the addition of new routes when loading a IDatasetForm or IGroupForm to ensure that the *_types() registered for the plugin are used as part of the URL. --- ckan/lib/helpers.py | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/ckan/lib/helpers.py b/ckan/lib/helpers.py index 2c3ccebf7aa..ce7b952b875 100644 --- a/ckan/lib/helpers.py +++ b/ckan/lib/helpers.py @@ -143,6 +143,14 @@ def subnav_link(c, text, action, **kwargs): url_for(action=action, **kwargs), class_=('active' if c.action == action else '') ) + +def subnav_named_route(c, text, routename,**kwargs): + """ Generate a subnav element based on a named route """ + return link_to( + text, + url_for(routename, **kwargs), + class_=('active' if c.action == kwargs['action'] else '') + ) def facet_items(c, name, limit=10): from pylons import request From d4f4bd770e2784e5d8168f0e29d4b982a6bed5c0 Mon Sep 17 00:00:00 2001 From: Ross Jones Date: Wed, 21 Dec 2011 17:25:53 +0000 Subject: [PATCH 23/55] We can now create custom groups using the registered group_type from the plugin and /type/new. Also implemented named routes for group controller. When registered the custom group form registers new routes for the types defined in group_types() so that they can be accessed via a top-level url. This uses named routes for the groups and the custom group types, although no checking is done yet to make sure it doesn't overwrite a top level object. --- ckan/config/routing.py | 23 +++++++++++---------- ckan/controllers/group.py | 34 +++++++++++++++++++++++++------- ckan/controllers/package.py | 17 ++++++++++++---- ckan/logic/schema.py | 2 ++ ckan/templates/group/layout.html | 11 ++++++----- 5 files changed, 61 insertions(+), 26 deletions(-) diff --git a/ckan/config/routing.py b/ckan/config/routing.py index 7bd7a0ea17d..c44a86cb9c7 100644 --- a/ckan/config/routing.py +++ b/ckan/config/routing.py @@ -7,9 +7,9 @@ """ from pylons import config from routes import Mapper -from ckan.controllers.package import register_pluggable_behaviour as register_pluggable_package_behaviour -from ckan.controllers.group import register_pluggable_behaviour as register_pluggable_group_behaviour from ckan.plugins import PluginImplementations, IRoutes +from ckan.controllers.package import register_pluggable_behaviour as register_package_behaviour +from ckan.controllers.group import register_pluggable_behaviour as register_group_behaviour routing_plugins = PluginImplementations(IRoutes) @@ -178,9 +178,6 @@ def make_map(): ########### ## /END API ########### - - register_pluggable_package_behaviour() - register_pluggable_group_behaviour() map.redirect("/packages", "/dataset") map.redirect("/packages/{url:.*}", "/dataset/{url}") @@ -232,17 +229,23 @@ def make_map(): ##map.connect('/group/new', controller='group_formalchemy', action='new') ##map.connect('/group/edit/{id}', controller='group_formalchemy', action='edit') - map.connect('/group', controller='group', action='index') - map.connect('/group/list', controller='group', action='list') - map.connect('/group/new', controller='group', action='new') - map.connect('/group/{action}/{id}', controller='group', + # These named routes are used for custom group forms which will use the + # names below based on the group.type (dataset_group is the default type) + map.connect('dataset_group_index', '/group', controller='group', action='index') + map.connect('dataset_group_list', '/group/list', controller='group', action='list') + map.connect('dataset_group_new', '/group/new', controller='group', action='new') + map.connect('dataset_group_action', '/group/{action}/{id}', controller='group', requirements=dict(action='|'.join([ 'edit', 'authz', 'history' ])) ) - map.connect('/group/{id}', controller='group', action='read') + map.connect('dataset_group_read', '/group/{id}', controller='group', action='read') + + register_package_behaviour(map) + register_group_behaviour(map) + # authz group map.redirect("/authorizationgroups", "/authorizationgroup") map.redirect("/authorizationgroups/{url:.*}", "/authorizationgroup/{url}") diff --git a/ckan/controllers/group.py b/ckan/controllers/group.py index ee9a8aa7982..56400c01af1 100644 --- a/ckan/controllers/group.py +++ b/ckan/controllers/group.py @@ -23,7 +23,7 @@ # The fallback behaviour _default_controller_behaviour = None -def register_pluggable_behaviour(): +def register_pluggable_behaviour(map): """ Register the various IGroupForm instances. @@ -42,6 +42,14 @@ def register_pluggable_behaviour(): _default_controller_behaviour = plugin for group_type in plugin.group_types(): + # Create the routes based on group_type here..... + map.connect('/%s/new' % (group_type,), controller='group', action='new') + map.connect('%s_read' % (group_type,), '/%s/{id}' % (group_type,), controller='group', action='read') + map.connect('%s_action' % (group_type,), + '/%s/{action}/{id}' % (group_type,), controller='group', + requirements=dict(action='|'.join(['edit', 'authz', 'history' ])) + ) + if group_type in _controller_behaviour_for: raise ValueError, "An existing IGroupForm is "\ "already associated with the package type "\ @@ -52,6 +60,7 @@ def register_pluggable_behaviour(): if _default_controller_behaviour is None: _default_controller_behaviour = DefaultGroupForm() + def _lookup_plugin(group_type): """ Returns the plugin controller associoated with the given group type. @@ -64,6 +73,7 @@ def _lookup_plugin(group_type): return _controller_behaviour_for.get(group_type, _default_controller_behaviour) + class DefaultGroupForm(object): """ Provides a default implementation of the pluggable Group controller behaviour. @@ -79,7 +89,7 @@ class DefaultGroupForm(object): Note - this isn't a plugin implementation. This is deliberate, as we don't want this being registered. """ - + def group_form(self): return 'group/new_group_form.html' @@ -212,17 +222,24 @@ def read(self, id): return render('group/read.html') def new(self, data=None, errors=None, error_summary=None): + + group_type = request.path.strip('/').split('/')[0] + if group_type == 'group': + group_type = None + if data: + data['type'] = group_type + context = {'model': model, 'session': model.Session, 'user': c.user or c.author, 'extras_as_string': True, 'schema': self._form_to_db_schema(), - 'save': 'save' in request.params} + 'save': 'save' in request.params } try: check_access('group_create',context) except NotAuthorized: abort(401, _('Unauthorized to create a group')) if context['save'] and not data: - return self._save_new(context) + return self._save_new(context, group_type) data = data or {} errors = errors or {} @@ -230,7 +247,7 @@ def new(self, data=None, errors=None, error_summary=None): vars = {'data': data, 'errors': errors, 'error_summary': error_summary} self._setup_template_variables(context,data) - c.form = render(self._group_form(), extra_vars=vars) + c.form = render(self._group_form(group_type=group_type), extra_vars=vars) return render('group/new.html') def edit(self, id, data=None, errors=None, error_summary=None): @@ -300,13 +317,16 @@ def _get_group_type(self, id): return data['type'] - def _save_new(self, context): + def _save_new(self, context, group_type=None): try: data_dict = clean_dict(unflatten( tuplize_dict(parse_params(request.params)))) + data_dict['type'] = group_type or 'dataset_group' context['message'] = data_dict.get('log_message', '') group = get_action('group_create')(context, data_dict) - h.redirect_to(controller='group', action='read', id=group['name']) + + # Redirect to the appropriate _read route for the type of group + h.redirect_to( group['type'] + '_read', id=group['name']) except NotAuthorized: abort(401, _('Unauthorized to read group %s') % '') except NotFound, e: diff --git a/ckan/controllers/package.py b/ckan/controllers/package.py index ea772f953a2..a71df1eb4ab 100644 --- a/ckan/controllers/package.py +++ b/ckan/controllers/package.py @@ -57,7 +57,7 @@ def search_url(params): # The fallback behaviour _default_controller_behaviour = None -def register_pluggable_behaviour(): +def register_pluggable_behaviour(map): """ Register the various IDatasetForm instances. @@ -83,6 +83,9 @@ def register_pluggable_behaviour(): _default_controller_behaviour = plugin for package_type in plugin.package_types(): + # Create a connection between the newly named type and the package controller + map.connect('/%s/new' % (package_type,), controller='package', action='new') + if package_type in _controller_behaviour_for: raise ValueError, "An existing IDatasetForm is "\ "already associated with the package type "\ @@ -416,10 +419,15 @@ def history(self, id): return render('package/history.html') def new(self, data=None, errors=None, error_summary=None): + + package_type = request.path.strip('/').split('/')[0] + if package_type == 'group': + package_type = None + context = {'model': model, 'session': model.Session, 'user': c.user or c.author, 'extras_as_string': True, 'save': 'save' in request.params, - 'schema': self._form_to_db_schema()} + 'schema': self._form_to_db_schema(package_type=package_type)} try: check_access('package_create',context) @@ -435,7 +443,7 @@ def new(self, data=None, errors=None, error_summary=None): vars = {'data': data, 'errors': errors, 'error_summary': error_summary} self._setup_template_variables(context, {'id': id}) - c.form = render(self._package_form(), extra_vars=vars) + c.form = render(self._package_form(package_type=package_type), extra_vars=vars) return render('package/new.html') @@ -562,10 +570,11 @@ def _get_package_type(self, id): return data['type'] - def _save_new(self, context): + def _save_new(self, context, package_type=None): try: data_dict = clean_dict(unflatten( tuplize_dict(parse_params(request.POST)))) + data_dict['type'] = package_type self._check_data_dict(data_dict) context['message'] = data_dict.get('log_message', '') pkg = get_action('package_create')(context, data_dict) diff --git a/ckan/logic/schema.py b/ckan/logic/schema.py index 44693429694..a546d4ad35a 100644 --- a/ckan/logic/schema.py +++ b/ckan/logic/schema.py @@ -141,6 +141,7 @@ def package_form_schema(): schema['extras_validation'] = [duplicate_extras_key, ignore] schema['save'] = [ignore] schema['return_to'] = [ignore] + schema['type'] = [ignore, unicode] ##changes schema.pop("id") @@ -158,6 +159,7 @@ def default_group_schema(): 'name': [not_empty, unicode, name_validator, group_name_validator], 'title': [ignore_missing, unicode], 'description': [ignore_missing, unicode], + 'type': [ignore_missing, unicode], 'state': [ignore], 'created': [ignore], 'extras': default_extras_schema(), diff --git a/ckan/templates/group/layout.html b/ckan/templates/group/layout.html index a5204e7af2c..e4633f8ea3f 100644 --- a/ckan/templates/group/layout.html +++ b/ckan/templates/group/layout.html @@ -8,13 +8,14 @@
    -
  • ${h.subnav_link(c, h.icon('group') + _('View'), controller='group', action='read', id=c.group.name)}
    • -
  • - ${h.subnav_link(c, h.icon('group_edit') + _('Edit'), controller='group', action='edit', id=c.group.name)} +
  • ${h.subnav_named_route(c, h.icon('group') + _('View'), c.group.type + '_read',controller='group', action='read', id=c.group.name)}
    • +
  • + + ${h.subnav_named_route( c,h.icon('group_edit') + _('Edit'), c.group.type + '_action', action='edit', id=c.group.name )}
    • -
  • ${h.subnav_link(c, h.icon('page_white_stack') + _('History'), controller='group', action='history', id=c.group.name)}
    • +
  • ${h.subnav_named_route(c, h.icon('page_white_stack') + _('History'), c.group.type + '_action', controller='group', action='history', id=c.group.name)}
  • - ${h.subnav_link(c, h.icon('lock') + _('Authorization'), controller='group', action='authz', id=c.group.name)} + ${h.subnav_named_route(c, h.icon('lock') + _('Authorization'), c.group.type + '_action', controller='group', action='authz', id=c.group.name)}
    • +
    +
    +
    + +
    +
    + ${h.file('file', size=50)} +
    +
    + +
    + +
    +
    + + + + + + diff --git a/ckan/templates/storage/success.html b/ckan/templates/storage/success.html new file mode 100644 index 00000000000..c3c387ddce0 --- /dev/null +++ b/ckan/templates/storage/success.html @@ -0,0 +1,22 @@ + + + Upload + + + + +
    +

    Upload - Successful

    + +

    Filed uploaded to:

    +

    ${c.file_url}

    + +

    Upload another »

    +
    + + + + diff --git a/ckan/tests/models/test_repo.py b/ckan/tests/models/test_repo.py index 12b7a8a31a8..d08b2b83e28 100644 --- a/ckan/tests/models/test_repo.py +++ b/ckan/tests/models/test_repo.py @@ -14,12 +14,14 @@ '', '', '', + '', '', '', '', '', '', '', + '', '', '', '', diff --git a/ckan/tests/test_plugins.py b/ckan/tests/test_plugins.py index d2d002ff664..ad32731e578 100644 --- a/ckan/tests/test_plugins.py +++ b/ckan/tests/test_plugins.py @@ -168,7 +168,7 @@ def test_mapper_plugin_fired(self): config['ckan.plugins'] = 'mapper_plugin' plugins.load_all(config) CreateTestData.create_arbitrary([{'name':u'testpkg'}]) - mapper_plugin = PluginGlobals.plugin_registry['MapperPlugin'].__instance__ + mapper_plugin = PluginGlobals.env().plugin_registry['MapperPlugin'].__instance__ assert len(mapper_plugin.added) == 2 # resource group table added automatically assert mapper_plugin.added[0].name == 'testpkg' @@ -176,7 +176,7 @@ def test_routes_plugin_fired(self): local_config = appconfig('config:%s' % config['__file__'], relative_to=conf_dir) local_config.local_conf['ckan.plugins'] = 'routes_plugin' app = make_app(local_config.global_conf, **local_config.local_conf) - routes_plugin = PluginGlobals.plugin_registry['RoutesPlugin'].__instance__ + routes_plugin = PluginGlobals.env().plugin_registry['RoutesPlugin'].__instance__ assert routes_plugin.calls_made == ['before_map', 'after_map'], \ routes_plugin.calls_made diff --git a/requires/lucid_missing.txt b/requires/lucid_missing.txt index 284c18fc29d..89e2eb8cac7 100644 --- a/requires/lucid_missing.txt +++ b/requires/lucid_missing.txt @@ -14,9 +14,10 @@ # Packages already on pypi.python.org solrpy==0.9.4 formalchemy==1.4.1 +pairtree==0.7.1-T +ofs>=0.4.1 apachemiddleware==0.1.1 licenses==0.6.1 # markupsafe is required by webhelpers==1.2 required by formalchemy with SQLAlchemy 0.6 markupsafe==0.9.2 - From e09a7375f7a13af06d9d35863db67863aa0038db Mon Sep 17 00:00:00 2001 From: Ross Jones Date: Wed, 4 Jan 2012 13:53:13 +0000 Subject: [PATCH 30/55] [huge, 1608] This is the merging of ckanext-storage into core along with some simplifying of the code. * Added StorageController and StorageApiController to handle the upload and callbacks for JS based uploads. * Added the templates from ckanext-storage with minor changes * Added a new RoleAction (file-upload) for permission to upload, need to clarify whether this is required or whether we will just restrict upload to logged in users. * Added deps for ofs and pairtree to lucid_missing.txt, although devs will have these if they have used ckanext-storage before We have an outstanding issue on this branch of failing to find index_id in test_index() when testing against postgres (skipped for sqlite) and this might just be a schema problem - will check. --- ckan/authz.py | 4 + ckan/config/routing.py | 34 +++ ckan/controllers/storage.py | 174 ++++++++++- ckan/controllers/storage_api.py | 286 +++++++++++++++++++ ckan/model/authz.py | 5 +- ckan/templates/package/new_package_form.html | 2 +- ckan/templates/storage/index.html | 58 ++++ ckan/templates/storage/success.html | 22 ++ ckan/tests/models/test_repo.py | 2 + ckan/tests/test_plugins.py | 4 +- requires/lucid_missing.txt | 3 +- 11 files changed, 588 insertions(+), 6 deletions(-) create mode 100644 ckan/controllers/storage_api.py create mode 100644 ckan/templates/storage/index.html create mode 100644 ckan/templates/storage/success.html diff --git a/ckan/authz.py b/ckan/authz.py index 65f0a335787..68324a3dcf5 100644 --- a/ckan/authz.py +++ b/ckan/authz.py @@ -47,6 +47,10 @@ def is_authorized(cls, username, action, domain_object): if isinstance(username, str): username = username.decode('utf8') assert isinstance(username, unicode), type(username) + + if len(username) == 0: + return False + for extension in cls.extensions: authorized = extension.is_authorized(username, action, diff --git a/ckan/config/routing.py b/ckan/config/routing.py index f946f2d29b9..9c8a09a1298 100644 --- a/ckan/config/routing.py +++ b/ckan/config/routing.py @@ -278,6 +278,40 @@ def make_map(): map.connect('ckanadmin_index', '/ckan-admin', controller='admin', action='index') map.connect('ckanadmin', '/ckan-admin/{action}', controller='admin') + # Storage routes + map.connect('storage_api', "/api/storage", + controller='storage_api', action='index') + map.connect('storage_api_set_metadata', '/api/storage/metadata/{label:.*}', + controller='storage_api', action='set_metadata', + conditions={'method': ['PUT','POST']}) + map.connect('storage_api_get_metadata', '/api/storage/metadata/{label:.*}', + controller='storage_api', action='get_metadata', + conditions={'method': ['GET']}) + map.connect('storage_api_auth_request', + '/api/storage/auth/request/{label:.*}', + controller='storage_api', + action='auth_request') + map.connect('storage_api_auth_form', + '/api/storage/auth/form/{label:.*}', + controller='storage_api', + action='auth_form') + map.connect('storage_upload', '/storage/upload', + controller='storage', + action='upload') + map.connect('storage_upload_handle', '/storage/upload_handle', + controller='storage', + action='upload_handle') + map.connect('storage_upload_success', '/storage/upload/success', + controller='storage', + action='success') + map.connect('storage_upload_success_empty', '/storage/upload/success_empty', + controller='storage', + action='success_empty') + map.connect('storage_file', '/storage/f/{label:.*}', + controller='storage', + action='file') + + for plugin in routing_plugins: map = plugin.after_map(map) diff --git a/ckan/controllers/storage.py b/ckan/controllers/storage.py index 71c41d4e08c..b8ca51062a7 100644 --- a/ckan/controllers/storage.py +++ b/ckan/controllers/storage.py @@ -25,4 +25,176 @@ import simplejson as json from logging import getLogger -log = getLogger(__name__) \ No newline at end of file +log = getLogger(__name__) + + +# pairtree_version0_1 file for identifying folders +BUCKET = config['storage.bucket'] +key_prefix = config.get('storage.key_prefix', 'file/') + + +_eq_re = re.compile(r"^(.*)(=[0-9]*)$") +def fix_stupid_pylons_encoding(data): + """ + Fix an apparent encoding problem when calling request.body + TODO: Investigate whether this is fixed in later versions? + """ + if data.startswith("%") or data.startswith("+"): + data = urllib.unquote_plus(data) + m = _eq_re.match(data) + if m: + data = m.groups()[0] + return data + + +def get_ofs(): + """ + Return a configured instance of the appropriate OFS driver, in all + cases here this will be the local file storage so we fix the implementation + to use pairtree. + """ + kw = {} + for k,v in config.items(): + if not k.startswith('ofs.'): + continue + kw[k[4:]] = v + ofs = get_impl("pairtree")(**kw) + return ofs + + +def authorize(method, bucket, key, user, ofs): + """ + Check authz for the user with a given bucket/key combo within a + particular ofs implementation. + """ + if not method in ['POST', 'GET', 'PUT', 'DELETE']: + abort(400) + if method != 'GET': + # do not allow overwriting + if ofs.exists(bucket, key): + abort(409) + # now check user stuff + username = user.name if user else '' + is_authorized = authz.Authorizer.is_authorized(username, 'file-upload', model.System()) + if not is_authorized: + h.flash_error('Not authorized to upload files.') + abort(401) + + + +class StorageController(BaseController): + '''Upload to storage backend. + ''' + @property + def ofs(self): + return get_ofs() + + def _get_form_for_remote(self): + # would be nice to use filename of file + # problem is 'we' don't know this at this point and cannot add it to + # success_action_redirect and hence cannnot display to user afterwards + # + '/${filename}' + label = key_prefix + request.params.get('filepath', str(uuid.uuid4())) + method = 'POST' + authorize(method, BUCKET, label, c.userobj, self.ofs) + content_length_range = int( + config.get('ckanext.storage.max_content_length', + 50000000)) + success_action_redirect = h.url_for('storage_upload_success', qualified=True, + bucket=BUCKET, label=label) + acl = 'public-read' + fields = [ { + 'name': self.ofs.conn.provider.metadata_prefix + 'uploaded-by', + 'value': c.userobj.id + }] + conditions = [ '{"%s": "%s"}' % (x['name'], x['value']) for x in fields ] + c.data = self.ofs.conn.build_post_form_args( + BUCKET, + label, + expires_in=3600, + max_content_length=content_length_range, + success_action_redirect=success_action_redirect, + acl=acl, + fields=fields, + conditions=conditions + ) + for idx,field in enumerate(c.data['fields']): + if field['name'] == 'content-length-range': + del c.data['fields'][idx] + c.data_json = json.dumps(c.data, indent=2) + + def upload(self): + label = key_prefix + request.params.get('filepath', str(uuid.uuid4())) + c.data = { + 'action': h.url_for('storage_upload_handle'), + 'fields': [ + { + 'name': 'key', + 'value': label + } + ] + } + return render('storage/index.html') + + def upload_handle(self): + bucket_id = BUCKET + params = dict(request.params.items()) + stream = params.get('file') + label = params.get('key') + authorize('POST', BUCKET, label, c.userobj, self.ofs) + if not label: + abort(400, "No label") + if not isinstance(stream, FieldStorage): + abort(400, "No file stream.") + del params['file'] + params['filename-original'] = stream.filename + params['_owner'] = c.userobj.id if c.userobj else "" + params['uploaded-by'] = c.userobj.id if c.userobj else "" + + self.ofs.put_stream(bucket_id, label, stream.file, params) + success_action_redirect = h.url_for('storage_upload_success', qualified=True, + bucket=BUCKET, label=label) + # Cannot redirect here as it breaks js file uploads (get infinite loop + # in FF and crash in Chrome) + # h.redirect_to(success_action_redirect) + return self.success(label) + + def success(self, label=None): + label=request.params.get('label', label) + h.flash_success('Upload successful') + c.file_url = h.url_for('storage_file', + label=label, + qualified=True + ) + c.upload_url = h.url_for('storage_upload') + return render('storage/success.html') + + def success_empty(self, label=None): + # very simple method that just returns 200 OK + return '' + + def file(self, label): + exists = self.ofs.exists(BUCKET, label) + if not exists: + # handle erroneous trailing slash by redirecting to url w/o slash + if label.endswith('/'): + label = label[:-1] + file_url = h.url_for( + 'storage_file', + label=label + ) + h.redirect_to(file_url) + else: + abort(404) + file_url = self.ofs.get_url(BUCKET, label) + if file_url.startswith("file://"): + metadata = self.ofs.get_metadata(BUCKET, label) + filepath = file_url[len("file://"):] + headers = { + # 'Content-Disposition':'attachment; filename="%s"' % label, + 'Content-Type':metadata.get('_format', 'text/plain')} + fapp = FileApp(filepath, headers=None, **headers) + return fapp(request.environ, self.start_response) + else: + h.redirect_to(file_url) + diff --git a/ckan/controllers/storage_api.py b/ckan/controllers/storage_api.py new file mode 100644 index 00000000000..594131e5b1c --- /dev/null +++ b/ckan/controllers/storage_api.py @@ -0,0 +1,286 @@ +import re +import urllib +import uuid +from datetime import datetime +from cgi import FieldStorage + +from ofs import get_impl +from pylons import request, response +from pylons.controllers.util import abort, redirect_to +from pylons import config +from paste.fileapp import FileApp + +from ckan.lib.base import BaseController, c, request, render, config, h, abort +from ckan.lib.jsonp import jsonpify +import ckan.model as model +import ckan.authz as authz + +try: + from cStringIO import StringIO +except ImportError: + from StringIO import StringIO +try: + import json +except: + import simplejson as json + +from logging import getLogger +log = getLogger(__name__) + + +BUCKET = config['storage.bucket'] +key_prefix = config.get('storage.key_prefix', 'file/') + + +_eq_re = re.compile(r"^(.*)(=[0-9]*)$") +def fix_stupid_pylons_encoding(data): + """ + Fix an apparent encoding problem when calling request.body + TODO: Investigate whether this is fixed in later versions? + """ + if data.startswith("%") or data.startswith("+"): + data = urllib.unquote_plus(data) + m = _eq_re.match(data) + if m: + data = m.groups()[0] + return data + + +def get_ofs(): + """ + Return a configured instance of the appropriate OFS driver, in all + cases here this will be the local file storage so we fix the implementation + to use pairtree. + """ + kw = {} + for k,v in config.items(): + if not k.startswith('ofs.'): + continue + kw[k[4:]] = v + ofs = get_impl("pairtree")(**kw) + return ofs + + +def authorize(method, bucket, key, user, ofs): + """ + Check authz for the user with a given bucket/key combo within a + particular ofs implementation. + """ + if not method in ['POST', 'GET', 'PUT', 'DELETE']: + abort(400) + if method != 'GET': + # do not allow overwriting + if ofs.exists(bucket, key): + abort(409) + # now check user stuff + username = user.name if user else '' + is_authorized = authz.Authorizer.is_authorized(username, 'file-upload', model.System()) + if not is_authorized: + h.flash_error('Not authorized to upload files.') + abort(401) + + + + +class StorageApiController(BaseController): + @property + def ofs(self): + return get_ofs() + + @jsonpify + def index(self): + info = { + 'metadata/{label}': { + 'description': 'Get or set metadata for this item in storage', + }, + 'auth/request/{label}': { + 'description': self.auth_request.__doc__, + }, + 'auth/form/{label}': { + 'description': self.auth_form.__doc__, + } + } + return info + + def set_metadata(self, label): + bucket = BUCKET + if not label.startswith("/"): label = "/" + label + + try: + data = fix_stupid_pylons_encoding(request.body) + if data: + metadata = loads(data) + else: + metadata = {} + except: + abort(400) + + try: + b = self.ofs._require_bucket(bucket) + except: + abort(409) + + k = self.ofs._get_key(b, label) + if k is None: + k = b.new_key(label) + metadata = metadata.copy() + metadata["_creation_time"] = str(datetime.utcnow()) + self.ofs._update_key_metadata(k, metadata) + k.set_contents_from_file(StringIO('')) + elif request.method == "PUT": + old = self.ofs.get_metadata(bucket, label) + to_delete = [] + for ok in old.keys(): + if ok not in metadata: + to_delete.append(ok) + if to_delete: + self.ofs.del_metadata_keys(bucket, label, to_delete) + self.ofs.update_metadata(bucket, label, metadata) + else: + self.ofs.update_metadata(bucket, label, metadata) + + k.make_public() + k.close() + + return self.get_metadata(bucket, label) + + @jsonpify + def get_metadata(self, label): + bucket = BUCKET + url = h.url_for('storage_file', + label=label, + qualified=True + ) + if not self.ofs.exists(bucket, label): + abort(404) + metadata = self.ofs.get_metadata(bucket, label) + metadata["_location"] = url + return metadata + + @jsonpify + def auth_request(self, label): + '''Provide authentication information for a request so a client can + interact with backend storage directly. + + :param label: label. + :param kwargs: sent either via query string for GET or json-encoded + dict for POST). Interpreted as http headers for request plus an + (optional) method parameter (being the HTTP method). + + Examples of headers are: + + Content-Type + Content-Encoding (optional) + Content-Length + Content-MD5 + Expect (should be '100-Continue') + + :return: is a json hash containing various attributes including a + headers dictionary containing an Authorization field which is good for + 15m. + + ''' + bucket = BUCKET + if request.POST: + try: + data = fix_stupid_pylons_encoding(request.body) + headers = loads(data) + except Exception, e: + from traceback import print_exc + msg = StringIO() + print_exc(msg) + log.error(msg.seek(0).read()) + abort(400) + else: + headers = dict(request.params) + if 'method' in headers: + method = headers['method'] + del headers['method'] + else: + method = 'POST' + + authorize(method, bucket, label, c.userobj, self.ofs) + + http_request = self.ofs.authenticate_request(method, bucket, label, + headers) + return { + 'host': http_request.host, + 'method': http_request.method, + 'path': http_request.path, + 'headers': http_request.headers + } + + def _get_remote_form_data(self, label): + method = 'POST' + content_length_range = int( + config.get('ckanext.storage.max_content_length', + 50000000)) + acl = 'public-read' + fields = [ { + 'name': self.ofs.conn.provider.metadata_prefix + 'uploaded-by', + 'value': c.userobj.id + }] + conditions = [ '{"%s": "%s"}' % (x['name'], x['value']) for x in + fields ] + # In FF redirect to this breaks js upload as FF attempts to open file + # (presumably because mimetype = javascript) and this stops js + # success_action_redirect = h.url_for('storage_api_get_metadata', qualified=True, + # label=label) + success_action_redirect = h.url_for('storage_upload_success_empty', qualified=True, + label=label) + data = self.ofs.conn.build_post_form_args( + BUCKET, + label, + expires_in=72000, + max_content_length=content_length_range, + success_action_redirect=success_action_redirect, + acl=acl, + fields=fields, + conditions=conditions + ) + # HACK: fix up some broken stuff from boto + # e.g. should not have content-length-range in list of fields! + for idx,field in enumerate(data['fields']): + if field['name'] == 'content-length-range': + del data['fields'][idx] + return data + + def _get_form_data(self, label): + data = { + 'action': h.url_for('storage_upload_handle', qualified=True), + 'fields': [ + { + 'name': 'key', + 'value': label + } + ] + } + return data + + @jsonpify + def auth_form(self, label): + '''Provide fields for a form upload to storage including + authentication. + + :param label: label. + :return: json-encoded dictionary with action parameter and fields list. + ''' + bucket = BUCKET + if request.POST: + try: + data = fix_stupid_pylons_encoding(request.body) + headers = loads(data) + except Exception, e: + from traceback import print_exc + msg = StringIO() + print_exc(msg) + log.error(msg.seek(0).read()) + abort(400) + else: + headers = dict(request.params) + + method = 'POST' + authorize(method, bucket, label, c.userobj, self.ofs) + data = self._get_form_data(label) + return data + diff --git a/ckan/model/authz.py b/ckan/model/authz.py index fbc7ee4c2ec..716d95e8c1a 100644 --- a/ckan/model/authz.py +++ b/ckan/model/authz.py @@ -47,7 +47,8 @@ class Action(Enum): SITE_READ = u'read-site' USER_READ = u'read-user' USER_CREATE = u'create-user' - + UPLOAD_ACTION = u'file-upload' + class Role(Enum): ADMIN = u'admin' EDITOR = u'editor' @@ -67,12 +68,14 @@ class Role(Enum): (Role.EDITOR, Action.USER_READ), (Role.EDITOR, Action.SITE_READ), (Role.EDITOR, Action.READ), + (Role.EDITOR, Action.UPLOAD_ACTION), (Role.ANON_EDITOR, Action.EDIT), (Role.ANON_EDITOR, Action.PACKAGE_CREATE), (Role.ANON_EDITOR, Action.USER_CREATE), (Role.ANON_EDITOR, Action.USER_READ), (Role.ANON_EDITOR, Action.SITE_READ), (Role.ANON_EDITOR, Action.READ), + (Role.ANON_EDITOR, Action.UPLOAD_ACTION), (Role.READER, Action.USER_CREATE), (Role.READER, Action.USER_READ), (Role.READER, Action.SITE_READ), diff --git a/ckan/templates/package/new_package_form.html b/ckan/templates/package/new_package_form.html index d2319dce199..4b76a8f32d8 100644 --- a/ckan/templates/package/new_package_form.html +++ b/ckan/templates/package/new_package_form.html @@ -100,7 +100,7 @@

    Errors in form

  • Add a resource:

    • - +
diff --git a/ckan/templates/storage/index.html b/ckan/templates/storage/index.html new file mode 100644 index 00000000000..f26f87b8542 --- /dev/null +++ b/ckan/templates/storage/index.html @@ -0,0 +1,58 @@ + + + Upload + + + + + +
+

Upload

+ +

This upload form is valid for a limited time (usually 1h or so). If the + form expires please reload the page.

+ +
+ + + + + +
+
+
+ +
+
+ ${h.file('file', size=50)} +
+
+ +
+ +
+
+
+
+ + + + diff --git a/ckan/templates/storage/success.html b/ckan/templates/storage/success.html new file mode 100644 index 00000000000..c3c387ddce0 --- /dev/null +++ b/ckan/templates/storage/success.html @@ -0,0 +1,22 @@ + + + Upload + + + + +
+

Upload - Successful

+ +

Filed uploaded to:

+

${c.file_url}

+ +

Upload another »

+
+ + + + diff --git a/ckan/tests/models/test_repo.py b/ckan/tests/models/test_repo.py index 12b7a8a31a8..d08b2b83e28 100644 --- a/ckan/tests/models/test_repo.py +++ b/ckan/tests/models/test_repo.py @@ -14,12 +14,14 @@ '', '', '', + '', '', '', '', '', '', '', + '', '', '', '', diff --git a/ckan/tests/test_plugins.py b/ckan/tests/test_plugins.py index d2d002ff664..ad32731e578 100644 --- a/ckan/tests/test_plugins.py +++ b/ckan/tests/test_plugins.py @@ -168,7 +168,7 @@ def test_mapper_plugin_fired(self): config['ckan.plugins'] = 'mapper_plugin' plugins.load_all(config) CreateTestData.create_arbitrary([{'name':u'testpkg'}]) - mapper_plugin = PluginGlobals.plugin_registry['MapperPlugin'].__instance__ + mapper_plugin = PluginGlobals.env().plugin_registry['MapperPlugin'].__instance__ assert len(mapper_plugin.added) == 2 # resource group table added automatically assert mapper_plugin.added[0].name == 'testpkg' @@ -176,7 +176,7 @@ def test_routes_plugin_fired(self): local_config = appconfig('config:%s' % config['__file__'], relative_to=conf_dir) local_config.local_conf['ckan.plugins'] = 'routes_plugin' app = make_app(local_config.global_conf, **local_config.local_conf) - routes_plugin = PluginGlobals.plugin_registry['RoutesPlugin'].__instance__ + routes_plugin = PluginGlobals.env().plugin_registry['RoutesPlugin'].__instance__ assert routes_plugin.calls_made == ['before_map', 'after_map'], \ routes_plugin.calls_made diff --git a/requires/lucid_missing.txt b/requires/lucid_missing.txt index 284c18fc29d..89e2eb8cac7 100644 --- a/requires/lucid_missing.txt +++ b/requires/lucid_missing.txt @@ -14,9 +14,10 @@ # Packages already on pypi.python.org solrpy==0.9.4 formalchemy==1.4.1 +pairtree==0.7.1-T +ofs>=0.4.1 apachemiddleware==0.1.1 licenses==0.6.1 # markupsafe is required by webhelpers==1.2 required by formalchemy with SQLAlchemy 0.6 markupsafe==0.9.2 - From a862c680444890d3d84fb5c20dbd2c25dbe5efb8 Mon Sep 17 00:00:00 2001 From: Ross Jones Date: Wed, 4 Jan 2012 15:33:28 +0000 Subject: [PATCH 31/55] Removed mistaken shortcut for authorizing blank usernames --- ckan/authz.py | 3 --- 1 file changed, 3 deletions(-) diff --git a/ckan/authz.py b/ckan/authz.py index 68324a3dcf5..343a351adc0 100644 --- a/ckan/authz.py +++ b/ckan/authz.py @@ -48,9 +48,6 @@ def is_authorized(cls, username, action, domain_object): username = username.decode('utf8') assert isinstance(username, unicode), type(username) - if len(username) == 0: - return False - for extension in cls.extensions: authorized = extension.is_authorized(username, action, From dc9b8242e1440b0d319f6fb7acc72ffbdf5c7cc5 Mon Sep 17 00:00:00 2001 From: Ross Jones Date: Wed, 4 Jan 2012 16:38:54 +0000 Subject: [PATCH 32/55] [config][s] Made it possible to turn on/off storage and minor changes to the controller identifying the next step --- ckan/config/routing.py | 64 +++++++++++++++++++------------------ ckan/controllers/storage.py | 17 ++++++++-- 2 files changed, 48 insertions(+), 33 deletions(-) diff --git a/ckan/config/routing.py b/ckan/config/routing.py index 9c8a09a1298..76e714d00ae 100644 --- a/ckan/config/routing.py +++ b/ckan/config/routing.py @@ -8,6 +8,7 @@ from pylons import config from routes import Mapper from ckan.plugins import PluginImplementations, IRoutes +from paste.deploy.converters import asbool routing_plugins = PluginImplementations(IRoutes) @@ -279,37 +280,38 @@ def make_map(): map.connect('ckanadmin', '/ckan-admin/{action}', controller='admin') # Storage routes - map.connect('storage_api', "/api/storage", - controller='storage_api', action='index') - map.connect('storage_api_set_metadata', '/api/storage/metadata/{label:.*}', - controller='storage_api', action='set_metadata', - conditions={'method': ['PUT','POST']}) - map.connect('storage_api_get_metadata', '/api/storage/metadata/{label:.*}', - controller='storage_api', action='get_metadata', - conditions={'method': ['GET']}) - map.connect('storage_api_auth_request', - '/api/storage/auth/request/{label:.*}', - controller='storage_api', - action='auth_request') - map.connect('storage_api_auth_form', - '/api/storage/auth/form/{label:.*}', - controller='storage_api', - action='auth_form') - map.connect('storage_upload', '/storage/upload', - controller='storage', - action='upload') - map.connect('storage_upload_handle', '/storage/upload_handle', - controller='storage', - action='upload_handle') - map.connect('storage_upload_success', '/storage/upload/success', - controller='storage', - action='success') - map.connect('storage_upload_success_empty', '/storage/upload/success_empty', - controller='storage', - action='success_empty') - map.connect('storage_file', '/storage/f/{label:.*}', - controller='storage', - action='file') + if asbool( config.get('storage.active', 'false') ): + map.connect('storage_api', "/api/storage", + controller='storage_api', action='index') + map.connect('storage_api_set_metadata', '/api/storage/metadata/{label:.*}', + controller='storage_api', action='set_metadata', + conditions={'method': ['PUT','POST']}) + map.connect('storage_api_get_metadata', '/api/storage/metadata/{label:.*}', + controller='storage_api', action='get_metadata', + conditions={'method': ['GET']}) + map.connect('storage_api_auth_request', + '/api/storage/auth/request/{label:.*}', + controller='storage_api', + action='auth_request') + map.connect('storage_api_auth_form', + '/api/storage/auth/form/{label:.*}', + controller='storage_api', + action='auth_form') + map.connect('storage_upload', '/storage/upload', + controller='storage', + action='upload') + map.connect('storage_upload_handle', '/storage/upload_handle', + controller='storage', + action='upload_handle') + map.connect('storage_upload_success', '/storage/upload/success', + controller='storage', + action='success') + map.connect('storage_upload_success_empty', '/storage/upload/success_empty', + controller='storage', + action='success_empty') + map.connect('storage_file', '/storage/f/{label:.*}', + controller='storage', + action='file') for plugin in routing_plugins: diff --git a/ckan/controllers/storage.py b/ckan/controllers/storage.py index b8ca51062a7..9f217e43f5b 100644 --- a/ckan/controllers/storage.py +++ b/ckan/controllers/storage.py @@ -15,6 +15,7 @@ import ckan.model as model import ckan.authz as authz + try: from cStringIO import StringIO except ImportError: @@ -154,9 +155,9 @@ def upload_handle(self): self.ofs.put_stream(bucket_id, label, stream.file, params) success_action_redirect = h.url_for('storage_upload_success', qualified=True, bucket=BUCKET, label=label) - # Cannot redirect here as it breaks js file uploads (get infinite loop + # Do not redirect here as it breaks js file uploads (get infinite loop # in FF and crash in Chrome) - # h.redirect_to(success_action_redirect) + return self.success(label) def success(self, label=None): @@ -167,6 +168,18 @@ def success(self, label=None): qualified=True ) c.upload_url = h.url_for('storage_upload') + + # TODO: + # Publish a job onto the queue for the archiver so that it can check + # this resource and upload to somewhere else out of bounds to this + # request + # from ckan.lib.celery_app import celery + # from ckan.model.types import make_uuid + # task_id = make_uuid() + # context = {} + # data = label + # celery.send_task("archiver.upload", args=[context, data], task_id=task_id) + return render('storage/success.html') def success_empty(self, label=None): From b48df83a7a7ace82be0e40f8af0f723b0cfe201d Mon Sep 17 00:00:00 2001 From: Ross Jones Date: Wed, 4 Jan 2012 17:32:13 +0000 Subject: [PATCH 33/55] [refactor][m] Cleaning up the StorageAPIController and making sure that the routing is kept clean by making sure the check is done in __before__ instead of the routing file. Also determine if it is active based on the presence of the storage.directory setting (being non-empty). --- ckan/config/routing.py | 68 ++++---- ckan/controllers/storage.py | 250 +++++++++++++++++++++++++++- ckan/controllers/storage_api.py | 286 -------------------------------- 3 files changed, 276 insertions(+), 328 deletions(-) delete mode 100644 ckan/controllers/storage_api.py diff --git a/ckan/config/routing.py b/ckan/config/routing.py index 76e714d00ae..2bb5d38f44d 100644 --- a/ckan/config/routing.py +++ b/ckan/config/routing.py @@ -8,7 +8,7 @@ from pylons import config from routes import Mapper from ckan.plugins import PluginImplementations, IRoutes -from paste.deploy.converters import asbool + routing_plugins = PluginImplementations(IRoutes) @@ -280,38 +280,40 @@ def make_map(): map.connect('ckanadmin', '/ckan-admin/{action}', controller='admin') # Storage routes - if asbool( config.get('storage.active', 'false') ): - map.connect('storage_api', "/api/storage", - controller='storage_api', action='index') - map.connect('storage_api_set_metadata', '/api/storage/metadata/{label:.*}', - controller='storage_api', action='set_metadata', - conditions={'method': ['PUT','POST']}) - map.connect('storage_api_get_metadata', '/api/storage/metadata/{label:.*}', - controller='storage_api', action='get_metadata', - conditions={'method': ['GET']}) - map.connect('storage_api_auth_request', - '/api/storage/auth/request/{label:.*}', - controller='storage_api', - action='auth_request') - map.connect('storage_api_auth_form', - '/api/storage/auth/form/{label:.*}', - controller='storage_api', - action='auth_form') - map.connect('storage_upload', '/storage/upload', - controller='storage', - action='upload') - map.connect('storage_upload_handle', '/storage/upload_handle', - controller='storage', - action='upload_handle') - map.connect('storage_upload_success', '/storage/upload/success', - controller='storage', - action='success') - map.connect('storage_upload_success_empty', '/storage/upload/success_empty', - controller='storage', - action='success_empty') - map.connect('storage_file', '/storage/f/{label:.*}', - controller='storage', - action='file') + map.connect('storage_api', "/api/storage", + controller='ckan.controllers.storage:StorageAPIController', + action='index') + map.connect('storage_api_set_metadata', '/api/storage/metadata/{label:.*}', + controller='ckan.controllers.storage:StorageAPIController', + action='set_metadata', + conditions={'method': ['PUT','POST']}) + map.connect('storage_api_get_metadata', '/api/storage/metadata/{label:.*}', + controller='ckan.controllers.storage:StorageAPIController', + action='get_metadata', + conditions={'method': ['GET']}) + map.connect('storage_api_auth_request', + '/api/storage/auth/request/{label:.*}', + controller='ckan.controllers.storage:StorageAPIController', + action='auth_request') + map.connect('storage_api_auth_form', + '/api/storage/auth/form/{label:.*}', + controller='ckan.controllers.storage:StorageAPIController', + action='auth_form') + map.connect('storage_upload', '/storage/upload', + controller='ckan.controllers.storage:StorageController', + action='upload') + map.connect('storage_upload_handle', '/storage/upload_handle', + controller='ckan.controllers.storage:StorageController', + action='upload_handle') + map.connect('storage_upload_success', '/storage/upload/success', + controller='ckan.controllers.storage:StorageController', + action='success') + map.connect('storage_upload_success_empty', '/storage/upload/success_empty', + controller='ckan.controllers.storage:StorageController', + action='success_empty') + map.connect('storage_file', '/storage/f/{label:.*}', + controller='ckan.controllers.storage:StorageController', + action='file') for plugin in routing_plugins: diff --git a/ckan/controllers/storage.py b/ckan/controllers/storage.py index 9f217e43f5b..e3aa73a156b 100644 --- a/ckan/controllers/storage.py +++ b/ckan/controllers/storage.py @@ -1,3 +1,4 @@ +import os import re import urllib import uuid @@ -9,13 +10,13 @@ from pylons.controllers.util import abort, redirect_to from pylons import config from paste.fileapp import FileApp +from paste.deploy.converters import asbool from ckan.lib.base import BaseController, c, request, render, config, h, abort from ckan.lib.jsonp import jsonpify import ckan.model as model import ckan.authz as authz - try: from cStringIO import StringIO except ImportError: @@ -32,7 +33,7 @@ # pairtree_version0_1 file for identifying folders BUCKET = config['storage.bucket'] key_prefix = config.get('storage.key_prefix', 'file/') - +storage_dir = config.get('storage.directory', '') _eq_re = re.compile(r"^(.*)(=[0-9]*)$") def fix_stupid_pylons_encoding(data): @@ -54,13 +55,25 @@ def get_ofs(): cases here this will be the local file storage so we fix the implementation to use pairtree. """ - kw = {} - for k,v in config.items(): - if not k.startswith('ofs.'): - continue - kw[k[4:]] = v - ofs = get_impl("pairtree")(**kw) - return ofs + return get_impl("pairtree")(storage_dir=storage_dir) + + +pairtree_marker_done = False +def create_pairtree_marker(): + """ + Make sure that the file pairtree_version0_1 is present in storage_dir + and if not then create it. + """ + global pairtree_marker_done + if pairtree_marker_done or not storage_dir: + return + + path = os.path.join(storage_dir, 'pairtree_version0_1') + if not os.path.exists( path ): + open(path, 'w').close() + + pairtree_marker_done = True + def authorize(method, bucket, key, user, ofs): @@ -86,6 +99,13 @@ def authorize(method, bucket, key, user, ofs): class StorageController(BaseController): '''Upload to storage backend. ''' + def __before__(self, action, **params): + super(StorageController, self).__before__(action, **params) + if not storage_dir: + abort(404) + else: + create_pairtree_marker() + @property def ofs(self): return get_ofs() @@ -211,3 +231,215 @@ def file(self, label): else: h.redirect_to(file_url) + + +class StorageAPIController(BaseController): + + def __before__(self, action, **params): + super(StorageAPIController, self).__before__(action, **params) + if not storage_dir: + abort(404) + else: + create_pairtree_marker() + + @property + def ofs(self): + return get_ofs() + + @jsonpify + def index(self): + info = { + 'metadata/{label}': { + 'description': 'Get or set metadata for this item in storage', + }, + 'auth/request/{label}': { + 'description': self.auth_request.__doc__, + }, + 'auth/form/{label}': { + 'description': self.auth_form.__doc__, + } + } + return info + + def set_metadata(self, label): + bucket = BUCKET + if not label.startswith("/"): label = "/" + label + + try: + data = fix_stupid_pylons_encoding(request.body) + if data: + metadata = json.loads(data) + else: + metadata = {} + except: + abort(400) + + try: + b = self.ofs._require_bucket(bucket) + except: + abort(409) + + k = self.ofs._get_key(b, label) + if k is None: + k = b.new_key(label) + metadata = metadata.copy() + metadata["_creation_time"] = str(datetime.utcnow()) + self.ofs._update_key_metadata(k, metadata) + k.set_contents_from_file(StringIO('')) + elif request.method == "PUT": + old = self.ofs.get_metadata(bucket, label) + to_delete = [] + for ok in old.keys(): + if ok not in metadata: + to_delete.append(ok) + if to_delete: + self.ofs.del_metadata_keys(bucket, label, to_delete) + self.ofs.update_metadata(bucket, label, metadata) + else: + self.ofs.update_metadata(bucket, label, metadata) + + k.make_public() + k.close() + + return self.get_metadata(bucket, label) + + @jsonpify + def get_metadata(self, label): + bucket = BUCKET + url = h.url_for('storage_file', + label=label, + qualified=True + ) + if not self.ofs.exists(bucket, label): + abort(404) + metadata = self.ofs.get_metadata(bucket, label) + metadata["_location"] = url + return metadata + + @jsonpify + def auth_request(self, label): + '''Provide authentication information for a request so a client can + interact with backend storage directly. + + :param label: label. + :param kwargs: sent either via query string for GET or json-encoded + dict for POST). Interpreted as http headers for request plus an + (optional) method parameter (being the HTTP method). + + Examples of headers are: + + Content-Type + Content-Encoding (optional) + Content-Length + Content-MD5 + Expect (should be '100-Continue') + + :return: is a json hash containing various attributes including a + headers dictionary containing an Authorization field which is good for + 15m. + + ''' + bucket = BUCKET + if request.POST: + try: + data = fix_stupid_pylons_encoding(request.body) + headers = json.loads(data) + except Exception, e: + from traceback import print_exc + msg = StringIO() + print_exc(msg) + log.error(msg.seek(0).read()) + abort(400) + else: + headers = dict(request.params) + if 'method' in headers: + method = headers['method'] + del headers['method'] + else: + method = 'POST' + + authorize(method, bucket, label, c.userobj, self.ofs) + + http_request = self.ofs.authenticate_request(method, bucket, label, + headers) + return { + 'host': http_request.host, + 'method': http_request.method, + 'path': http_request.path, + 'headers': http_request.headers + } + + def _get_remote_form_data(self, label): + method = 'POST' + content_length_range = int( + config.get('ckanext.storage.max_content_length', + 50000000)) + acl = 'public-read' + fields = [ { + 'name': self.ofs.conn.provider.metadata_prefix + 'uploaded-by', + 'value': c.userobj.id + }] + conditions = [ '{"%s": "%s"}' % (x['name'], x['value']) for x in + fields ] + # In FF redirect to this breaks js upload as FF attempts to open file + # (presumably because mimetype = javascript) and this stops js + # success_action_redirect = h.url_for('storage_api_get_metadata', qualified=True, + # label=label) + success_action_redirect = h.url_for('storage_upload_success_empty', qualified=True, + label=label) + data = self.ofs.conn.build_post_form_args( + BUCKET, + label, + expires_in=72000, + max_content_length=content_length_range, + success_action_redirect=success_action_redirect, + acl=acl, + fields=fields, + conditions=conditions + ) + # HACK: fix up some broken stuff from boto + # e.g. should not have content-length-range in list of fields! + for idx,field in enumerate(data['fields']): + if field['name'] == 'content-length-range': + del data['fields'][idx] + return data + + def _get_form_data(self, label): + data = { + 'action': h.url_for('storage_upload_handle', qualified=True), + 'fields': [ + { + 'name': 'key', + 'value': label + } + ] + } + return data + + @jsonpify + def auth_form(self, label): + '''Provide fields for a form upload to storage including + authentication. + + :param label: label. + :return: json-encoded dictionary with action parameter and fields list. + ''' + bucket = BUCKET + if request.POST: + try: + data = fix_stupid_pylons_encoding(request.body) + headers = json.loads(data) + except Exception, e: + from traceback import print_exc + msg = StringIO() + print_exc(msg) + log.error(msg.seek(0).read()) + abort(400) + else: + headers = dict(request.params) + + method = 'POST' + authorize(method, bucket, label, c.userobj, self.ofs) + data = self._get_form_data(label) + return data + diff --git a/ckan/controllers/storage_api.py b/ckan/controllers/storage_api.py deleted file mode 100644 index 594131e5b1c..00000000000 --- a/ckan/controllers/storage_api.py +++ /dev/null @@ -1,286 +0,0 @@ -import re -import urllib -import uuid -from datetime import datetime -from cgi import FieldStorage - -from ofs import get_impl -from pylons import request, response -from pylons.controllers.util import abort, redirect_to -from pylons import config -from paste.fileapp import FileApp - -from ckan.lib.base import BaseController, c, request, render, config, h, abort -from ckan.lib.jsonp import jsonpify -import ckan.model as model -import ckan.authz as authz - -try: - from cStringIO import StringIO -except ImportError: - from StringIO import StringIO -try: - import json -except: - import simplejson as json - -from logging import getLogger -log = getLogger(__name__) - - -BUCKET = config['storage.bucket'] -key_prefix = config.get('storage.key_prefix', 'file/') - - -_eq_re = re.compile(r"^(.*)(=[0-9]*)$") -def fix_stupid_pylons_encoding(data): - """ - Fix an apparent encoding problem when calling request.body - TODO: Investigate whether this is fixed in later versions? - """ - if data.startswith("%") or data.startswith("+"): - data = urllib.unquote_plus(data) - m = _eq_re.match(data) - if m: - data = m.groups()[0] - return data - - -def get_ofs(): - """ - Return a configured instance of the appropriate OFS driver, in all - cases here this will be the local file storage so we fix the implementation - to use pairtree. - """ - kw = {} - for k,v in config.items(): - if not k.startswith('ofs.'): - continue - kw[k[4:]] = v - ofs = get_impl("pairtree")(**kw) - return ofs - - -def authorize(method, bucket, key, user, ofs): - """ - Check authz for the user with a given bucket/key combo within a - particular ofs implementation. - """ - if not method in ['POST', 'GET', 'PUT', 'DELETE']: - abort(400) - if method != 'GET': - # do not allow overwriting - if ofs.exists(bucket, key): - abort(409) - # now check user stuff - username = user.name if user else '' - is_authorized = authz.Authorizer.is_authorized(username, 'file-upload', model.System()) - if not is_authorized: - h.flash_error('Not authorized to upload files.') - abort(401) - - - - -class StorageApiController(BaseController): - @property - def ofs(self): - return get_ofs() - - @jsonpify - def index(self): - info = { - 'metadata/{label}': { - 'description': 'Get or set metadata for this item in storage', - }, - 'auth/request/{label}': { - 'description': self.auth_request.__doc__, - }, - 'auth/form/{label}': { - 'description': self.auth_form.__doc__, - } - } - return info - - def set_metadata(self, label): - bucket = BUCKET - if not label.startswith("/"): label = "/" + label - - try: - data = fix_stupid_pylons_encoding(request.body) - if data: - metadata = loads(data) - else: - metadata = {} - except: - abort(400) - - try: - b = self.ofs._require_bucket(bucket) - except: - abort(409) - - k = self.ofs._get_key(b, label) - if k is None: - k = b.new_key(label) - metadata = metadata.copy() - metadata["_creation_time"] = str(datetime.utcnow()) - self.ofs._update_key_metadata(k, metadata) - k.set_contents_from_file(StringIO('')) - elif request.method == "PUT": - old = self.ofs.get_metadata(bucket, label) - to_delete = [] - for ok in old.keys(): - if ok not in metadata: - to_delete.append(ok) - if to_delete: - self.ofs.del_metadata_keys(bucket, label, to_delete) - self.ofs.update_metadata(bucket, label, metadata) - else: - self.ofs.update_metadata(bucket, label, metadata) - - k.make_public() - k.close() - - return self.get_metadata(bucket, label) - - @jsonpify - def get_metadata(self, label): - bucket = BUCKET - url = h.url_for('storage_file', - label=label, - qualified=True - ) - if not self.ofs.exists(bucket, label): - abort(404) - metadata = self.ofs.get_metadata(bucket, label) - metadata["_location"] = url - return metadata - - @jsonpify - def auth_request(self, label): - '''Provide authentication information for a request so a client can - interact with backend storage directly. - - :param label: label. - :param kwargs: sent either via query string for GET or json-encoded - dict for POST). Interpreted as http headers for request plus an - (optional) method parameter (being the HTTP method). - - Examples of headers are: - - Content-Type - Content-Encoding (optional) - Content-Length - Content-MD5 - Expect (should be '100-Continue') - - :return: is a json hash containing various attributes including a - headers dictionary containing an Authorization field which is good for - 15m. - - ''' - bucket = BUCKET - if request.POST: - try: - data = fix_stupid_pylons_encoding(request.body) - headers = loads(data) - except Exception, e: - from traceback import print_exc - msg = StringIO() - print_exc(msg) - log.error(msg.seek(0).read()) - abort(400) - else: - headers = dict(request.params) - if 'method' in headers: - method = headers['method'] - del headers['method'] - else: - method = 'POST' - - authorize(method, bucket, label, c.userobj, self.ofs) - - http_request = self.ofs.authenticate_request(method, bucket, label, - headers) - return { - 'host': http_request.host, - 'method': http_request.method, - 'path': http_request.path, - 'headers': http_request.headers - } - - def _get_remote_form_data(self, label): - method = 'POST' - content_length_range = int( - config.get('ckanext.storage.max_content_length', - 50000000)) - acl = 'public-read' - fields = [ { - 'name': self.ofs.conn.provider.metadata_prefix + 'uploaded-by', - 'value': c.userobj.id - }] - conditions = [ '{"%s": "%s"}' % (x['name'], x['value']) for x in - fields ] - # In FF redirect to this breaks js upload as FF attempts to open file - # (presumably because mimetype = javascript) and this stops js - # success_action_redirect = h.url_for('storage_api_get_metadata', qualified=True, - # label=label) - success_action_redirect = h.url_for('storage_upload_success_empty', qualified=True, - label=label) - data = self.ofs.conn.build_post_form_args( - BUCKET, - label, - expires_in=72000, - max_content_length=content_length_range, - success_action_redirect=success_action_redirect, - acl=acl, - fields=fields, - conditions=conditions - ) - # HACK: fix up some broken stuff from boto - # e.g. should not have content-length-range in list of fields! - for idx,field in enumerate(data['fields']): - if field['name'] == 'content-length-range': - del data['fields'][idx] - return data - - def _get_form_data(self, label): - data = { - 'action': h.url_for('storage_upload_handle', qualified=True), - 'fields': [ - { - 'name': 'key', - 'value': label - } - ] - } - return data - - @jsonpify - def auth_form(self, label): - '''Provide fields for a form upload to storage including - authentication. - - :param label: label. - :return: json-encoded dictionary with action parameter and fields list. - ''' - bucket = BUCKET - if request.POST: - try: - data = fix_stupid_pylons_encoding(request.body) - headers = loads(data) - except Exception, e: - from traceback import print_exc - msg = StringIO() - print_exc(msg) - log.error(msg.seek(0).read()) - abort(400) - else: - headers = dict(request.params) - - method = 'POST' - authorize(method, bucket, label, c.userobj, self.ofs) - data = self._get_form_data(label) - return data - From 4d4753347ff1937c3e18d5d8cb47c8651fb02711 Mon Sep 17 00:00:00 2001 From: Ross Jones Date: Thu, 5 Jan 2012 15:42:29 +0000 Subject: [PATCH 34/55] [s] Change to make sure OFS stores the user's name in the metadata rather than their ID --- ckan/controllers/storage.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/ckan/controllers/storage.py b/ckan/controllers/storage.py index e3aa73a156b..312f55b74fd 100644 --- a/ckan/controllers/storage.py +++ b/ckan/controllers/storage.py @@ -126,7 +126,7 @@ def _get_form_for_remote(self): acl = 'public-read' fields = [ { 'name': self.ofs.conn.provider.metadata_prefix + 'uploaded-by', - 'value': c.userobj.id + 'value': c.userobj.name }] conditions = [ '{"%s": "%s"}' % (x['name'], x['value']) for x in fields ] c.data = self.ofs.conn.build_post_form_args( @@ -169,8 +169,8 @@ def upload_handle(self): abort(400, "No file stream.") del params['file'] params['filename-original'] = stream.filename - params['_owner'] = c.userobj.id if c.userobj else "" - params['uploaded-by'] = c.userobj.id if c.userobj else "" + params['_owner'] = c.userobj.name if c.userobj else "" + params['uploaded-by'] = c.userobj.name if c.userobj else "" self.ofs.put_stream(bucket_id, label, stream.file, params) success_action_redirect = h.url_for('storage_upload_success', qualified=True, @@ -377,7 +377,7 @@ def _get_remote_form_data(self, label): acl = 'public-read' fields = [ { 'name': self.ofs.conn.provider.metadata_prefix + 'uploaded-by', - 'value': c.userobj.id + 'value': c.userobj.name }] conditions = [ '{"%s": "%s"}' % (x['name'], x['value']) for x in fields ] From b45d76e74dd2206dfd55851a07b9d02640eefca2 Mon Sep 17 00:00:00 2001 From: Ross Jones Date: Fri, 6 Jan 2012 09:52:55 +0000 Subject: [PATCH 35/55] [s] Made change to allow task_status_update to happen without requirement that user is a sysadmin/siteuser. --- ckan/logic/action/update.py | 2 +- ckan/logic/auth/update.py | 3 +++ 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/ckan/logic/action/update.py b/ckan/logic/action/update.py index 5bd99b8bc94..08d7572051b 100644 --- a/ckan/logic/action/update.py +++ b/ckan/logic/action/update.py @@ -379,7 +379,7 @@ def task_status_update(context, data_dict): if task_status is None: raise NotFound(_('TaskStatus was not found.')) - + check_access('task_status_update', context, data_dict) data, errors = validate(data_dict, schema, context) diff --git a/ckan/logic/auth/update.py b/ckan/logic/auth/update.py index 1206be19c10..4d31324d67b 100644 --- a/ckan/logic/auth/update.py +++ b/ckan/logic/auth/update.py @@ -150,6 +150,9 @@ def task_status_update(context, data_dict): model = context['model'] user = context['user'] + if 'ignore_auth' in context and context['ignore_auth']: + return {'success': True} + authorized = Authorizer().is_sysadmin(unicode(user)) if not authorized: return {'success': False, 'msg': _('User %s not authorized to update task_status table') % str(user)} From a4013bc7433293368de4c76f39b8fc4fb7c0ec5e Mon Sep 17 00:00:00 2001 From: Ross Jones Date: Fri, 6 Jan 2012 12:26:59 +0000 Subject: [PATCH 36/55] [1608] Cleanup/Removing unnecessary TODO (implemented elsewhere) --- ckan/controllers/storage.py | 21 +++------------------ 1 file changed, 3 insertions(+), 18 deletions(-) diff --git a/ckan/controllers/storage.py b/ckan/controllers/storage.py index 312f55b74fd..9724b7b180a 100644 --- a/ckan/controllers/storage.py +++ b/ckan/controllers/storage.py @@ -177,7 +177,6 @@ def upload_handle(self): bucket=BUCKET, label=label) # Do not redirect here as it breaks js file uploads (get infinite loop # in FF and crash in Chrome) - return self.success(label) def success(self, label=None): @@ -187,19 +186,7 @@ def success(self, label=None): label=label, qualified=True ) - c.upload_url = h.url_for('storage_upload') - - # TODO: - # Publish a job onto the queue for the archiver so that it can check - # this resource and upload to somewhere else out of bounds to this - # request - # from ckan.lib.celery_app import celery - # from ckan.model.types import make_uuid - # task_id = make_uuid() - # context = {} - # data = label - # celery.send_task("archiver.upload", args=[context, data], task_id=task_id) - + c.upload_url = h.url_for('storage_upload') return render('storage/success.html') def success_empty(self, label=None): @@ -212,13 +199,11 @@ def file(self, label): # handle erroneous trailing slash by redirecting to url w/o slash if label.endswith('/'): label = label[:-1] - file_url = h.url_for( - 'storage_file', - label=label - ) + file_url = h.url_for( 'storage_file', label=label ) h.redirect_to(file_url) else: abort(404) + file_url = self.ofs.get_url(BUCKET, label) if file_url.startswith("file://"): metadata = self.ofs.get_metadata(BUCKET, label) From 69792aa8a98598cea7a458207ebcc527b8c9946b Mon Sep 17 00:00:00 2001 From: Ross Jones Date: Mon, 9 Jan 2012 16:09:55 +0000 Subject: [PATCH 37/55] Allow the author of a resource (revision) to call task_status_update and not just sysadm --- ckan/controllers/storage.py | 2 +- ckan/logic/auth/update.py | 11 ++++++++++- 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/ckan/controllers/storage.py b/ckan/controllers/storage.py index 9724b7b180a..f1d1c4080f8 100644 --- a/ckan/controllers/storage.py +++ b/ckan/controllers/storage.py @@ -169,7 +169,7 @@ def upload_handle(self): abort(400, "No file stream.") del params['file'] params['filename-original'] = stream.filename - params['_owner'] = c.userobj.name if c.userobj else "" + #params['_owner'] = c.userobj.name if c.userobj else "" params['uploaded-by'] = c.userobj.name if c.userobj else "" self.ofs.put_stream(bucket_id, label, stream.file, params) diff --git a/ckan/logic/auth/update.py b/ckan/logic/auth/update.py index 4d31324d67b..e99c98b0353 100644 --- a/ckan/logic/auth/update.py +++ b/ckan/logic/auth/update.py @@ -1,5 +1,5 @@ from ckan.logic import check_access_old, NotFound -from ckan.logic.auth import get_package_object, get_group_object, get_authorization_group_object, \ +from ckan.logic.auth import get_package_object, get_resource_object, get_group_object, get_authorization_group_object, \ get_user_object, get_resource_object from ckan.logic.auth.create import check_group_auth, package_relationship_create from ckan.authz import Authorizer @@ -153,6 +153,15 @@ def task_status_update(context, data_dict): if 'ignore_auth' in context and context['ignore_auth']: return {'success': True} + try: + resource = get_resource_object(context, data_dict) + except: + id = data_dict.get('entity_id',None) + resource = model.Resource.get(id) + + if resource.revision.author == user: + return {'success': True} + authorized = Authorizer().is_sysadmin(unicode(user)) if not authorized: return {'success': False, 'msg': _('User %s not authorized to update task_status table') % str(user)} From 8c1a7c6aafebb12465dbfa6e23548f9948115329 Mon Sep 17 00:00:00 2001 From: Ross Jones Date: Tue, 10 Jan 2012 15:24:44 +0000 Subject: [PATCH 38/55] [1608] Cleaned up revision message (removed None) and allow resource owners to update them. --- ckan/logic/action/update.py | 3 ++- ckan/logic/auth/update.py | 5 +++-- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/ckan/logic/action/update.py b/ckan/logic/action/update.py index 08d7572051b..f4b93aa836d 100644 --- a/ckan/logic/action/update.py +++ b/ckan/logic/action/update.py @@ -166,6 +166,7 @@ def resource_update(context, data_dict): context["resource"] = resource if not resource: + logging.error('Could not find resource ' + id) raise NotFound(_('Resource was not found.')) check_access('resource_update', context, data_dict) @@ -181,7 +182,7 @@ def resource_update(context, data_dict): if 'message' in context: rev.message = context['message'] else: - rev.message = _(u'REST API: Update object %s') % data.get("name") + rev.message = _(u'REST API: Update object %s') % data.get("name", "") resource = resource_dict_save(data, context) if not context.get('defer_commit'): diff --git a/ckan/logic/auth/update.py b/ckan/logic/auth/update.py index e99c98b0353..ac5f01197cd 100644 --- a/ckan/logic/auth/update.py +++ b/ckan/logic/auth/update.py @@ -159,8 +159,9 @@ def task_status_update(context, data_dict): id = data_dict.get('entity_id',None) resource = model.Resource.get(id) - if resource.revision.author == user: - return {'success': True} + if resource and resource.revision: + if resource.revision.author == user: + return {'success': True} authorized = Authorizer().is_sysadmin(unicode(user)) if not authorized: From d6395ac446fa38faa925a592d6f0e61f1a83d347 Mon Sep 17 00:00:00 2001 From: Ross Jones Date: Wed, 11 Jan 2012 13:55:00 +0000 Subject: [PATCH 39/55] Remove unnecessary code --- ckan/controllers/storage.py | 33 --------------------------------- 1 file changed, 33 deletions(-) diff --git a/ckan/controllers/storage.py b/ckan/controllers/storage.py index f1d1c4080f8..3998319eda6 100644 --- a/ckan/controllers/storage.py +++ b/ckan/controllers/storage.py @@ -110,39 +110,6 @@ def __before__(self, action, **params): def ofs(self): return get_ofs() - def _get_form_for_remote(self): - # would be nice to use filename of file - # problem is 'we' don't know this at this point and cannot add it to - # success_action_redirect and hence cannnot display to user afterwards - # + '/${filename}' - label = key_prefix + request.params.get('filepath', str(uuid.uuid4())) - method = 'POST' - authorize(method, BUCKET, label, c.userobj, self.ofs) - content_length_range = int( - config.get('ckanext.storage.max_content_length', - 50000000)) - success_action_redirect = h.url_for('storage_upload_success', qualified=True, - bucket=BUCKET, label=label) - acl = 'public-read' - fields = [ { - 'name': self.ofs.conn.provider.metadata_prefix + 'uploaded-by', - 'value': c.userobj.name - }] - conditions = [ '{"%s": "%s"}' % (x['name'], x['value']) for x in fields ] - c.data = self.ofs.conn.build_post_form_args( - BUCKET, - label, - expires_in=3600, - max_content_length=content_length_range, - success_action_redirect=success_action_redirect, - acl=acl, - fields=fields, - conditions=conditions - ) - for idx,field in enumerate(c.data['fields']): - if field['name'] == 'content-length-range': - del c.data['fields'][idx] - c.data_json = json.dumps(c.data, indent=2) def upload(self): label = key_prefix + request.params.get('filepath', str(uuid.uuid4())) From a4c267a357642234a2edc7ed6c53e61c8369c09b Mon Sep 17 00:00:00 2001 From: Ross Jones Date: Thu, 12 Jan 2012 15:22:06 +0000 Subject: [PATCH 40/55] Remove the change to task_status_update auth so that it as it was. --- ckan/controllers/storage.py | 2 ++ ckan/logic/auth/update.py | 10 ---------- 2 files changed, 2 insertions(+), 10 deletions(-) diff --git a/ckan/controllers/storage.py b/ckan/controllers/storage.py index 3998319eda6..05c2140a5e6 100644 --- a/ckan/controllers/storage.py +++ b/ckan/controllers/storage.py @@ -166,6 +166,8 @@ def file(self, label): # handle erroneous trailing slash by redirecting to url w/o slash if label.endswith('/'): label = label[:-1] + # This may be best being cached_url until we have moved it into + # permanent storage file_url = h.url_for( 'storage_file', label=label ) h.redirect_to(file_url) else: diff --git a/ckan/logic/auth/update.py b/ckan/logic/auth/update.py index ac5f01197cd..b2b15fc6831 100644 --- a/ckan/logic/auth/update.py +++ b/ckan/logic/auth/update.py @@ -152,16 +152,6 @@ def task_status_update(context, data_dict): if 'ignore_auth' in context and context['ignore_auth']: return {'success': True} - - try: - resource = get_resource_object(context, data_dict) - except: - id = data_dict.get('entity_id',None) - resource = model.Resource.get(id) - - if resource and resource.revision: - if resource.revision.author == user: - return {'success': True} authorized = Authorizer().is_sysadmin(unicode(user)) if not authorized: From 0c6a187799a1fef3914e13df44e8ff4c112a1ac8 Mon Sep 17 00:00:00 2001 From: Ross Jones Date: Thu, 19 Jan 2012 10:02:05 +0000 Subject: [PATCH 41/55] Importing the tests from the extension and ensuring that they work from inside core --- ckan/tests/functional/test_storage.py | 71 +++++++++++++++++++++++++++ ckan/tests/functional/test_upload.py | 56 +++++++++++++++++++++ 2 files changed, 127 insertions(+) create mode 100644 ckan/tests/functional/test_storage.py create mode 100644 ckan/tests/functional/test_upload.py diff --git a/ckan/tests/functional/test_storage.py b/ckan/tests/functional/test_storage.py new file mode 100644 index 00000000000..714090fdd41 --- /dev/null +++ b/ckan/tests/functional/test_storage.py @@ -0,0 +1,71 @@ +import os +from paste.deploy import appconfig +import paste.fixture +from ckan.config.middleware import make_app +import ckan.model as model +from ckan.tests import conf_dir, url_for, CreateTestData +from ckan.controllers.admin import get_sysadmins + +class TestStorageAPIController: + @classmethod + def setup_class(cls): + config = appconfig('config:test.ini', relative_to=conf_dir) + config.local_conf['ckan.plugins'] = 'storage' + config.local_conf['ofs.impl'] = 'pairtree' + config.local_conf['ofs.storage_dir'] = '/tmp/ckan-test-ckanext-storage' + wsgiapp = make_app(config.global_conf, **config.local_conf) + cls.app = paste.fixture.TestApp(wsgiapp) + + def test_index(self): + url = url_for('storage_api') + res = self.app.get(url) + out = res.json + assert len(res.json) == 3 + + def test_authz(self): + url = url_for('storage_api_auth_form', label='abc') + res = self.app.get(url, status=[200]) + +class TestStorageAPIControllerLocal: + @classmethod + def setup_class(cls): + config = appconfig('config:test.ini', relative_to=conf_dir) + config.local_conf['ckan.plugins'] = 'storage' + config.local_conf['ofs.impl'] = 'pairtree' + config.local_conf['ofs.storage_dir'] = '/tmp/ckan-test-ckanext-storage' + wsgiapp = make_app(config.global_conf, **config.local_conf) + cls.app = paste.fixture.TestApp(wsgiapp) + CreateTestData.create() + model.Session.remove() + user = model.User.by_name('tester') + cls.extra_environ = {'Authorization': str(user.apikey)} + + @classmethod + def teardown_class(cls): + CreateTestData.delete() + + def test_auth_form(self): + url = url_for('storage_api_auth_form', label='abc') + res = self.app.get(url, extra_environ=self.extra_environ, status=200) + assert res.json['action'] == u'http://localhost/storage/upload_handle', res.json + assert res.json['fields'][-1]['value'] == 'abc', res + + url = url_for('storage_api_auth_form', label='abc/xxx') + res = self.app.get(url, extra_environ=self.extra_environ, status=200) + assert res.json['fields'][-1]['value'] == 'abc/xxx' + + def test_metadata(self): + url = url_for('storage_api_get_metadata', label='abc') + res = self.app.get(url, status=404) + + # TODO: test get metadata on real setup ... + label = 'abc' + url = url_for('storage_api_set_metadata', + extra_environ=self.extra_environ, + label=label, + data=dict( + label=label + ) + ) + # res = self.app.get(url, status=404) + diff --git a/ckan/tests/functional/test_upload.py b/ckan/tests/functional/test_upload.py new file mode 100644 index 00000000000..8d374b101c1 --- /dev/null +++ b/ckan/tests/functional/test_upload.py @@ -0,0 +1,56 @@ +import os +from paste.deploy import appconfig +import paste.fixture + +from ckan.config.middleware import make_app +from ckan.tests import conf_dir, url_for, CreateTestData +import ckan.model as model + + +class TestStorageController: + @classmethod + def setup_class(cls): + config = appconfig('config:test.ini', relative_to=conf_dir) + config.local_conf['ckan.plugins'] = 'storage' + wsgiapp = make_app(config.global_conf, **config.local_conf) + cls.app = paste.fixture.TestApp(wsgiapp) + CreateTestData.create() + + @classmethod + def teardown_class(cls): + model.Session.remove() + model.repo.rebuild_db() + + def test_02_authorization(self): + from ckan.model.authz import Action + import ckan.model as model + import ckan.authz as authz + john = model.User(name=u'john') + model.Session.add(john) + is_authorized = authz.Authorizer.is_authorized(john.name, Action.UPLOAD_ACTION, model.System()) + assert is_authorized + + def test_03_authorization_wui(self): + url = url_for('storage_upload') + res = self.app.get(url, status=[200] ) + if res.status == 302: + res = res.follow() + assert 'Login' in res, res + + def test_04_index(self): + extra_environ = {'REMOTE_USER': 'tester'} + url = url_for('storage_upload') + out = self.app.get(url, extra_environ=extra_environ) + assert 'Upload' in out, out + #assert 'action="https://commondatastorage.googleapis.com/ckan' in out, out + #assert 'key" value="' in out, out + #assert 'policy" value="' in out, out + #assert 'failure_action_redirect' in out, out + #assert 'success_action_redirect' in out, out + + url = url_for('storage_upload', filepath='xyz.txt') + out = self.app.get(url, extra_environ=extra_environ) + assert 'file/xyz.txt' in out, out + + # TODO: test file upload itself + From 9d1b838a99258bec6ad293e431b1f2a53fded6a8 Mon Sep 17 00:00:00 2001 From: David Read Date: Wed, 18 Jan 2012 18:49:54 +0000 Subject: [PATCH 42/55] [tests]: Refactored test to show better the intermittant problem with solr results. --- ckan/tests/functional/test_pagination.py | 118 +++++++++++++++++------ 1 file changed, 91 insertions(+), 27 deletions(-) diff --git a/ckan/tests/functional/test_pagination.py b/ckan/tests/functional/test_pagination.py index adc95ddd9d9..436622d0469 100644 --- a/ckan/tests/functional/test_pagination.py +++ b/ckan/tests/functional/test_pagination.py @@ -1,8 +1,32 @@ +import re + +from nose.tools import assert_equal + from ckan.lib.create_test_data import CreateTestData import ckan.model as model from ckan.tests import TestController, url_for, setup_test_search_index -class TestPagination(TestController): +def scrape_search_results(response, object_type): + assert object_type in ('dataset', 'group', 'user') + results = re.findall('href="/%s/%s_(\d\d)"' % (object_type, object_type), + str(response)) + return results + +def test_scrape(): + html = ''' +
  • + user_00 +
    • + ... +
  • + user_01 +
    • + + ''' + res = scrape_search_results(html, 'user') + assert_equal(res, ['00', '01']) + +class TestPaginationPackage(TestController): @classmethod def setup_class(cls): setup_test_search_index() @@ -10,63 +34,103 @@ def setup_class(cls): # no. entities per page is hardcoded into the controllers, so # create enough of each here so that we can test pagination - cls.num_groups = 21 cls.num_packages_in_large_group = 51 - cls.num_users = 21 - groups = [u'group_%s' % str(i).zfill(2) for i in range(1, cls.num_groups)] - users = [u'user_%s' % str(i).zfill(2) for i in range(cls.num_users)] packages = [] for i in range(cls.num_packages_in_large_group): packages.append({ - 'name': u'package_%s' % str(i).zfill(2), + 'name': u'dataset_%s' % str(i).zfill(2), 'groups': u'group_00' }) - CreateTestData.create_arbitrary( - packages, extra_group_names=groups, extra_user_names = users, - ) + CreateTestData.create_arbitrary(packages) @classmethod def teardown_class(self): model.repo.rebuild_db() - - def test_search(self): + + def test_package_search_p1(self): res = self.app.get(url_for(controller='package', action='search', q='groups:group_00')) assert 'href="/dataset?q=groups%3Agroup_00&page=2"' in res - assert 'href="/dataset/package_00"' in res, res - assert 'href="/dataset/package_19"' in res, res + pkg_numbers = scrape_search_results(res, 'dataset') + assert_equal(['00', '01', '02', '03', '04', '05', '06', '07', '08', '09', '10', '11', '12', '13', '14', '15', '16', '17', '18', '19'], pkg_numbers) + def test_package_search_p2(self): res = self.app.get(url_for(controller='package', action='search', q='groups:group_00', page=2)) assert 'href="/dataset?q=groups%3Agroup_00&page=1"' in res - assert 'href="/dataset/package_20"' in res - assert 'href="/dataset/package_39"' in res + pkg_numbers = scrape_search_results(res, 'dataset') + assert_equal(['20', '21', '22', '23', '24', '25', '26', '27', '28', '29', '30', '31', '32', '33', '34', '35', '36', '37', '38', '39'], pkg_numbers) + + def test_group_read_p1(self): + res = self.app.get(url_for(controller='group', action='read', id='group_00')) + assert 'href="/group/group_00?page=2' in res + pkg_numbers = scrape_search_results(res, 'dataset') + assert_equal(['00', '01', '02', '03', '04', '05', '06', '07', '08', '09', '10', '11', '12', '13', '14', '15', '16', '17', '18', '19', '20', '21', '22', '23', '24', '25', '26', '27', '28', '29'], pkg_numbers) + + def test_group_read_p2(self): + res = self.app.get(url_for(controller='group', action='read', id='group_00', page=2)) + assert 'href="/group/group_00?page=1' in res + pkg_numbers = scrape_search_results(res, 'dataset') + assert_equal(['30', '31', '32', '33', '34', '35', '36', '37', '38', '39', '40', '41', '42', '43', '44', '45', '46', '47', '48', '49', '50'], pkg_numbers) + +class TestPaginationGroup(TestController): + @classmethod + def setup_class(cls): + # no. entities per page is hardcoded into the controllers, so + # create enough of each here so that we can test pagination + cls.num_groups = 21 + + groups = [u'group_%s' % str(i).zfill(2) for i in range(0, cls.num_groups)] + + CreateTestData.create_arbitrary( + [], extra_group_names=groups + ) + + @classmethod + def teardown_class(self): + model.repo.rebuild_db() def test_group_index(self): res = self.app.get(url_for(controller='group', action='index')) - assert 'href="/group?page=2"' in res - assert 'href="/group/group_19"' in res + assert 'href="/group?page=2"' in res, res + grp_numbers = scrape_search_results(res, 'group') + assert_equal(['00', '01', '02', '03', '04', '05', '06', '07', '08', '09', '10', '11', '12', '13', '14', '15', '16', '17', '18', '19'], grp_numbers) res = self.app.get(url_for(controller='group', action='index', page=2)) assert 'href="/group?page=1"' in res - assert 'href="/group/group_20"' in res + grp_numbers = scrape_search_results(res, 'group') + assert_equal(['20'], grp_numbers) - def test_group_read(self): - res = self.app.get(url_for(controller='group', action='read', id='group_00')) - assert 'href="/group/group_00?page=2' in res - assert 'href="/dataset/package_29"' in res +class TestPaginationUsers(TestController): + @classmethod + def setup_class(cls): + # Delete default user as it appears in the first page of results + model.User.by_name(u'logged_in').purge() + model.repo.commit_and_remove() - res = self.app.get(url_for(controller='group', action='read', id='group_00', page=2)) - assert 'href="/group/group_00?page=1' in res - assert 'href="/dataset/package_30"' in res + # no. entities per page is hardcoded into the controllers, so + # create enough of each here so that we can test pagination + cls.num_users = 21 + + users = [u'user_%s' % str(i).zfill(2) for i in range(cls.num_users)] + + CreateTestData.create_arbitrary( + [], extra_user_names = users, + ) + + @classmethod + def teardown_class(self): + model.repo.rebuild_db() def test_users_index(self): # allow for 2 extra users shown on user listing, 'logged_in' and 'visitor' res = self.app.get(url_for(controller='user', action='index')) - assert 'href="/user/user_18"' in res assert 'href="/user?q=&order_by=name&page=2"' in res + user_numbers = scrape_search_results(res, 'user') + assert_equal(['00', '01', '02', '03', '04', '05', '06', '07', '08', '09', '10', '11', '12', '13', '14', '15', '16', '17', '18', '19'], user_numbers) res = self.app.get(url_for(controller='user', action='index', page=2)) - assert 'href="/user/user_20"' in res assert 'href="/user?q=&order_by=name&page=1"' in res + user_numbers = scrape_search_results(res, 'user') + assert_equal(['20'], user_numbers) From c1042e9df5749dfaddbcdae46bb3fc8f86d89125 Mon Sep 17 00:00:00 2001 From: David Read Date: Thu, 19 Jan 2012 11:46:03 +0000 Subject: [PATCH 43/55] [#1683][lib] Workaround for SOLR 1.4 ordering problem. --- ckan/lib/search/query.py | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/ckan/lib/search/query.py b/ckan/lib/search/query.py index 73a31107544..0cd266304ad 100644 --- a/ckan/lib/search/query.py +++ b/ckan/lib/search/query.py @@ -247,7 +247,14 @@ def run(self, query): query['q'] = "*:*" # number of results - query['rows'] = min(1000, int(query.get('rows', 10))) + rows_to_return = min(1000, int(query.get('rows', 10))) + if rows_to_return > 0: + # #1683 Work around problem of last result being out of order + # in SOLR 1.4 + rows_to_query = rows_to_return + 1 + else: + rows_to_query = rows_to_return + query['rows'] = rows_to_query # order by score if no 'sort' term given order_by = query.get('sort') @@ -297,6 +304,9 @@ def run(self, query): self.count = response.get('numFound', 0) self.results = response.get('docs', []) + # #1683 Filter out the last row that is sometimes out of order + self.results = self.results[:rows_to_return] + # get any extras and add to 'extras' dict for result in self.results: extra_keys = filter(lambda x: x.startswith('extras_'), result.keys()) From ecfe0598311a458b7399fde9b52a126e339a8ff7 Mon Sep 17 00:00:00 2001 From: Ross Jones Date: Thu, 19 Jan 2012 12:23:08 +0000 Subject: [PATCH 44/55] Added configuration settings for storage --- doc/configuration.rst | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/doc/configuration.rst b/doc/configuration.rst index b69c91b02b4..0d3642a2440 100644 --- a/doc/configuration.rst +++ b/doc/configuration.rst @@ -131,6 +131,8 @@ And there is an option for the default expiry time if not specified:: ckan.cache.default_expires = 600 + + Authentication Settings ----------------------- @@ -206,6 +208,35 @@ Default value: (none) If you want to specify the ordering of all or some of the locales as they are offered to the user, then specify them here in the required order. Any locales that are available but not specified in this option, will still be offered at the end of the list. +Storage Settings +---------------- + +.. index:: + single: storage.bucket, storage.directory + +storage.bucket +^^^^^^^^^^^^^^ + +Example:: + + storage.bucket = ckan + +Default value: ``None`` + +This setting will change the bucket name for the uploaded files. + +storage.directory +^^^^^^^^^^^^^^^^^ + +Example:: + + storage.directory = /data/uploads/ + +Default value: ``None`` + +Use this to specify where uploaded files should be stored, and also to turn on the handling of file storage. The folder should exist, and will automatically be turned into a valid pairtree repository if it is not already. + + Theming Settings ---------------- From 4369a1f4ed8f0eceed58a750441dda3502983a4c Mon Sep 17 00:00:00 2001 From: David Read Date: Thu, 19 Jan 2012 12:25:11 +0000 Subject: [PATCH 45/55] [master,noticket][tests][xs]: Make test tidy up - was causing downstream test failures. --- ckan/tests/lib/test_cli.py | 4 ++++ ckan/tests/lib/test_dictization.py | 5 ----- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/ckan/tests/lib/test_cli.py b/ckan/tests/lib/test_cli.py index f6e383f0c78..4adda216572 100644 --- a/ckan/tests/lib/test_cli.py +++ b/ckan/tests/lib/test_cli.py @@ -19,6 +19,10 @@ def setup_class(cls): model.Package.by_name(u'warandpeace').delete() model.repo.commit_and_remove() + @classmethod + def teardown_class(cls): + model.repo.rebuild_db() + def test_simple_dump_csv(self): csv_filepath = '/tmp/dump.tmp' self.db.args = ('simple-dump-csv %s' % csv_filepath).split() diff --git a/ckan/tests/lib/test_dictization.py b/ckan/tests/lib/test_dictization.py index 9d594bba9e4..e608f3db85c 100644 --- a/ckan/tests/lib/test_dictization.py +++ b/ckan/tests/lib/test_dictization.py @@ -103,11 +103,6 @@ def teardown_class(cls): model.repo.rebuild_db() model.Session.remove() - def teardonwn(self): - model.Session.remove() - - - def remove_changable_columns(self, dict): for key, value in dict.items(): if key.endswith('id') and key <> 'license_id': From a3df67512e7e7a36b613fcfb0e820e9182d092e9 Mon Sep 17 00:00:00 2001 From: David Read Date: Thu, 19 Jan 2012 13:12:04 +0000 Subject: [PATCH 46/55] [master][noticket]: Removed long deprecated date format functions. --- ckan/model/__init__.py | 20 -------------------- 1 file changed, 20 deletions(-) diff --git a/ckan/model/__init__.py b/ckan/model/__init__.py index b94e940e5d2..cd0ba3f03c1 100644 --- a/ckan/model/__init__.py +++ b/ckan/model/__init__.py @@ -284,26 +284,6 @@ def _get_revision_user(self): Revision.groups = property(_get_groups) Revision.user = property(_get_revision_user) -def strptimestamp(s): - '''Convert a string of an ISO date into a datetime.datetime object. - - raises TypeError if the number of numbers in the string is not between 3 - and 7 (see datetime constructor). - raises ValueError if any of the numbers are out of range. - ''' - # TODO: METHOD DEPRECATED - use ckan.lib.helpers.date_str_to_datetime - log.warn('model.strptimestamp is deprecated - use ckan.lib.helpers.date_str_to_datetime instead') - import datetime, re - return datetime.datetime(*map(int, re.split('[^\d]', s))) - -def strftimestamp(t): - '''Takes a datetime.datetime and returns it as an ISO string. For - a pretty printed string, use ckan.lib.helpers.render_datetime. - ''' - # TODO: METHOD DEPRECATED - use ckan.lib.helpers.datetime_to_date_str - log.warn('model.strftimestamp is deprecated - use ckan.lib.helpers.datetime_to_date_str instead') - return t.isoformat() - def revision_as_dict(revision, include_packages=True, include_groups=True,ref_package_by='name'): revision_dict = OrderedDict(( ('id', revision.id), From 8b55590ced6c8fbf5eb40284e5459c3c55e693ee Mon Sep 17 00:00:00 2001 From: Ross Jones Date: Thu, 19 Jan 2012 13:49:46 +0000 Subject: [PATCH 47/55] Added file-upload documentation with basic outline of what is provided and added to index --- doc/file-upload.rst | 19 +++++++++++++++++++ doc/index.rst | 1 + 2 files changed, 20 insertions(+) create mode 100644 doc/file-upload.rst diff --git a/doc/file-upload.rst b/doc/file-upload.rst new file mode 100644 index 00000000000..f085ff84577 --- /dev/null +++ b/doc/file-upload.rst @@ -0,0 +1,19 @@ +============ +File uploads +============ + +CKAN allows users to upload files directly to file storage on the CKAN server. The uploaded files will be stored in the configured location. + +The important settings for the CKAN .ini file are + +:: + + storage.bucket = ckan + storage.directory = /data/uploads/ + +The directory where files will be stored should exist or be created before the system is used. + +It is also possible to have uploaded CSV and Excel files stored in the Webstore which provides a structured data store built on a relational database backend. The configuration of this process is described at `the CKAN wiki `_. + +Storing data in the webstore allows for the direct retrieval of the data in a tabular format. It is possible to fetch a single row of the data, all of the data and have it returned in HTML, CSV or JSON format. More information and the API documentation for the webstore is available at `read the docs +`_. \ No newline at end of file diff --git a/doc/index.rst b/doc/index.rst index a5539df13c0..57669fece51 100644 --- a/doc/index.rst +++ b/doc/index.rst @@ -31,6 +31,7 @@ Contents: database_dumps upgrade i18n + file-upload configuration api test From ebbdcff8db31855f4868bfc71dcd3e27a3268c0d Mon Sep 17 00:00:00 2001 From: David Read Date: Fri, 20 Jan 2012 10:53:24 +0000 Subject: [PATCH 48/55] [master][noticket][nose plugin][xs]: Change Nose default so that it does not print docstring when test fails. Otherwise, use --docstrings option. --- ckan/ckan_nose_plugin.py | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/ckan/ckan_nose_plugin.py b/ckan/ckan_nose_plugin.py index 785d3543def..0666b7761c3 100644 --- a/ckan/ckan_nose_plugin.py +++ b/ckan/ckan_nose_plugin.py @@ -47,10 +47,21 @@ def options(self, parser, env): '--ckan-migration', action='store_true', dest='ckan_migration', - help='set this when wanting to test migrations') + help='set this when wanting to test migrations') + parser.add_option( + '--docstrings', + action='store_true', + dest='docstrings', + help='set this to display test docstrings instead of module names') def configure(self, settings, config): CkanNose.settings = settings if settings.is_ckan: self.enabled = True self.is_first_test = True + + def describeTest(self, test): + if not CkanNose.settings.docstrings: + # display module name instead of docstring + return False + From 8cc0dc22692088cb534af5d35e108a5bd115ebde Mon Sep 17 00:00:00 2001 From: David Read Date: Fri, 20 Jan 2012 10:54:28 +0000 Subject: [PATCH 49/55] [master][noticket][doc]: Install tips improved. --- doc/common-error-messages.rst | 4 ++++ doc/install-from-package.rst | 2 +- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/doc/common-error-messages.rst b/doc/common-error-messages.rst index a072112f056..e453daddca4 100644 --- a/doc/common-error-messages.rst +++ b/doc/common-error-messages.rst @@ -129,3 +129,7 @@ This occurs when installing CKAN source to a virtual environment when using an o This occurs when upgrading to CKAN 1.5.1 with a database with duplicate user names. See :ref:`upgrading` +``ERROR: must be member of role "okfn"`` & ``WARNING: no privileges could be revoked for "public"`` +===================================================================================================== + +These are seen when loading a CKAN database from another machine. It is the result of the database tables being owned by a user that doesn't exist on the new machine. The owner of the table is not important, so this error is harmless and can be ignored. \ No newline at end of file diff --git a/doc/install-from-package.rst b/doc/install-from-package.rst index a6e927fecda..17b98a483ee 100644 --- a/doc/install-from-package.rst +++ b/doc/install-from-package.rst @@ -721,7 +721,7 @@ Now you need to make some manual changes. In the following commands replace ``st If you get error ``sqlalchemy.exc.IntegrityError: (IntegrityError) could not create unique index "user_name_key`` then you need to rename users with duplicate names before it will work. For example:: sudo -u ckanstd paster --plugin=pylons shell /etc/ckan/std/std.ini - model.meta.engine.execute('SELECT name, count(name) AS NumOccurrences FROM "user" GROUP BY name HAVING(COUNT(name)>0);').fetchall() + model.meta.engine.execute('SELECT name, count(name) AS NumOccurrences FROM "user" GROUP BY name HAVING(COUNT(name)>1);').fetchall() users = model.Session.query(model.User).filter_by(name='https://www.google.com/accounts/o8/id?id=ABCDEF').all() users[1].name = users[1].name[:-1] model.repo.commit_and_remove() From cbf2243a5a631024710f586130a4cac9bede0c17 Mon Sep 17 00:00:00 2001 From: Ross Jones Date: Fri, 20 Jan 2012 12:11:13 +0000 Subject: [PATCH 50/55] Fixing up tests after merge conflicts --- ckan/lib/dictization/model_dictize.py | 2 +- ckan/tests/lib/test_dictization_schema.py | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/ckan/lib/dictization/model_dictize.py b/ckan/lib/dictization/model_dictize.py index 5172615ee00..7a9a71c79c0 100644 --- a/ckan/lib/dictization/model_dictize.py +++ b/ckan/lib/dictization/model_dictize.py @@ -192,7 +192,7 @@ def package_dictize(pkg, context): result_dict['isopen'] = pkg.isopen if isinstance(pkg.isopen,bool) else pkg.isopen() # type - result_dict['type']= pkg_object.type + result_dict['type']= pkg.type # creation and modification date result_dict['metadata_modified'] = pkg.metadata_modified.isoformat() \ diff --git a/ckan/tests/lib/test_dictization_schema.py b/ckan/tests/lib/test_dictization_schema.py index abb7d828160..2cac490b366 100644 --- a/ckan/tests/lib/test_dictization_schema.py +++ b/ckan/tests/lib/test_dictization_schema.py @@ -141,6 +141,7 @@ def test_2_group_schema(self): expected = {'description': u'These are books that David likes.', 'id': group.id, 'name': u'david', + 'type': u'group', 'packages': sorted([{'id': group_pack[0].id}, {'id': group_pack[1].id, }], key=lambda x:x["id"]), From 57bdf0eefdd61347503e06b747b23c52ea8af0cf Mon Sep 17 00:00:00 2001 From: Ross Jones Date: Fri, 20 Jan 2012 12:30:00 +0000 Subject: [PATCH 51/55] Updated migration to update old data --- ckan/logic/schema.py | 2 +- .../versions/047_rename_package_group_member.py | 8 ++++++++ notes.txt | 12 ------------ 3 files changed, 9 insertions(+), 13 deletions(-) delete mode 100644 notes.txt diff --git a/ckan/logic/schema.py b/ckan/logic/schema.py index 1722b1c05a6..3142df75a95 100644 --- a/ckan/logic/schema.py +++ b/ckan/logic/schema.py @@ -142,7 +142,7 @@ def package_form_schema(): schema['extras_validation'] = [duplicate_extras_key, ignore] schema['save'] = [ignore] schema['return_to'] = [ignore] - schema['type'] = [ignore, unicode] + schema['type'] = [ignore_missing, unicode] ##changes schema.pop("id") diff --git a/ckan/migration/versions/047_rename_package_group_member.py b/ckan/migration/versions/047_rename_package_group_member.py index dd3bca18c27..49f7d811b40 100644 --- a/ckan/migration/versions/047_rename_package_group_member.py +++ b/ckan/migration/versions/047_rename_package_group_member.py @@ -101,6 +101,14 @@ def upgrade(migrate_engine): ADD COLUMN "type" text; ALTER TABLE "package_revision" ADD COLUMN "type" text; + +update "package" set type = 'package'; +update package_revision set type = 'package'; + +ALTER TABLE "package" + ADD COLUMN "type" set not null; +ALTER TABLE "package_revision" + ADD COLUMN "type" set not null; COMMIT; ''' diff --git a/notes.txt b/notes.txt deleted file mode 100644 index c307d2508de..00000000000 --- a/notes.txt +++ /dev/null @@ -1,12 +0,0 @@ -Notes for per-package-type behaviour. - - - I don't really know where the best place to create the mappings from - package-types to plugin instances is. At the moment, I'm creating the - mapping just prior to the package routing stuff in routing.py. - - - I've assumed package-type is a string. If it's not then only the docs - need to change (as long as it's some hashable value anyway). - - - Running the tests seems to be involing the routing.py more than once. I - haven't looked into why this is. Running the server seems to work fine - thuough. From 42be122bfd1e1411837da266e160bc5a9f7450f7 Mon Sep 17 00:00:00 2001 From: Ross Jones Date: Fri, 20 Jan 2012 13:01:38 +0000 Subject: [PATCH 52/55] [xs] change to the migration for existing data --- ckan/migration/versions/047_rename_package_group_member.py | 7 ------- 1 file changed, 7 deletions(-) diff --git a/ckan/migration/versions/047_rename_package_group_member.py b/ckan/migration/versions/047_rename_package_group_member.py index 49f7d811b40..f277762a0dd 100644 --- a/ckan/migration/versions/047_rename_package_group_member.py +++ b/ckan/migration/versions/047_rename_package_group_member.py @@ -102,13 +102,6 @@ def upgrade(migrate_engine): ALTER TABLE "package_revision" ADD COLUMN "type" text; -update "package" set type = 'package'; -update package_revision set type = 'package'; - -ALTER TABLE "package" - ADD COLUMN "type" set not null; -ALTER TABLE "package_revision" - ADD COLUMN "type" set not null; COMMIT; ''' From 2983d249b46b3bb9beec89fab23710827b9dd4a7 Mon Sep 17 00:00:00 2001 From: Ross Jones Date: Fri, 20 Jan 2012 13:12:31 +0000 Subject: [PATCH 53/55] [xs] Small fix to the storage test --- ckan/tests/functional/test_storage.py | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/ckan/tests/functional/test_storage.py b/ckan/tests/functional/test_storage.py index 714090fdd41..0b7b24e3351 100644 --- a/ckan/tests/functional/test_storage.py +++ b/ckan/tests/functional/test_storage.py @@ -10,9 +10,7 @@ class TestStorageAPIController: @classmethod def setup_class(cls): config = appconfig('config:test.ini', relative_to=conf_dir) - config.local_conf['ckan.plugins'] = 'storage' - config.local_conf['ofs.impl'] = 'pairtree' - config.local_conf['ofs.storage_dir'] = '/tmp/ckan-test-ckanext-storage' + config.local_conf['storage.directory'] = '/tmp/' wsgiapp = make_app(config.global_conf, **config.local_conf) cls.app = paste.fixture.TestApp(wsgiapp) @@ -30,9 +28,7 @@ class TestStorageAPIControllerLocal: @classmethod def setup_class(cls): config = appconfig('config:test.ini', relative_to=conf_dir) - config.local_conf['ckan.plugins'] = 'storage' - config.local_conf['ofs.impl'] = 'pairtree' - config.local_conf['ofs.storage_dir'] = '/tmp/ckan-test-ckanext-storage' + config.local_conf['storage.directory'] = '/tmp/' wsgiapp = make_app(config.global_conf, **config.local_conf) cls.app = paste.fixture.TestApp(wsgiapp) CreateTestData.create() From 7bcadaf1761466fcb1ecf2d59e22ddb807dff37e Mon Sep 17 00:00:00 2001 From: Ross Jones Date: Fri, 20 Jan 2012 13:30:27 +0000 Subject: [PATCH 54/55] [1690] Fixed the naming of the storage settings --- ckan/controllers/storage.py | 8 ++++---- ckan/tests/functional/test_storage.py | 4 ++-- doc/configuration.rst | 14 +++++++------- doc/file-upload.rst | 4 ++-- 4 files changed, 15 insertions(+), 15 deletions(-) diff --git a/ckan/controllers/storage.py b/ckan/controllers/storage.py index 05c2140a5e6..35aba2822da 100644 --- a/ckan/controllers/storage.py +++ b/ckan/controllers/storage.py @@ -31,9 +31,9 @@ # pairtree_version0_1 file for identifying folders -BUCKET = config['storage.bucket'] -key_prefix = config.get('storage.key_prefix', 'file/') -storage_dir = config.get('storage.directory', '') +BUCKET = config.get('ckan.storage.bucket', 'default') +key_prefix = config.get('ckan.storage.key_prefix', 'file/') +storage_dir = config.get('ckan.storage.directory', '') _eq_re = re.compile(r"^(.*)(=[0-9]*)$") def fix_stupid_pylons_encoding(data): @@ -326,7 +326,7 @@ def auth_request(self, label): def _get_remote_form_data(self, label): method = 'POST' content_length_range = int( - config.get('ckanext.storage.max_content_length', + config.get('ckan.storage.max_content_length', 50000000)) acl = 'public-read' fields = [ { diff --git a/ckan/tests/functional/test_storage.py b/ckan/tests/functional/test_storage.py index 0b7b24e3351..bed299334e4 100644 --- a/ckan/tests/functional/test_storage.py +++ b/ckan/tests/functional/test_storage.py @@ -10,7 +10,7 @@ class TestStorageAPIController: @classmethod def setup_class(cls): config = appconfig('config:test.ini', relative_to=conf_dir) - config.local_conf['storage.directory'] = '/tmp/' + config.local_conf['ckan.storage.directory'] = '/tmp/' wsgiapp = make_app(config.global_conf, **config.local_conf) cls.app = paste.fixture.TestApp(wsgiapp) @@ -28,7 +28,7 @@ class TestStorageAPIControllerLocal: @classmethod def setup_class(cls): config = appconfig('config:test.ini', relative_to=conf_dir) - config.local_conf['storage.directory'] = '/tmp/' + config.local_conf['ckan.storage.directory'] = '/tmp/' wsgiapp = make_app(config.global_conf, **config.local_conf) cls.app = paste.fixture.TestApp(wsgiapp) CreateTestData.create() diff --git a/doc/configuration.rst b/doc/configuration.rst index 26e980f2a8e..63c2d93084f 100644 --- a/doc/configuration.rst +++ b/doc/configuration.rst @@ -248,25 +248,25 @@ Storage Settings ---------------- .. index:: - single: storage.bucket, storage.directory + single: ckan.storage.bucket, ckan.storage.directory -storage.bucket -^^^^^^^^^^^^^^ +ckan.storage.bucket +^^^^^^^^^^^^^^^^^^^ Example:: - storage.bucket = ckan + ckan.storage.bucket = ckan Default value: ``None`` This setting will change the bucket name for the uploaded files. -storage.directory -^^^^^^^^^^^^^^^^^ +ckan.storage.directory +^^^^^^^^^^^^^^^^^^^^^^ Example:: - storage.directory = /data/uploads/ + ckan.storage.directory = /data/uploads/ Default value: ``None`` diff --git a/doc/file-upload.rst b/doc/file-upload.rst index f085ff84577..f093b1f2bbc 100644 --- a/doc/file-upload.rst +++ b/doc/file-upload.rst @@ -8,8 +8,8 @@ The important settings for the CKAN .ini file are :: - storage.bucket = ckan - storage.directory = /data/uploads/ + ckan.storage.bucket = ckan + ckan.storage.directory = /data/uploads/ The directory where files will be stored should exist or be created before the system is used. From bda3142f5ba2e23bf787f04dba6f7d993388a3df Mon Sep 17 00:00:00 2001 From: Ross Jones Date: Fri, 20 Jan 2012 14:59:01 +0000 Subject: [PATCH 55/55] [xs] Minor change to upload test --- ckan/tests/functional/test_upload.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ckan/tests/functional/test_upload.py b/ckan/tests/functional/test_upload.py index 8d374b101c1..0c25e4b5efa 100644 --- a/ckan/tests/functional/test_upload.py +++ b/ckan/tests/functional/test_upload.py @@ -11,7 +11,7 @@ class TestStorageController: @classmethod def setup_class(cls): config = appconfig('config:test.ini', relative_to=conf_dir) - config.local_conf['ckan.plugins'] = 'storage' + config.local_conf['ckan.storage.directory'] = '/tmp' wsgiapp = make_app(config.global_conf, **config.local_conf) cls.app = paste.fixture.TestApp(wsgiapp) CreateTestData.create()