refactor: Reduce API implementation classes visibility #227

guicamest · 2023-10-26T10:09:14Z

Description of changes

Classes visibility are reduced in order to avoid leaking the classes to the user of the library who could potentially write some code with this information. This will benefit the maintenance of the library, as it'll provide more freedom to implementation changes without risking breaking user code. After all, this library is an implementation of NIO so that users shouldn't care if a Path is an S3Path or AWSS3Path. This is how the default implementations are implemented, for example, class LinuxFileSystem is not public.

Code coverage

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

guicamest · 2023-10-26T19:40:39Z

Looking at FileSystems.newFileSystem javadoc, it looks like the method is intented for creating filesystems, not just a FileSystem object. If this is correct, then the implementation of newFileSystem in the S3FileSystemProvider would be incorrect, as it is getting a FileSystem object, not creating a FileSystem. Given the description on the README about the relationship FileSystem <-> Buckets, it would mean creating a Bucket).

The default FileSystem implementations throw when newFileSystem is called:

IMHO, I think it will be a good opportunity to remove the current implementation of newFileSystem for v2.x.x , leaving it unimplemented (UnsupportedOperation) until:

it is implemented for bucket creation (new feature, non breaking) or,
left unimplemented but following the specification

What are your thoughts @markjschreiber ?

markjschreiber · 2023-10-26T21:46:00Z

You make a good point. I would be in favor of being able to use it to create a bucket. The map of key value pairs passed to it would need to map to properties that can be set on the CreateBucketRequest. We would need to move the current functionality of the newFileSystem over to the getFileSystem method on the S3FileSystemProvider.

guicamest · 2023-10-26T21:50:40Z

We would need to move the current functionality of the newFileSystem over to the getFileSystem method on the S3FileSystemProvider.

Exactly!

Would it be ok to turn it into unimplemented for this PR and leave the implementation for a future PR? Otherwise it'll be a hell to review as it is already big.

src/main/java/software/amazon/nio/spi/s3/Constants.java

markjschreiber

Thanks for this refactoring. I have asked a couple of questions in the review and there are some suggested changes.

src/main/java/software/amazon/nio/spi/s3/NotYetImplementedException.java

src/main/java/software/amazon/nio/spi/s3/S3FileSystem.java

src/main/java/software/amazon/nio/spi/s3/S3FileSystemProvider.java

markjschreiber · 2023-10-30T16:06:33Z

src/main/java/software/amazon/nio/spi/s3/S3FileSystemProvider.java

-    public S3FileSystem newFileSystem(URI uri) {
-        return newFileSystem(uri, Collections.EMPTY_MAP);
+    public FileSystem newFileSystem(URI uri, Map<String, ?> env) {
+        throw new NotYetImplementedException("This method is not yet supported in v2.x. It might be implemented for bucket creation");


Can you add an issue to implement this for bucket creation?

Sure I'll do

Created #243, feel free to add any information you consider relevant 🙏

markjschreiber · 2023-10-30T16:18:26Z

src/main/java/software/amazon/nio/spi/s3/S3XFileSystemProvider.java

@@ -14,34 +14,16 @@
 * limitations under the License.
 */

-package software.amazon.nio.spi.s3x;
+package software.amazon.nio.spi.s3;


You seem to have moved some of the s3x... classes into the s3 package. Is this intentional? If so you should probably move the rest as well?

Yes it was intentional. By changing the visibility of fileSystemInfo in the superclass of the S3XFileSystemProvider, I needed to move this one to the same package.

The one left in s3x package is S3XFileSystemInfo, I can move it👍

Done in 1c80df9

markjschreiber · 2023-10-30T19:17:37Z

Merging the other PRs has created a conflict here. You can probably resolve it by rebasing this onto HEAD

…t it is just an implementation of BasicFileAttributeView

members

…tream

…igured. Hide `S3Path` from `FileSystem.getPath`

markjschreiber

Looks great

guicamest mentioned this pull request Oct 26, 2023

cleanup: protected *and* deprecated methods #223

Closed

guicamest force-pushed the classesVisibility branch from 775803e to 6b6a9ca Compare October 26, 2023 19:44

guicamest force-pushed the classesVisibility branch from 6b6a9ca to ea23c1e Compare October 29, 2023 13:38

guicamest marked this pull request as ready for review October 29, 2023 17:02

markjschreiber reviewed Oct 30, 2023

View reviewed changes

src/main/java/software/amazon/nio/spi/s3/Constants.java Show resolved Hide resolved

markjschreiber requested changes Oct 30, 2023

View reviewed changes

guicamest added 20 commits October 30, 2023 20:26

Reduce visibility of S3Path

b344f57

Reduce visibility of S3Path methods

0fc1835

Move API methods first in S3Path

4886978

Reduce visibility of S3BasicFileAttributes

0b1fa01

Rename S3FileAttributeView to S3BasicFileAttributeView to reflect tha…

d300fdf

…t it is just an implementation of BasicFileAttributeView

Reduce visibility of S3BasicFileAttributeView and hide it from outside

15bf6c3

Hide S3BasicFileAttributes from outside when readAttributes

eb4f039

Reduce visibility of S3ReadAheadByteChannel and its methods

4c2898f

Move API methods first in S3ReadAheadByteChannel

f18c077

Make logger static in S3ReadAheadByteChannel

d086bc2

Reduce visibility of S3SeekableByteChannel and its methods

9295fd1

cleanup(S3SeekableByteChannel): deprecated/unused methods

4f904f7

cleanup(S3SeekableByteChannel): remove constructor only used in tests

bae136b

Reduce visibility of S3WritableByteChannel and its methods

e941f51

cleanup(S3WritableByteChannel): Remove unused constructor

2b71e91

Reduce visibility of PosixLikePathRepresentation, its methods and

cd5ca48

members

Remove StringUtils since it has only *one* and *unused* method

606f455

cleanup: Move FixedS3ClientProvider to test sourceSet

132697a

Unimplement newFileSystem

aec4fa2

test: FileSystemProvider.newFileSystem

8d5cfd5

guicamest added 12 commits October 30, 2023 20:30

test: FileSystems.newFileSystem(URI,env) should throw NotYetImplemented

9b20f2b

cleanup: Unnecessary override of newFileSystem in S3XFileSystemProvider

9cbef52

Reduce visibility of FileSystemProvider methods and members

5bb8ad3

Move API methods first in S3FileSystemProvider

ddf2486

Hide S3Path from outside when FileSystemProvider.getPath

fdbf89a

Reduce visibility of S3FileSystem

ca054e5

Reduce visibility of S3FileSystem methods and members

58fed67

Move API methods first in S3FileSystem

615d0a0

Reduce visibility of S3XFileSystemProvider, its methods and members

d33f612

Hide S3FileSystem from outside when FileSystemProvider.getFileSystem

5931b80

Reduce visibility of S3PathIterator constructor

803d987

cleanup: IOException not thrown in S3FileSystemProvider.newDirectoryS…

37fd462

…tream

guicamest force-pushed the classesVisibility branch from 732a951 to 37fd462 Compare October 30, 2023 19:30

Move S3XFileSystemInfo to s3.util package

1c80df9

guicamest mentioned this pull request Oct 30, 2023

Implement FileSystem.newFileSystem(URI uri, Map<String, ?> env) to create an S3 bucket #243

Closed

Set visibility of S3FileSystem to public so that client can be conf…

5e11e34

…igured. Hide `S3Path` from `FileSystem.getPath`

guicamest requested a review from markjschreiber October 30, 2023 20:34

guicamest added 4 commits October 30, 2023 22:45

chore: Simplify set creation using Java 11 method

dcad497

Use Collectors.toMap in S3BasicFileAttributes.asMap

f1252bc

cleanup: Remove unused protected method

50e7ac6

cleanup: Remove unused import

bf050eb

guicamest force-pushed the classesVisibility branch from 2bf05f2 to bf050eb Compare October 30, 2023 21:47

markjschreiber approved these changes Oct 30, 2023

View reviewed changes

markjschreiber merged commit e53d7fe into awslabs:main Oct 30, 2023
1 check passed

guicamest deleted the classesVisibility branch October 31, 2023 05:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: Reduce API implementation classes visibility #227

refactor: Reduce API implementation classes visibility #227

guicamest commented Oct 26, 2023 •

edited

Loading

guicamest commented Oct 26, 2023 •

edited

Loading

markjschreiber commented Oct 26, 2023 •

edited

Loading

guicamest commented Oct 26, 2023

markjschreiber left a comment

markjschreiber Oct 30, 2023

guicamest Oct 30, 2023

guicamest Oct 30, 2023

markjschreiber Oct 30, 2023

guicamest Oct 30, 2023

guicamest Oct 30, 2023

markjschreiber commented Oct 30, 2023

markjschreiber left a comment

refactor: Reduce API implementation classes visibility #227

refactor: Reduce API implementation classes visibility #227

Conversation

guicamest commented Oct 26, 2023 • edited Loading

Description of changes

Code coverage

guicamest commented Oct 26, 2023 • edited Loading

markjschreiber commented Oct 26, 2023 • edited Loading

guicamest commented Oct 26, 2023

markjschreiber left a comment

Choose a reason for hiding this comment

markjschreiber Oct 30, 2023

Choose a reason for hiding this comment

guicamest Oct 30, 2023

Choose a reason for hiding this comment

guicamest Oct 30, 2023

Choose a reason for hiding this comment

markjschreiber Oct 30, 2023

Choose a reason for hiding this comment

guicamest Oct 30, 2023

Choose a reason for hiding this comment

guicamest Oct 30, 2023

Choose a reason for hiding this comment

markjschreiber commented Oct 30, 2023

markjschreiber left a comment

Choose a reason for hiding this comment

guicamest commented Oct 26, 2023 •

edited

Loading

guicamest commented Oct 26, 2023 •

edited

Loading

markjschreiber commented Oct 26, 2023 •

edited

Loading