don't break on 201 Created response from S3

fixes RECNVS-1 also, don't store the attachment.thumbnail_url in the user.avatar_image_url; the former is potentially time-limited (and also possibly too long to fit in the field), while the latter is intended for deferred use. test-plan: - do not have inst-fs enabled - have S3 storage configured, not local storage - regression test uploads from: - user and course files areas - course image picker in course settings - course content import - user avatar image picker - rich content editor - conversations - submissions Change-Id: I0f194873977b621e1cab8413843ffe0345a34b1b Reviewed-on: https://gerrit.instructure.com/129587 QA-Review: Collin Parrish <[email protected]> Tested-by: Jenkins Reviewed-by: Andrew Huff <[email protected]> Product-Review: Jacob Fugal <[email protected]>
ralacas · Oct 27, 2017 · 470e06b · 470e06b
1 parent 1f515ab
commit 470e06b
Show file tree

Hide file tree

Showing 9 changed files with 161 additions and 39 deletions.
diff --git a/app/coffeescripts/models/File.coffee b/app/coffeescripts/models/File.coffee
@@ -78,9 +78,16 @@ define [
       FilesystemObject::save.call this, null,
         multipart: true
         success: (data) =>
-          if data.location?
+          if @uploadParams.success_url
+            # s3 upload, need to ping success_url to finalize and get back
+            # attachment information
+            $.ajaxJSON(@uploadParams.success_url, 'GET', {}, onUpload, onError)
+          else if data.location?
+            # inst-fs upload, need to request attachment information from
+            # location
             $.ajaxJSON(data.location, 'GET', {}, onUpload, onError)
           else
+            # local-storage upload, this _is_ the attachment information
             onUpload(data)
         error: onError
 

diff --git a/app/coffeescripts/react_files/modules/FileUploader.coffee b/app/coffeescripts/react_files/modules/FileUploader.coffee
@@ -30,19 +30,23 @@ define [
         @deferred.reject(event.target.status)
         return
 
-      url = if event.target.status == 201
-        $.parseJSON(event.target.response).location
+      if @uploadData.upload_params.success_url
+        # s3 upload, need to ping success_url to finalize and get back
+        # attachment information
+        $.getJSON(@uploadData.upload_params.success_url).then(@onUploadSuccess)
       else
-        @uploadData.upload_params.success_url
-
-      if url
-        $.getJSON(url).then (results) =>
-          f = @addFileToCollection(results)
-          @deferred.resolve(f)
-      else
-        results = $.parseJSON(event.target.response)
-        f = @addFileToCollection(results)
-        @deferred.resolve(f)
+        response = $.parseJSON(event.target.response)
+        if event.target.status == 201
+          # inst-fs upload, need to request attachment information from
+          # location
+          $.getJSON(response.location).then(@onUploadSuccess)
+        else
+          # local-storage upload, this _is_ the attachment information
+          @onUploadSuccess(response)
+
+    onUploadSuccess: (fileJson) =>
+      file = @addFileToCollection(fileJson)
+      @deferred.resolve(file)
 
     addFileToCollection: (attrs) =>
       uploadedFile = new BBFile(attrs, 'no/url/needed/') #we've already done the upload, no preflight needed

diff --git a/app/coffeescripts/react_files/modules/ZipUploader.coffee b/app/coffeescripts/react_files/modules/ZipUploader.coffee
@@ -53,11 +53,13 @@ define [
         @deferred.reject()
         return
 
-      url = @uploadData.upload_params.success_url
-      if url
-        $.getJSON(url).then (results) =>
-          @getContentMigration()
-      else
+      if @uploadData.upload_params.success_url
+        # s3 upload, need to ping success_url to finalize before polling the
+        # content migration
+        $.getJSON(@uploadData.upload_params.success_url).then(@getContentMigration)
+      else if uploadResponse.target.status == 201
+        # local-storage or inst-fs upload. we don't need to attachment
+        # information, we can just start polling the content migration
         @getContentMigration()
 
     # get the content migration when ready and use progress api to pull migration progress

diff --git a/app/coffeescripts/views/profiles/AvatarDialogView.coffee b/app/coffeescripts/views/profiles/AvatarDialogView.coffee
@@ -171,12 +171,16 @@ define [
       })
 
     onPostAvatar: (preflightResponse, postAvatarResponse) =>
-      if postAvatarResponse.location
-        @getCanvasJSON(postAvatarResponse.location).then(@onServiceSuccess)
-      else if preflightResponse.success_url
-        @s3Success(preflightResponse, postAvatarResponse).then(@onServiceSuccess)
+      if preflightResponse.success_url
+        # s3 upload, need to ping success_url to finalize and get back
+        # attachment information
+        @s3Success(preflightResponse, postAvatarResponse).then(@onUploadSuccess)
+      else if postAvatarResponse.location
+        # inst-fs upload, need to request attachment information from location
+        @getCanvasJSON(postAvatarResponse.location).then(@onUploadSuccess)
       else
-        @waitAndSaveUserAvatar(postAvatarResponse.avatar.token, postAvatarResponse.avatar.url)
+        # local-storage upload, this _is_ the attachment information
+        @onUploadSuccess(postAvatarResponse)
 
     getCanvasJSON: (location) =>
       $.getJSON("#{location}?include=avatar")
@@ -189,7 +193,7 @@ define [
         etag:   $s3.find('ETag').text()
       })
 
-    onServiceSuccess: (response) =>
+    onUploadSuccess: (response) =>
       @waitAndSaveUserAvatar(response.avatar.token, response.avatar.url)
 
     # need to wait for the avatar to get processed by background jobs before

diff --git a/app/helpers/application_helper.rb b/app/helpers/application_helper.rb
@@ -904,14 +904,9 @@ def planner_enabled?
   end
 
   def thumbnail_image_url(attachment)
-    if attachment.instfs_hosted?
-      attachment.thumbnail_url
-    else
-      # this thumbnail url is a route that redirects to local/s3 appropriately.
-      # deferred redirect through route because computing
-      # attachment.thumbnail_url for non-InstFS attachments may cause immediate
-      # generation of a dynamic thumbnail
-      super(attachment, attachment.uuid)
-    end
+    # this thumbnail url is a route that redirects to local/s3 appropriately.
+    # deferred redirect through route because it may be saved for later use
+    # after a direct link to attachment.thumbnail_url would have expired
+    super(attachment, attachment.uuid)
   end
 end
diff --git a/app/jsx/course_settings/actions.js b/app/jsx/course_settings/actions.js
@@ -189,12 +189,25 @@ import 'compiled/jquery.rails_flash_notifications'
                  .then((response) => {
                     const formData = Helpers.createFormData(response.data.upload_params);
                     const successUrl = response.data.upload_params.success_url
+                    const config = { responseType: (successUrl ? 'xml' : 'json') }
                     formData.append('file', file);
-                    ajaxLib.post(response.data.upload_url, formData)
+                    ajaxLib.post(response.data.upload_url, formData, config)
                            .then((response) => {
-                             return ajaxLib.get(successUrl).then((successResp) => {
-                               dispatch(this.prepareSetImage(successResp.data.url, successResp.data.id, courseId, ajaxLib));
-                             })
+                             if (successUrl) {
+                               // s3 upload, need to ping success_url to
+                               // finalize and get back attachment information
+                               return ajaxLib.get(successUrl)
+                             } else if (response.status === 201) {
+                               // inst-fs upload, need to request attachment
+                               // information from location
+                               return ajaxLib.get(response.data.location)
+                             } else {
+                               // local-storage upload, this _is_ the attachment information
+                               return response
+                             }
+                           })
+                           .then((successResp) => {
+                             dispatch(this.prepareSetImage(successResp.data.url, successResp.data.id, courseId, ajaxLib));
                            })
                            .catch((response) => {
                               dispatch(this.errorUploadingImage());

diff --git a/lib/api/v1/attachment.rb b/lib/api/v1/attachment.rb
@@ -70,7 +70,12 @@ def attachment_json(attachment, user, url_options = {}, options = {})
     if downloadable
       thumbnail_url = attachment.thumbnail_url
       if options[:thumbnail_url]
-        url = thumbnail_url
+        # not the same as thumbnail_url above because:
+        # * that one's going to be a direct (and possibly signed) s3, inst-fs,
+        #   etc. link for immediate use.
+        # * this one's a more compact canvas link to be stored for later use;
+        #   it will resolve to the former when accessed
+        url = thumbnail_image_url(attachment)
       else
         h = { :download => '1', :download_frd => '1' }
         h.merge!(:verifier => attachment.uuid) unless options[:omit_verifier_in_app] && respond_to?(:in_app?, true) && in_app?

diff --git a/public/javascripts/jquery.instructure_forms.js b/public/javascripts/jquery.instructure_forms.js
@@ -313,9 +313,16 @@ import './vendor/jquery.scrollTo'
           var post_params = data.upload_params;
           $(file).attr('name', data.file_param);
           $.ajaxJSONFiles(data.upload_url, 'POST', post_params, $(file), function(data) {
-            if (data.location) {
+            if (post_params.success_url) {
+              // s3 upload, need to ping success_url to finalize and get back
+              // attachment information
+              $.ajaxJSON(post_params.success_url, 'GET', {}, onUpload, onError);
+            } else if (data.location) {
+              // inst-fs upload, need to request attachment information from
+              // location
               $.ajaxJSON(data.location, 'GET', {}, onUpload, onError);
             } else {
+              // local-storage upload, this _is_ the attachment information
               onUpload(data)
             }
           }, onError, {onlyGivenParameters: true });

diff --git a/spec/coffeescripts/react_files/modules/FileUploaderSpec.coffee b/spec/coffeescripts/react_files/modules/FileUploaderSpec.coffee
@@ -63,6 +63,91 @@ define [
 
     server.restore()
 
+  test 'pings success_url after upload if set (S3)', ->
+    server = sinon.fakeServer.create()
+    server.respondWith('POST',
+                       '/api/v1/folders/1/files',
+                       [ 200,
+                         {"Content-Type": "application/json"},
+                         '{"upload_url": "/s3/upload/url", "upload_params": {"success_url": "/success/url"}}'
+                       ]
+    )
+    server.respondWith('POST',
+                       '/s3/upload/url',
+                       [ 201,
+                         {"Content-Type": "application/xml"},
+                         '<s3metadata />'
+                       ]
+    )
+    server.respondWith('GET',
+                       '/success/url',
+                       [ 200,
+                         {"Content-Type": "application/json"},
+                         '{ "id": "s3-id" }'
+                       ]
+    )
+    @stub(@uploader, 'addFileToCollection')
+    promise = @uploader.upload()
+    server.respond() # preflight
+    server.respond() # upload to s3
+    server.respond() # success_url
+    ok(@uploader.addFileToCollection.calledWith(id: "s3-id"), "got metadata from success_url")
+    server.restore()
+
+  test 'reads location after upload if 201 but no success_url (inst-fs)', ->
+    server = sinon.fakeServer.create()
+    server.respondWith('POST',
+                       '/api/v1/folders/1/files',
+                       [ 200,
+                         {"Content-Type": "application/json"},
+                         '{"upload_url": "/inst-fs/upload/url", "upload_params": {}}'
+                       ]
+    )
+    server.respondWith('POST',
+                       '/inst-fs/upload/url',
+                       [ 201,
+                         {"Content-Type": "application/json"},
+                         '{ "location": "/attachment/url" }'
+                       ]
+    )
+    server.respondWith('GET',
+                       '/attachment/url',
+                       [ 200,
+                         {"Content-Type": "application/json"},
+                         '{ "id": "inst-fs-id" }'
+                       ]
+    )
+    @stub(@uploader, 'addFileToCollection')
+    promise = @uploader.upload()
+    server.respond() # preflight
+    server.respond() # upload to inst-fs
+    server.respond() # location
+    ok(@uploader.addFileToCollection.calledWith(id: "inst-fs-id"), "got metadata from location")
+    server.restore()
+
+  test 'takes response body directly if 200 instead of 201 (local storage)', ->
+    server = sinon.fakeServer.create()
+    server.respondWith('POST',
+                       '/api/v1/folders/1/files',
+                       [ 200,
+                         {"Content-Type": "application/json"},
+                         '{"upload_url": "/inst-fs/upload/url", "upload_params": {}}'
+                       ]
+    )
+    server.respondWith('POST',
+                       '/inst-fs/upload/url',
+                       [ 200,
+                         {"Content-Type": "application/json"},
+                         '{ "id": "local-storage-id" }'
+                       ]
+    )
+    @stub(@uploader, 'addFileToCollection')
+    promise = @uploader.upload()
+    server.respond() # preflight
+    server.respond() # upload to local storage
+    ok(@uploader.addFileToCollection.calledWith(id: "local-storage-id"), "got metadata from response")
+    server.restore()
+
   test 'roundProgress returns back rounded values', ->
     @stub(@uploader, 'getProgress').returns(0.18) # progress is [0 .. 1]
     equal @uploader.roundProgress(), 18