Null safety master #595

fjnoyp · 2021-05-26T18:19:42Z

Description of changes:

This is a temporary PR to run unit tests for merging all of our current null safety changes into the master branch.

We have yet to determine how the final tagged release of Null Safety will be handled.

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

Null Safety upgrade for (to min Dart 2.12): Core Flutter All Plugin Interface Packages

Jordan-Nelson · 2021-06-02T20:32:39Z

...android/src/main/kotlin/com/amazonaws/amplify/amplify_auth_cognito/FlutterNextStepUtility.kt

+   val result =  mutableMapOf<String, Any>(resultKey to resultValue)
+   val codeDeliveryDetailsResult = mutableMapOf<String, String?>()
+
+   if (codeDeliveryDetails is AuthCodeDeliveryDetails) {


Are these changes needed for null safety? Anything passed over the method channel could be null. Do any of the changes in kotlin/swift impact nullability on the dart side?

FYI - Some of these refactors are going to conflict with changes I have in an open PR. Not a huge deal. We can discuss how we want to reconcile it before either PR is merged potentially. I would suggest we limit how much refactoring is done in the null safety branches though (or any change of this scope). There could be a lot of drift between the null safety branches and master.

After looking again, I don't think these will actually conflict since you didn't consume this utility in FlutterUpdateUserAttributeResult.kt. We will have two similar utils but I guess they could be combined later.

Should FlutterUpdateUserAttributeResult.kt be updated? Was there an issue with how the next step was being set or were you just trying to consolidate duplicated logic?

@haverchuck could you provide your insights here. I think Dustin might have added this as part of his code updates so I think he should have the final say on the best next step. From what I understand/see I'm guessing these should be consolidated.

We were just trying to consolidate code with this file - it was requested as part of code review.
I think it would make sense to update the FlutterUpdateUserAttributeResult class to use this same utility.

Jordan-Nelson · 2021-06-02T20:47:16Z

packages/amplify_auth_cognito/lib/amplify_auth_cognito.dart

@@ -121,20 +121,20 @@ class AmplifyAuthCognito extends AuthPluginInterface {
  }

  Future<UpdateUserAttributeResult> updateUserAttribute(
-      {@required UpdateUserAttributeRequest request}) async {
+      {UpdateUserAttributeRequest? request}) async {


I think this should be required? I see all the other requests are also optional so maybe I am missing something. I can't think of a scenario in which this should be optional though.

@haverchuck can you add your insights here?

Yeah I would think this is required, since the customer has to pass the attribute-to-be-updated.

Jordan-Nelson · 2021-06-02T20:47:57Z

packages/amplify_auth_cognito/lib/method_channel_auth_cognito.dart

@@ -275,26 +275,26 @@ class AmplifyAuthCognitoMethodChannel extends AmplifyAuthCognito {

  @override
  Future<UpdateUserAttributeResult> updateUserAttribute(
-      {@required UpdateUserAttributeRequest request}) async {
+      {UpdateUserAttributeRequest? request}) async {


Similar to above, I think this should be required. Not sure if I am missing something.

Same as above.

Jordan-Nelson · 2021-06-02T20:49:23Z

packages/amplify_auth_plugin_interface/lib/amplify_auth_plugin_interface.dart

@@ -94,19 +94,19 @@ abstract class AuthPluginInterface extends AmplifyPluginInterface {
  }

  Future<UpdateUserAttributeResult> updateUserAttribute(
-      {@required UpdateUserAttributeRequest request}) {
+      {UpdateUserAttributeRequest? request}) {


Same as above (I would expect this to be required)

Same as above.

Jordan-Nelson · 2021-06-03T16:32:39Z

packages/amplify_auth_cognito/lib/method_channel_auth_cognito.dart

+                    attributeName: codeDeliveryDetails["attributeName"] ?? null,
+                    deliveryMedium:
+                        codeDeliveryDetails["deliveryMedium"] ?? null,
+                    destination: codeDeliveryDetails["destination"])


This is throwing an error, type 'Null' is not a subtype of type 'String'. You are checking that codeDeliveryDetails != null, but codeDeliveryDetails["destination"] can still be null. I am guessing this is the main reason you changed the kotlin/swift side and added that util (to consistently return nulls, instead of empty maps)?

This can be fixed by updating the swift/kotlin side for FlutterUpdateUserAttributeResult in the same way that the other results (signUp, signIn, resetPassword, etc.) were updated so that it returns null instead of an empty map. This should be straightforward now, but a heads up that it will cause this PR to conflict with #601. My first thought was that I could pull some of these changes into #601 so that we don't have to worry about conflicts later but I don't think I can really do that since even just the changes on the swift/kt side could be considered breaking (returning null instead of an empty map is technically a change to the public AP). If we go this route, I think we will have to just reconcile some conflicts/inconsistencies when you next pull master into this branch.

Another option would be to add checks for empty maps (or null codeDeliveryDetails["destination"]) on the dart side (which maybe should be added anyway?) and then I think the changes on the swift/kt side would not be needed. Returning nulls instead of empty maps and consolidating the logic makes sense but it seems a little out of scope for dart null safety upgrade and if I am understanding the purpose of the changes, it seems easy enough to resolve on the dart side.

It is worth noting that with either approach there is more work that should eventually be done to consolidate logic and return more logical values across auth.

Thanks. @haverchuck I noticed we made destination field of AuthCodeDeliveryDetails class required, can we make it nullable like all of the other fields in that class?

@Jordan-Nelson Which method is throwing this error?

updateUserAttribute(). Only on iOS when you update an attribute that doesn't have a confirmation code (anything other than email/phone)

Heh - sorry I realized just now I could have seen this for myself. This makes sense. I'm taking a look.

Jordan-Nelson · 2021-06-03T18:22:58Z

packages/amplify_auth_cognito/lib/src/CognitoSession/AWSCognitoUserPoolTokens.dart

-  }
+  AWSCognitoUserPoolTokens.init(
+      {required LinkedHashMap<dynamic, dynamic> tokens})
+      : this.accessToken = tokens["accessToken"],


This throws _TypeError (type 'Null' is not a subtype of type 'String') when the user is not sign in (seen on iOS, have not tested Android)

This is a scenario in which null safety ideally should force you to do a null check, but it looks like it doesn't force you to do that with dynamics. It would if the type of tokens was LinkedHashMap<String, String>, not LinkedHashMap<dynamic, dynamic>. I'm not saying we should necessarily change it. LinkedHashMap<dynamic, dynamic> might be the correct type. Its definitely something to pay attention to though.

The method throwing the error is Amplify.Auth.fetchAuthSession(). I could only repro this on iOS

Working on a fix for this - for now on the platform side.

I also think <String, String> is right here - going to play around with it.

Hmm - I think we had this as dynamic because of how the data was typed coming over the method channel.

codecov-commenter · 2021-06-09T16:07:39Z

Codecov Report

Merging #595 (bf394bb) into master (2516e81) will increase coverage by 1.04%.
The diff coverage is 80.91%.

@@            Coverage Diff             @@
##           master     #595      +/-   ##
==========================================
+ Coverage   68.32%   69.36%   +1.04%     
==========================================
  Files         252      262      +10     
  Lines        8252     8604     +352     
  Branches      346      353       +7     
==========================================
+ Hits         5638     5968     +330     
- Misses       2481     2504      +23     
+ Partials      133      132       -1

Flag	Coverage Δ
android-unit-tests	`63.86% <82.83%> (+2.16%)`	⬆️
flutter-unit-tests	`60.91% <82.39%> (+0.50%)`	⬆️
ios-unit-tests	`75.17% <78.48%> (+0.78%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...s/amplify_api/ios/Classes/FlutterApiResponse.swift	`0.00% <0.00%> (ø)`
packages/amplify_api/lib/amplify_api.dart	`60.71% <0.00%> (ø)`
...ognito/types/FlutterConfirmUserAttributeRequest.kt	`70.00% <ø> (ø)`
...utterResendUserAttributeConfirmationCodeRequest.kt	`57.14% <ø> (ø)`
...amplify_auth_cognito/types/FlutterSignUpRequest.kt	`47.82% <ø> (ø)`
...auth_cognito/utils/UserAttributeDeserialization.kt	`26.08% <ø> (ø)`
...y_auth_cognito/ios/Classes/AuthCognitoBridge.swift	`0.64% <0.00%> (-0.04%)`	⬇️
...s/Utils/AuthCodeDeliveryDetailsSerialization.swift	`0.00% <0.00%> (ø)`
...CognitoPasswords/CognitoUpdatePasswordOptions.dart	`0.00% <0.00%> (ø)`
...cognito/lib/src/CognitoSession/AWSCredentials.dart	`0.00% <0.00%> (ø)`
... and 119 more

* Version Update to 0.2.0-nullsafety.0 * Update amplify.dart Co-authored-by: Dustin Noyes <[email protected]>

* Fix HubEventElements for NS * Refactor and add tests

* chore: pin Amplify iOS to '~> 1.9.2' (#589) * set amplify ios to '~> 1.9' * set amplify ios to '~> 1.9.2' * add 'ObjectMapper' back to podspec Co-authored-by: Ashish Nanda <[email protected]> * chore: foundation for integration tests and basic auth suite with signIn and signOut (#568) * feat(auth): add updateUserAttributes (batch) (#601) * feat: add auth.updateUserAttributes * Apply suggestions from code review Co-authored-by: Chris F <[email protected]> * address pr comments * refactor android user attributes * consolidate user attr logic on iOS * refactor deliveryDetails serialization * add missing newline * revert missing attribute changes * update serializeAuthUpdateAttributeResult for nil * move comment to to of file * bump amplify-android to 1.17.7 Co-authored-by: Chris F <[email protected]> * chore(amplify_api): add httpStatusCode property to ApiException when available from REST response (#590) Add integer property httpStatusCode to ApiException. In android and iOS, look for the status code in the response/error object and add to the serialized map. In flutter, add the new property to the serialization logic specifically for ApiException. * fix(auth): Add clientMetadata to confirmSignUp API options (#619) * Chore: remove check for duplicate error in Storage (#618) * remove check for dup err cb * bump amplify-android to version 1.17.8 * fix(auth): iOS/Android user attribute inconsistencies (#620) * feat: add auth.updateUserAttributes * Apply suggestions from code review Co-authored-by: Chris F <[email protected]> * address pr comments * refactor android user attributes * consolidate user attr logic on iOS * refactor deliveryDetails serialization * add missing newline * revert missing attribute changes * update serializeAuthUpdateAttributeResult for nil * move comment to to of file * bump amplify-android to 1.17.7 * iOS: handle missing and custom attributes * add constant for customKeyPrefix Co-authored-by: Chris F <[email protected]> * Add support of DataStore custom configuration (#610) * refactor: Minor refactor of DataStore methods * Free up `configure` method for native passthrough * Move observe configuration into its own explicit method * Corrected observe setup method in swift to call the invokeMethod callback so the call can be properly awaited upon in dart layer * Broaden `configureDataStore` naming to convey more than model provider being able to be configured through this method * Allow plugins to be added in parallel instead of in a series * Update unit tests to correctly assert observe call result * Remove setUpObserve API. Restore setUpObserve call to configure * feat(datastore) Allow configure DataStore with custom configuration * Reintroduce configureModelProvider as deprecated method * chore(datastore) Add unit tests for custom DataStore configuration * Update configure to not throw error if DataStore plugin is not found * Split datastore configuration unit tests by use cases Co-authored-by: Chris Fang <[email protected]> * Use "flutter pub" in melos.yaml (and everywhere else) (#603) * Use dart pub in melos.yaml * Fix all references to 'pub _' with 'flutter pub _' * Work around hanging build; don't install in postbootstrap * fix(tests): add --no-pub to integration tests (#637) * add --no-pub to integration tests * fix typo * fix: CONTRIBUTING Dead Links (#630) * fix(amplify_auth_cognito): adds userAttributes to confirmSignIn (#607) * chore: Add CircleCI badge (#631) * chore: Add CircleCI badge * fix: changed badge hyperlink * fix: badage now points towards master branch * Update README.md Co-authored-by: Jordan Nelson <[email protected]> Co-authored-by: Jordan Nelson <[email protected]> * chore: upgrade amplify-android to 1.19.0 (#650) * fix: amplify-ios version bump (#648) * fix: amplify-ios version bump ~> 1.11.0 (#665) * chore: release 0.1.6 (#667) * fix: amplify-ios version bump ~> 1.11.0 * chore: release 0.1.6 * Update CHANGELOG.md Co-authored-by: Noyes <[email protected]> * Pagination with Datastore #500 (#673) * Fix compile errors * Fix broken unit tests * Fix Android Unit Tests * fix(amplify_datastore): force cast exception codegen 1) add new exception for force cast in codegen models # Conflicts: # packages/amplify_datastore_plugin_interface/lib/src/publicTypes.dart * Fix ios unit tests This file was corrupted during the rebase * Update codegen models to latest * PR Comments * Fix Android Unit Tests * Last PR Comment Co-authored-by: Jordan Nelson <[email protected]> Co-authored-by: Ashish Nanda <[email protected]> Co-authored-by: Travis Sheppard <[email protected]> Co-authored-by: Chris F <[email protected]> Co-authored-by: Hui Zhao <[email protected]> Co-authored-by: Chris Fang <[email protected]> Co-authored-by: A.J. Gardner <[email protected]> Co-authored-by: Elijah Quartey <[email protected]> Co-authored-by: Dustin Noyes <[email protected]> Co-authored-by: Noyes <[email protected]> Co-authored-by: Mo Malaka <[email protected]>

This reverts commit 00dbc7a.

…der"" (#682) remove deprecated method - configureModelProvider Co-authored-by: Chris Fang <[email protected]>

* remove use of type token from auth session * remove TypeToken from exception util

* update user attr res serialization * update unit tests

* Fix enum string function * Update logic * Revert "Update logic" This reverts commit 75d257a.

# Conflicts: # example/pubspec.yaml # packages/amplify_analytics_pinpoint/CHANGELOG.md # packages/amplify_analytics_pinpoint/pubspec.yaml # packages/amplify_analytics_plugin_interface/CHANGELOG.md # packages/amplify_analytics_plugin_interface/pubspec.yaml # packages/amplify_api/CHANGELOG.md # packages/amplify_api/pubspec.yaml # packages/amplify_api_plugin_interface/CHANGELOG.md # packages/amplify_api_plugin_interface/lib/src/exceptions/ApiException.dart # packages/amplify_api_plugin_interface/pubspec.yaml # packages/amplify_auth_cognito/CHANGELOG.md # packages/amplify_auth_cognito/android/src/main/kotlin/com/amazonaws/amplify/amplify_auth_cognito/utils/AuthCodeDeliveryDetailsSerialization.kt # packages/amplify_auth_cognito/android/src/main/kotlin/com/amazonaws/amplify/amplify_auth_cognito/utils/UserAttributeSerialization.kt # packages/amplify_auth_cognito/android/src/test/kotlin/com/amazonaws/amplify/amplify_auth_cognito/AmplifyAuthCognitoPluginTest.kt # packages/amplify_auth_cognito/example/lib/Widgets/UpdateUserAttributes.dart # packages/amplify_auth_cognito/ios/Classes/Utils/UserAttributeSerialization.swift # packages/amplify_auth_cognito/lib/amplify_auth_cognito.dart # packages/amplify_auth_cognito/lib/method_channel_auth_cognito.dart # packages/amplify_auth_cognito/lib/src/CognitoSignIn/CognitoConfirmSignInOptions.dart # packages/amplify_auth_cognito/lib/src/CognitoSignUp/CognitoConfirmSignUpOptions.dart # packages/amplify_auth_cognito/pubspec.yaml # packages/amplify_auth_cognito/test/amplify_auth_cognito_confirmSignIn_test.dart # packages/amplify_auth_cognito/test/amplify_auth_cognito_updateUserAttributes_test.dart # packages/amplify_auth_plugin_interface/CHANGELOG.md # packages/amplify_auth_plugin_interface/lib/amplify_auth_plugin_interface.dart # packages/amplify_auth_plugin_interface/lib/src/Session/UpdateUserAttributesRequest.dart # packages/amplify_auth_plugin_interface/lib/src/SignUp/ConfirmSignUpRequest.dart # packages/amplify_auth_plugin_interface/pubspec.yaml # packages/amplify_core/CHANGELOG.md # packages/amplify_core/pubspec.yaml # packages/amplify_datastore/CHANGELOG.md # packages/amplify_datastore/lib/amplify_datastore.dart # packages/amplify_datastore/lib/method_channel_datastore.dart # packages/amplify_datastore/pubspec.yaml # packages/amplify_datastore_plugin_interface/CHANGELOG.md # packages/amplify_datastore_plugin_interface/lib/amplify_datastore_plugin_interface.dart # packages/amplify_datastore_plugin_interface/lib/src/publicTypes.dart # packages/amplify_datastore_plugin_interface/pubspec.yaml # packages/amplify_flutter/CHANGELOG.md # packages/amplify_flutter/lib/categories/amplify_auth_category.dart # packages/amplify_flutter/lib/categories/amplify_datastore_category.dart # packages/amplify_flutter/pubspec.yaml # packages/amplify_storage_plugin_interface/CHANGELOG.md # packages/amplify_storage_plugin_interface/pubspec.yaml # packages/amplify_storage_s3/CHANGELOG.md # packages/amplify_storage_s3/pubspec.yaml

fjnoyp requested a review from a team as a code owner May 26, 2021 18:19

fjnoyp force-pushed the null-safety-master branch from 3244c3e to 0ab3211 Compare June 1, 2021 21:07

fjnoyp added 5 commits June 1, 2021 17:36

feat(amplify): Null Safety Core (#492)

9adafd2

Null Safety upgrade for (to min Dart 2.12): Core Flutter All Plugin Interface Packages

feat(amplify_auth): Null Safety Auth (#536)

307db67

feat(amplify_analytics): Null safety (#483)

c658308

feat(amplify_storage_s3): Null safety (#485)

e4af0c1

feat(amplify_api): Null safety api (#484)

ba1181f

fjnoyp force-pushed the null-safety-master branch from 0ab3211 to ba1181f Compare June 2, 2021 00:39

fjnoyp added 2 commits June 1, 2021 17:39

fix: Broken Unit Tests

692d60b

fix: upgrade Auth changes to null safety

ec7fab7

Jordan-Nelson reviewed Jun 2, 2021

View reviewed changes

fix: Broken Auth Unit Tests

015cecf

fjnoyp force-pushed the null-safety-master branch from 79eb0cc to 015cecf Compare June 2, 2021 23:15

Jordan-Nelson reviewed Jun 3, 2021

View reviewed changes

haverchuck and others added 3 commits June 8, 2021 12:43

fix(amplify_auth_cognito): fixes auth null safety issues (#613)

d4e9dbb

Example app null safety fixes (#617)

9479d4a

Null safety master fixes pr (#614)

400d987

fjnoyp and others added 11 commits June 9, 2021 11:08

chore(release): 0.2.0-nullsafety.0 (#622)

ea81a38

* Version Update to 0.2.0-nullsafety.0 * Update amplify.dart Co-authored-by: Dustin Noyes <[email protected]>

Update FlutterFetchCognitoAuthSessionResult.kt (#653)

e68a4be

Null safety datastore (#649)

75f78a1

chore: release 0.2.0-nullsafety.1 (#656)

e456ecb

bug(datastore): HubEvent null check (#670)

b34b71f

* Fix HubEventElements for NS * Refactor and add tests

fix(amplify_datstore): mprovider compile issue (#681)

d23a816

chore: remove deprecated method - configureModelProvider

00dbc7a

Revert "chore: remove deprecated method - configureModelProvider"

9f0d90a

This reverts commit 00dbc7a.

Revert "Revert "chore: remove deprecated method - configureModelProvi…

dc252de

…der"" (#682) remove deprecated method - configureModelProvider Co-authored-by: Chris Fang <[email protected]>

fix(Auth): Remove use of TypeToken (#683)

04358e8

* remove use of type token from auth session * remove TypeToken from exception util

Jordan-Nelson and others added 5 commits June 29, 2021 18:15

fix(auth): update user attribute serialization (#684)

1b197f6

* update user attr res serialization * update unit tests

Fix enum string function (#687)

d3bdb80

* Fix enum string function * Update logic * Revert "Update logic" This reverts commit 75d257a.

fix(auth): uses nextStep helper for signInResult on Android (#689)

5eeca68

chore: release 0.2.0 (#690)

bf394bb

fjnoyp closed this Jul 2, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Null safety master #595

Null safety master #595

fjnoyp commented May 26, 2021

Jordan-Nelson Jun 2, 2021

Jordan-Nelson Jun 3, 2021 •

edited

Loading

fjnoyp Jun 3, 2021

haverchuck Jun 3, 2021

Jordan-Nelson Jun 2, 2021

fjnoyp Jun 3, 2021

haverchuck Jun 3, 2021

Jordan-Nelson Jun 2, 2021

haverchuck Jun 3, 2021

Jordan-Nelson Jun 2, 2021

haverchuck Jun 3, 2021

Jordan-Nelson Jun 3, 2021 •

edited

Loading

fjnoyp Jun 3, 2021

haverchuck Jun 3, 2021

Jordan-Nelson Jun 3, 2021

haverchuck Jun 4, 2021

Jordan-Nelson Jun 3, 2021

Jordan-Nelson Jun 3, 2021

Jordan-Nelson Jun 3, 2021

haverchuck Jun 3, 2021

haverchuck Jun 3, 2021

haverchuck Jun 3, 2021

codecov-commenter commented Jun 9, 2021 •

edited

Loading

Null safety master #595

Null safety master #595

Conversation

fjnoyp commented May 26, 2021

Choose a reason for hiding this comment

Jordan-Nelson Jun 3, 2021 • edited Loading

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

Jordan-Nelson Jun 3, 2021 • edited Loading

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

codecov-commenter commented Jun 9, 2021 • edited Loading

Codecov Report

Jordan-Nelson Jun 3, 2021 •

edited

Loading

Jordan-Nelson Jun 3, 2021 •

edited

Loading

codecov-commenter commented Jun 9, 2021 •

edited

Loading