External update mechanism #62
Conversation
WalkthroughThis pull request introduces a new Experimental Update API to the Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🔇 Additional comments (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
🧹 Nitpick comments (11)
src/main/java/org/cryptomator/integrations/update/UpdateFailedException.java (1)
3-12: Add JavaDoc documentation for better API usability.The exception class follows standard Java patterns correctly. Consider adding JavaDoc to document when this exception is thrown and its purpose in the update framework.
+/** + * Thrown when an update operation fails or is cancelled. + * This exception may wrap underlying causes of update failures. + */ public class UpdateFailedException extends Exception { + /** + * Constructs an UpdateFailedException with the specified detail message. + * + * @param message the detail message + */ public UpdateFailedException(String message) { super(message); } + /** + * Constructs an UpdateFailedException with the specified detail message and cause. + * + * @param message the detail message + * @param cause the cause of the exception + */ public UpdateFailedException(String message, Throwable cause) { super(message, cause); } }src/main/java/org/cryptomator/integrations/update/ProgressListener.java (1)
3-6: Add JavaDoc documentation for the functional interface.The functional interface design is clean and follows best practices. Consider adding JavaDoc to document the contract and usage.
+/** + * Functional interface for listening to progress updates during update operations. + */ @FunctionalInterface public interface ProgressListener { + /** + * Called when progress is made during an update operation. + * + * @param progress the current progress information + */ void onProgress(Progress progress); }src/main/java/org/cryptomator/integrations/update/UpdateAvailableListener.java (1)
3-6: Add JavaDoc documentation for consistency with other API interfaces.The functional interface follows the same clean pattern as other listeners in the package. Consider adding JavaDoc documentation for consistency.
+/** + * Functional interface for listening to update availability notifications. + */ @FunctionalInterface public interface UpdateAvailableListener { + /** + * Called when an update becomes available. + * + * @param updateAvailable information about the available update + */ void onUpdateAvailable(UpdateAvailable updateAvailable); }src/main/java/org/cryptomator/integrations/update/SpawnExitedListener.java (1)
3-6: Complete the documentation set for all listener interfaces.The functional interface maintains consistency with other listeners in the package. Adding JavaDoc will complete the documentation set for the update API.
+/** + * Functional interface for listening to spawned process exit events during updates. + */ @FunctionalInterface public interface SpawnExitedListener { + /** + * Called when a spawned process exits during the update process. + * + * @param spawnExited information about the exited process + */ void onSpawnExited(SpawnExited spawnExited); }src/main/java/org/cryptomator/integrations/update/UpdateService.java (3)
9-11: Fix typos in class documentation.There are several spelling errors in the class-level JavaDoc.
- * This is the interface used by Cryptomator to provide a way to update Cryptomator in a convinient way. - * It's idependent of the supported platforms and package distribution channels. + * This is the interface used by Cryptomator to provide a way to update Cryptomator in a convenient way. + * It's independent of the supported platforms and package distribution channels.
19-22: Fix typos in method documentation.Multiple spelling errors in the
isSupported()method documentation.- * @return <code>true</code> if this UppdateService can update the app. + * @return <code>true</code> if this UpdateService can update the app. * @implSpec This method must not throw any exceptions and should fail fast - * returning <code>false</code> if it's not possible to use this UppdateService + * returning <code>false</code> if it's not possible to use this UpdateService
36-36: Fix typo in exception documentation.Spelling error in the
@throwsdocumentation.- * @throws UpdateFailedException If the udpate wasn't successful or was cancelled. + * @throws UpdateFailedException If the update wasn't successful or was cancelled.src/main/java/org/cryptomator/integrations/update/SpawnStartedListener.java (1)
4-5: Consider adding null safety documentation.The functional interface design is clean and follows Java conventions. However, consider adding documentation to clarify whether the
spawnStartedparameter can be null to prevent potential NPEs in implementations.@FunctionalInterface public interface SpawnStartedListener { + /** + * Called when a spawn process has started. + * @param spawnStarted the spawn information, must not be null + */ void onSpawnStarted(SpawnStarted spawnStarted); }src/main/java/org/cryptomator/integrations/update/SpawnStarted.java (1)
3-14: Consider adding equals, hashCode, and toString methods.Data classes typically benefit from proper equals/hashCode implementation for use in collections and toString for debugging. Also, consider adding documentation to explain what
relPidrepresents.+/** + * Represents information about a spawned process. + * @param pid the process ID + * @param relPid the relative process ID (explain what this means) + */ public class SpawnStarted { private final long pid; private final long relPid; // ... existing code ... public long getPid() { return pid; } public long getRelPid() { return relPid; } + + @Override + public boolean equals(Object obj) { + if (this == obj) return true; + if (obj == null || getClass() != obj.getClass()) return false; + SpawnStarted that = (SpawnStarted) obj; + return pid == that.pid && relPid == that.relPid; + } + + @Override + public int hashCode() { + return Long.hashCode(pid) * 31 + Long.hashCode(relPid); + } + + @Override + public String toString() { + return "SpawnStarted{pid=" + pid + ", relPid=" + relPid + '}'; + } }src/main/java/org/cryptomator/integrations/update/SpawnExited.java (1)
3-14: Consider adding equals, hashCode, and toString methods.For consistency with data class best practices and to enable proper usage in collections and debugging scenarios.
+/** + * Represents information about a spawned process that has exited. + */ public class SpawnExited { private final long pid; private final long exitStatus; // ... existing code ... public long getPid() { return pid; } public long getExitStatus() { return exitStatus; } + + @Override + public boolean equals(Object obj) { + if (this == obj) return true; + if (obj == null || getClass() != obj.getClass()) return false; + SpawnExited that = (SpawnExited) obj; + return pid == that.pid && exitStatus == that.exitStatus; + } + + @Override + public int hashCode() { + return Long.hashCode(pid) * 31 + Long.hashCode(exitStatus); + } + + @Override + public String toString() { + return "SpawnExited{pid=" + pid + ", exitStatus=" + exitStatus + '}'; + } }src/main/java/org/cryptomator/integrations/update/UpdateAvailable.java (1)
3-23: Consider adding equals, hashCode, toString, and documentation.For consistency with data class best practices and to provide clarity on what each commit type represents.
+/** + * Represents information about available software updates. + * Contains commit references for running, local, and remote versions. + */ public class UpdateAvailable { private final String runningCommit; private final String localCommit; private final String remoteCommit; // ... existing code ... + /** + * @return the commit hash of the currently running version + */ public String getRunningCommit() { return runningCommit; } + /** + * @return the commit hash of the locally available version + */ public String getLocalCommit() { return localCommit; } + /** + * @return the commit hash of the remote version + */ public String getRemoteCommit() { return remoteCommit; } + + @Override + public boolean equals(Object obj) { + if (this == obj) return true; + if (obj == null || getClass() != obj.getClass()) return false; + UpdateAvailable that = (UpdateAvailable) obj; + return Objects.equals(runningCommit, that.runningCommit) && + Objects.equals(localCommit, that.localCommit) && + Objects.equals(remoteCommit, that.remoteCommit); + } + + @Override + public int hashCode() { + return Objects.hash(runningCommit, localCommit, remoteCommit); + } + + @Override + public String toString() { + return "UpdateAvailable{" + + "runningCommit='" + runningCommit + '\'' + + ", localCommit='" + localCommit + '\'' + + ", remoteCommit='" + remoteCommit + '\'' + + '}'; + } }You'll need to add this import:
+import java.util.Objects;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
src/main/java/module-info.java(3 hunks)src/main/java/org/cryptomator/integrations/common/DistributionChannel.java(1 hunks)src/main/java/org/cryptomator/integrations/update/Progress.java(1 hunks)src/main/java/org/cryptomator/integrations/update/ProgressListener.java(1 hunks)src/main/java/org/cryptomator/integrations/update/SpawnExited.java(1 hunks)src/main/java/org/cryptomator/integrations/update/SpawnExitedListener.java(1 hunks)src/main/java/org/cryptomator/integrations/update/SpawnStarted.java(1 hunks)src/main/java/org/cryptomator/integrations/update/SpawnStartedListener.java(1 hunks)src/main/java/org/cryptomator/integrations/update/UpdateAvailable.java(1 hunks)src/main/java/org/cryptomator/integrations/update/UpdateAvailableListener.java(1 hunks)src/main/java/org/cryptomator/integrations/update/UpdateFailedException.java(1 hunks)src/main/java/org/cryptomator/integrations/update/UpdateService.java(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/main/java/org/cryptomator/integrations/update/UpdateService.java (1)
src/main/java/org/cryptomator/integrations/common/IntegrationsLoader.java (1)
IntegrationsLoader(19-163)
🔇 Additional comments (7)
src/main/java/org/cryptomator/integrations/common/DistributionChannel.java (1)
12-41: Well-designed annotation following Java best practices.The annotation implementation is excellent:
- Proper retention policy for runtime access
- Correct use of repeatable annotation pattern
- Comprehensive enum values covering major distribution channels
- Appropriate experimental marking for new API
- Safe default value (UNKNOWN)
src/main/java/module-info.java (6)
9-9: LGTM!The import statement is correctly added for the new UpdateService.
24-24: LGTM!The package export is correctly added to make the update integration API accessible.
34-34: LGTM!The service usage declaration is correctly added to enable service loading of UpdateService implementations.
9-9: LGTM!The import statement correctly references the new UpdateService interface, following the existing pattern in the module descriptor.
24-24: LGTM!The export declaration properly exposes the new update package to other modules, maintaining consistency with the existing export structure.
34-34: LGTM!The uses declaration correctly enables service loading for UpdateService, which is essential for the integration API pattern used throughout this module.
src/main/java/org/cryptomator/integrations/update/Progress.java
Outdated
Show resolved
Hide resolved
src/main/java/org/cryptomator/integrations/update/Progress.java
Outdated
Show resolved
Hide resolved
src/main/java/org/cryptomator/integrations/update/Progress.java
Outdated
Show resolved
Hide resolved
src/main/java/org/cryptomator/integrations/update/Progress.java
Outdated
Show resolved
Hide resolved
src/main/java/org/cryptomator/integrations/update/SpawnExited.java
Outdated
Show resolved
Hide resolved
src/main/java/org/cryptomator/integrations/update/UpdateAvailable.java
Outdated
Show resolved
Hide resolved
src/main/java/org/cryptomator/integrations/update/UpdateService.java
Outdated
Show resolved
Hide resolved
src/main/java/org/cryptomator/integrations/update/UpdateService.java
Outdated
Show resolved
Hide resolved
src/main/java/org/cryptomator/integrations/update/UpdateService.java
Outdated
Show resolved
Hide resolved
src/main/java/org/cryptomator/integrations/update/UpdateService.java
Outdated
Show resolved
Hide resolved
overheadhunter
left a comment
overheadhunter
left a comment
There was a problem hiding this comment.
Impressive overall proof of concept, however I believe the API can and needs to be more generic. After all we want the new API to be a common denominator for basically all update mechanisms.
I imagine a very lean API (as suggested in a separate PoC: UpdateMechanism and UpdateProcess)
src/main/java/org/cryptomator/integrations/common/DistributionChannel.java
Outdated
Show resolved
Hide resolved
src/main/java/org/cryptomator/integrations/update/Progress.java
Outdated
Show resolved
Hide resolved
src/main/java/org/cryptomator/integrations/update/UpdateService.java
Outdated
Show resolved
Hide resolved
src/main/java/org/cryptomator/integrations/update/UpdateService.java
Outdated
Show resolved
Hide resolved
src/main/java/org/cryptomator/integrations/update/UpdateService.java
Outdated
Show resolved
Hide resolved
src/main/java/org/cryptomator/integrations/update/UpdateService.java
Outdated
Show resolved
Hide resolved
Foremost, thanks for the quick review, and thanks for the suggestion on how to change the API to fit together with your PoC - UpdateMechanism and UpdateProcess. |
|
Changes to correlate with API as suggested in PoC: UpdateMechanism and UpdateProcess were implemented and tested. @overheadhunter Tested again with integrations-linux and Cryptomator. Hopefully, no more changes to integrations-api are needed. 🤞🏻 |
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (2)
src/main/java/org/cryptomator/integrations/update/UpdateProcess.java (2)
31-40: Clarifyawait(timeout)return valueThe Javadoc states “returns true if the update is prepared” but omits what
falsemeans (timeout vs. error vs. cancellation). Document all possible outcomes to avoid misuse.
48-51: Expose consistent failure semantics
applyUpdate()throwsIOException, whileprepareUpdate()inUpdateMechanismthrows the customUpdateFailedException. For symmetry (and simpler caller handling) consider using the same checked exception type here as well.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/main/java/module-info.java(3 hunks)src/main/java/org/cryptomator/integrations/update/DownloadUpdateProcess.java(1 hunks)src/main/java/org/cryptomator/integrations/update/UpdateFailedException.java(1 hunks)src/main/java/org/cryptomator/integrations/update/UpdateMechanism.java(1 hunks)src/main/java/org/cryptomator/integrations/update/UpdateProcess.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/java/org/cryptomator/integrations/update/UpdateFailedException.java
- src/main/java/module-info.java
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/main/java/org/cryptomator/integrations/update/UpdateProcess.java (1)
src/main/java/org/cryptomator/integrations/update/UpdateFailedException.java (1)
ApiStatus(7-17)
src/main/java/org/cryptomator/integrations/update/DownloadUpdateProcess.java
Outdated
Show resolved
Hide resolved
src/main/java/org/cryptomator/integrations/update/DownloadUpdateProcess.java
Show resolved
Hide resolved
src/main/java/org/cryptomator/integrations/update/DownloadUpdateProcess.java
Show resolved
Hide resolved
src/main/java/org/cryptomator/integrations/update/DownloadUpdateProcess.java
Outdated
Show resolved
Hide resolved
src/main/java/org/cryptomator/integrations/update/DownloadUpdateProcess.java
Show resolved
Hide resolved
src/main/java/org/cryptomator/integrations/update/UpdateMechanism.java
Outdated
Show resolved
Hide resolved
src/main/java/org/cryptomator/integrations/update/UpdateProcess.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
src/main/java/org/cryptomator/integrations/update/DownloadUpdateProcess.java (1)
163-170: EOF handling inverted – negative length fed toMessageDigest
The condition should ben > 0; passing ‑1 todigest.updateandcounter.addthrowsIllegalArgumentExceptionand corrupts progress.-if (n == -1) { - digest.update(b, off, n); - counter.add(n); -} +if (n > 0) { + digest.update(b, off, n); + counter.add(n); +}
🧹 Nitpick comments (3)
src/main/java/org/cryptomator/integrations/update/SemVerComparator.java (1)
9-13: Make the comparator non-instantiable and clearly singletonSince an INSTANCE is provided, prevent additional instantiation and clarify intent.
Apply this diff:
-public class SemVerComparator implements Comparator<String> { +public final class SemVerComparator implements Comparator<String> { public static final SemVerComparator INSTANCE = new SemVerComparator(); + + private SemVerComparator() { + // singleton + }src/main/java/org/cryptomator/integrations/update/DownloadUpdateProcess.java (2)
48-53:thisstill captured during construction
Moving thestart()call out is an improvement, but the thread capturingthisis still created before the object is fully constructed. Consider building/starting the thread instartDownload()instead to avoid premature escape entirely.
55-63: Progress can exceed 1.0
If the final download size differs from the initial estimate,loadedBytesmay outruntotalBytes, yielding values > 1. Clamp the result to1.0for predictable progress reporting.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/main/java/org/cryptomator/integrations/update/DownloadUpdateProcess.java(1 hunks)src/main/java/org/cryptomator/integrations/update/SemVerComparator.java(1 hunks)src/main/java/org/cryptomator/integrations/update/UpdateMechanism.java(1 hunks)src/main/java/org/cryptomator/integrations/update/UpdateProcess.java(1 hunks)src/test/java/org/cryptomator/integrations/update/SemVerComparatorTest.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/java/org/cryptomator/integrations/update/UpdateMechanism.java
- src/main/java/org/cryptomator/integrations/update/UpdateProcess.java
🔇 Additional comments (1)
src/test/java/org/cryptomator/integrations/update/SemVerComparatorTest.java (1)
10-75: Good coverage of SemVer precedence, including build metadata and spec’s exampleThe tests validate equality, ordering, and the spec’s precedence example. Solid baseline.
src/main/java/org/cryptomator/integrations/update/DownloadUpdateProcess.java
Outdated
Show resolved
Hide resolved
src/test/java/org/cryptomator/integrations/update/SemVerComparatorTest.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
src/main/java/org/cryptomator/integrations/update/UpdateMechanism.java (1)
15-18: Consider defaulting to highest‑priority implementation if property is unsetToday, get() returns empty when cryptomator.updateMechanism is not provided. Optionally fall back to IntegrationsLoader.load(UpdateMechanism.class) to keep out‑of‑the‑box behavior.
Example:
static Optional<UpdateMechanism> get() { - return Optional.ofNullable(System.getProperty(UPDATE_MECHANISM_PROPERTY)) - .flatMap(name -> IntegrationsLoader.loadSpecific(UpdateMechanism.class, name)); + var impl = System.getProperty(UPDATE_MECHANISM_PROPERTY); + return (impl == null || impl.isBlank()) + ? IntegrationsLoader.load(UpdateMechanism.class) + : IntegrationsLoader.loadSpecific(UpdateMechanism.class, impl); }Is the current “required explicit property” contract intentional for this API?
src/main/java/org/cryptomator/integrations/update/UpdateStep.java (1)
44-49: Document/annotate progress range
Add @range(from = -1, to = 1) to preparationProgress() to encode the contract (−1.0 indeterminate; 0.0–1.0 progress).- double preparationProgress(); + @Range(from = -1, to = 1) + double preparationProgress();src/main/java/org/cryptomator/integrations/update/UpdateStepAdapter.java (1)
16-18: Confirm JDK baseline (virtual threads and join(Duration))Thread.ofVirtual() and Thread.join(Duration) require a recent JDK (21+). If baseline < 21, this won’t compile/run.
If wider compatibility is needed, consider this fallback:
- this.thread = Thread.ofVirtual().name("UpdateStep", 0).unstarted(this); + ThreadFactory tf; + try { + tf = Thread.ofVirtual().name("UpdateStep", 0); + } catch (Throwable t) { // older JDK + tf = r -> { + Thread th = new Thread(r, "UpdateStep"); + th.setDaemon(true); + return th; + }; + } + this.thread = tf instanceof Thread.Builder ? ((Thread.Builder) tf).unstarted(this) : tf.newThread(this);- return thread.join(Duration.of(timeout, unit.toChronoUnit())); + long millis = unit.toMillis(timeout); + thread.join(millis); + return !thread.isAlive();Also applies to: 49-52
src/main/java/org/cryptomator/integrations/update/DownloadUpdateStep.java (2)
48-55: Progress/UX nits (optional)
- description(): minor cosmetic tweaks (remove trailing space, unify messages).
- preparationProgress(): divide by local total once to avoid double volatile reads (micro‑nit).
- HTTP status check: include URI in error to aid diagnostics.
- case NEW -> "Download... "; + case NEW -> "Download…"; ... - return (double) loadedBytes.sum() / totalBytes.get(); + long t = totalBytes.get(); + return (double) loadedBytes.sum() / t; ... - throw new IOException("Failed to download update, status code: " + response.statusCode()); + throw new IOException("Failed to download update from " + source + ", status code: " + response.statusCode());Also applies to: 62-70, 98-111
22-47: Align with UpdateStepAdapter to avoid duplicated lifecycle logic (optional)DownloadUpdateStep re‑implements threading/await/cancel that UpdateStepAdapter already provides. Consider extending UpdateStepAdapter and moving download() into call(), returning the next step on success. This unifies nextStep() semantics and reduces duplication.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/main/java/org/cryptomator/integrations/common/IntegrationsLoader.java(1 hunks)src/main/java/org/cryptomator/integrations/update/DownloadUpdateStep.java(1 hunks)src/main/java/org/cryptomator/integrations/update/UpdateMechanism.java(1 hunks)src/main/java/org/cryptomator/integrations/update/UpdateStep.java(1 hunks)src/main/java/org/cryptomator/integrations/update/UpdateStepAdapter.java(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
src/main/java/org/cryptomator/integrations/common/IntegrationsLoader.java (1)
src/main/java/org/cryptomator/integrations/common/ClassLoaderFactory.java (1)
ClassLoaderFactory(18-76)
src/main/java/org/cryptomator/integrations/update/UpdateStep.java (1)
src/main/java/org/cryptomator/integrations/update/UpdateFailedException.java (1)
ApiStatus(7-17)
src/main/java/org/cryptomator/integrations/update/UpdateMechanism.java (3)
src/main/java/org/cryptomator/integrations/common/IntegrationsLoader.java (1)
IntegrationsLoader(19-176)src/main/java/org/cryptomator/integrations/update/UpdateFailedException.java (1)
ApiStatus(7-17)src/main/java/org/cryptomator/integrations/update/SemVerComparator.java (1)
SemVerComparator(9-84)
🔇 Additional comments (2)
src/main/java/org/cryptomator/integrations/update/UpdateMechanism.java (1)
26-32: Verify SNAPSHOT policyisUpdateAvailable() returns true for any installed SNAPSHOT regardless of updateVersion. Confirm this matches desired semantics (i.e., nightly/dev builds are always considered “updatable”).
src/main/java/org/cryptomator/integrations/update/DownloadUpdateStep.java (1)
87-96: HttpClient in try‑with‑resources: compile errorjava.net.http.HttpClient is not AutoCloseable; the TWR block won’t compile. Also add request/connection timeouts to avoid hangs.
Apply:
- protected void download() { - var request = HttpRequest.newBuilder().uri(source).GET().build(); - try (HttpClient client = HttpClient.newBuilder().followRedirects(HttpClient.Redirect.ALWAYS).build()) { - downloadInternal(client, request); - } catch (IOException e) { - downloadException = e; - } finally { - downloadCompleted.countDown(); - } - } + protected void download() { + var request = HttpRequest.newBuilder() + .uri(source) + .timeout(java.time.Duration.ofSeconds(30)) + .GET() + .build(); + var client = HttpClient.newBuilder() + .followRedirects(HttpClient.Redirect.ALWAYS) + .connectTimeout(java.time.Duration.ofSeconds(10)) + .build(); + try { + downloadInternal(client, request); + } catch (IOException e) { + downloadException = e; + } finally { + downloadCompleted.countDown(); + } + }If you prefer explicit imports, add:
+import java.time.Duration;at the top.
⛔ Skipped due to learnings
Learnt from: overheadhunter PR: cryptomator/integrations-api#62 File: src/main/java/org/cryptomator/integrations/update/DownloadUpdateProcess.java:107-110 Timestamp: 2025-08-09T10:00:34.301Z Learning: In JDK 21 and later, java.net.http.HttpClient implements AutoCloseable interface, allowing it to be used in try-with-resources statements. This includes new methods like close(), shutdown(), shutdownNow(), awaitTermination(), and isTerminated() for better resource management.
| public static <T> Optional<T> loadSpecific(Class<T> clazz, String implementationClassName) { | ||
| return ServiceLoader.load(clazz, ClassLoaderFactory.forPluginDir()).stream() | ||
| .filter(provider -> provider.type().getName().equals(implementationClassName)) | ||
| .map(ServiceLoader.Provider::get) | ||
| .findAny(); | ||
| } |
There was a problem hiding this comment.
loadSpecific bypasses OS/availability checks and can throw at instantiation
This maps Provider::get directly, skipping isSupportedOperatingSystem, passesStaticAvailabilityCheck, instantiateServiceProvider (error handling), and passesInstanceAvailabilityCheck. A mismatched provider may be instantiated or ServiceConfigurationError may surface to callers.
Apply this diff to reuse the vetted pipeline and add null checks:
public static <T> Optional<T> loadSpecific(Class<T> clazz, String implementationClassName) {
- return ServiceLoader.load(clazz, ClassLoaderFactory.forPluginDir()).stream()
- .filter(provider -> provider.type().getName().equals(implementationClassName))
- .map(ServiceLoader.Provider::get)
- .findAny();
+ Objects.requireNonNull(clazz, "Service to load not specified.");
+ Objects.requireNonNull(implementationClassName, "Implementation class name not specified.");
+ var sl = ServiceLoader.load(clazz, ClassLoaderFactory.forPluginDir());
+ return loadAll(sl, clazz)
+ .filter(impl -> impl.getClass().getName().equals(implementationClassName))
+ .findAny();
}Committable suggestion skipped: line range outside the PR's diff.
| // write bytes to file | ||
| try (var in = new DownloadInputStream(response.body(), loadedBytes, sha256); | ||
| var src = Channels.newChannel(in); | ||
| var dst = FileChannel.open(destination, StandardOpenOption.WRITE, StandardOpenOption.CREATE_NEW)) { | ||
| dst.transferFrom(src, 0, Long.MAX_VALUE); | ||
| } | ||
|
|
||
| // verify checksum if provided | ||
| byte[] calculatedChecksum = sha256.digest(); | ||
| if (checksum != null && !MessageDigest.isEqual(calculatedChecksum, checksum)) { | ||
| throw new IOException("Checksum verification failed for downloaded file: " + destination); | ||
| } |
There was a problem hiding this comment.
Prevent leftover partial files; write to temp and atomically move after checksum verification
Failures (I/O, checksum mismatch) currently leave a partial file at destination. Write to a “.part” file, delete on failure, and move atomically on success. Also permit overwrite behavior to be explicit.
- try (var in = new DownloadInputStream(response.body(), loadedBytes, sha256);
- var src = Channels.newChannel(in);
- var dst = FileChannel.open(destination, StandardOpenOption.WRITE, StandardOpenOption.CREATE_NEW)) {
- dst.transferFrom(src, 0, Long.MAX_VALUE);
- }
+ var tmp = destination.resolveSibling(destination.getFileName() + ".part");
+ try (var in = new DownloadInputStream(response.body(), loadedBytes, sha256);
+ var src = Channels.newChannel(in);
+ var dst = FileChannel.open(tmp, StandardOpenOption.WRITE, StandardOpenOption.CREATE, StandardOpenOption.TRUNCATE_EXISTING)) {
+ dst.transferFrom(src, 0, Long.MAX_VALUE);
+ } catch (IOException ioe) {
+ try { java.nio.file.Files.deleteIfExists(tmp); } catch (IOException ignore) {}
+ throw ioe;
+ }
...
- if (checksum != null && !MessageDigest.isEqual(calculatedChecksum, checksum)) {
- throw new IOException("Checksum verification failed for downloaded file: " + destination);
- }
+ if (checksum != null && !MessageDigest.isEqual(calculatedChecksum, checksum)) {
+ try { java.nio.file.Files.deleteIfExists(tmp); } catch (IOException ignore) {}
+ throw new IOException("Checksum verification failed for downloaded file: " + destination);
+ }
+ // move into place after successful verification
+ java.nio.file.Files.move(tmp, destination, java.nio.file.StandardCopyOption.ATOMIC_MOVE, java.nio.file.StandardCopyOption.REPLACE_EXISTING);Also applies to: 126-128, 130-135
🤖 Prompt for AI Agents
In src/main/java/org/cryptomator/integrations/update/DownloadUpdateStep.java
around lines 123-134, avoid leaving partial files by writing download output to
a temporary ".part" file (e.g., destination + ".part") using CREATE_NEW, verify
checksum against the temp file's stream, and on successful verification
atomically move/rename the temp file to the final destination (use Files.move
with ATOMIC_MOVE and allow an explicit overwrite flag to control whether
REPLACE_EXISTING is used). Ensure any exception or checksum mismatch deletes the
temp file in a finally block so no leftover partial remains, and log or
propagate errors consistently.
| * Blocks the current thread until this update step completed or an error occured. | ||
| * <p> | ||
| * If the step is already complete, this method returns immediately. | ||
| * | ||
| * @throws InterruptedException if the current thread is interrupted while waiting. | ||
| */ |
There was a problem hiding this comment.
Fix typos in Javadoc (“occurred”)
- * Blocks the current thread until this update step completed or an error occured.
+ * Blocks the current thread until this update step completed or an error occurred.
...
- * Blocks the current thread until this update step completed or an error occured, or until the specified timeout expires.
+ * Blocks the current thread until this update step completed or an error occurred, or until the specified timeout expires.Also applies to: 65-73
🤖 Prompt for AI Agents
In src/main/java/org/cryptomator/integrations/update/UpdateStep.java around
lines 56-61 and 65-73, the Javadoc contains the misspelling "occured"; update
both occurrences to the correct spelling "occurred" so the comments read "an
error occurred" (and adjust any corresponding grammar if needed).
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/main/java/org/cryptomator/integrations/update/UpdateStep.java (1)
69-69: Typo already flagged in previous review.The misspelling "occured" should be "occurred" in both Javadoc comments (lines 69 and 78).
Also applies to: 78-78
🧹 Nitpick comments (3)
src/main/java/org/cryptomator/integrations/update/NoopUpdateStep.java (1)
19-19: Consider clarifying the await() no-op behavior.The empty await() implementation is correct for a no-op step (nothing to wait for), but a brief comment explaining this rationale would help future maintainers understand why blocking isn't needed here.
@Override - public void await() {} + public void await() { + // No-op: step is always immediately complete + }src/main/java/org/cryptomator/integrations/update/UpdateStep.java (2)
57-61: Consider adding @range annotation for tooling support.The Javadoc specifies that preparationProgress() returns "a value between 0.0 and 1.0 ... or -1.0," but the imported @range annotation isn't used. Adding it would provide better IDE support and static analysis.
+ @Range(from = -1, to = 1) double preparationProgress();Note: Verify that the @range annotation from JetBrains supports double values with this specific range including -1.0 as a special sentinel.
82-84: Clarify the return value documentation.The Javadoc states "@return true if the update is prepared," but this doesn't clearly explain when false is returned. It should indicate that false means the timeout expired before completion.
* @param timeout the maximum time to wait * @param unit the time unit of the {@code timeout} argument - * @return true if the update is prepared + * @return true if the step completed within the timeout, false if the timeout expired */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/java/org/cryptomator/integrations/update/NoopUpdateStep.java(1 hunks)src/main/java/org/cryptomator/integrations/update/UpdateStep.java(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/main/java/org/cryptomator/integrations/update/UpdateStep.java (1)
src/main/java/org/cryptomator/integrations/update/UpdateFailedException.java (1)
ApiStatus(7-17)
🔇 Additional comments (5)
src/main/java/org/cryptomator/integrations/update/NoopUpdateStep.java (1)
5-29: LGTM! Clean no-op implementation.The record correctly implements all UpdateStep methods with appropriate no-op semantics. Package-private visibility is suitable since NoopUpdateStep serves as an internal implementation detail for the EXIT and RETRY sentinel constants.
src/main/java/org/cryptomator/integrations/update/UpdateStep.java (4)
21-26: LGTM! Well-documented sentinel constants.The EXIT and RETRY constants provide clear, type-safe sentinels for controlling update flow. Using NoopUpdateStep instances is appropriate since these represent terminal or retry states.
29-42: LGTM! Clean factory pattern.The static factory method provides a convenient way to create UpdateStep instances from a Callable. The delegation to UpdateStepAdapter is a clean abstraction.
88-95: LGTM! Proper interrupt handling.The isDone() default implementation correctly restores the interrupt flag when InterruptedException is caught. This follows best practices for interrupt handling.
97-108: LGTM! Well-documented nextStep contract.The nextStep() method provides a clear contract for chaining update steps, with appropriate exception handling and the @implSpec note about statelessness/new instances.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
pom.xml(1 hunks)src/main/java/org/cryptomator/integrations/update/UpdateInfo.java(1 hunks)src/main/java/org/cryptomator/integrations/update/UpdateMechanism.java(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/main/java/org/cryptomator/integrations/update/UpdateMechanism.java (3)
src/main/java/org/cryptomator/integrations/common/IntegrationsLoader.java (1)
IntegrationsLoader(19-176)src/main/java/org/cryptomator/integrations/update/UpdateFailedException.java (1)
ApiStatus(7-17)src/main/java/org/cryptomator/integrations/update/SemVerComparator.java (1)
SemVerComparator(9-84)
🔇 Additional comments (2)
pom.xml (1)
37-37: Mockito 5.20.0 version bump is verified as safe.Mockito 5.20.0 exists with new features and bug fixes, with no breaking changes documented between 5.16.1 and 5.20.0. Since this is a test-only dependency, the upgrade carries no production risk and introduces only improvements.
src/main/java/org/cryptomator/integrations/update/UpdateInfo.java (1)
3-4: Record shape looks goodThe minimal record cleanly models version plus its originating mechanism; no issues from my side.
| static Optional<UpdateMechanism> get() { | ||
| return Optional.ofNullable(System.getProperty(UPDATE_MECHANISM_PROPERTY)) | ||
| .flatMap(name -> IntegrationsLoader.loadSpecific(UpdateMechanism.class, name)); | ||
| } |
There was a problem hiding this comment.
Ensure get() falls back to the default provider
With the current implementation, get() yields Optional.empty() whenever the system property isn’t set or the configured class name can’t be resolved—even if suitable UpdateMechanism implementations are registered. That effectively disables the update mechanism unless consumers remember to bypass this helper and call IntegrationsLoader.load(UpdateMechanism.class) themselves. Please fall back to the default loader (and ideally log when the explicit class lookup fails) so get() remains a reliable entry point. Sample fix:
- return Optional.ofNullable(System.getProperty(UPDATE_MECHANISM_PROPERTY))
- .flatMap(name -> IntegrationsLoader.loadSpecific(UpdateMechanism.class, name));
+ var explicitlyConfigured = Optional.ofNullable(System.getProperty(UPDATE_MECHANISM_PROPERTY))
+ .flatMap(name -> IntegrationsLoader.loadSpecific(UpdateMechanism.class, name));
+ return explicitlyConfigured.isPresent() ? explicitlyConfigured : IntegrationsLoader.load(UpdateMechanism.class);🤖 Prompt for AI Agents
In src/main/java/org/cryptomator/integrations/update/UpdateMechanism.java around
lines 17–20, the get() helper currently returns empty if the system property is
missing or the named provider cannot be resolved; change it to first check
System.getProperty(UPDATE_MECHANISM_PROPERTY) and if absent return
IntegrationsLoader.load(UpdateMechanism.class), and if present attempt
IntegrationsLoader.loadSpecific(UpdateMechanism.class, name) but when that
returns empty log a warning/error with the attempted class name and then fall
back to IntegrationsLoader.load(UpdateMechanism.class) so get() always returns
any available registered provider.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/main/java/org/cryptomator/integrations/update/UpdateMechanism.java (1)
18-22:get()should fall back to default service loader.The current implementation returns
Optional.empty()when the system property is unset or references an unavailable implementation, even if validUpdateMechanismproviders exist. This breaksget()as a reliable entry point—callers must bypass it and directly useIntegrationsLoader.load(UpdateMechanism.class).Fall back to the default loader when explicit configuration is absent or fails.
Apply this diff to add fallback logic:
@SuppressWarnings("rawtypes") static Optional<UpdateMechanism> get() { - return Optional.ofNullable(System.getProperty(UPDATE_MECHANISM_PROPERTY)) - .flatMap(name -> IntegrationsLoader.loadSpecific(UpdateMechanism.class, name)); + var explicitlyConfigured = Optional.ofNullable(System.getProperty(UPDATE_MECHANISM_PROPERTY)) + .flatMap(name -> IntegrationsLoader.loadSpecific(UpdateMechanism.class, name)); + return explicitlyConfigured.isPresent() ? explicitlyConfigured : IntegrationsLoader.load(UpdateMechanism.class); }
🧹 Nitpick comments (1)
src/main/java/org/cryptomator/integrations/update/UpdateInfo.java (1)
23-26: Consider simplifying the method name.The method name
useToPrepareFirstStep()is somewhat verbose. Consider alternatives likeprepareFirstStep(),getFirstStep(), orstartUpdate()for improved readability. However, if this naming aligns with conventions established in the referenced UpdateMechanism/UpdateProcess PoC classes, consistency may be preferable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/main/java/org/cryptomator/integrations/update/BasicUpdateInfo.java(1 hunks)src/main/java/org/cryptomator/integrations/update/UpdateInfo.java(1 hunks)src/main/java/org/cryptomator/integrations/update/UpdateMechanism.java(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/main/java/org/cryptomator/integrations/update/UpdateMechanism.java (3)
src/main/java/org/cryptomator/integrations/common/IntegrationsLoader.java (1)
IntegrationsLoader(19-176)src/main/java/org/cryptomator/integrations/update/UpdateFailedException.java (1)
ApiStatus(7-17)src/main/java/org/cryptomator/integrations/update/SemVerComparator.java (1)
SemVerComparator(9-84)
🔇 Additional comments (5)
src/main/java/org/cryptomator/integrations/update/BasicUpdateInfo.java (1)
1-4: LGTM! Clean and minimal record implementation.The record correctly implements the
UpdateInfointerface with proper self-referential generics. This design aligns well with the PR's goal of a lean, minimalistic API.src/main/java/org/cryptomator/integrations/update/UpdateMechanism.java (3)
30-36: LGTM! Version comparison logic is sound.The SNAPSHOT special case and SemVer-based comparison provide appropriate update detection semantics.
38-47: LGTM! Method signature is well-designed.The
@Blockingand@Nullableannotations clearly communicate the method's behavior, and throwingUpdateFailedExceptionon errors (rather than silently returningfalseornull) allows callers to distinguish check failures from "no update available."
49-56: LGTM! Clear contract for update preparation.The method signature and documentation clearly define the first step in the update process.
src/main/java/org/cryptomator/integrations/update/UpdateInfo.java (1)
5-5: LGTM: Self-referential generic pattern appropriately used.The F-bounded polymorphism pattern (
UpdateInfo<T extends UpdateInfo<T>>) enables type-safe delegation between specificUpdateInfoimplementations and their correspondingUpdateMechanism<T>instances. This design allows implementations to maintain compile-time type safety while avoiding manual casts at call sites.
# Conflicts: # pom.xml
|
Closing in favour of #72 |
2 participants
No description provided.