[SVCS-426] Update googledrive provider to use googledrive v3 API #276

TomBaxter · 2017-10-02T20:55:00Z

Ticket

Purpose

Update GoogleDrive provider to v3 of the GoogleDrive API

Changes

Substantial changes to all aspects of the provider.

Of particular note:

GD v3 API has no method of downloading GoogleDoc revisions.
This PR leaves behind GD v2 calls, in order to maintain this
functionality as long as possible.

GD v3 returns minimal representations of resources. Fields must be
specified, to be returned.

Many field names changed. Most commonly in the provider:
title -> name
modifiedDate -> modifiedTime
fileSize -> size

etags are no longer available from GoogleDrive

exportLinks are no longer available in the 'file' representation of
GoogleDoc files.

GD v3 returns lists of resources as either 'files' or 'revisions' as
opposed to GD v2 which returned 'items' for all resource lists.

Side effects

None expected

QA Notes

This provider will need a full test of all functionality.

Deployment Notes

We will need to keep an eye on 'End of Life' announcements for GoogleDrive v2 API. As we have kept v2 call for GoogleDocs revisions downloads.

coveralls · 2017-10-17T16:37:25Z

Coverage increased (+0.08%) to 89.091% when pulling d7c8a4e on TomBaxter:feature/osf-426 into 481c9d9 on CenterForOpenScience:develop.

AddisonSchiller

Mostly style changes + a few questions to start.

AddisonSchiller

One very long line length change needed.
One moving of functions change.
after this, will be good for next phase

AddisonSchiller · 2017-11-13T17:35:15Z

waterbutler/providers/googledrive/provider.py

@@ -75,10 +74,12 @@ class GoogleDriveProvider(provider.BaseProvider):
    NAME = 'googledrive'
    BASE_URL = settings.BASE_URL
    FOLDER_MIME_TYPE = 'application/vnd.google-apps.folder'
+    FILE_FIELDS = {'fields': 'version, id, name, size, modifiedTime, createdTime, mimeType, webViewLink, webContentLink, md5Checksum, capabilities(canDelete, canEdit, canTrash, canDownload, canRename, canReadRevisions, canShare, canCopy)'}


Line length

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

waterbutler/providers/googledrive/provider.py

@@ -140,6 +141,45 @@ def can_intra_move(self,
                       path=None) -> bool:
        return self == other

+    def build_move_url(self, src_path, dest_path):


nitpick: you split up can_intra_move and can_intra_copy with all the build URL methods.
Move all the can_<do something> methods so they are all right by each other again (including can_duplicate_names

coveralls · 2017-11-13T18:25:36Z

Coverage increased (+0.9%) to 90.003% when pulling e790d6d on TomBaxter:feature/osf-426 into 473191c on CenterForOpenScience:develop.

coveralls · 2017-11-13T23:16:13Z

Coverage increased (+0.06%) to 90.003% when pulling b0e31a2 on TomBaxter:feature/osf-426 into 26bf209 on CenterForOpenScience:develop.

AddisonSchiller

Looking good. Ready for next phase.

cslzchen · 2017-11-29T16:53:56Z

@TomBaxter Let's sit down and do an in-person review for this.

cslzchen

A quick look view a few questions. Let's discuss in person @TomBaxter 🎆

cslzchen · 2017-12-11T15:11:48Z

waterbutler/providers/googledrive/metadata.py

@@ -159,7 +159,8 @@ def created_utc(self):

    @property
    def etag(self):
-        return self.raw['etag']
+        # Google Doc revision representations do not return etag
+        return '{}::{}'.format(self.raw['id'], self.raw['modifiedTime'])


How do we use etag or its alternative?
etag tracks both file and metadata change while modifiedTime only tracks file content change. Can this be a problem? Is it possible to still use v2 API for etag?
It is possible for file have the different modifiedTime but the same hash.

Agreed, I brought this up with @felliott at the time of writing PR. Can't remember the specifics of the conversation though. Let's revisit with him. Not sure which is the less of two evils. Sub par etag or staying on V2.

More question for @felliott: What do we use etag for?

cslzchen · 2017-12-11T15:24:45Z

waterbutler/providers/googledrive/provider.py

-                    'id': dest_path.parent.identifier
-                }],
-                'title': dest_path.name
+                'name': dest_path.name


Is dropping 'parents' intentional?

For myself, double check where parents is retrieved.

Yes, v2 has a new way of selecting parents.

cslzchen · 2017-12-14T18:35:09Z

waterbutler/providers/googledrive/provider.py

+        is_docs_file = drive_utils.is_docs_file(metadata.raw)
+        # GoogleDrive v3 API does not allow the download of GoogleDoc revisions
+        # Use v2 API for this functionality
+        if revision and is_docs_file:


👍

The if..elif...else... makes the logic much clearer than metadata.raw.get('downloadUrl') or drive_utils.get_export_link(metadata.raw). I used to have a hard time understanding the how it works.

Added comments to make the meaning of the four conditions more clear.

cslzchen · 2017-12-14T18:42:29Z

waterbutler/providers/googledrive/provider.py

@@ -146,6 +149,45 @@ def can_intra_copy(self,
        # gdrive doesn't support intra-copy on folders
        return self == other and (path and path.is_file)

+    def build_move_url(self, src_path, dest_path):


Too many methods here. It is not intuitive to use. Let's discuss if we can make only two util methods: build_v3_url() and build_v2_url()? The actions are parsed as arguments.

Yes, I made them separate for ease of use in tests. Have now re-factored them all out. Complete.

cslzchen · 2017-12-14T18:42:57Z

waterbutler/providers/googledrive/provider.py

+        # GoogleDrive v3 API does not allow the download of GoogleDoc revisions
+        # Use v2 API for this functionality
+        if revision and is_docs_file:
+            meta_url = self.build_url('files', path.identifier, 'revisions', revision)


As mentioned before, let's make a build_url_v2().

I'm happy to do this, however, I was holding off creating a new method for logic that will only be run in one place and likely removed entirely in the future. If it turns out that we want to use v2 for retrieving etags then it might make more sense to me to make build_url_v2 it's own thing.

cslzchen · 2017-12-14T18:54:22Z

waterbutler/providers/googledrive/provider.py

+            meta_url = self.build_url('files', path.identifier, 'revisions', revision)
+            meta_url = meta_url.replace('/v3/', '/v2/', 1)
+
+            async with self.request(


There is some difference between the old and the new logic. Let's discuss.

cslzchen · 2017-12-14T20:07:39Z

waterbutler/providers/googledrive/provider.py


    def _build_upload_metadata(self, folder_id: str, name: str) -> dict:
        return {
-            'parents': [
-                {
-                    'kind': 'drive#parentReference',


Is this change related to V3 upgrade?

I'm not sure why it was coded that way previously. I don't believe there was a change in the API. And I don't believe it was coded correctly originally.

cslzchen · 2017-12-14T20:12:52Z

waterbutler/providers/googledrive/utils.py

@@ -24,6 +24,7 @@
        'type': 'application/vnd.openxmlformats-officedocument.presentationml.presentation',
    },
 ]
+DOCS_MIMES = [format['mime_type'] for format in DOCS_FORMATS]
 DOCS_DEFAULT_FORMAT = {


Should we add 'mime_type': None to the default one?

Checked code. mime_type is only referenced after a check for is_google_doc , so adding mime_type = '' for consistency, though it currently will never be referenced.

cslzchen · 2017-12-14T20:18:13Z

waterbutler/providers/googledrive/provider.py

@@ -685,22 +763,22 @@ def _build_upload_metadata(self, folder_id: str, name: str) -> dict:
        resp = await self.make_request(
            'GET',
            self.build_url('files',
-                           q="'{}' in parents".format(file_id),


We don't have trashed = false before?

Prevents returning deleted files.

cslzchen · 2017-12-14T20:22:06Z

waterbutler/providers/googledrive/provider.py

        if drive_utils.is_docs_file(data):
-            if can_access_revisions:
+            if data['capabilities'].get('canReadRevisions', None) and data['capabilities']['canReadRevisions']:


How about this?

if data.get('capabilities', {}).get('canReadRevisions', None):

These do the same thing, but yours is shorter. Change made.

coveralls · 2017-12-18T16:39:24Z

Coverage increased (+0.03%) to 89.802% when pulling f3d87c5 on TomBaxter:feature/osf-426 into 3ec764a on CenterForOpenScience:develop.

cslzchen

Thanks for the updates. Looks good to me. I will test it fully locally (and make a PR with minor changes if necessary).

coveralls · 2018-01-03T23:10:27Z

Coverage increased (+0.03%) to 89.898% when pulling 85d497b on TomBaxter:feature/osf-426 into cae41fc on CenterForOpenScience:develop.

[SVCS-426 ] GoogleDrive migration guide https://developers.google.com/drive/v3/web/migration Of particular note: GD v3 API has no method of downloading GoogleDoc revisions. This PR leaves behind GD v2 calls, in order to maintain this functionality as long as possible. GD v3 returns minimal representations of resources. Fields must be specified, to be returned. Many field names changed. Most commonly in the provider: title -> name modifiedDate -> modifiedTime fileSize -> size etags are no longer available from GoogleDrive exportLinks are no longer available in the 'file' representation of GoogleDoc files. GD v3 returns lists of resources as either 'files' or 'revisions' as opposed to GD v2 which returned 'items' for all resource lists.

coveralls · 2018-01-05T20:07:26Z

Coverage increased (+0.03%) to 89.907% when pulling ac5dc54 on TomBaxter:feature/osf-426 into f17d276 on CenterForOpenScience:develop.

cslzchen · 2018-01-05T20:43:03Z

Update: secondary PR https://github.com/TomBaxter/waterbutler/pull/1 pending.

[SVCS-426] Style Refactor and Minor Code Change

coveralls · 2018-01-08T16:41:51Z

Coverage increased (+0.03%) to 89.912% when pulling 6be0f45 on TomBaxter:feature/osf-426 into f17d276 on CenterForOpenScience:develop.

cslzchen · 2018-01-09T02:40:40Z

Update: a second secondary PR https://github.com/TomBaxter/waterbutler/pull/2 pending.

- gdrive metadata and provider tests

[SVCS-426] Update Style and Trivial Code Change for Tests

coveralls · 2018-01-09T16:09:08Z

Coverage increased (+0.1%) to 90.019% when pulling 9f314e9 on TomBaxter:feature/osf-426 into f17d276 on CenterForOpenScience:develop.

cslzchen

LGTM and move to PCR. 🎆 🎆 🎆 🎆 🎆

@felliott The side effect of the style refactor we did is that the diffs are harder to read. You can refer to the old commit: ac5dc54 for better diffs.

cslzchen · 2018-01-10T15:03:10Z

waterbutler/providers/googledrive/provider.py

            ]

        # Use dummy ID if no revisions found
        metadata = await self.metadata(path, raw=True)
+        # TODO: considering keep using v2 to obtain etag


Phase 2: etag is no longer available through v3, but we can still use v2.\

Phase 2: please be ware that using id, modifiedTime (and even with md5Checksum) is not equivalent to etag. Here is a useful discussion I found: https://stackoverflow.com/questions/42174600/alternative-for-etag-field-in-google-drive-v3.

TomBaxter changed the title ~~[WIP] Update googledrive provider to use googledrive v3 API~~ Update googledrive provider to use googledrive v3 API Oct 17, 2017

AddisonSchiller suggested changes Oct 30, 2017

View reviewed changes

cslzchen added the To Be Replaced label Oct 30, 2017

cslzchen changed the title ~~Update googledrive provider to use googledrive v3 API~~ [SVCS-426] Update googledrive provider to use googledrive v3 API Oct 30, 2017

AddisonSchiller suggested changes Nov 13, 2017

View reviewed changes

AddisonSchiller approved these changes Nov 14, 2017

View reviewed changes

cslzchen added Code Review and removed To Be Replaced labels Nov 14, 2017

This was referenced Dec 6, 2017

[SVCS-528] [Blocked] Improve gdoc rendering - WB Part #304

Open

[SVCS-528] Improve gdoc rendering - MFR Part CenterForOpenScience/modular-file-renderer#303

Open

cslzchen reviewed Dec 14, 2017

View reviewed changes

cslzchen added the Add'l Dev label Dec 15, 2017

cslzchen reviewed Dec 19, 2017

View reviewed changes

cslzchen removed the Add'l Dev label Dec 19, 2017

cslzchen added the Add'l Dev label Jan 5, 2018

cslzchen removed the Add'l Dev label Jan 5, 2018

cslzchen and others added 2 commits January 8, 2018 11:27

Refactor style with minor code change for gdrive.

34763f0

Merge pull request #1 from cslzchen/fix/svcs-426-gdrive-v3

6be0f45

[SVCS-426] Style Refactor and Minor Code Change

Update style with trivial code change for tests:

9266287

- gdrive metadata and provider tests

Merge pull request #2 from cslzchen/fix/svcs-426-gdrive-v3

9f314e9

[SVCS-426] Update Style and Trivial Code Change for Tests

cslzchen approved these changes Jan 10, 2018

View reviewed changes

cslzchen added Final Review and removed Code Review labels Jan 10, 2018

[SVCS-426] Update googledrive provider to use googledrive v3 API #276

Are you sure you want to change the base?

[SVCS-426] Update googledrive provider to use googledrive v3 API #276

Conversation

TomBaxter commented Oct 2, 2017 • edited

Ticket

Purpose

Changes

Side effects

QA Notes

Deployment Notes

coveralls commented Oct 17, 2017 • edited

AddisonSchiller left a comment

Choose a reason for hiding this comment

AddisonSchiller left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AddisonSchiller Nov 13, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Nov 13, 2017 • edited

coveralls commented Nov 13, 2017 • edited

AddisonSchiller left a comment

Choose a reason for hiding this comment

cslzchen commented Nov 29, 2017

cslzchen 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

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

TomBaxter Dec 15, 2017 • 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

coveralls commented Dec 18, 2017 • edited

cslzchen left a comment

Choose a reason for hiding this comment

coveralls commented Jan 3, 2018 • edited

coveralls commented Jan 5, 2018 • edited

cslzchen commented Jan 5, 2018

coveralls commented Jan 8, 2018 • edited

cslzchen commented Jan 9, 2018

coveralls commented Jan 9, 2018 • edited

cslzchen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomBaxter commented Oct 2, 2017 •

edited

coveralls commented Oct 17, 2017 •

edited

AddisonSchiller Nov 13, 2017 •

edited

coveralls commented Nov 13, 2017 •

edited

coveralls commented Nov 13, 2017 •

edited

TomBaxter Dec 15, 2017 •

edited

coveralls commented Dec 18, 2017 •

edited

coveralls commented Jan 3, 2018 •

edited

coveralls commented Jan 5, 2018 •

edited

coveralls commented Jan 8, 2018 •

edited

coveralls commented Jan 9, 2018 •

edited