-
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
Conversation
bcdc5f5
to
21a7e54
Compare
dcf5056
to
1f1552b
Compare
e62a64a
to
125a4c0
Compare
125a4c0
to
9dc9c24
Compare
@@ -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 comment
The reason will be displayed to describe this comment to others. Learn more.
P.S. this used in AuthConfiguration.fromAmplifyOutputs
AuthConfiguration is an internal type used in Auth State Machine and is differnt from AuthConfig.
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 we have a follow up task to remove use of CognitoOAuthConfig
? Maybe if AuthConfiguration
used OAuthOutputs
instead of CognitoOAuthConfig
the changes in the state machine would be minimal?
@@ -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 comment
The reason will be displayed to describe this comment to others. Learn more.
P.S. this used in AuthConfiguration.fromAmplifyOutputs
. AuthConfiguration is an internal type used in Auth State Machine and is differnt from AuthConfig.
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.
Same question as above. Do we have a follow up task to remove use of this?
fb83966
to
a9cb8f8
Compare
@@ -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 comment
The 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 comment
The reason will be displayed to describe this comment to others. Learn more.
yes, it is not exporeted.
} on Object catch (e) { | ||
throw ConfigurationError( | ||
'The provided configuration is not a valid json. ' | ||
'Check underlyingException.', | ||
recoverySuggestion: | ||
'Inspect your amplifyconfiguration.dart and ensure that ' | ||
'the string is proper json', | ||
'Inspect your amplify_output.dart or amplifyconfiguration.dart ' |
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.
'Inspect your amplify_output.dart or amplifyconfiguration.dart ' | |
'Inspect your amplify_outputs.dart or amplifyconfiguration.dart ' |
try { | ||
final Map<String, Object?> json; | ||
late final AmplifyConfig amplifyConfig; |
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: Can this be moved to where it is defined and late
be removed?
underlyingException: e, | ||
); | ||
} | ||
await _configurePlugins(amplifyConfig); | ||
_configCompleter.complete(amplifyConfig.toAmplifyOutputs()); | ||
try { |
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.
suggestion: The multiple try/catch blocks gets a little hard to follow. I would suggestion refactoring with small private functions.
Future<void> configure(String configuration) async {
if (isConfigured) {
throw const AmplifyAlreadyConfiguredException(
'Amplify has already been configured and re-configuration is not supported.',
recoverySuggestion:
'Check if Amplify is already configured using Amplify.isConfigured.',
);
}
late final AmplifyOutputs amplifyOutputs;
try {
final json = _decodeJson(configuration);
amplifyOutputs = _parseJsonConfig(json);
await _configurePlugins(amplifyOutputs);
_configCompleter.complete(amplifyOutputs);
} on ConfigurationError catch (e, st) {
// Complete with the configuration error and reset the completer so
// that 1) `configure` can be called again and 2) listeners registered
// on `asyncConfig` are notified of the error so they can handle it as
// appropriate. For example, the Authenticator listens for `asyncConfig`
// to complete before updating the UI.
_configCompleter.completeError(e, st);
rethrow;
} on Object {
// At this point, configuration is complete in the sense that the
// configuration file has been validated and plugins have had their
// `configure` methods run with no `ConfigurationException`s.
//
// Any other errors which occur during plugin configuration should be
// handled by the developer, but since they are unrelated to
// configuration, listeners to `Amplify.asyncConfig` should be allowed to
// proceed with the validated configuration.
_configCompleter.complete(amplifyOutputs);
_configCompleter = Completer();
rethrow;
}
}
AmplifyOutputs _parseJsonConfig(Map<String, Object?> json) {
try {
return AmplifyOutputs.fromJson(json);
} on Object {
try {
final amplifyConfig = AmplifyConfig.fromJson(json);
return amplifyConfig.toAmplifyOutputs();
} on Object catch (e) {
throw ConfigurationError(
'The provided configuration can not be decoded to AmplifyOutputs '
'or AmplifyConfig. '
'Check underlyingException.',
recoverySuggestion:
'If using Amplify Gen 2 ensure that the json string '
'can be decoded to AmplifyOutputs type. '
'If using Amplify Gen 1 ensure that the json '
'string can be decoded to AmplifyConfig type.',
underlyingException: e,
);
}
}
}
Map<String, Object?> _decodeJson(String configuration) {
try {
return jsonDecode(configuration) as Map<String, Object?>;
} on Object catch (e) {
throw ConfigurationError(
'The provided configuration is not a valid json. '
'Check underlyingException.',
recoverySuggestion:
'Inspect your amplify_output.dart or amplifyconfiguration.dart '
'and ensure that the string is proper json',
underlyingException: e,
);
}
}
@@ -55,6 +55,7 @@ class AmplifyOutputs | |||
|
|||
/// {@macro amplify_core.amplify_outputs.rest_api_outputs} | |||
@internal | |||
@JsonKey(includeToJson: false) |
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 original outputs
. 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.
@@ -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 comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have a follow up task to remove use of CognitoOAuthConfig
? Maybe if AuthConfiguration
used OAuthOutputs
instead of CognitoOAuthConfig
the changes in the state machine would be minimal?
@@ -13,7 +13,7 @@ import 'package:amplify_secure_storage_dart/amplify_secure_storage_dart.dart'; | |||
import 'package:stream_transform/stream_transform.dart'; | |||
import 'package:test/test.dart'; | |||
|
|||
const badConfig = AmplifyConfig(); | |||
final badConfig = const AmplifyConfig().toAmplifyOutputs(); |
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.
Should this be updated to not use AmplifyConfig
?
@@ -56,7 +56,8 @@ void main() { | |||
|
|||
final config = AmplifyConfig.fromJson( | |||
jsonDecode(amplifyconfig) as Map<String, Object?>, | |||
); | |||
// ignore: invalid_use_of_internal_member | |||
).toAmplifyOutputs(); |
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: Similar to other comments - Should this test be fully migrated to use AmplifyOutputs? Is there a task to capture that?
I would suggest we add TODOs to capture follow up work
@@ -21,7 +21,8 @@ void main() { | |||
), | |||
}, | |||
), | |||
); | |||
// ignore: invalid_use_of_internal_member | |||
).toAmplifyOutputs(); |
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.
Similar to other comments - Should we remove AmplifyConfig
completely from the tests?
@@ -94,7 +96,7 @@ class StorageS3Service { | |||
static final _defaultS3SignerConfiguration = | |||
sigv4.S3ServiceConfiguration(signPayload: false); | |||
|
|||
final S3PluginConfig _s3PluginConfig; | |||
final StorageOutputs _storageOutptus; |
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.
final StorageOutputs _storageOutptus; | |
final StorageOutputs _storageOutputs; |
@@ -24,7 +24,8 @@ void main() { | |||
), | |||
}, | |||
), | |||
); | |||
// ignore: invalid_use_of_internal_member | |||
).toAmplifyOutputs(); |
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.
Same as other comments about testing
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.
Approving with the understanding that remaining references to AmplifyConfig related types (various types in AuthConfiguration, EndpointConfig/AWSApiConfig, and various unit tests) will be removed in a follow up PR.
…ared with AmplifyConfig
* feat(core): add AmplifyOutputs types based on the CLI Gen2 client config schema (#4859) * chore: map AmplifyConfig to AmplifyOutputs (#4935) * chore: map AmplifyConfig to AmplifyOutputs * chore: formatting, license headers * chore: rename amplify_outputs_mapping_test.dart * chore: update API config * chore: support multiple graphql or rest APIs (#4982) * chore: update gen2 version and config mapping tests (#5010) chore: update gen2 version and tests * chore: supporting missing auth config options (#5011) * chore: map null passwordSettings to null passwordPolicy (#5024) * chore(core): updated asyncConfig to use AmplifyOutputs (#4995) * feat(core): add AmplifyOutputs types based on the CLI Gen2 client config schema (#4859) * chore: map AmplifyConfig to AmplifyOutputs (#4935) * chore: map AmplifyConfig to AmplifyOutputs * chore: formatting, license headers * chore: rename amplify_outputs_mapping_test.dart * chore: update API config * chore: support multiple graphql or rest APIs (#4982) * chore(core): updated asyncConfig to use AmplifyOutputs * feat(core): add AmplifyOutputs types based on the CLI Gen2 client config schema (#4859) * chore: map AmplifyConfig to AmplifyOutputs (#4935) * chore: map AmplifyConfig to AmplifyOutputs * chore: formatting, license headers * chore: rename amplify_outputs_mapping_test.dart * chore: update API config * chore: support multiple graphql or rest APIs (#4982) * chore(core): updated exports to expose MFA/Password Policies/Identity Providers * chore(auth): updated unit test to use AmplifyOutputs * chore(authenticator): updated to use AmplifyOutputs type * chore(test): updated test stub to use AmplifyOutputs * chore: update gen2 version and config mapping tests (#5010) chore: update gen2 version and tests * chore: supporting missing auth config options (#5011) * chore(core): updated unit tests AmplifyConfig Json to AmplifyOutputs Json * chore(authenticator): updated unit tests AmplifyOutputs Json values * chore(core): updated amplify_core.dart file format * chore(core): re-internalized IdentityProvider, MFA, and PasswordPolicies * chore(authenticator): renamed InheritedConfig.amplifyConfig to InheritedConfig.amplifyOutputs * chore(authenticator): Renamed FormFieldValidator.validateNewPassword's AmplifyOutputs parameter --------- Co-authored-by: NikaHsn <nika.hasani@gmail.com> Co-authored-by: Jordan Nelson <Jordanryannelson@gmail.com> * chore: fix oauth outputs (#5028) * chore: fix oauth outputs * fix naming typo --------- Co-authored-by: Nika Hassani <nikaws@amazon.com> * chore(infra-gen2): Gen 2 infra (#5026) * chore(infra): gen 2 api rename (#5040) * chore(infra): Add verbose flag to deploy step (#5042) * chore: update configure apis to use AmplifyOutputs instead of AmplifyConfig (#5017) * chore(core): add validation checks for AmplifyOutputs Json deserialization (#5077) * Chore/authenticator password validation (#5078) * chore(test): Removed linter ignore * chore(authenticator): cleaned up extra import * chore(authenticator): Updated password policy validator to use a type not affiliated with config * chore(authenticator): updated AmplifyConfig references to AmplifyOutputs --------- Co-authored-by: Jordan Nelson <Jordanryannelson@gmail.com> Co-authored-by: Tyler-Larkin <tyllark@amazon.com> Co-authored-by: Nika Hassani <nikaws@amazon.com> Co-authored-by: Elijah Quartey <Equartey@users.noreply.github.com>
* feat(core): add AmplifyOutputs types based on the CLI Gen2 client config schema (#4859) * chore: map AmplifyConfig to AmplifyOutputs (#4935) * chore: map AmplifyConfig to AmplifyOutputs * chore: formatting, license headers * chore: rename amplify_outputs_mapping_test.dart * chore: update API config * chore: support multiple graphql or rest APIs (#4982) * chore: update gen2 version and config mapping tests (#5010) chore: update gen2 version and tests * chore: supporting missing auth config options (#5011) * chore: map null passwordSettings to null passwordPolicy (#5024) * chore(core): updated asyncConfig to use AmplifyOutputs (#4995) * feat(core): add AmplifyOutputs types based on the CLI Gen2 client config schema (#4859) * chore: map AmplifyConfig to AmplifyOutputs (#4935) * chore: map AmplifyConfig to AmplifyOutputs * chore: formatting, license headers * chore: rename amplify_outputs_mapping_test.dart * chore: update API config * chore: support multiple graphql or rest APIs (#4982) * chore(core): updated asyncConfig to use AmplifyOutputs * feat(core): add AmplifyOutputs types based on the CLI Gen2 client config schema (#4859) * chore: map AmplifyConfig to AmplifyOutputs (#4935) * chore: map AmplifyConfig to AmplifyOutputs * chore: formatting, license headers * chore: rename amplify_outputs_mapping_test.dart * chore: update API config * chore: support multiple graphql or rest APIs (#4982) * chore(core): updated exports to expose MFA/Password Policies/Identity Providers * chore(auth): updated unit test to use AmplifyOutputs * chore(authenticator): updated to use AmplifyOutputs type * chore(test): updated test stub to use AmplifyOutputs * chore: update gen2 version and config mapping tests (#5010) chore: update gen2 version and tests * chore: supporting missing auth config options (#5011) * chore(core): updated unit tests AmplifyConfig Json to AmplifyOutputs Json * chore(authenticator): updated unit tests AmplifyOutputs Json values * chore(core): updated amplify_core.dart file format * chore(core): re-internalized IdentityProvider, MFA, and PasswordPolicies * chore(authenticator): renamed InheritedConfig.amplifyConfig to InheritedConfig.amplifyOutputs * chore(authenticator): Renamed FormFieldValidator.validateNewPassword's AmplifyOutputs parameter --------- Co-authored-by: NikaHsn <nika.hasani@gmail.com> Co-authored-by: Jordan Nelson <Jordanryannelson@gmail.com> * chore: fix oauth outputs (#5028) * chore: fix oauth outputs * fix naming typo --------- Co-authored-by: Nika Hassani <nikaws@amazon.com> * chore(infra-gen2): Gen 2 infra (#5026) * chore(infra): gen 2 api rename (#5040) * chore(infra): Add verbose flag to deploy step (#5042) * chore: update configure apis to use AmplifyOutputs instead of AmplifyConfig (#5017) * chore(core): add validation checks for AmplifyOutputs Json deserialization (#5077) * Chore/authenticator password validation (#5078) * chore(test): Removed linter ignore * chore(authenticator): cleaned up extra import * chore(authenticator): Updated password policy validator to use a type not affiliated with config * chore(authenticator): updated AmplifyConfig references to AmplifyOutputs --------- Co-authored-by: Jordan Nelson <Jordanryannelson@gmail.com> Co-authored-by: Tyler-Larkin <tyllark@amazon.com> Co-authored-by: Nika Hassani <nikaws@amazon.com> Co-authored-by: Elijah Quartey <Equartey@users.noreply.github.com>
* feat(core): add AmplifyOutputs types based on the CLI Gen2 client config schema (#4859) * chore: map AmplifyConfig to AmplifyOutputs (#4935) * chore: map AmplifyConfig to AmplifyOutputs * chore: formatting, license headers * chore: rename amplify_outputs_mapping_test.dart * chore: update API config * chore: support multiple graphql or rest APIs (#4982) * chore: update gen2 version and config mapping tests (#5010) chore: update gen2 version and tests * chore: supporting missing auth config options (#5011) * chore: map null passwordSettings to null passwordPolicy (#5024) * chore(core): updated asyncConfig to use AmplifyOutputs (#4995) * feat(core): add AmplifyOutputs types based on the CLI Gen2 client config schema (#4859) * chore: map AmplifyConfig to AmplifyOutputs (#4935) * chore: map AmplifyConfig to AmplifyOutputs * chore: formatting, license headers * chore: rename amplify_outputs_mapping_test.dart * chore: update API config * chore: support multiple graphql or rest APIs (#4982) * chore(core): updated asyncConfig to use AmplifyOutputs * feat(core): add AmplifyOutputs types based on the CLI Gen2 client config schema (#4859) * chore: map AmplifyConfig to AmplifyOutputs (#4935) * chore: map AmplifyConfig to AmplifyOutputs * chore: formatting, license headers * chore: rename amplify_outputs_mapping_test.dart * chore: update API config * chore: support multiple graphql or rest APIs (#4982) * chore(core): updated exports to expose MFA/Password Policies/Identity Providers * chore(auth): updated unit test to use AmplifyOutputs * chore(authenticator): updated to use AmplifyOutputs type * chore(test): updated test stub to use AmplifyOutputs * chore: update gen2 version and config mapping tests (#5010) chore: update gen2 version and tests * chore: supporting missing auth config options (#5011) * chore(core): updated unit tests AmplifyConfig Json to AmplifyOutputs Json * chore(authenticator): updated unit tests AmplifyOutputs Json values * chore(core): updated amplify_core.dart file format * chore(core): re-internalized IdentityProvider, MFA, and PasswordPolicies * chore(authenticator): renamed InheritedConfig.amplifyConfig to InheritedConfig.amplifyOutputs * chore(authenticator): Renamed FormFieldValidator.validateNewPassword's AmplifyOutputs parameter --------- Co-authored-by: NikaHsn <nika.hasani@gmail.com> Co-authored-by: Jordan Nelson <Jordanryannelson@gmail.com> * chore: fix oauth outputs (#5028) * chore: fix oauth outputs * fix naming typo --------- Co-authored-by: Nika Hassani <nikaws@amazon.com> * chore(infra-gen2): Gen 2 infra (#5026) * chore(infra): gen 2 api rename (#5040) * chore(infra): Add verbose flag to deploy step (#5042) * chore: update configure apis to use AmplifyOutputs instead of AmplifyConfig (#5017) * chore(core): add validation checks for AmplifyOutputs Json deserialization (#5077) * Chore/authenticator password validation (#5078) * chore(test): Removed linter ignore * chore(authenticator): cleaned up extra import * chore(authenticator): Updated password policy validator to use a type not affiliated with config * chore(authenticator): updated AmplifyConfig references to AmplifyOutputs --------- Co-authored-by: Jordan Nelson <Jordanryannelson@gmail.com> Co-authored-by: Tyler-Larkin <tyllark@amazon.com> Co-authored-by: Nika Hassani <nikaws@amazon.com> Co-authored-by: Elijah Quartey <Equartey@users.noreply.github.com>
Issue #, if available:
Description of changes:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.