You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@hop.apache.org by "hansva (via GitHub)" <gi...@apache.org> on 2026/10/10 10:06:49 UTC
[PR] minor s3 issues in file/folder names, fixes #8841 (hop)
hansva opened a new pull request, #8843:
URL: https://github.com/apache/hop/pull/8843
Folder turning into a non-existent object:
doAttach() now recomputes the key from the file name every time, instead of reusing the key/ a previous attach left behind.
% in object names:
the bucket and key are taken from the decoded path, so file%25.txt reads the object file%.txt;
listings escape % in child names (files, subfolders and bucket names), the same way Commons VFS's own local-file provider does.
Error messages:
reading a folder or a missing object now raises "not a file" (read-not-file) instead of a generic read error;
listing a file now raises "not a folder" (list-children-not-folder) instead of returning an empty list. The shared suite found this extra case once the % failure no longer hid it.
------------------------
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
- [ ] Run `mvn clean install apache-rat:check` to make sure basic checks pass. A more thorough check will be performed on your pull request automatically.
- [ ] If you have a group of commits related to the same change, please squash your commits into one and force push your branch using `git rebase -i`.
- [ ] Mention the appropriate issue in your description (for example: `addresses #123`), if applicable.
To make clear that you license your contribution under the [Apache License Version 2.0, January 2004](http://www.apache.org/licenses/LICENSE-2.0)
you have to acknowledge this by using the following check-box.
- [ ] I hereby declare this contribution to be licensed under the [Apache License Version 2.0, January 2004](http://www.apache.org/licenses/LICENSE-2.0)
- [ ] In any other case, please file an [Apache Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] minor s3 issues in file/folder names, fixes #8841 (hop)
Posted by "hansva (via GitHub)" <gi...@apache.org>.
hansva commented on code in PR #8843:
URL: https://github.com/apache/hop/pull/8843#discussion_r4237882097
##########
plugins/tech/aws/src/main/java/org/apache/hop/vfs/s3/s3/vfs/S3FileProvider.java:
##########
@@ -53,80 +57,101 @@ public S3FileProvider(IVariables variables, S3Meta s3Meta) {
setFileNameParser(S3FileNameParser.getInstance());
}
+ /**
+ * Look file systems up by the options they are created with. Commons VFS caches a file system
+ * under its own options but looks it up under the caller's, which are empty when resolving
+ * through HopVfs; a named connection's options never matched, so every resolve built a new file
+ * system and S3 client, and the provider kept all of them.
+ */
+ @Override
+ protected synchronized FileSystem getFileSystem(
+ final FileName rootFileName, final FileSystemOptions fileSystemOptions)
+ throws FileSystemException {
+ return super.getFileSystem(rootFileName, options(fileSystemOptions));
+ }
+
@Override
public FileSystem doCreateFileSystem(
final FileName name, final FileSystemOptions fileSystemOptions) {
- // Named connections (s3test:// etc.) must use a dedicated options instance so we never
- // mutate the default options used by s3://. Otherwise using s3test:// would overwrite
- // the default S3 configuration.
- final FileSystemOptions options;
- if (s3Meta != null && variables != null) {
- options = new FileSystemOptions();
- } else {
- options = fileSystemOptions != null ? fileSystemOptions : getDefaultFileSystemOptions();
+ return new S3FileSystem(name, options(fileSystemOptions));
+ }
+
+ /**
+ * The options of the file system for the given request. For s3:// the caller's options, or the
+ * defaults. For a named connection one options instance, built from the connection the first time
+ * it is needed: dedicated, so it never changes the defaults s3:// uses, and the same every time,
+ * so its file system is found again.
+ */
+ private synchronized FileSystemOptions options(final FileSystemOptions fileSystemOptions) {
+ if (s3Meta == null || variables == null) {
+ return fileSystemOptions != null ? fileSystemOptions : getDefaultFileSystemOptions();
}
+ if (connectionOptions == null) {
+ connectionOptions = buildConnectionOptions();
Review Comment:
can we defer this change or mark it as a feature request? On the snapshot: that matches the other named providers. GCS resolves its credentials in the provider constructor. Azure and MinIO resolve on first file system creation and keep that file system for the provider's lifetime. All of them pick up changes through HopVfs.refresh() when the connection is saved. Before this PR, S3 only re-read variables because every resolve built a new file system and client, which is the leak being fixed. Re-resolving on every lookup would also run secret resolvers on every file resolve. Picking up variable or secret changes without a save would be good, but it should be designed for all named providers at once (refresh on change, or a TTL) rather than only S3. I'd keep that out of this PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] minor s3 issues in file/folder names, fixes #8841 (hop)
Posted by "mattcasters (via GitHub)" <gi...@apache.org>.
mattcasters commented on code in PR #8843:
URL: https://github.com/apache/hop/pull/8843#discussion_r4237442007
##########
plugins/tech/aws/src/main/java/org/apache/hop/vfs/s3/s3/vfs/S3FileProvider.java:
##########
@@ -53,80 +57,101 @@ public S3FileProvider(IVariables variables, S3Meta s3Meta) {
setFileNameParser(S3FileNameParser.getInstance());
}
+ /**
+ * Look file systems up by the options they are created with. Commons VFS caches a file system
+ * under its own options but looks it up under the caller's, which are empty when resolving
+ * through HopVfs; a named connection's options never matched, so every resolve built a new file
+ * system and S3 client, and the provider kept all of them.
+ */
+ @Override
+ protected synchronized FileSystem getFileSystem(
+ final FileName rootFileName, final FileSystemOptions fileSystemOptions)
+ throws FileSystemException {
+ return super.getFileSystem(rootFileName, options(fileSystemOptions));
+ }
+
@Override
public FileSystem doCreateFileSystem(
final FileName name, final FileSystemOptions fileSystemOptions) {
- // Named connections (s3test:// etc.) must use a dedicated options instance so we never
- // mutate the default options used by s3://. Otherwise using s3test:// would overwrite
- // the default S3 configuration.
- final FileSystemOptions options;
- if (s3Meta != null && variables != null) {
- options = new FileSystemOptions();
- } else {
- options = fileSystemOptions != null ? fileSystemOptions : getDefaultFileSystemOptions();
+ return new S3FileSystem(name, options(fileSystemOptions));
+ }
+
+ /**
+ * The options of the file system for the given request. For s3:// the caller's options, or the
+ * defaults. For a named connection one options instance, built from the connection the first time
+ * it is needed: dedicated, so it never changes the defaults s3:// uses, and the same every time,
+ * so its file system is found again.
+ */
+ private synchronized FileSystemOptions options(final FileSystemOptions fileSystemOptions) {
+ if (s3Meta == null || variables == null) {
+ return fileSystemOptions != null ? fileSystemOptions : getDefaultFileSystemOptions();
}
+ if (connectionOptions == null) {
+ connectionOptions = buildConnectionOptions();
Review Comment:
**[bug]** `connectionOptions` is resolved once and then reused for every lookup. `buildConnectionOptions()` copies region, endpoint, keys, session token, credentials file, profile, and cache TTL from the provider's `IVariables` at that moment. That provider is the one registered for the VFS manager (the GUI session, or a namespace) and is documented as what named connections resolve credentials with. Editing a project variable, a parent-scope `Set variables` value, or a `#{resolver:...}` secret does not call `HopVfs.refresh()` — only saving the connection replaces the provider — so later resolves keep the first snapshot. A rotated access key or session token is ignored until then.
Holding one options instance is not what makes the cache hit. Commons VFS 2.10 stores file systems in a `TreeMap` keyed by `FileSystemKey.compareTo`, and `FileSystemOptions.compareTo` treats equal contents as the same key. A newly built options object with the same resolved values still returns the existing `S3FileSystem`.
**Suggestion:** Build the named connection's options on each `getFileSystem` / `doCreateFileSystem` call instead of caching the instance. Unchanged settings still reuse one file system and one S3 client. When the resolved values differ, close and drop the previous file system before the new one is cached so old clients do not accumulate. Extend `S3FileProviderFileSystemReuseTest` with a variable change between two resolves.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] minor s3 issues in file/folder names, fixes #8841 (hop)
Posted by "hansva (via GitHub)" <gi...@apache.org>.
hansva merged PR #8843:
URL: https://github.com/apache/hop/pull/8843
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org