-
Notifications
You must be signed in to change notification settings - Fork 249
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
chore: update configure apis to use AmplifyOutputs instead of AmplifyConfig #5017
Changes from all commits
b60fb0a
9dc9c24
a9cb8f8
835ebfd
fb3c59c
7f458e7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,7 +2,9 @@ | |
// SPDX-License-Identifier: Apache-2.0 | ||
|
||
import 'package:amplify_core/amplify_core.dart'; | ||
import 'package:amplify_core/src/config/amplify_outputs/auth/auth_outputs.dart'; | ||
import 'package:json_annotation/json_annotation.dart'; | ||
import 'package:meta/meta.dart'; | ||
|
||
part 'oauth.g.dart'; | ||
|
||
|
@@ -27,6 +29,46 @@ class CognitoOAuthConfig | |
this.tokenUriQueryParameters, | ||
}); | ||
|
||
@internal | ||
factory CognitoOAuthConfig.fromAuthOutputs(AuthOutputs authOutputs) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P.S. this used in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we have a follow up task to remove use of |
||
if (authOutputs.userPoolClientId == null) { | ||
throw ConfigurationError('Invalid config: no User Pool Client Id found.'); | ||
} | ||
if (authOutputs.oauth == null) { | ||
throw ConfigurationError('Invalid config: no oAuth configuration found.'); | ||
} | ||
|
||
final appClientId = authOutputs.userPoolClientId!; | ||
final appClientSecret = authOutputs.appClientSecret; | ||
final scopes = authOutputs.oauth!.scopes; | ||
final signInUri = authOutputs.oauth!.signInUri; | ||
final signOutUri = authOutputs.oauth!.signOutUri; | ||
final signInUriQueryParameters = | ||
authOutputs.oauth!.signInUriQueryParameters; | ||
final signOutUriQueryParameters = | ||
authOutputs.oauth!.signOutUriQueryParameters; | ||
final signInRedirectUri = authOutputs.oauth!.redirectSignInUri.join(','); | ||
final signOutRedirectUri = authOutputs.oauth!.redirectSignOutUri.join(','); | ||
final webDomain = authOutputs.oauth!.domain; | ||
final tokenUri = authOutputs.oauth!.tokenUri; | ||
final tokenUriQueryParameters = authOutputs.oauth!.tokenUriQueryParameters; | ||
|
||
return CognitoOAuthConfig( | ||
appClientId: appClientId, | ||
appClientSecret: appClientSecret, | ||
scopes: scopes, | ||
signInUri: signInUri, | ||
signOutUri: signOutUri, | ||
signInRedirectUri: signInRedirectUri, | ||
signOutRedirectUri: signOutRedirectUri, | ||
webDomain: webDomain, | ||
signInUriQueryParameters: signInUriQueryParameters, | ||
signOutUriQueryParameters: signOutUriQueryParameters, | ||
tokenUri: tokenUri, | ||
tokenUriQueryParameters: tokenUriQueryParameters, | ||
); | ||
} | ||
|
||
factory CognitoOAuthConfig.fromJson(Map<String, Object?> json) => | ||
_$CognitoOAuthConfigFromJson(json); | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,6 +2,8 @@ | |
// SPDX-License-Identifier: Apache-2.0 | ||
|
||
import 'package:amplify_core/amplify_core.dart'; | ||
import 'package:amplify_core/src/config/amplify_outputs/auth/auth_outputs.dart'; | ||
import 'package:meta/meta.dart'; | ||
|
||
part 'user_pool.g.dart'; | ||
|
||
|
@@ -17,6 +19,29 @@ class CognitoUserPoolConfig | |
this.endpoint, | ||
}); | ||
|
||
@internal | ||
factory CognitoUserPoolConfig.fromAuthOutputs(AuthOutputs authOutputs) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P.S. this used in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same question as above. Do we have a follow up task to remove use of this? |
||
if (authOutputs.userPoolId == null) { | ||
throw ConfigurationError( | ||
'Invalid Cognito User Pool config: No User Pool Id found', | ||
); | ||
} | ||
if (authOutputs.userPoolClientId == null) { | ||
throw ConfigurationError( | ||
'Invalid Cognito User Pool config: No User Pool Client Id found', | ||
); | ||
} | ||
return CognitoUserPoolConfig( | ||
poolId: authOutputs.userPoolId!, | ||
appClientId: authOutputs.userPoolClientId!, | ||
appClientSecret: authOutputs.appClientSecret, | ||
region: authOutputs.awsRegion, | ||
hostedUI: authOutputs.oauth == null | ||
? null | ||
: CognitoOAuthConfig.fromAuthOutputs(authOutputs), | ||
); | ||
} | ||
|
||
factory CognitoUserPoolConfig.fromJson(Map<String, Object?> json) => | ||
_$CognitoUserPoolConfigFromJson(json); | ||
final String poolId; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,6 +2,7 @@ | |
// SPDX-License-Identifier: Apache-2.0 | ||
|
||
import 'package:amplify_core/amplify_core.dart'; | ||
import 'package:amplify_core/src/config/amplify_outputs/notifications/notifications_outputs.dart'; | ||
|
||
/// {@template amplify_core.push.service_provider_client} | ||
/// A base class for new service providers to implement and add functionality | ||
|
@@ -11,7 +12,7 @@ import 'package:amplify_core/amplify_core.dart'; | |
abstract class ServiceProviderClient { | ||
/// Initialize this client, used by the plugin during configuration. | ||
Future<void> init({ | ||
required NotificationsPinpointPluginConfig config, | ||
required NotificationsOutputs config, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Q: Is this an internal type? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes, it is not exporeted. |
||
required AmplifyAuthProviderRepository authProviderRepo, | ||
}); | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -73,10 +73,10 @@ android { | |
} | ||
|
||
dependencies { | ||
implementation 'com.amplifyframework:aws-auth-cognito:2.15.0' | ||
implementation "com.amplifyframework:aws-api:2.15.0" | ||
implementation "com.amplifyframework:aws-datastore:2.15.0" | ||
implementation "com.amplifyframework:aws-api-appsync:2.15.0" | ||
implementation 'com.amplifyframework:aws-auth-cognito:2.19.1' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Q: Should this go to main in a separate PR? |
||
implementation "com.amplifyframework:aws-api:2.19.1" | ||
implementation "com.amplifyframework:aws-datastore:2.19.1" | ||
implementation "com.amplifyframework:aws-api-appsync:2.19.1" | ||
implementation 'com.google.code.gson:gson:2.10.1' | ||
implementation 'org.jetbrains.kotlinx:kotlinx-coroutines-android:1.7.1' | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Q: was there a particular reason to make this change?
I think this change is fine, but am curious about what the motivation was. I had considered mvoing this change when I added rest API but decided not to because I thought it might be weird if
AmplifyOutputs.fromJson(outputs.toJson())
would not result in the originaloutputs
. Since AmplifyOutputs is private I don't think it really matters though.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in
packages/amplify_datastore/lib/amplify_datastore.dart
we call the.toJson()
method before calling NativeAmplifyBridge.configure(). having the rest_api included in toJson causes the config to become invalid Gen 2 config.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do Amplify iOS and/or Android consider the config invalid when there are extra key/value pairs? That seems potentially problematic since new key/value pairs would generally be considered non breaking.