[SVCS-486] Add Cloudfiles Provider #283

Johnetordoff · 2017-10-12T19:58:14Z

Ticket

https://openscience.atlassian.net/browse/SVCS-486

Purpose

Expand the cloudfiles provider to make more like a real provider,

Changes

Adds upload, download, fetch metadata, move, copy, folders creation etc to the Cloudfiles provider, also includes unit tests and moves fixtures to new file. Changes some methods to support unicode file names.

Side effects

None that I know of.

QA Notes

Should include testing for special characters in filenames.

Deployment Notes

This should be deployed be for the corrosponding osf.io PR, CenterForOpenScience/osf.io#7781

…isions etc.

…erbutler into cloudfiles * 'develop' of https://github.com/CenterForOpenScience/waterbutler: Add PR template.

coveralls · 2017-10-13T16:38:57Z

Coverage increased (+0.2%) to 89.217% when pulling 7a759d0 on Johnetordoff:cloudfiles into 481c9d9 on CenterForOpenScience:develop.

coveralls · 2017-10-17T18:26:43Z

Coverage increased (+0.2%) to 89.217% when pulling 9fef6c6 on Johnetordoff:cloudfiles into 481c9d9 on CenterForOpenScience:develop.

felliott

This is a light pass, I will do a more thorough one once I've got my local docker set up. The thing about adding links to the API docs in the method docstrings should be done for all methods. It's a huge time-saver when trying to debug. I've started adding them as I go, and this would be a great chance to fill out RsCf.

felliott · 2017-10-19T19:25:20Z

waterbutler/providers/cloudfiles/provider.py

@@ -32,7 +35,8 @@ def ensure_connection(func):
 class CloudFilesProvider(provider.BaseProvider):
    """Provider for Rackspace CloudFiles.

-    API Docs: https://developer.rackspace.com/docs/cloud-files/v1/developer-guide/#document-developer-guide
+    API Docs: https://developer.rackspace.com/docs/cloud-files/v1/developer-guide/
+    #document-developer-guide


Nitpick: you don't need to wrap urls in docstrings. If flake complains about them, we can append a # noqa: E501 to it.

felliott · 2017-10-19T19:26:40Z

waterbutler/providers/cloudfiles/metadata.py


    @property
    def extra(self):
        return {
            'hashes': {
-                'md5': self.raw['etag'].replace('"', ''),
+                'md5': self.raw['ETAG'],


Did Cloudfiles stop wrapping their etags in double-quotes? If so, they need to be added for the etag property. etag should have them, hashes.md5 shouldn't.

Should quotes be added? They don't appear naturally as part of the metadata response.

The etag will only be quoted if it is a manifest object over 5G

See response -> etag -> description in https://developer.rackspace.com/docs/cloud-files/v1/storage-api-reference/object-services-operations/
and
https://developer.rackspace.com/docs/cloud-files/v1/use-cases/additional-object-services-information/#creating-large-objects

felliott · 2017-10-19T19:27:53Z

waterbutler/providers/cloudfiles/provider.py

@@ -211,18 +232,23 @@ def sign_url(self, path, method='GET', endpoint=None, seconds=settings.TEMP_URL_
        :param int seconds: Time for the url to live
        :rtype str:
        """
+        from urllib.parse import unquote


Move to top of file.

felliott · 2017-10-19T19:29:12Z

waterbutler/providers/cloudfiles/provider.py

+
+    @ensure_connection
+    async def create_folder(self, path, folder_precheck=None):
+


Please add a docstring with a link to the API docs for the endpoint this uses.

felliott · 2017-10-19T19:30:11Z

waterbutler/providers/cloudfiles/provider.py

+
+    @ensure_connection
+    async def revisions(self, path):
+        version_location = await self._get_version_location()


Please add a docstring w/ url to API docs.

felliott · 2017-10-19T21:00:24Z

waterbutler/providers/cloudfiles/provider.py

        """Deletes the key at the specified path
        :param str path: The path of the key to delete
-        :rtype ResponseStreamReader:
+        :param bool confirm_delete: not used in this provider


What happens if the user tries to DELETE /resources/mst3k/providers/cloudfiles/? The user should receive a warning that deleting everything in the root folder is only allowed if confirm_delete=1 is given. It also should not attempt to delete the actual root folder. I don't know how that would play out on S3, but on a dropbox-like provider, that would break the addon as the user would no-longer be connected to their storage root

felliott · 2017-10-19T21:09:26Z

waterbutler/providers/cloudfiles/provider.py

+        )
+        json_resp = await resp.json()
+        current = (await self.metadata(path)).to_revision()
+        return [current] + [CloudFilesRevisonMetadata(revision_data) for revision_data in json_resp]


Does this response not include the metadata for the latest revision? If so, that's worthy of a comment.

I added the comment in the docstring.

felliott · 2017-10-19T21:13:40Z

tests/providers/cloudfiles/fixtures.py

+import json
+import pytest
+import aiohttp
+import aiohttpretty


Nitpick: pytest, aiohttp* are third-party and should be separated from the core libs. unittest is a core lib.

coveralls · 2017-10-20T19:31:11Z

Coverage increased (+0.4%) to 89.402% when pulling 03442e3 on Johnetordoff:cloudfiles into 481c9d9 on CenterForOpenScience:develop.

coveralls · 2017-10-20T20:35:11Z

Coverage increased (+0.4%) to 89.388% when pulling 87e5fca on Johnetordoff:cloudfiles into 481c9d9 on CenterForOpenScience:develop.

coveralls · 2017-10-24T19:37:05Z

Coverage increased (+0.2%) to 89.388% when pulling eaab942 on Johnetordoff:cloudfiles into cc68aca on CenterForOpenScience:develop.

coveralls · 2017-11-07T16:45:32Z

Coverage increased (+0.2%) to 89.305% when pulling a48290b on Johnetordoff:cloudfiles into ba55731 on CenterForOpenScience:develop.

AddisonSchiller

1st pass, will for sure need to do another large review after these changes.
Lot of style changes and other things, here goes:

Lots of extra blank lines to be fixed, especially in fixtures/tests
other style changes in a few places (import orders etc)
unused variable body in a test
A few exception raising tests have no real tests beyond the exception raising. Suggestion: consider adding a few for robustness
5.Cloud files api link doesn't work correctly
Throughout the Cloudfiles.provider, update docstrings to reflect changes (I noted a few but there are probably more). Suggestion: Add default variable typing and default return typing where appropriate.
Questions: Why are you removing **kwargs all over the place? Most providers seem to just leave it even if it isn't used.
delete needs changes. It should default to 0, and look for confirm_delete == 1 rather than not confirm_delete also deleting a root should follow special steps over regular deleting. See how other providers handle it. How you have it set up, the actual root will be deleted (I think). It should just delete the contents of the root.
in quite a few places, you call _metadata_<helper_function> over just calling metadata for style and consitency reasons, you should always be calling just metadata
path_str variable is only used once. Just replace it with its value, path.path
Move functions around, specifically revisions and create_folder. They are in the middle of the _<helper_function>'s and should be more near metadata
conflict checking on create_folder. Other providers check for a 409 and respond accordingly
the _get_version_location error has a link in it. Unsure if that is kosher.
get_metadata_revision should be named in the form of _metadata_revision. Similar to the other metadata() helper functions

AddisonSchiller · 2017-11-15T17:40:55Z

tests/providers/cloudfiles/fixtures.py

+from waterbutler.providers.cloudfiles import CloudFilesProvider
+
+
+


Extra blank line. Should be 2 instead of 3.

AddisonSchiller · 2017-11-15T17:42:15Z

tests/providers/cloudfiles/fixtures.py

+    return CloudFilesProvider(auth, credentials, settings)
+
+
+


Extra blank line. Should be 2 instead of 3.

AddisonSchiller · 2017-11-15T17:42:33Z

tests/providers/cloudfiles/fixtures.py

+    return streams.FileStreamReader(file_like)
+
+
+


Extra blank line. Should be 2 instead of 3.

AddisonSchiller · 2017-11-15T17:43:17Z

tests/providers/cloudfiles/fixtures.py

+@pytest.fixture
+def folder_root_empty():
+    return []
+


Needs another blank line. Should be 2 instead of 1.

AddisonSchiller · 2017-11-15T17:43:39Z

tests/providers/cloudfiles/fixtures.py

+def container_header_metadata_without_verision_location():
+    with open(os.path.join(os.path.dirname(__file__), 'fixtures/fixtures.json'), 'r') as fp:
+        return json.load(fp)['container_header_metadata_without_verision_location']
+


Needs another blank line. Should be 2 instead of 1.

AddisonSchiller · 2017-11-15T19:10:32Z

waterbutler/providers/cloudfiles/provider.py

+                                           'versions follow the instructions here: '
+                                           'https://developer.rackspace.com/docs/cloud-files/v1/'
+                                           'use-cases/additional-object-services-information/'
+                                           '#object-versioning')


Might not be the best to add links in error messages. They get outdated, etc.
@felliott ?

Spoke to @felliott about this. My understanding is that for now, we want to treat this more like other providers. If they don't have versioning set up, let's keep it simple. No error message, let's just not show versions, only current.

AddisonSchiller · 2017-11-15T19:12:24Z

waterbutler/providers/cloudfiles/provider.py

+    @ensure_connection
+    async def revisions(self, path):
+        """Get past versions of the request file from special user designated version container,
+        if the user hasn't designated a version_location container in raises an infomative error


"in raises" -> "it raises"

also revisions should probably be moved next to metadata, and not in the middle of the _<helper_function>'s

AddisonSchiller · 2017-11-15T19:13:40Z

waterbutler/providers/cloudfiles/provider.py

+        the revision list after other revisions are returned. More info about versioning with Cloud
+        Files here:
+
+        https://developer.rackspace.com/docs/cloud-files/v1/use-cases/additional-object-services-information/#object-versioning


line length. Although idk how to fix line length in a docstring

AddisonSchiller · 2017-11-15T19:17:09Z

waterbutler/providers/cloudfiles/provider.py

+        return [current] + [CloudFilesRevisonMetadata(revision_data) for revision_data in json_resp]
+
+    @ensure_connection
+    async def get_metadata_revision(self, path, version=None, revision=None):


This function should follow the same naming scheme as the other metadata helper functions:
eg: _metadata_<for some thing>
so this should be called _metadata_revision

AddisonSchiller · 2017-11-15T19:21:27Z

waterbutler/providers/cloudfiles/provider.py

        if data.get('subdir'):
            return CloudFilesFolderMetadata(data)
        elif data['content_type'] == 'application/directory':
            return CloudFilesFolderMetadata({'subdir': data['name'] + '/'})
        return CloudFilesFileMetadata(data)
+
+    @ensure_connection
+    async def create_folder(self, path, **kwargs):


create_folder should probably be moved so it is not in the middle of the _<helper_function>'s

…erbutler into cloudfiles * 'develop' of https://github.com/CenterForOpenScience/waterbutler: (77 commits) final cleanups for onedrive make provider readonly start cleaning up and reorganizing onedrive implement Microsoft OneDrive provider using dropbox as base added onedrive provider -- copied from Dropbox don't assume file metadata has a modified/created date add mypy type annotations add modified/created dates to file metadata expand docstrings add ruby serialization workaround to download clean up commit_sha vs. branch_name handling add tests for revisions and uninit-ed repos add artificial test for missing folder remove unused and obsolete code rewrite test suite for provider changes and coverage update some more docstrings on the provider GL provider is read-only: folder creation is not allowed add workaround for non-existent directories not being reported in GL document workarounds in download & _fetch_file_contents remove unneeded error wrapping from metadata ...

coveralls · 2017-11-15T23:50:35Z

Coverage increased (+0.2%) to 90.106% when pulling f7a3f60 on Johnetordoff:cloudfiles into 26bf209 on CenterForOpenScience:develop.

into cloudfiles * 'cloudfiles' of https://github.com/johnetordoff/waterbutler:

coveralls · 2017-11-16T15:53:12Z

Coverage increased (+0.2%) to 90.104% when pulling 3e62ede on Johnetordoff:cloudfiles into 26bf209 on CenterForOpenScience:develop.

coveralls · 2017-11-17T15:15:00Z

Coverage increased (+0.2%) to 90.104% when pulling e4ad0a9 on Johnetordoff:cloudfiles into 26bf209 on CenterForOpenScience:develop.

…dfiles provider.

coveralls · 2017-11-27T18:20:38Z

Coverage increased (+0.2%) to 90.127% when pulling 64c9a23 on Johnetordoff:cloudfiles into 26bf209 on CenterForOpenScience:develop.

coveralls · 2017-12-05T01:25:10Z

Coverage increased (+0.05%) to 89.997% when pulling 9a58bed on Johnetordoff:cloudfiles into 26bf209 on CenterForOpenScience:develop.

TomBaxter

A few things to look at here. My notes/questions about revisions probably aren't actionable. Made a few comments about _delete_folder_contents . In addition to those comments, I tested deleting root and it returned 204 success, but it didn't actually delete the contents of root. Shaping up nicely, back to you.

TomBaxter · 2018-01-03T16:16:45Z

waterbutler/providers/cloudfiles/provider.py

        """Deletes the key at the specified path
        :param str path: The path of the key to delete
-        :rtype ResponseStreamReader:
+        :param int confirm_delete: Must be 1 to confirm root folder delete, this deletes entire
+        container object.


Will this delete container object? Folder deletes against root folder should delete the contents of the folder/container, but not the folder/container itself.

TomBaxter · 2018-01-03T16:19:13Z

waterbutler/providers/cloudfiles/provider.py

+
+        if path.is_root and not confirm_delete:
+            raise exceptions.DeleteError(
+                'query argument confirm_delete=1 is required for deleting the entire container.',


As above, delete root folder shouldn't actually delete folder, only contents.

TomBaxter · 2018-01-03T16:26:46Z

waterbutler/providers/cloudfiles/provider.py

            )
+
+        if path.is_dir:
+            await self._delete_folder_contents(path)


Currently _delete_folder_contents is being used only for the purpose of deleting the contents of the root folder, but not the folder itself. The if should probably be if path.is_root . Other (non-root) folders should be deleted along with their contents.
The thought behind not deleting root folder, even when specifically requested, is that it busts the osf.io addon config.

TomBaxter · 2018-01-03T16:29:58Z

waterbutler/providers/cloudfiles/provider.py

+            for item in metadata
+        ]
+
+        delete_files.append(os.path.join('/', self.container, path.path))


_delete_folder_contents shouldn't delete the folder itself.

TomBaxter · 2018-01-03T18:22:37Z

waterbutler/providers/cloudfiles/provider.py

+        await resp.release()
+
+    @ensure_connection
+    async def metadata(self, path, recursive=False, version=None, revision=None, **kwargs):
        """Get Metadata about the requested file or folder
        :param str path: The path to a key or folder
        :rtype dict:
        :rtype list:
        """


update doc strings for new args or maybe switch to type hints.

TomBaxter · 2018-01-05T15:14:26Z

waterbutler/providers/cloudfiles/provider.py

@@ -339,11 +381,12 @@ def _extract_endpoints(self, data):
        )
        data = await resp.json()

-        # no data and the provider path is not root, we are left with either a file or a directory marker
+        # no data and the provider path is not root, we are left with either a file or a directory
+        # marker
        if not data and not path.is_root:
            # Convert the parent path into a directory marker (file) and check for an empty folder


r/(file)/(item)/

TomBaxter · 2018-01-05T15:18:42Z

waterbutler/providers/cloudfiles/provider.py

        if not data and not path.is_root:
            # Convert the parent path into a directory marker (file) and check for an empty folder
-            dir_marker = path.parent.child(path.name, folder=False)
-            metadata = await self._metadata_file(dir_marker, is_folder=True, **kwargs)
+            dir_marker = path.parent.child(path.name, folder=path.is_dir)


Is path.parent.child(path.name, folder=path.is_dir) different then path? Can't wrap my head around it. How am I not my father's son?

The function child is to create a new child path with a new name, so you are your father's child, but your sibling is your father's child with a different name.

TomBaxter · 2018-01-05T15:25:58Z

waterbutler/providers/cloudfiles/provider.py

+        resp = await self.make_request(
+            'PUT',
+            functools.partial(self.sign_url, path, 'PUT'),
+            expects=(200, 201),


Are both these codes possible, I would think that the response would always be one or the other, hopefully 201, on success.

TomBaxter · 2018-01-05T15:30:42Z

waterbutler/providers/cloudfiles/provider.py

+                                           'versions follow the instructions here: '
+                                           'https://developer.rackspace.com/docs/cloud-files/v1/'
+                                           'use-cases/additional-object-services-information/'
+                                           '#object-versioning')


Spoke to @felliott about this. My understanding is that for now, we want to treat this more like other providers. If they don't have versioning set up, let's keep it simple. No error message, let's just not show versions, only current.

TomBaxter · 2018-01-05T15:33:41Z

waterbutler/providers/cloudfiles/provider.py

+
+    @ensure_connection
+    async def revisions(self, path):
+        """Get past versions of the request file from special user designated version container,


r/request/requested/

coveralls · 2018-01-10T20:16:35Z

Coverage increased (+0.2%) to 90.177% when pulling 78230fc on Johnetordoff:cloudfiles into 26bf209 on CenterForOpenScience:develop.

coveralls · 2018-01-11T19:37:14Z

Coverage increased (+0.2%) to 90.134% when pulling e63f8bf on Johnetordoff:cloudfiles into 26bf209 on CenterForOpenScience:develop.

Johnetordoff added 2 commits October 12, 2017 15:39

Expand cloudfiles provider to preform all major functions, handle rev…

291eb4a

…isions etc.

Merge branch 'develop' of https://github.com/CenterForOpenScience/wat…

435f830

…erbutler into cloudfiles * 'develop' of https://github.com/CenterForOpenScience/waterbutler: Add PR template.

Johnetordoff changed the title ~~Cloudfiles~~ [WIP][SVCS-486]Add Cloudfiles Provider Oct 12, 2017

Add new revisions behavior and clean up metadata

7a759d0

Johnetordoff changed the title ~~[WIP][SVCS-486]Add Cloudfiles Provider~~ [SVCS-486]Add Cloudfiles Provider Oct 17, 2017

Allow all kwargs for registrations.

9fef6c6

felliott requested changes Oct 19, 2017

View reviewed changes

fix import order

03442e3

Johnetordoff added 3 commits October 20, 2017 15:37

Add doc strings and clean up

a596fa5

flake fix

d27bd74

fix misspelling

87e5fca

Merge branch 'develop' into cloudfiles

eaab942

Johnetordoff changed the title ~~[SVCS-486]Add Cloudfiles Provider~~ [SVCS-486] Add Cloudfiles Provider Nov 7, 2017

Merge branch 'develop' into cloudfiles

a48290b

Johnetordoff mentioned this pull request Nov 7, 2017

[SVCS-548] Chunked Uploads for CloudFiles #289

Open

AddisonSchiller suggested changes Nov 15, 2017

View reviewed changes

Johnetordoff and others added 2 commits November 15, 2017 16:58

Merge branch 'develop' into cloudfiles

f7a3f60

Johnetordoff added 4 commits November 16, 2017 09:42

clean up

b78cffa

Merge branch 'cloudfiles' of https://github.com/johnetordoff/waterbutler

b158cfe

into cloudfiles * 'cloudfiles' of https://github.com/johnetordoff/waterbutler:

remove blank lines

3ad9fbe

remove blank lines

3e62ede

Standardize import indentation

e4ad0a9

reorganize delete methods and add tests for deleting the root to Clou…

64c9a23

…dfiles provider.

add kwargs for registrations

9a58bed

cslzchen added the Code Review label Dec 15, 2017

cslzchen removed the Code Review label Jan 3, 2018

TomBaxter suggested changes Jan 5, 2018

View reviewed changes

style changes and add type hinting to Cloudfiles

78230fc

fix recursive delete and clean up docstrings.

e63f8bf

cslzchen added the On Hold label Jun 19, 2018


		@ensure_connection
		async def create_folder(self, path, folder_precheck=None):

		from waterbutler.providers.cloudfiles import CloudFilesProvider

[SVCS-486] Add Cloudfiles Provider #283

Are you sure you want to change the base?

[SVCS-486] Add Cloudfiles Provider #283

Conversation

Johnetordoff commented Oct 12, 2017

Ticket

Purpose

Changes

Side effects

QA Notes

Deployment Notes

coveralls commented Oct 13, 2017 • edited

coveralls commented Oct 17, 2017 • edited

felliott left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Oct 20, 2017 • edited

coveralls commented Oct 20, 2017 • edited

coveralls commented Oct 24, 2017 • edited

coveralls commented Nov 7, 2017 • edited

AddisonSchiller left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Nov 15, 2017 • edited

coveralls commented Nov 16, 2017 • edited

coveralls commented Nov 17, 2017 • edited

coveralls commented Nov 27, 2017 • edited

coveralls commented Dec 5, 2017 • edited

TomBaxter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jan 10, 2018 • edited

coveralls commented Jan 11, 2018 • edited

coveralls commented Oct 13, 2017 •

edited

coveralls commented Oct 17, 2017 •

edited

coveralls commented Oct 20, 2017 •

edited

coveralls commented Oct 20, 2017 •

edited

coveralls commented Oct 24, 2017 •

edited

coveralls commented Nov 7, 2017 •

edited

AddisonSchiller left a comment •

edited

coveralls commented Nov 15, 2017 •

edited

coveralls commented Nov 16, 2017 •

edited

coveralls commented Nov 17, 2017 •

edited

coveralls commented Nov 27, 2017 •

edited

coveralls commented Dec 5, 2017 •

edited

coveralls commented Jan 10, 2018 •

edited

coveralls commented Jan 11, 2018 •

edited