feat(storage): make access level optional for storage APIs #2793
Conversation
| @@ -99,7 +96,7 @@ class StorageS3Service { | |||
| final resolvedPrefix = await getResolvedPrefix( | |||
| prefixResolver: _prefixResolver, | |||
| logger: _logger, | |||
| accessLevel: options.accessLevel, | |||
| accessLevel: options.accessLevel ?? _s3PluginConfig.defaultAccessLevel, | |||
There was a problem hiding this comment.
I understand this ?? if null check is due to Storage<Operation>Options.accessLevel is nullable now, and its default value should be set by the plugin, but this default value has been set here in the StorageS3Dart implementation (where I think it's a proper location).
Hum we can't find a better way than accessLevel: options.accessLevel! we probably need to remove the default value setting from the StorageS3Dart implementation.
There was a problem hiding this comment.
using the default value here instead of accessLevel: options.accessLevel! makes the code more robust as we do not rely on callers to set value for nullable field.
There was a problem hiding this comment.
I agree. The storage options interface is not appropriate since it has a nullable access level. Either passing a non-null access level as an additional parameter to the service (e.g. resolvedAccessLevel) or handling it inline like this seems fine.
Since other S3 plugin options logic lives here, I think the current handling is appropriate.
There was a problem hiding this comment.
@dnys1 thanks for suggesting the resolvedAccessLevel. I'm more toward handling it inline as there is no logic than checking for null.
| @@ -99,7 +96,7 @@ class StorageS3Service { | |||
| final resolvedPrefix = await getResolvedPrefix( | |||
| prefixResolver: _prefixResolver, | |||
| logger: _logger, | |||
| accessLevel: options.accessLevel, | |||
| accessLevel: options.accessLevel ?? _s3PluginConfig.defaultAccessLevel, | |||
There was a problem hiding this comment.
I agree. The storage options interface is not appropriate since it has a nullable access level. Either passing a non-null access level as an additional parameter to the service (e.g. resolvedAccessLevel) or handling it inline like this seems fine.
Since other S3 plugin options logic lives here, I think the current handling is appropriate.
| }); | ||
|
|
||
| final StorageAccessLevel accessLevel; | ||
| final StorageAccessLevel? accessLevel; |
There was a problem hiding this comment.
I might recommend adding the following and using throughout.
StorageAccessLevel resolveAccessLevel({required StorageAccessLevel defaultAccessLevel}) =>
accessLevel ?? defaultAccessLevel;
| @@ -261,7 +263,7 @@ void main() { | |||
| }); | |||
|
|
|||
| test( | |||
| 'should invoke S3Object.listObjectV2 with expected parameters and return expected results with listAll is set to true in the options', | |||
| 'should invoke S3Object.listObjectV2 with expected parameters and return expected results with listAll and acess level is set to true in the options', | |||
| @@ -390,6 +392,47 @@ void main() { | |||
| ); | |||
| }); | |||
|
|
|||
| test('should invoke S3Client.headObject with default access level', | |||
There was a problem hiding this comment.
Could update this description a bit, as the default access level actually is not a parameter of the headObject, but used in prefix resolving.
There was a problem hiding this comment.
I think the key passed to headObject is prefixed with access level, something like public#object-key, will update the description to make it more clear.
NikaHsn
force-pushed
the
storage/feat/next/optional-access-level
branch
from
9c34a1e
to
93aa5a7
Compare
…t impl and let s3 service to handle it
NikaHsn
force-pushed
the
storage/feat/next/optional-access-level
branch
from
93aa5a7
to
31d6697
Compare
Issue #, if available:
Description of changes:
defaultAccessLevelcan be set byAmplify.Amplify.configure(...)with fallback toguest.By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.