Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add generated iOS plugin, documentation, and serialization methods #11

Merged
merged 8 commits into from
Jul 20, 2021

Conversation

Mrhea
Copy link
Contributor

@Mrhea Mrhea commented Jul 14, 2021

This PR adds the code generated by Flutter for an iOS plugin and the changes to it necessary to connect to Tozny's Flutter client, first versions of serialization methods relevant to implementing Tozny SDK methods, and documentation for how to setup and run the plugin on Xcode.

There are also changes to the Flutter Dart client to handle the errors introduced by the null safety system in the newer version of the sdk.

Note that this PR does not contain an implementation of write record, this is noted in the [WASH-929] Add docs and serialization methods commit. The plugin is confirmed to be working because getPlatformVersion method works and shows the expected output in a simulator.

Matthew Rhea added 7 commits July 9, 2021 09:27
- Adds new writeRecord method to Tozny iOS plugin
- Adds WIP tests that need to be completed before writeRecord
  fully implemented

Client in Dart test is needs to use credentials from an existing
TozStore client. Completed tests will be added shortly.
- Remove unnecessary code and comments throughout plugin code
- Fixes parameters in the writeRecord method in the plugin class

Note that this commit will fail to run the app because of a
serialization. This will be resolved in the near future.
- Updates SwiftFlutterPlugin writeRecord method to use correct
  JSON serialization methods
- Adds FlutterConfig class as a level of indirection to bridge
  the tozny_model.dart ClientCredentials class to the E3db Swift
  SDK's Config class
- Updates the tozny_model.dart ClientCredentials class to
  include the clientName field as it is required by the
  E3db Swift SDK's Config class
- Updates tozny_model.dart and plugin_identity.dart to fix
  non-nullable errors caused by updated dev-dependency 'sdk'

Note that the changes to Dart code need to be error handled
properly as making many of the fields nullable possibly
introduces downstream errors and thrown exceptions when fields
expected to be non-null are null.
- Adds a WIP extension for the JSONDecoder object in
  Swift: convertFromFlutterConfigToE3dbConfig
- The extension has not had its first version written,
  but the general idea is to use this such that the
  decoder will customize the values of keys in FlutterConfig
  to match those of an E3DB Config object
- Removes unnecessarily complicated solution to converting
  a FlutterConfig to E3db Config
- Clients are initialized properly now, but there exists
  a suspected authentication issue when writing records
- Changes the types of url and uuid fields in FlutterConfig
  to be Strings

The `flutter create` command will complain if the repository name does not adhere to this convention, so make sure to rename the repository's name if necessary.

At the time of this writing the Tozny plugin's repository is named such that it will encounter this problem.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am going to rename this repository so that it is compliant. This will require existing clones of the repo to update their upstream URL. To do that in the directory rungit remote set-url origin git@github.com:tozny/flutter_plugin

Copy link
Contributor

@efabens efabens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://stackoverflow.com/questions/49478/git-ignore-file-for-xcode-projects

We briefly talked in this, but then I dug a little more. I think some of the code files should get added to the gitignore. But I wouldn't consider it an emergency

DEVELOPMENT.md Outdated

The Tozny iOS plugin requires [XCode](https://developer.apple.com/xcode/) and [Cocoapods](https://cocoapods.org/) installed in order to be built.

Before opening the project, make sure that all necessary project dependencies and pods are installed by running the `flutter pub get` and `pod install` commands. This installs the dependencies and pods found in the project's Podfile and each module's `.podspec` file.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is pod install run from the root directory?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, from /example/ios. Will update, thanks for catching that

Copy link
Contributor

@efabens efabens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a great start. Good looking documentation.

@@ -14,7 +14,7 @@ packages:
name: analyzer
url: "https://pub.dartlang.org"
source: hosted
version: "1.7.2"
version: "1.7.1"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the reason for downgrading the versions in this file?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this was done automatically after a flutter pub get I did at some point. I will trace down why this happened and get them upgraded again, but off the top of my head I only know this was not done intentionally

@@ -14,7 +14,7 @@ packages:
name: analyzer
url: "https://pub.dartlang.org"
source: hosted
version: "1.7.2"
version: "1.7.1"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same question re dependencies?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same reply above

data.updateValue(E3dbSerializer.userAgentTokenToJson(tok: id.agentInfo()), forKey: "user_agent_token")

let jsonData = try! JSONSerialization.data(withJSONObject: data, options: .prettyPrinted)
guard let jsonString = String(data: jsonData, encoding: .utf8) else { return "Something bad happened." } // TODO: Error
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's update these "Something bad happened" to "Failed to <what failed in 3-7 words>" so for this something like Failed to stringify an idClient JsonObject.

Also as I am not a swift expert would it be better to have this throw an error of some kind if this method fails? With this string a downstream consumer will need to equality check for failures, is there a more canonical way to show failures to method callers? Potentially that is what the error handling notes you have are about, if so we should do that sooner rather than later.

Copy link
Contributor Author

@Mrhea Mrhea Jul 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, that is the intention of the error handling notes. An option is to, instead of guard (which returns early), provide a default value. e.g. let jsonString = String(...) ?? "". There are better patterns where I can throw errors, though. The try! is a forced unwrapping, which is generally discouraged. Updating the error handling will include removing forced unwraps

I agree the guard pattern is not what should be used here, these will be fixed.

EDIT: Well, guard let may be appropriate... it is supposed to provide an early exit. If I mark the function with throws (e.g. static func idClientToJson(...) throws {...} ), then I can throw an error in the guard's else clause. If I represent serialization errors in an enum, then throw in case of failure then it'll propagate to the surrounding scope

Example of error representation:

enum E3dbSerializationError: Error {
    case invalidEncoding(message: String), 
    ...
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should return some type of error whether it is checked and handled or a runtime exception that crashes the program with a serialization error (That appears to be the approach we use on the java side). But propagating a string that downstream functions will not be able to correctly handle seems like the worst of the three options

data.updateValue(meta.fileMeta, forKey: "file_meta")

let jsonData = try! JSONSerialization.data(withJSONObject: data, options: .prettyPrinted)
guard let jsonString = String(data: jsonData, encoding: .utf8) else { return "Something bad happened." } // TODO: ERROR HANDLING
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see above for this and all the below lines that are the same structure.

sjonz
sjonz previously approved these changes Jul 15, 2021
@sjonz
Copy link

sjonz commented Jul 15, 2021

It might be worthwhile to group some of these commits together in an interactive rebase? Not required, but it would be nice to clean up the WIP commits at least.

@sjonz sjonz dismissed their stale review July 15, 2021 00:42

accidental click

@Mrhea Mrhea force-pushed the wash-929-e3db-serialization-and-documentation branch 4 times, most recently from 4a3c35e to 013c2c1 Compare July 16, 2021 16:57
- Adds development documentation for setting up and
  running the Flutter iOS plugin
- Adds FlutterConfigSerializer to bridge Flutter Dart
  client's ClientCredentials object to E3db's Config
- Adds RealmConfigSerializer
- Adds encoding and decoding methods for E3db objects

Note that this PR removed an incomplete version of the
writeRecord method.
@Mrhea Mrhea force-pushed the wash-929-e3db-serialization-and-documentation branch from 013c2c1 to ecfcb1b Compare July 20, 2021 18:40
@Mrhea Mrhea merged commit 957ffc1 into trunk Jul 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants