feat(amplify_storage_s3): UploadFile and GetUrl #15

Ashish-Nanda · 2020-08-07T22:48:29Z

Description of changes:
Draft PR that adds support for UploadFile and GetUrl functionality for Amplify Flutter.
Todo:
Error handling improvements, tests and other refinement mentioned in TODOs

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

packages/amplify_core/example/ios/Podfile

packages/amplify_core/lib/amplify_core.dart

packages/amplify_core_plugin_interface/lib/amplify_storage_category.dart

packages/amplify_storage_plugin_interface/README.md

packages/amplify_storage_plugin_interface/lib/src/GetUrl/GetUrlOptions.dart

packages/amplify_storage_s3/README.md

packages/amplify_storage_plugin_interface/README.md

packages/amplify_storage_plugin_interface/lib/src/GetUrl/GetUrlOptions.dart

packages/amplify_storage_plugin_interface/lib/src/Storage/StorageOptions.dart

packages/amplify_storage_plugin_interface/lib/src/UploadFile/UploadFileOptions.dart

packages/amplify_storage_plugin_interface/pubspec.yaml

Amplifiyer · 2020-08-07T23:08:29Z

packages/amplify_storage_plugin_interface/test/amplify_storage_plugin_interface_test.dart

+// import 'package:flutter_test/flutter_test.dart';
+
+// import 'package:amplify_storage_plugin_interface/amplify_storage_plugin_interface.dart';
+
+// void main() {
+//   test('adds one to input values', () {
+//     final calculator = Calculator();
+//     expect(calculator.addOne(2), 3);
+//     expect(calculator.addOne(-7), -6);
+//     expect(calculator.addOne(0), 1);
+//     expect(() => calculator.addOne(null), throwsNoSuchMethodError);
+//   });
+// }


probably should delete the file if not needed.

Yeah, delete this whole test directory if we don't have actual tests.

packages/amplify_storage_s3/README.md

packages/amplify_storage_s3/example/README.md

...android/src/main/kotlin/com/amazonaws/amplify/amplify_storage_s3/AmplifyStorageOperations.kt

...3/android/src/main/kotlin/com/amazonaws/amplify/amplify_storage_s3/AmplifyStorageS3Plugin.kt

...droid/src/main/kotlin/com/amazonaws/amplify/amplify_storage_s3/types/FlutterGetUrlRequest.kt

packages/amplify_storage_s3/lib/amplify_storage_s3.dart

packages/amplify_storage_s3/lib/method_channel_storage_s3.dart

...d/src/main/kotlin/com/amazonaws/amplify/amplify_storage_s3/types/FlutterUploadFileRequest.kt

packages/amplify_storage_s3/example/lib/main.dart

packages/amplify_storage_s3/ios/Classes/AmplifyStorageOperations.swift

...android/src/main/kotlin/com/amazonaws/amplify/amplify_storage_s3/AmplifyStorageOperations.kt

Amplifiyer · 2020-08-08T00:20:23Z

packages/amplify_storage_s3/ios/Classes/AmplifyStorageOperations.swift

+            let req = FlutterGetUrlRequest(request: request)
+            print("GetUrlRequest.Options object is: \(String(describing: req.options))")
+            _ = Amplify.Storage.getURL(key: req.key,
+                               options: nil,


not req.options?

Draft PR. Lot of this PR is WIP but I wanted you to get your eyes on it before you leave.

Definitely. I didn't see a Todo on this one that's why left a comment to keep track

Amplifiyer · 2020-08-08T00:25:16Z

packages/amplify_storage_s3/ios/Classes/FlutterUploadFileRequest.swift

+    var options: StorageUploadFileRequest.Options? = nil
+    init(request: Dictionary<String, AnyObject>) {
+        self.key = request["key"] as! String
+        self.file = NSURL(fileURLWithPath: request["path"] as! String) as URL


Can this be validated that the path is valid? in the isValid() before init

Yes I can add that to isValid()

packages/amplify_storage_s3/ios/Classes/FlutterUploadFileRequest.swift

packages/amplify_storage_s3/lib/method_channel_storage_s3.dart

…storage

haverchuck · 2020-08-14T17:56:04Z

packages/amplify_core_plugin_interface/lib/amplify_storage_category.dart

+    }
+  }
+
+  Future<UploadFileResponse> uploadFile({@required UploadFileRequest request}) {


Assuming that this is where a subsequent PR will allow discrete parameters.

Yea -- where did you guys land with this? In the design review, we had proposed to do a 1 pager weighing pros and cons. Were there any cons to "exploding" the common args into the method signature? Or what's the status?

We are doing it. For now we are just creating the request instance inside the method and exposing an interface with named parameters to the customer. I dont have that in this PR, it will be a followup.

No real downside to that, and definite upside of better customer experience.

haverchuck · 2020-08-14T17:58:23Z

packages/amplify_storage_plugin_interface/lib/src/GetUrl/GetUrlResponse.dart

@@ -0,0 +1,6 @@
+class GetUrlResponse {


Assuming that closing on this will be a subsequent PR prior to P0.

^ @haverchuck Closing on what?

haverchuck · 2020-08-14T18:04:28Z

packages/amplify_storage_plugin_interface/lib/src/UploadFile/UploadFileOptions.dart

+  Map<String, dynamic> serializeAsMap() {
+    final Map<String, dynamic> optionsMap = <String, dynamic>{};
+    if (accessLevel != null) {
+      optionsMap["accessLevel"] = describeEnum(accessLevel);


haverchuck · 2020-08-14T18:05:56Z

packages/amplify_storage_plugin_interface/lib/src/types.dart

@@ -0,0 +1,10 @@
+export 'Storage/StorageAccessLevel.dart';
+export 'Storage/StorageOptions.dart';
+


nit: as these type files grow, it can be helpful to create little comment headings (IMO)

Keeping diligent about the use of whitespace also does work. The comments could end up like:

// Do the needful export 'do' export 'the' export 'needful'

Or, I guess you could document the grouping strategy, once, at the top of the document. That could be frugal.

haverchuck · 2020-08-14T18:08:33Z

...android/src/main/kotlin/com/amazonaws/amplify/amplify_storage_s3/AmplifyStorageOperations.kt

+
+        fun uploadFile(@NonNull flutterResult: MethodChannel.Result, @NonNull request: HashMap<String, *>, mainActivity: Activity?) {
+            if (FlutterUploadFileRequest.isValid(request)) {
+                Log.i("AmplifyFlutter", "uploadFile request:" + request.toString())


Could be helpful to use amplify-android's logging utility.

haverchuck · 2020-08-14T18:12:48Z

packages/amplify_storage_s3/example/ios/Podfile.lock

@@ -0,0 +1,153 @@
+PODS:


This file should not be checked in.

haverchuck · 2020-08-14T18:16:54Z

packages/amplify_storage_s3/ios/amplify_storage_s3.podspec

+Pod::Spec.new do |s|
+  s.name             = 'amplify_storage_s3'
+  s.version          = '0.0.1'
+  s.summary          = 'A new flutter plugin project.'


Need to update this boilerplate.

haverchuck · 2020-08-14T18:18:00Z

packages/amplify_storage_s3/pubspec.yaml

+
+environment:
+  sdk: ">=2.7.0 <3.0.0"
+  flutter: ">=1.10.0"


I think we need to bump this to >=1.20.0 and retest

haverchuck · 2020-08-14T18:18:40Z

packages/amplify_storage_s3/test/amplify_storage_s3_test.dart

@@ -0,0 +1,23 @@
+// import 'package:flutter/services.dart';


Assuming that we will have tests here prior to p0; ditto for android unit tests.

haverchuck

Left some minor requests for revisions.

jamesonwilliams · 2020-08-14T21:09:15Z

packages/amplify_storage_s3/pubspec.yaml

+  plugin:
+    platforms:
+      android:
+        package: com.amazonaws.amplify.amplify_storage_s3


com.amplifyframework.flutter.storage.s3?

jamesonwilliams · 2020-08-14T21:15:48Z

packages/amplify_core_plugin_interface/lib/amplify_storage_category.dart

+class StorageCategory {
+  const StorageCategory();
+
+  static List<StoragePluginInterface> plugins = [];


In iOS and Android, the category and plugin both extend a common interface. This ensures that the customer APIs are fully addressed by the plugin developer. How come you all decided not to pursue this pattern for Flutter?

jamesonwilliams · 2020-08-14T21:17:00Z

packages/amplify_core_plugin_interface/lib/amplify_storage_category.dart

+    }
+  }
+
+  Future<UploadFileResponse> uploadFile({@required UploadFileRequest request}) {


Yea -- where did you guys land with this? In the design review, we had proposed to do a 1 pager weighing pros and cons. Were there any cons to "exploding" the common args into the method signature? Or what's the status?

jamesonwilliams · 2020-08-14T21:18:09Z

packages/amplify_core_plugin_interface/lib/amplify_storage_category.dart

+
+  Future<GetUrlResponse> getUrl({@required GetUrlRequest request}) {
+    /// call `getUrl` on the plugin
+    return plugins[0].getUrl(request: request);


Android and iOS have a method for this purpose, like getSelectedPlugin(). It does range checking on the list, and if there is not any plugin, will return a friendly message: "Kind friend, please consider the opportunity to add a plugin." etc. A little better than "0 not in range."

jamesonwilliams · 2020-08-14T21:18:47Z

packages/amplify_storage_plugin_interface/CHANGELOG.md

@@ -0,0 +1,3 @@
+## [0.0.1] - TODO: Add release date.


I would just delete this file until you have real content for it.

jamesonwilliams · 2020-08-14T21:37:54Z

...android/src/main/kotlin/com/amazonaws/amplify/amplify_storage_s3/AmplifyStorageOperations.kt

+                            }
+                    )
+                } catch (e: Exception) {
+                    prepareError(flutterResult, "ERROR_ENCOUNTERED_IN_GET_URL_OPERATION", e.localizedMessage, "")


These screaming snake strings are weird. They look like they want to be constants in the language, but yet they are strings. Is the intention to show these to a user? If so, style these appropriately for consumption as natural langauge.

An error was enountered in the GetUrl operation.

instead of

ERROR_ENCOUNTERED_IN_GET_URL_OPERATION

You are correct. I Intend to make them constants. There is a TODO uptop, line 17.

jamesonwilliams · 2020-08-14T21:38:57Z

...android/src/main/kotlin/com/amazonaws/amplify/amplify_storage_s3/AmplifyStorageOperations.kt

+        private fun prepareGetUrlResponse(@NonNull flutterResult: MethodChannel.Result, result: StorageGetUrlResult) {
+            Handler (Looper.getMainLooper()).post {
+                Log.i("AmplifyFlutter", "Successfully got Url: " + result.url.toString())
+                val response: HashMap<String, String> = HashMap()


Liskov Principle.

Map on LHS. Implementation type on RHS. Same comment for entire code base.

jamesonwilliams · 2020-08-14T21:40:26Z

...android/src/main/kotlin/com/amazonaws/amplify/amplify_storage_s3/AmplifyStorageOperations.kt

+    }
+}
+
+//TODO: Move to Error or Util Class


Hm, well, maybe. Right now, you have one method inside of a companion object, and then this method is at global scope. Shouldn't they at least be the same? Why is this not in the companion object, or why is the companion method not at global scope?

jamesonwilliams · 2020-08-14T21:42:38Z

...android/src/main/kotlin/com/amazonaws/amplify/amplify_storage_s3/AmplifyStorageOperations.kt

+    Handler (Looper.getMainLooper()).post {
+        Log.i("AmplifyFlutter", "Error: " + localizedError + msg);
+        var errorDetails: HashMap<String, Any> = HashMap();
+        errorDetails.put("PLATFORM_EXCEPTIONS", mapOf(


If someone will be expected to reliably read this key back out, don't use an inline string constant. Instead, wearhouse this value as a public constant somewhere. Perhaps in the companion object, perhaps in a separate enum somewhere, perhaps as a StringDef. But, as a consumer I'm going to do:

errorDetails.get("PLATFORM_EXCEPTIONSSS") // Ooops, this doesn't work at runtime and I get confused because my project has thousands of lines of code -- n^2 complexity to this problem, the more runtime errors I ahve instead of compile time

jamesonwilliams · 2020-08-14T21:45:02Z

...droid/src/main/kotlin/com/amazonaws/amplify/amplify_storage_s3/types/FlutterGetUrlRequest.kt

+    val key: String = request["key"] as String
+    val options: StorageGetUrlOptions = setOptions(request)
+
+    private fun setOptions(request: HashMap<String, *>): StorageGetUrlOptions {


Map, Liskov

jamesonwilliams · 2020-08-15T19:28:46Z

packages/amplify_storage_s3/ios/Classes/SwiftAmplifyStorageS3Plugin.swift

+        result(FlutterMethodNotImplemented)
+     }
+  }
+


Suggested change

jamesonwilliams · 2020-08-15T19:28:56Z

packages/amplify_storage_s3/ios/Classes/FlutterUploadFileRequest.swift

+        }
+        return nil
+    }
+


Suggested change

jamesonwilliams · 2020-08-15T19:29:11Z

packages/amplify_storage_s3/ios/Classes/FlutterUploadFileRequest.swift

+
+    private func setOptions(request: Dictionary<String, AnyObject>) -> StorageUploadFileRequest.Options? {
+
+        if(request["options"] != nil) {


Suggested change

if(request["options"] != nil) {

if (request["options"] != nil) {

jamesonwilliams · 2020-08-15T19:29:21Z

packages/amplify_storage_s3/ios/Classes/FlutterUploadFileRequest.swift

+    }
+
+    private func setOptions(request: Dictionary<String, AnyObject>) -> StorageUploadFileRequest.Options? {
+


Suggested change

jamesonwilliams · 2020-08-15T19:29:38Z

packages/amplify_storage_s3/ios/Classes/FlutterUploadFileRequest.swift

+
+    static func isValid(request: Dictionary<String, AnyObject>) -> Bool {
+        var valid: Bool = true;
+        if(!(request["key"] != nil && request["key"] is String)){


Suggested change

if(!(request["key"] != nil && request["key"] is String)){

if (!(request["key"] != nil && request["key"] is String)) {

jamesonwilliams · 2020-08-15T19:50:21Z

packages/amplify_storage_s3/example/android/app/build.gradle

+apply from: "$flutterRoot/packages/flutter_tools/gradle/flutter.gradle"
+
+android {
+    compileSdkVersion 28


I'd target 30. Make sure target and compile versions match.

jamesonwilliams · 2020-08-15T19:51:10Z

...d/src/main/kotlin/com/amazonaws/amplify/amplify_storage_s3/types/FlutterUploadFileRequest.kt

+    val file: File = File(request["path"] as String)
+    val options: StorageUploadFileOptions = setOptions(request)
+
+    private fun setOptions(request: HashMap<String, *>): StorageUploadFileOptions {


Use just the Map interface, for all method specs. HashMap only gets used when you're creating an instance.

Same comment for entire code base, not just this file.

jamesonwilliams · 2020-08-15T19:52:04Z

...droid/src/main/kotlin/com/amazonaws/amplify/amplify_storage_s3/types/FlutterGetUrlRequest.kt

+                        val accessLevel: StorageAccessLevel? = StorageAccessLevel.values().find { it.name == accessLevelStringOption.toUpperCase() }
+                        options.accessLevel(accessLevel)
+                    }
+                    "targetIdentityId" -> {


Can you get these strings into an enum? Using string literals like this is error prone, if you'll use them in more than one place.

jamesonwilliams · 2020-08-15T19:53:06Z

...android/src/main/kotlin/com/amazonaws/amplify/amplify_storage_s3/AmplifyStorageOperations.kt

+            }
+        }
+
+        fun getUrl(@NonNull flutterResult: MethodChannel.Result, @NonNull request: HashMap<String, *>, mainActivity: Activity?) {


Remove Activity from the method spec.

jamesonwilliams · 2020-08-15T19:54:29Z

...3/android/src/main/kotlin/com/amazonaws/amplify/amplify_storage_s3/AmplifyStorageS3Plugin.kt

+import io.flutter.plugin.common.PluginRegistry.Registrar
+
+
+class AmplifyStorageS3Plugin : FlutterPlugin, ActivityAware, MethodCallHandler {


Do you need to implement ActivityAware, or are you doing this because you wanted to use a mainActivity to exploit its runOnUiThread?

If at all possible, don't mess with an Activity, at all. You can run on the main thread via Handler(Looper.getMainLooper()). But even for that, make sure you have to. You have to, for UI operations. That's about it.

Ashish-Nanda · 2020-08-17T01:47:08Z

Created a PR off a new branch where I addressed most of the above comments along with other improvements. Closing this draft PR.

Ashish-Nanda added 3 commits July 20, 2020 08:22

Create storage category plugin and interface packages

7ac7453

Implement UploadFile for Storage category

aa2810c

Implement GetUrl for Storage category

3729b82

Ashish-Nanda requested a review from Amplifiyer August 7, 2020 22:48

haverchuck reviewed Aug 7, 2020

View reviewed changes

packages/amplify_core/example/ios/Podfile Show resolved Hide resolved

haverchuck reviewed Aug 7, 2020

View reviewed changes

packages/amplify_core/lib/amplify_core.dart Outdated Show resolved Hide resolved

haverchuck reviewed Aug 7, 2020

View reviewed changes

packages/amplify_core_plugin_interface/lib/amplify_storage_category.dart Outdated Show resolved Hide resolved

haverchuck reviewed Aug 7, 2020

View reviewed changes

packages/amplify_storage_plugin_interface/README.md Show resolved Hide resolved

haverchuck reviewed Aug 7, 2020

View reviewed changes

packages/amplify_storage_plugin_interface/lib/src/GetUrl/GetUrlOptions.dart Show resolved Hide resolved

haverchuck reviewed Aug 7, 2020

View reviewed changes

packages/amplify_storage_s3/README.md Outdated Show resolved Hide resolved