Custom File Upload Controller #1114

flovilmart · 2017-02-24T00:01:37Z

@kcoop I like the idea, let's bring it in!

Added new files to other targets.

flovilmart · 2017-02-24T00:04:56Z

Parse/PFFile.h

@@ -152,6 +154,11 @@ NS_ASSUME_NONNULL_BEGIN
 ///--------------------------------------

 /**
+ Sets a custom upload controller that synchronously uploads the file using its own policy.
+ */
+@property (nonatomic, strong, readwrite, nullable) id<PFFileUploadController> uploadController;


I'd put the controller part of the configuration to avoid mismatches, keep it there as well to use the default uploader for a particular file no?

flovilmart · 2017-02-24T00:06:32Z

Parse/Internal/File/Controller/PFFileController.m

-        return [[self _cacheFileAsyncWithState:fileState atPath:sourceFilePath] continueWithSuccessResult:fileState];
-    }];
+
+    if (uploadController) {


why not have a default controller that proceeded the PFRESTFileCommand this way the upload controller could be:

uploadController = uploadController ?: [PFRestFileUploadController default]

flovilmart · 2017-02-24T00:08:08Z

Parse/PFFileUploadController.h

+ to application's parse server. Allows for direct uploads to other file storage
+ providers.
+ */
+@protocol PFFileUploadController <NSObject>


we could very well have the PFRESTFileUploadController: NSObject<PFFileUploadController> what do you think?

Absolutely. I originally started to go that route, but wanted to minimize touching the code base. I'll make that change. I assume you want the PFRESTFileUploadController to have a public .h?

It's an internal one, not really a need to be exposed. The protocol needs to be however.

OK, will do. Few things to do first, but will get to it in a bit.

flovilmart · 2017-02-24T00:12:41Z

For your use case, you'd wanna use that upload controller for all files right? Would there be an easy way to add that probably to the parse initialization? or it would not work for your use case?

kcoop · 2017-02-24T00:15:36Z

Yes, that would be a better API. Could create a default at initialization, and have an override on PFFile? I don't really need the latter, but seems like someone might.

flovilmart · 2017-02-24T00:17:08Z

KISS, if we can easily access/init at init time, let's not provide an override at PFFile time. We'll revisit if one really need to go the per-PFFile route. What do you think?

kcoop · 2017-02-24T00:17:43Z

Works for me. I'll do that as part of the change.

flovilmart · 2017-02-24T00:18:28Z

Awesome! There's no rush here but 🙇 for that PR :)

kcoop · 2017-02-24T00:23:00Z

BTW, while I don't have the time to go through the learning curve of adding a cocoapod/package.json to PFFileS3UploaderController and the associated cloud code, they would be generally useful. Any interest in doing that packaging if I put up public repos for them?

flovilmart · 2017-02-24T00:29:19Z

Yeh, I could have a look :)

flovilmart · 2017-02-24T03:21:06Z

For your S3 uploaded, we can put them on parse-server-modules org. If you don't mind :)

…configuration. Created a default PFFileUploadController policy for parse-server REST upload rather than hard coding it in PFFileController.

…eUploadController; it exposed too many internals.

kcoop · 2017-02-24T05:56:05Z

Sounds good. I'll put up public repos in the next couple of days.

Encapsulating the default REST upload implementation as an implementor of PFFileUploadController sounded good, but it caused a lot of internals to be exposed (most notably, it required the instance of PFFileController for its commandRunner). I've backed out my earlier erroneous commit.

Also, I think I may have found an existing bug in PFFileController, but didn't want to make the change without making sure it doesn't have side effects. The mime type doesn't get passed along to the PFFileState. It might explain #997, which has actually been a nuisance for me for years now.

      PFRESTFileCommand *command = [PFRESTFileCommand uploadCommandForFileWithName:fileState.name sessionToken:sessionToken];
       @weakify(self);
       return [[self.dataSource.commandRunner runFileUploadCommandAsync:command
                                                        withContentType:fileState.mimeType
                                                  contentSourceFilePath:sourceFilePath
                                                                options:PFCommandRunningOptionRetryIfFailed
                                                      cancellationToken:cancellationToken
                                                          progressBlock:progressBlock] continueWithSuccessBlock:^id(BFTask<PFCommandResult *> *task) {
           @strongify(self);
           PFCommandResult *result = task.result;
           PFFileState *fileState = [[PFFileState alloc] initWithName:result.result[@"name"]
                                                            urlString:result.result[@"url"]
                                                             mimeType:nil];  // <-- Shouldn't this pass along the mime type?
           return [[self _cacheFileAsyncWithState:fileState atPath:sourceFilePath] continueWithSuccessResult:fileState];
       }];

kcoop · 2017-02-24T23:46:01Z

I've created a simple container repo that has the server and iOS code in it. https://github.com/kcoop/Parse-S3FileUploaderSTS

Sorry, no instructions for STS config in the readme. Sadly didn't take notes as I implemented it, and there's a bit involved.

kcoop · 2017-02-25T07:00:58Z

Sorry for the chatty commits. Haven't been able to run test suites. rake test:ios doesn't seem to work.

$ rake test:ios
2017-02-24 22:44:12.153 xcodebuild[51689:31382307] [MT] PluginLoading: Required plug-in compatibility UUID E0A62D1F-3C18-4D74-BFE5-A4167D643966 for plug-in at path '~/Library/Application Support/Developer/Shared/Xcode/Plug-ins/OMQuickHelp.xcplugin' not present in DVTPlugInCompatibilityUUIDs
xcodebuild: error: Unable to find a destination matching the provided destination specifier:
		{ platform:iOS Simulator, OS:9.1, name:iPhone 4s }

	The requested device could not be found because no available devices matched the request.

	Available destinations for the "Parse-iOS" scheme:
		{ platform:iOS Simulator, id:3B4EA573-0421-416D-8600-E36705A30D1D, OS:10.2, name:iPad Air }
		{ platform:iOS Simulator, id:E911EFFA-E20C-41BE-A37E-13F5DE7367AD, OS:10.2, name:iPad Air 2 }
		{ platform:iOS Simulator, id:2BC62390-D8F6-4C2B-8C8A-93C80D5F8DC5, OS:10.2, name:iPad Pro (9.7 inch) }
		{ platform:iOS Simulator, id:C2940DC0-3075-45CC-BEFC-CAC1F6FA5CA4, OS:10.2, name:iPad Pro (12.9 inch) }
		{ platform:iOS Simulator, id:953D8CEF-2DC0-42D7-B550-83ED49EF1EDD, OS:10.2, name:iPad Retina }
		{ platform:iOS Simulator, id:34D27E97-46F9-4AC0-B786-B852AF366FB8, OS:10.2, name:iPhone 5 }
		{ platform:iOS Simulator, id:4D712BEA-F8E6-45F5-921C-2BAB4239F644, OS:10.2, name:iPhone 5s }
		{ platform:iOS Simulator, id:EE1C615E-A5AB-4770-A137-D7F8C1DCDD8F, OS:10.2, name:iPhone 6 }
		{ platform:iOS Simulator, id:2DE8FFAF-F29A-4EB7-BA0B-F0E13C4C193D, OS:10.2, name:iPhone 6 Plus }
		{ platform:iOS Simulator, id:D1E3E37A-5F69-4C78-A49B-8C26DB2F2B79, OS:10.2, name:iPhone 6s }
		{ platform:iOS Simulator, id:0EDB1F73-3BCE-4B04-83EB-5542C0E6BFD8, OS:10.2, name:iPhone 6s Plus }
		{ platform:iOS Simulator, id:34954810-6CF2-4067-B625-A4356B043030, OS:10.2, name:iPhone 7 }
		{ platform:iOS Simulator, id:AD543E4B-B121-4A84-87FC-6D906CF92716, OS:10.2, name:iPhone 7 Plus }
		{ platform:iOS Simulator, id:D1FF99E3-CB7E-4099-98E4-967DE91173F0, OS:10.2, name:iPhone SE }

	Ineligible destinations for the "Parse-iOS" scheme:
		{ platform:iOS, id:dvtdevice-DVTiPhonePlaceholder-iphoneos:placeholder, name:Generic iOS Device }
		{ platform:iOS Simulator, id:dvtdevice-DVTiOSDeviceSimulatorPlaceholder-iphonesimulator:placeholder, name:Generic iOS Simulator Device }
iOS Tests Failed!

I'm also having difficulty figuring out how the latest test failure could be related to my commits:

ExtensionDataSharingTests
  testMigratingDataFromMainSandbox, (([[NSFileManager defaultManager] fileExistsAtPath:path]) is false) failed - /Users/travis/Library/Developer/CoreSimulator/Devices/742929A4-D5DE-45EC-A02A-5B6B0746E872/data/Library/Private Documents/Test/yolo/Parse shouldn't exist.

kcoop · 2017-02-28T00:47:07Z

Am I missing something obvious with not being able to run the test suite? Shouldn't it just be rake test:ios?

kcoop · 2017-03-01T00:26:06Z

@flovilmart, I'd like to get this buttoned down so I can move on to other things (I'd rather commit a Podfile.lock that refers to your repo than mine).

I'm more than happy to debug this failure, but I don't have a good workflow for doing so. I don't see any docs for how I can run the tests, let alone how to attach the debugger as they run.

BTW, I'm really enjoying seeing files upload to S3 without touching my server. Well, maybe not today, thanks Amazon. :-P

flovilmart · 2017-03-01T00:54:51Z

For the tests, I believe the command is bundle exec rake test:iOS

Otherwise I can have a look later this week

kcoop · 2017-03-01T01:05:41Z

That's basically what I've been doing, still fails as mentioned above. The Rakefile that's in head doesn't refer to the same set of destination specifiers, it only refers to iPhone 4 and iPhone 6. Seems like something in the travis build sets these up? Any thoughts on how to attach a debugger to this?

And again, the test that's failing checks whether the Parse directory is present in the file system, and I guess it is? Strange that the changes in this pull request would do that.

Anyway, no worries, I understand busy. Just let me know.

flovilmart · 2017-03-01T01:26:41Z

I'll get the PR branch and have a look.otherwise run tests in Xcode should work no?

kcoop · 2017-03-01T01:35:14Z

Doesn't build.

flovilmart · 2017-03-01T02:48:01Z

uhm.. yeah it's not updated for xcode8, there is a PR for that, but...

kcoop · 2017-03-01T02:53:33Z

Huh. Now it passes.

flovilmart · 2017-03-01T03:01:15Z

Seems that travis is still drunk from all the S3 issue and they incurred a huge build backlog.

flovilmart

A few nits, otherwise that looks awesome

flovilmart · 2017-02-24T13:06:22Z

Parse/PFFileUploadController.h

+
+#import <Foundation/Foundation.h>
+#import <Bolts/BFTask.h>
+#import "PFFileUploadResult.h"


We should add that protocol in the global import header, CI is complaining about non modular header

Also, we should just use forward declaration through @class PFFileUploadResult instead of header import here.

flovilmart · 2017-03-01T03:03:46Z

Parse/ParseClientConfiguration.h

@@ -10,6 +10,7 @@
 #import <Foundation/Foundation.h>

 #import <Parse/PFConstants.h>
+#import "PFFileUploadController.h"


import should probably be #import <Parse/PFFileUploadController.h> or even better, a forward @class PFFileUploadController

Ok, done, it builds, but I have to commit to test it. Here goes...

I've also added #import <Parse/PFFileUploadController.h> to the global import header.

Out of curiosity, why prefer the forward @protocol over the import? Don't see any cycles here.

It's because the header is 'exported' that avoids the confusion between import <module/header.h> vs import "header.h"

flovilmart · 2017-03-01T03:09:00Z

🙇 sorry for the back and forth 🙇

… header file. Changed ref to PFFileUploadController.h to a forward declaration.

kcoop · 2017-03-03T22:19:35Z

I think Travis needs a double espresso and a dunk in a bucket of ice water.

flovilmart · 2017-04-07T12:08:18Z

@kcoop let's merge that, I'll release the next version soon.

…1.14.4 * commit 'b5c49540caf930dbaf47a9c0f07cddac6cdd9674': Parse 1.14.4 (parse-community#1131) Custom File Upload Controller (parse-community#1114) transfer code Bundle check (parse-community#1119) # Conflicts: # Parse/Internal/Object/Subclassing/PFObjectSubclassingController.m

kcoop added 3 commits February 21, 2017 05:04

Added pluggable PFFileUploadController protocol to PFFile.

bf13868

Added PFFile’s filename to protocol.

17aacaf

Added missing implementation file for PFFileUploadResult.

dc76a52

Added new files to other targets.

facebook-github-bot added CLA Signed labels Feb 24, 2017

flovilmart changed the title ~~Upload controller~~ Custom File Upload Controller Feb 24, 2017

flovilmart commented Feb 24, 2017

View reviewed changes

kcoop added 2 commits February 23, 2017 21:19

Converted to specifying the PFFileUploadController globally on Parse.…

ee60efd

…configuration. Created a default PFFileUploadController policy for parse-server REST upload rather than hard coding it in PFFileController.

Backed out implementing standard upload as an implementation of PFFil…

e7517cd

…eUploadController; it exposed too many internals.

kcoop added 3 commits February 23, 2017 22:15

Added missing private variable declaration.

3668cd1

Changed copy to strong for global config reference.

dcc7c90

Added missing equality and copy semantics in ParseClientConfiguration.

e72968b

kcoop added 2 commits February 24, 2017 17:26

Marked PFFileUploadController.h as public in project.

0b1905a

Marked PFFileUploadResult.h as public in project.

d3a8dde

flovilmart commented Mar 1, 2017

View reviewed changes

Added PFFileUploadController.h and PFFileUploadResult.h to the global…

715c185

… header file. Changed ref to PFFileUploadController.h to a forward declaration.

flovilmart merged commit 2e21ca6 into parse-community:master Apr 7, 2017

Uh oh!

Custom File Upload Controller #1114

Custom File Upload Controller #1114

Uh oh!

Conversation

flovilmart commented Feb 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

flovilmart commented Feb 24, 2017

Uh oh!

kcoop commented Feb 24, 2017

Uh oh!

flovilmart commented Feb 24, 2017

Uh oh!

kcoop commented Feb 24, 2017

Uh oh!

flovilmart commented Feb 24, 2017

Uh oh!

kcoop commented Feb 24, 2017

Uh oh!

flovilmart commented Feb 24, 2017

Uh oh!

flovilmart commented Feb 24, 2017

Uh oh!

kcoop commented Feb 24, 2017

Uh oh!

kcoop commented Feb 24, 2017

Uh oh!

kcoop commented Feb 25, 2017

Uh oh!

kcoop commented Feb 28, 2017

Uh oh!

kcoop commented Mar 1, 2017

Uh oh!

flovilmart commented Mar 1, 2017

Uh oh!

kcoop commented Mar 1, 2017

Uh oh!

flovilmart commented Mar 1, 2017

Uh oh!

kcoop commented Mar 1, 2017

Uh oh!

flovilmart commented Mar 1, 2017

Uh oh!

kcoop commented Mar 1, 2017

Uh oh!

flovilmart commented Mar 1, 2017

Uh oh!

flovilmart left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

flovilmart commented Mar 1, 2017

Uh oh!

kcoop commented Mar 3, 2017

Uh oh!

flovilmart commented Apr 7, 2017

Uh oh!

Uh oh!

flovilmart commented Feb 24, 2017 •

edited

Loading