prosgarz35 commented on PR #3197: URL: https://github.com/apache/james-project/pull/3197#issuecomment-5836139546
# Deep KISS/DRY Analysis — PR #3197: Apache James File Blob Store Sharding > **Branch:** `feature/sharded-file-blobstore` > **PR URL:** https://github.com/apache/james-project/pull/3197 > **Scope:** All files modified or introduced by this PR > **Principles applied:** KISS (Keep It Simple, Stupid) · DRY (Don't Repeat Yourself) · PoLA (Principle of Least Astonishment) --- ## Overall Verdict The implementation is **solid and functionally correct**. The core mechanism — hierarchical BlobId storage via `createTempFile` + `ATOMIC_MOVE` — is well-designed. However, **9 specific issues** were identified that affect readability, correctness, and maintainability. None are catastrophic, but several (especially ❶ and ❺) will be flagged in code review by project maintainers. --- ## File 1: `FileBlobStoreDAO.java` **Full path:** ``` server/blob/blob-file/src/main/java/org/apache/james/blob/file/FileBlobStoreDAO.java ``` --- ### Issue ❶ — `getBucketRoot` has a write side-effect when called from read operations **Priority: HIGH · Principle violated: KISS, PoLA** **Lines affected:** 83, 92–102, 113, 144, 201, 260 **Current code:** ```java // Called from read(), readBytes(), listBlobs(), save(), delete() — ALL methods private File getBucketRoot(BucketName bucketName) { File bucketRoot = new File(root, bucketName.asString()); if (!bucketRoot.exists()) { try { FileUtils.forceMkdir(bucketRoot); // ← SIDE EFFECT: creates a directory! } catch (IOException e) { throw new ObjectStoreIOException("Cannot create bucket", e); } } return bucketRoot; } ``` **Why this is a problem:** `read()`, `readBytes()`, `listBlobs()`, and `delete()` all call `getBucketRoot()`. This means that **reading a non-existent blob creates the bucket directory on disk** before throwing `ObjectNotFoundException`. This violates the Principle of Least Astonishment: a read operation should never have write side-effects. Concrete consequences: - After a failed `read()`, an empty bucket directory now exists on disk — a ghost artifact. - `listBlobs()` on a non-existent bucket creates it, then returns an empty stream. Silent and misleading. - The method `deleteBucket()` (line 241) explicitly avoids this by using `new File(root, bucketName.asString())` directly — proving the maintainer already knew `getBucketRoot` should not be called for reads. **Proposed change — split into two methods:** ```java // Pure path computation — no side effects. Use for read, delete, listBlobs. private File bucketRoot(BucketName bucketName) { return new File(root, bucketName.asString()); } // Ensures directory exists — use ONLY for save operations. private File ensureBucketRoot(BucketName bucketName) { File bucketRoot = bucketRoot(bucketName); if (!bucketRoot.exists()) { try { FileUtils.forceMkdir(bucketRoot); } catch (IOException e) { throw new ObjectStoreIOException("Cannot create bucket", e); } } return bucketRoot; } ``` **Update all call sites:** - `read()` → `bucketRoot(bucketName)` - `readBytes()` → `bucketRoot(bucketName)` - `listBlobs()` → `bucketRoot(bucketName)` (+ add `onErrorResume`, see Issue ❸) - `delete()` → `bucketRoot(bucketName)` - `save(byte[], ...)` → `ensureBucketRoot(bucketName)` - `save(InputStream, ...)` → `ensureBucketRoot(bucketName)` - `deleteBucket()` → already uses `new File(root, ...)` directly, no change needed --- ### Issue ❷ — `new File(bucketRoot, blobId.asString())` repeated 5 times **Priority: MEDIUM · Principle violated: DRY** **Lines affected:** 84, 114, 134, 145, 202 **Current code (repeated identically across 5 methods):** ```java // In read() File blob = new File(bucketRoot, blobId.asString()); // In readBytes() File blob = new File(bucketRoot, blobId.asString()); // In save(BucketName, BlobId, byte[], BlobMetadata) File blob = new File(bucketRoot, blobId.asString()); // In save(BucketName, BlobId, InputStream, BlobMetadata) File blob = new File(bucketRoot, blobId.asString()); // In delete() File blob = new File(bucketRoot, blobId.asString()); ``` **Why this is a problem:** This expression is the **central abstraction of the class**: it materializes a `BlobId` into a filesystem path. It embodies the key design decision that `blobId.asString()` may contain `/` characters (forming nested directories). Repeating it 5 times means that if the path construction logic ever needs to change, 5 places must be updated simultaneously. **Proposed change — extract a private helper method:** ```java /** * Resolves the {@link BlobId} to an actual {@link File} within the given bucket root. * BlobId strings may contain '/' separators (e.g. for MinIOGenerationAwareBlobId), * which Java's {@link File} constructor interprets as path separators, creating * the necessary subdirectory structure automatically. */ private File blobFile(File bucketRoot, BlobId blobId) { return new File(bucketRoot, blobId.asString()); } ``` **Result — every method becomes a uniform two-liner:** ```java File bucketRoot = bucketRoot(bucketName); // or ensureBucketRoot(bucketName) File blob = blobFile(bucketRoot, blobId); ``` --- ### Issue ❸ — `listBlobs` uses `getBucketRoot` instead of handling missing bucket gracefully **Priority: MEDIUM · Principle violated: KISS** **Lines affected:** 258–270 **Current code:** ```java @Override public Publisher<BlobId> listBlobs(BucketName bucketName) { return Mono.fromCallable(() -> { File bucketRoot = getBucketRoot(bucketName); // ← creates directory! Path rootPath = bucketRoot.toPath(); return Files.walk(rootPath) .filter(Files::isRegularFile) .map(path -> blobIdFactory.parse(toBlobId(rootPath.relativize(path)))); }) .flatMapMany(Flux::fromStream) .subscribeOn(Schedulers.boundedElastic()); } ``` **Why this is a problem:** `listBuckets()` (line 250) already demonstrates the correct pattern for handling a missing root directory: ```java .onErrorResume(NoSuchFileException.class, e -> Flux.empty()) ``` But `listBlobs()` does not follow this pattern — instead it uses `getBucketRoot` to create the directory and then walks an empty tree. The result is functionally equivalent (empty stream) but with the undesirable side-effect of creating an empty directory. **Proposed change:** ```java @Override public Publisher<BlobId> listBlobs(BucketName bucketName) { return Mono.fromCallable(() -> { Path rootPath = bucketRoot(bucketName).toPath(); // no side effect return Files.walk(rootPath) .filter(Files::isRegularFile) .map(path -> blobIdFactory.parse(toBlobId(rootPath.relativize(path)))); }) .flatMapMany(Flux::fromStream) .onErrorResume(NoSuchFileException.class, e -> Flux.empty()) // mirrors listBuckets() .subscribeOn(Schedulers.boundedElastic()); } ``` This makes `listBlobs` consistent with `listBuckets` — both return an empty result for a non-existent bucket, without creating artifacts on disk. --- ### Issue ❹ — `toBlobId` uses `StreamSupport.stream(spliterator)` — unnecessarily complex **Priority: LOW · Principle violated: KISS** **Lines affected:** 272–276 **Current code:** ```java private String toBlobId(Path relativePath) { return StreamSupport.stream(relativePath.spliterator(), false) .map(Path::toString) .collect(Collectors.joining("/")); } ``` **Why this is a problem:** `StreamSupport.stream(relativePath.spliterator(), false)` is an uncommon idiom that surprises most Java developers who read it. The simpler alternative — replacing the OS-native path separator with `/` — is shorter, immediately readable, and achieves the same result. **Proposed change:** ```java private String toBlobId(Path relativePath) { return relativePath.toString().replace(File.separatorChar, '/'); } ``` > **Note:** This works correctly on all platforms because within the JVM, `Path.toString()` uses the OS path separator, and we normalize it to `/` for the BlobId string. On Linux (where the prod server runs) `File.separatorChar` is already `/`, so the replacement is a no-op. --- ### Issue ❺ — TOCTOU race condition in `delete` **Priority: MEDIUM · Principle violated: Correctness + KISS** **Lines affected:** 200–210 **Current code:** ```java return Mono.fromRunnable(Throwing.runnable(() -> { File bucketRoot = getBucketRoot(bucketName); File blob = new File(bucketRoot, blobId.asString()); if (blob.exists()) { // ← TIME OF CHECK FileUtils.deleteQuietly(blob); // ← TIME OF USE (gap here!) deleteEmptyParentDirs(bucketRoot, blob); } })) ``` **Why this is a problem:** There is a classic Time-Of-Check-To-Time-Of-Use (TOCTOU) gap between `blob.exists()` and `FileUtils.deleteQuietly(blob)`. If two threads concurrently call `delete` on the same blob: 1. Thread A: `exists()` → `true` 2. Thread B: `exists()` → `true` 3. Thread A: deletes the file 4. Thread B: `deleteQuietly` on a non-existent file (silently ignored) → then calls `deleteEmptyParentDirs` unnecessarily `FileUtils.deleteQuietly` already returns `false` when the file does not exist — its return value can be used as the atomic check-and-delete operation, eliminating the race. **Proposed change:** ```java return Mono.fromRunnable(Throwing.runnable(() -> { File bucketRoot = bucketRoot(bucketName); File blob = blobFile(bucketRoot, blobId); if (FileUtils.deleteQuietly(blob)) { // ← atomic check-and-delete deleteEmptyParentDirs(bucketRoot, blob); } })) .subscribeOn(Schedulers.boundedElastic()) .then(); ``` This is **shorter** (removes `blob.exists()` check), **safer** (eliminates the race window), and **clearer** (the `if` block now reads as "if we actually deleted something, clean up parents"). --- ## File 2: `FileWithFolderHierarchyTest.java` **Full path:** ``` server/blob/blob-file/src/test/java/org/apache/james/blob/file/FileWithFolderHierarchyTest.java ``` --- ### Issue ❻ — `static` clock field is missing `private` and `final` **Priority: HIGH · Principle violated: KISS** **Line affected:** 52 **Current code:** ```java static UpdatableTickingClock clock = new UpdatableTickingClock(Instant.parse("2021-08-19T10:15:30.00Z")); ``` **Why this is a problem:** - Missing `private`: the field is package-visible, accessible from any class in the same package, and also directly accessed by the `@Nested class Compatible` at lines 129–130. Accidental access from outside is possible. - Missing `final`: the field can be reassigned by any code in the class, including nested classes. Even though `UpdatableTickingClock` is mutable (which is intentional for testing), the *reference itself* should be final. - `static` is correct here (shared between outer class and `@Nested`), but without `private final` it violates Java conventions and Checkstyle rules the project enforces. **Proposed change:** ```java private static final UpdatableTickingClock clock = new UpdatableTickingClock(Instant.parse("2021-08-19T10:15:30.00Z")); ``` No functional change — just adds `private` and `final` to enforce proper encapsulation. --- ### Issue ❼ — Hardcoded `BlobId` string in assertion without explanation **Priority: MEDIUM · Principle violated: KISS (test fragility)** **Lines affected:** 91–100 **Current code:** ```java @ParameterizedTest @MethodSource("storagePolicies") void saveShouldReturnBlobIdOfString(BlobStore.StoragePolicy storagePolicy) { BlobStore store = testee(); BucketName defaultBucketName = store.getDefaultBucketName(); BlobId blobId = Mono.from(store.save(defaultBucketName, "toto", storagePolicy)).block(); String blobIdString = blobId.asString(); assertThat(blobIdString).isEqualTo("1/628/M/f/emXjFVhqwZi9eYtmKc5A"); // ← hardcoded assertThat(blobId).isEqualTo(blobIdFactory().parse(blobIdString)); } ``` **Why this is a problem:** The hardcoded value `"1/628/M/f/emXjFVhqwZi9eYtmKc5A"` is completely opaque to the reader. It is not clear: - What the segments (`1`, `628`, `M`, `f`, `emXjFVhqwZi9eYtmKc5A`) mean. - Whether this is a regression test for the exact format (intentional) or just a snapshot that was copied in. - What happens if `MinIOGenerationAwareBlobId` format changes in the future. If this is a **format regression test** (to ensure the `generation/bucket/prefix/hash` structure is stable), add a comment: ```java // Regression: MinIOGenerationAwareBlobId must serialize as "generation/bucketShard/charPrefix1/charPrefix2/hash" // The fixed clock (2021-08-19T10:15:30Z) and content ("toto") make this deterministic. assertThat(blobIdString).isEqualTo("1/628/M/f/emXjFVhqwZi9eYtmKc5A"); ``` If the test only intends to verify that the blob ID is **hierarchical** (contains `/`), use: ```java // BlobIds produced by MinIOGenerationAwareBlobId must contain '/' (folder hierarchy) assertThat(blobIdString).contains("/"); assertThat(blobId).isEqualTo(blobIdFactory().parse(blobIdString)); // roundtrip preserved ``` **Recommendation:** Keep the hardcoded assertion (it protects against accidental format changes), but add the explanatory comment. --- ### Issue ❽ — `"file://var/blob"` duplicated between `FileBlobStoreDAO` and test tearDown **Priority: LOW · Principle violated: DRY** **Lines affected:** - `FileBlobStoreDAO.java` line 77: `root = fileSystem.getFile("file://var/blob");` - `FileWithFolderHierarchyTest.java` line 69: `FileUtils.deleteQuietly(fileSystem.getFile("file://var/blob"));` **Why this is a problem:** The string `"file://var/blob"` appears in both the production class and the test teardown. If the storage path is ever changed in `FileBlobStoreDAO`, the test teardown will silently stop cleaning up the correct directory, causing test pollution between runs. **Proposed change:** Extract the constant into `FileBlobStoreDAO`: ```java // In FileBlobStoreDAO.java static final String BLOB_ROOT_URI = "file://var/blob"; // package-visible for tests @Inject public FileBlobStoreDAO(FileSystem fileSystem, BlobId.Factory blobIdFactory) throws FileNotFoundException { root = fileSystem.getFile(BLOB_ROOT_URI); this.blobIdFactory = blobIdFactory; } ``` ```java // In FileWithFolderHierarchyTest.java @AfterEach void tearDown() throws Exception { FileUtils.deleteQuietly(fileSystem.getFile(FileBlobStoreDAO.BLOB_ROOT_URI)); } ``` > **Alternative (lower coupling):** expose `getRoot()` from `FileBlobStoreDAO` and use it in tearDown. But a package-private constant is simpler and avoids adding a public API surface. --- ## File 3: `BlobDeduplicationGCModule.java` **Full path:** ``` server/container/guice/blob/deduplication-gc/src/main/java/org/apache/james/modules/blobstore/BlobDeduplicationGCModule.java ``` --- ### Issue ❾ — Variable named `compatibilityModeActivated` describes the wrong concept **Priority: LOW · Principle violated: KISS (naming clarity)** **Lines affected:** 72–82 **Current code:** ```java @Singleton @Provides public BlobId.Factory generationAwareBlobIdFactory(Clock clock, PlainBlobId.Factory delegate, GenerationAwareBlobId.Configuration configuration) { boolean compatibilityModeActivated = Optional.ofNullable(System.getProperty("james.blobstore.folder.hierarchy")) .or(() -> Optional.ofNullable(System.getProperty("james.s3.minio.compatibility.mode"))) .map(Boolean::parseBoolean) .orElse(false); if (compatibilityModeActivated) { return new MinIOGenerationAwareBlobId.Factory(clock, configuration, delegate); } else { return new GenerationAwareBlobId.Factory(clock, delegate, configuration); } } ``` **Why this is a problem:** `compatibilityModeActivated` sounds like a *legacy/backward-compatibility mode* — i.e., "we are downgrading to old behavior for compatibility". But the opposite is true: setting this flag activates **new** behavior (`MinIOGenerationAwareBlobId` with folder hierarchy). A reader unfamiliar with the history will be confused by the name. **Proposed change:** ```java boolean useFolderHierarchy = Optional.ofNullable(System.getProperty("james.blobstore.folder.hierarchy")) .or(() -> Optional.ofNullable(System.getProperty("james.s3.minio.compatibility.mode"))) .map(Boolean::parseBoolean) .orElse(false); if (useFolderHierarchy) { return new MinIOGenerationAwareBlobId.Factory(clock, configuration, delegate); } else { return new GenerationAwareBlobId.Factory(clock, delegate, configuration); } ``` The legacy system property name `james.s3.minio.compatibility.mode` is kept as a fallback — that is correct. But the *variable* that holds the result of reading either property should describe **what it means functionally**, not what one of its aliases is named. --- ## Summary Table | # | File | Issue | Changed? | Priority | |---|------|-------|----------|----------| | ❶ | `FileBlobStoreDAO.java` | Split `getBucketRoot` into `bucketRoot()` + `ensureBucketRoot()` to remove write side-effect from reads | **Yes — refactor** | High | | ❷ | `FileBlobStoreDAO.java` | Extract `blobFile(bucketRoot, blobId)` to eliminate 5× duplication of path construction | **Yes — extract method** | Medium | | ❸ | `FileBlobStoreDAO.java` | `listBlobs` — use `bucketRoot()` + `onErrorResume(NoSuchFileException)` instead of `getBucketRoot` | **Yes — refactor** | Medium | | ❹ | `FileBlobStoreDAO.java` | Simplify `toBlobId` to `relativePath.toString().replace(File.separatorChar, '/')` | Yes — simplify | Low | | ❺ | `FileBlobStoreDAO.java` | Fix TOCTOU in `delete` by replacing `exists()` check with `deleteQuietly()` return value | **Yes — fix bug** | Medium | | ❻ | `FileWithFolderHierarchyTest.java` | Add `private` and `final` to `static clock` field | **Yes — fix** | High | | ❼ | `FileWithFolderHierarchyTest.java` | Add explanatory comment to hardcoded `blobId` assertion | Yes — add comment | Medium | | ❽ | `FileWithFolderHierarchyTest.java` | Extract `"file://var/blob"` to a shared constant in `FileBlobStoreDAO` | Yes — extract constant | Low | | ❾ | `BlobDeduplicationGCModule.java` | Rename `compatibilityModeActivated` → `useFolderHierarchy` | Yes — rename | Low | --- ## What should NOT be changed The following parts of the implementation are well-designed and should be preserved as-is: | Component | Why it's good | |-----------|---------------| | `createTempFile()` + `ATOMIC_MOVE` in `replaceBlob()` | Correct atomic write pattern; temp file in same directory guarantees same-filesystem move | | `deleteEmptyParentDirs()` with `FileUtils.directoryContains()` guard | Reliable and safe; cannot accidentally delete above `bucketRoot` | | `writeToTempFile()` with `FileLock` | Prevents concurrent partial writes | | `@Nested class Compatible` in test | Valuable cross-format coverage for `GenerationAwareBlobId` ↔ `MinIOGenerationAwareBlobId` | | `listBuckets()` with `onErrorResume(NoSuchFileException)` | Clean reactive pattern — `listBlobs` should mirror this | | `FileBlobStoreDAOTest` with `@DisabledOnOs(OS.WINDOWS)` | Honest limitation documentation; concurrent lock tests are OS-specific | | Double system property check in `BlobDeduplicationGCModule` | Correct backward-compatible fallback chain using Optional; logic is clean | -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
