Migrate to DownloadHandler API for Vaadin 25 compatibility #189
Conversation
WalkthroughAdds DownloadHandler-based export APIs, a semaphore-backed concurrency core, a StreamResourceWriter→DownloadHandler adapter, GridExporter download handler methods, version bumps, and documentation (migration guide + README) for migrating from StreamResource to DownloadHandler. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 0
🧹 Nitpick comments (3)
src/main/java/com/flowingcode/vaadin/addons/gridexporter/StreamResourceWriterAdapter.java (1)
50-54: Add null check for writer parameter.The
writerparameter should be validated to prevent potential NPE. While the adapter pattern delegates resource management appropriately, defensive validation improves robustness.Apply this diff to add validation:
public StreamResourceWriterAdapter(StreamResourceWriter writer, String filename, String contentType) { + this.writer = Objects.requireNonNull(writer, "writer cannot be null"); - this.writer = writer; this.filename = filename; this.contentType = contentType; }src/main/java/com/flowingcode/vaadin/addons/gridexporter/GridExporter.java (1)
617-621: Use pattern matching for instanceof.Since the project targets Java 17, you can simplify the instanceof check using pattern matching.
Apply this diff:
private void setButtonEnabled(boolean enabled) { - if (button instanceof HasEnabled) { - grid.getUI().ifPresent(ui -> ui.access(() -> ((HasEnabled) button).setEnabled(enabled))); + if (button instanceof HasEnabled hasEnabled) { + grid.getUI().ifPresent(ui -> ui.access(() -> hasEnabled.setEnabled(enabled))); } }src/main/java/com/flowingcode/vaadin/addons/gridexporter/ConcurrentDownloadHandler.java (1)
28-28: Remove unused import.The
java.io.OutputStreamimport is not used in this file.Apply this diff:
import java.io.IOException; import java.io.InterruptedIOException; -import java.io.OutputStream; import java.nio.channels.InterruptedByTimeoutException;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
MIGRATION_GUIDE.md(1 hunks)README.md(1 hunks)pom.xml(1 hunks)src/main/java/com/flowingcode/vaadin/addons/gridexporter/ConcurrentDownloadHandler.java(1 hunks)src/main/java/com/flowingcode/vaadin/addons/gridexporter/GridExporter.java(5 hunks)src/main/java/com/flowingcode/vaadin/addons/gridexporter/StreamResourceWriterAdapter.java(1 hunks)src/test/java/com/flowingcode/vaadin/addons/gridexporter/GridExporterCustomLinkDemo.java(2 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: javier-godoy
Repo: FlowingCode/GridExporterAddon PR: 119
File: src/main/java/com/flowingcode/vaadin/addons/gridexporter/CsvStreamResourceWriter.java:77-93
Timestamp: 2024-07-27T22:14:37.039Z
Learning: User javier-godoy requested to open a follow-up GitHub issue to discuss resource management in the refactored code of CsvStreamResourceWriter, highlighting the absence of try-with-resources in the previous implementation.
Learnt from: javier-godoy
Repo: FlowingCode/GridExporterAddon PR: 119
File: src/main/java/com/flowingcode/vaadin/addons/gridexporter/CsvStreamResourceWriter.java:77-93
Timestamp: 2024-10-08T21:28:24.972Z
Learning: User javier-godoy requested to open a follow-up GitHub issue to discuss resource management in the refactored code of CsvStreamResourceWriter, highlighting the absence of try-with-resources in the previous implementation.
Learnt from: javier-godoy
Repo: FlowingCode/GridExporterAddon PR: 143
File: src/main/java/com/flowingcode/vaadin/addons/gridexporter/GridExporter.java:315-315
Timestamp: 2024-10-08T21:28:24.972Z
Learning: In the `GridExporter` class, CSV exports are not subject to concurrent controls, so `getCsvStreamResource` returns a `StreamResource` instead of a `GridExporterStreamResource`.
Learnt from: javier-godoy
Repo: FlowingCode/GridExporterAddon PR: 143
File: src/main/java/com/flowingcode/vaadin/addons/gridexporter/GridExporter.java:315-315
Timestamp: 2024-09-26T18:26:03.345Z
Learning: In the `GridExporter` class, CSV exports are not subject to concurrent controls, so `getCsvStreamResource` returns a `StreamResource` instead of a `GridExporterStreamResource`.
📚 Learning: 2024-10-08T21:28:24.972Z
Learnt from: javier-godoy
Repo: FlowingCode/GridExporterAddon PR: 143
File: src/main/java/com/flowingcode/vaadin/addons/gridexporter/GridExporter.java:315-315
Timestamp: 2024-10-08T21:28:24.972Z
Learning: In the `GridExporter` class, CSV exports are not subject to concurrent controls, so `getCsvStreamResource` returns a `StreamResource` instead of a `GridExporterStreamResource`.
Applied to files:
MIGRATION_GUIDE.mdsrc/main/java/com/flowingcode/vaadin/addons/gridexporter/GridExporter.javasrc/main/java/com/flowingcode/vaadin/addons/gridexporter/StreamResourceWriterAdapter.javasrc/test/java/com/flowingcode/vaadin/addons/gridexporter/GridExporterCustomLinkDemo.java
📚 Learning: 2024-06-09T06:20:09.702Z
Learnt from: javier-godoy
Repo: FlowingCode/GridExporterAddon PR: 125
File: src/main/java/com/flowingcode/vaadin/addons/gridexporter/GridExporter.java:338-402
Timestamp: 2024-06-09T06:20:09.702Z
Learning: Zero or negative costs in the `setConcurrentDownloadCost` method do not require any permits, allowing such downloads to bypass the semaphore mechanism.
Applied to files:
src/main/java/com/flowingcode/vaadin/addons/gridexporter/GridExporter.javasrc/main/java/com/flowingcode/vaadin/addons/gridexporter/ConcurrentDownloadHandler.java
📚 Learning: 2024-10-28T17:35:31.077Z
Learnt from: javier-godoy
Repo: FlowingCode/GridExporterAddon PR: 154
File: src/main/java/com/flowingcode/vaadin/addons/gridexporter/BaseStreamResourceWriter.java:98-103
Timestamp: 2024-10-28T17:35:31.077Z
Learning: `BaseStreamResourceWriter` and its extending classes are not public API currently, but the return type must be refactored as we are going to expose `BaseStreamResourceWriter` in the near future.
Applied to files:
src/main/java/com/flowingcode/vaadin/addons/gridexporter/StreamResourceWriterAdapter.java
🧬 Code graph analysis (1)
src/main/java/com/flowingcode/vaadin/addons/gridexporter/GridExporter.java (1)
src/main/java/com/flowingcode/vaadin/addons/gridexporter/GridExporterConcurrentSettings.java (1)
GridExporterConcurrentSettings(37-123)
🪛 GitHub Check: SonarCloud Code Analysis
src/main/java/com/flowingcode/vaadin/addons/gridexporter/GridExporter.java
[warning] 333-333: Do not forget to remove this deprecated code someday.
[warning] 388-388: Do not forget to remove this deprecated code someday.
[warning] 347-347: Do not forget to remove this deprecated code someday.
[warning] 402-402: Do not forget to remove this deprecated code someday.
[warning] 319-319: Do not forget to remove this deprecated code someday.
[warning] 375-375: Do not forget to remove this deprecated code someday.
[warning] 361-361: Do not forget to remove this deprecated code someday.
[warning] 618-618: Replace this instanceof check and cast with 'instanceof HasEnabled hasenabled'
src/main/java/com/flowingcode/vaadin/addons/gridexporter/ConcurrentDownloadHandler.java
[warning] 28-28: Remove this unused import 'java.io.OutputStream'.
🪛 LanguageTool
MIGRATION_GUIDE.md
[style] ~146-~146: Try using a more formal synonym for ‘check’.
Context: ...any issues during migration, please: 1. Check that you're using Vaadin 24.8.0 or late...
(VERIFY)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-vaadin24
🔇 Additional comments (14)
pom.xml (1)
7-7: LGTM!The version updates correctly reflect the new feature set (DownloadHandler API) and required Vaadin dependency.
Also applies to: 13-13
README.md (1)
57-83: LGTM!The migration documentation is clear, comprehensive, and provides helpful quick-start guidance for users upgrading to the new API.
src/test/java/com/flowingcode/vaadin/addons/gridexporter/GridExporterCustomLinkDemo.java (1)
79-79: LGTM!The test correctly demonstrates migration to the new
getExcelDownloadHandler()API.MIGRATION_GUIDE.md (1)
1-189: LGTM!The migration guide is comprehensive, well-structured, and provides clear before/after examples for all export formats.
src/main/java/com/flowingcode/vaadin/addons/gridexporter/GridExporter.java (5)
226-232: LGTM!Formatting improvement with no functional changes.
310-405: LGTM!The deprecation strategy is well-documented with clear migration paths to the new DownloadHandler-based methods. The removal timeline (version 3.0.0) provides adequate time for users to migrate.
406-489: LGTM!The new DownloadHandler-based methods are correctly implemented. CSV export appropriately uses
StreamResourceWriterAdapterdirectly without concurrent controls, consistent with the existing design where CSV exports are not throttled. Based on learnings, CSV exports are intentionally not subject to concurrent controls.
496-500: LGTM!The helper method cleanly composes the adapter and concurrent handler layers.
570-634: LGTM!The
GridExporterConcurrentDownloadHandlercorrectly implements the concurrency framework with appropriate lifecycle hooks and UI association.src/main/java/com/flowingcode/vaadin/addons/gridexporter/ConcurrentDownloadHandler.java (5)
46-58: LGTM!The constants and static fields are well-defined for the global concurrency control mechanism.
60-90: LGTM!The
ConfigurableSemaphorecorrectly manages dynamic permit allocation with proper synchronization.
103-137: LGTM!The limit management correctly handles enabling/disabling the semaphore and validates input parameters.
139-148: LGTM!The cost-to-permits conversion correctly implements fixed-point arithmetic for fractional costs and properly handles zero/negative costs to bypass the semaphore. Based on learnings, this aligns with the documented behavior where non-positive costs require no permits.
277-315: LGTM!The download handling logic correctly implements:
- Lifecycle callbacks (onAccept/onFinish)
- Permit acquisition with timeout
- UI change detection
- Proper resource cleanup in finally blocks
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/main/java/com/flowingcode/vaadin/addons/gridexporter/ConcurrentStreamResourceWriter.java (1)
176-185: Fix indentation and spacing consistency.The
ifstatement on line 178 has inconsistent indentation (appears to be misaligned with the surrounding code) and missing spaces around the!=operator.if (semaphore.tryAcquire(permits, getTimeout(), TimeUnit.NANOSECONDS)) { try { - if (ui != null && getAttachedUI()!=ui) { - // The UI has changed or was detached after acquirig the semaphore - throw new IOException("Detached UI"); - } + if (ui != null && getAttachedUI() != ui) { + // The UI has changed or was detached after acquiring the semaphore + throw new IOException("Detached UI"); + } delegate.accept(stream, session); } finally { semaphore.release(permits);Note: There's also a typo "acquirig" → "acquiring" in the comment.
src/main/java/com/flowingcode/vaadin/addons/gridexporter/ConcurrentDownloadHandler.java (1)
56-64: Minor Javadoc inconsistency: "Sets" should be "Returns".The Javadoc at line 57 says "Sets the timeout" but this is a getter method that returns the timeout value.
/** - * Sets the timeout for acquiring a permit to start a download when there are + * Returns the timeout for acquiring a permit to start a download when there are * not enough permits * available in the semaphore. * * @see GridExporter#setConcurrentDownloadTimeout(long, TimeUnit) * @return the timeout in nanoseconds. */ public abstract long getTimeout();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/main/java/com/flowingcode/vaadin/addons/gridexporter/ConcurrentDownloadHandler.java(1 hunks)src/main/java/com/flowingcode/vaadin/addons/gridexporter/ConcurrentOperationBase.java(1 hunks)src/main/java/com/flowingcode/vaadin/addons/gridexporter/ConcurrentStreamResourceWriter.java(2 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: javier-godoy
Repo: FlowingCode/GridExporterAddon PR: 119
File: src/main/java/com/flowingcode/vaadin/addons/gridexporter/CsvStreamResourceWriter.java:77-93
Timestamp: 2024-07-27T22:14:37.039Z
Learning: User javier-godoy requested to open a follow-up GitHub issue to discuss resource management in the refactored code of CsvStreamResourceWriter, highlighting the absence of try-with-resources in the previous implementation.
Learnt from: javier-godoy
Repo: FlowingCode/GridExporterAddon PR: 119
File: src/main/java/com/flowingcode/vaadin/addons/gridexporter/CsvStreamResourceWriter.java:77-93
Timestamp: 2024-10-08T21:28:24.972Z
Learning: User javier-godoy requested to open a follow-up GitHub issue to discuss resource management in the refactored code of CsvStreamResourceWriter, highlighting the absence of try-with-resources in the previous implementation.
Learnt from: javier-godoy
Repo: FlowingCode/GridExporterAddon PR: 143
File: src/main/java/com/flowingcode/vaadin/addons/gridexporter/GridExporter.java:315-315
Timestamp: 2024-10-08T21:28:24.972Z
Learning: In the `GridExporter` class, CSV exports are not subject to concurrent controls, so `getCsvStreamResource` returns a `StreamResource` instead of a `GridExporterStreamResource`.
Learnt from: javier-godoy
Repo: FlowingCode/GridExporterAddon PR: 143
File: src/main/java/com/flowingcode/vaadin/addons/gridexporter/GridExporter.java:315-315
Timestamp: 2024-09-26T18:26:03.345Z
Learning: In the `GridExporter` class, CSV exports are not subject to concurrent controls, so `getCsvStreamResource` returns a `StreamResource` instead of a `GridExporterStreamResource`.
📚 Learning: 2024-10-08T21:28:24.972Z
Learnt from: javier-godoy
Repo: FlowingCode/GridExporterAddon PR: 143
File: src/main/java/com/flowingcode/vaadin/addons/gridexporter/GridExporter.java:315-315
Timestamp: 2024-10-08T21:28:24.972Z
Learning: In the `GridExporter` class, CSV exports are not subject to concurrent controls, so `getCsvStreamResource` returns a `StreamResource` instead of a `GridExporterStreamResource`.
Applied to files:
src/main/java/com/flowingcode/vaadin/addons/gridexporter/ConcurrentStreamResourceWriter.javasrc/main/java/com/flowingcode/vaadin/addons/gridexporter/ConcurrentOperationBase.java
📚 Learning: 2024-10-28T17:35:31.077Z
Learnt from: javier-godoy
Repo: FlowingCode/GridExporterAddon PR: 154
File: src/main/java/com/flowingcode/vaadin/addons/gridexporter/BaseStreamResourceWriter.java:98-103
Timestamp: 2024-10-28T17:35:31.077Z
Learning: `BaseStreamResourceWriter` and its extending classes are not public API currently, but the return type must be refactored as we are going to expose `BaseStreamResourceWriter` in the near future.
Applied to files:
src/main/java/com/flowingcode/vaadin/addons/gridexporter/ConcurrentStreamResourceWriter.java
📚 Learning: 2024-06-09T06:20:09.702Z
Learnt from: javier-godoy
Repo: FlowingCode/GridExporterAddon PR: 125
File: src/main/java/com/flowingcode/vaadin/addons/gridexporter/GridExporter.java:338-402
Timestamp: 2024-06-09T06:20:09.702Z
Learning: Zero or negative costs in the `setConcurrentDownloadCost` method do not require any permits, allowing such downloads to bypass the semaphore mechanism.
Applied to files:
src/main/java/com/flowingcode/vaadin/addons/gridexporter/ConcurrentStreamResourceWriter.javasrc/main/java/com/flowingcode/vaadin/addons/gridexporter/ConcurrentOperationBase.javasrc/main/java/com/flowingcode/vaadin/addons/gridexporter/ConcurrentDownloadHandler.java
🧬 Code graph analysis (1)
src/main/java/com/flowingcode/vaadin/addons/gridexporter/ConcurrentOperationBase.java (2)
src/main/java/com/flowingcode/vaadin/addons/gridexporter/ConcurrentDownloadHandler.java (1)
SuppressWarnings(40-212)src/main/java/com/flowingcode/vaadin/addons/gridexporter/ConcurrentStreamResourceWriter.java (1)
SuppressWarnings(42-200)
🪛 GitHub Check: SonarCloud Code Analysis
src/main/java/com/flowingcode/vaadin/addons/gridexporter/ConcurrentOperationBase.java
[warning] 35-35: Add a private constructor to hide the implicit public one.
🔇 Additional comments (8)
src/main/java/com/flowingcode/vaadin/addons/gridexporter/ConcurrentStreamResourceWriter.java (1)
43-53: LGTM! Clean delegation to the base class.The refactoring to extend
ConcurrentOperationBaseand delegate the staticsetLimit/getLimitmethods centralizes the concurrency control logic effectively. This aligns well with the newConcurrentDownloadHandlerthat will share the same base.src/main/java/com/flowingcode/vaadin/addons/gridexporter/ConcurrentOperationBase.java (5)
35-44: Abstract class - SonarCloud warning is a false positive.Since
ConcurrentOperationBaseis declaredabstract, it cannot be instantiated directly, and subclasses will provide their own constructors. The SonarCloud suggestion to add a private constructor doesn't apply here.
113-121: Minor: Potential TOCTOU betweenenabledcheck and synchronized read.The
enabledvolatile check on line 114 and the synchronized block on lines 115-117 are not atomic. If another thread callssetLimit(POSITIVE_INFINITY)between these, the returned value may be stale. However, sincegetLimit()is a status/monitoring method and not used for critical decisions, this is acceptable.
45-74: LGTM! Well-designed configurable semaphore.The
ConfigurableSemaphorecorrectly usesreducePermitsfor decreasing andreleasefor increasing, maintaining consistency. ThetoString()provides helpful debugging output with the fixed-point representation.
123-132: LGTM! Cost-to-permits conversion correctly handles edge cases.The implementation properly handles zero/negative costs (returning 0 permits to bypass the semaphore), caps at
MAX_COST, and ensures any positive cost requires at least 1 permit. This aligns with the established behavior where zero or negative costs bypass the semaphore mechanism. Based on learnings, this is the expected behavior.
87-100: LGTM! Proper limit configuration with infinity handling.The method correctly validates input, handles
POSITIVE_INFINITYto disable the semaphore, and uses synchronization when updating permits. The guard against zero/negative values prevents misconfiguration.src/main/java/com/flowingcode/vaadin/addons/gridexporter/ConcurrentDownloadHandler.java (2)
40-54: LGTM! Clean parallel implementation to ConcurrentStreamResourceWriter.The class structure mirrors
ConcurrentStreamResourceWriterappropriately, extendingConcurrentOperationBaseand wrapping a delegateDownloadHandler. This provides consistent concurrency control for the new DownloadHandler API.
171-210: LGTM! Correct concurrency-controlled download handling.The
handleDownloadRequestimplementation correctly:
- Invokes
onAccept()before acquiring permits- Bypasses semaphore when
enabledis false- Computes permits from cost within synchronized block
- Checks for UI detachment after permit acquisition (when
failOnUiChangeis set)- Guarantees
onFinish()via finally block- Properly handles
InterruptedExceptionby preserving interrupt statusThe implementation is consistent with
ConcurrentStreamResourceWriter.accept(), ensuring uniform behavior across both APIs.
Extract common semaphore logic into ConcurrentOperationBase for reuse in concurrent handlers.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/main/java/com/flowingcode/vaadin/addons/gridexporter/ConcurrentDownloadHandler.java (1)
22-27: Remove unused import.The
VaadinSessionimport on line 24 is not used in this file. The session is obtained fromevent.getSession()directly.import com.vaadin.flow.server.streams.DownloadHandler; import com.vaadin.flow.server.streams.DownloadEvent; -import com.vaadin.flow.server.VaadinSession; import java.io.IOException; import java.io.InterruptedIOException; import java.nio.channels.InterruptedByTimeoutException;src/main/java/com/flowingcode/vaadin/addons/gridexporter/ConcurrentOperationBase.java (1)
44-50: Consider documenting the volatile fields.The
enabledandfailOnUiChangefields arevolatile, which provides visibility guarantees but not atomicity for compound operations. The current usage appears safe sinceenabledis only set within synchronized blocks or checked before synchronized access. However, adding brief comments explaining the concurrency semantics would improve maintainability.src/main/java/com/flowingcode/vaadin/addons/gridexporter/GridExporter.java (1)
1042-1079: Consider addressing the wildcard type if feasible.The
GridExporter<?>return type fromgetExporter()(line 1043) triggers a static analysis warning. While this is a private interface and the impact is limited, the wildcard is used because inner classes reference the enclosingGridExporter<T>which could potentially be typed more specifically.Since this is internal to the class and the wildcard doesn't escape to public API, this is a minor concern that can be deferred.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/main/java/com/flowingcode/vaadin/addons/gridexporter/ConcurrentDownloadHandler.java(1 hunks)src/main/java/com/flowingcode/vaadin/addons/gridexporter/ConcurrentOperationBase.java(1 hunks)src/main/java/com/flowingcode/vaadin/addons/gridexporter/ConcurrentStreamResourceWriter.java(2 hunks)src/main/java/com/flowingcode/vaadin/addons/gridexporter/GridExporter.java(5 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: javier-godoy
Repo: FlowingCode/GridExporterAddon PR: 119
File: src/main/java/com/flowingcode/vaadin/addons/gridexporter/CsvStreamResourceWriter.java:77-93
Timestamp: 2024-10-08T21:28:24.972Z
Learning: User javier-godoy requested to open a follow-up GitHub issue to discuss resource management in the refactored code of CsvStreamResourceWriter, highlighting the absence of try-with-resources in the previous implementation.
Learnt from: javier-godoy
Repo: FlowingCode/GridExporterAddon PR: 119
File: src/main/java/com/flowingcode/vaadin/addons/gridexporter/CsvStreamResourceWriter.java:77-93
Timestamp: 2024-07-27T22:14:37.039Z
Learning: User javier-godoy requested to open a follow-up GitHub issue to discuss resource management in the refactored code of CsvStreamResourceWriter, highlighting the absence of try-with-resources in the previous implementation.
Learnt from: javier-godoy
Repo: FlowingCode/GridExporterAddon PR: 143
File: src/main/java/com/flowingcode/vaadin/addons/gridexporter/GridExporter.java:315-315
Timestamp: 2024-09-26T18:26:03.345Z
Learning: In the `GridExporter` class, CSV exports are not subject to concurrent controls, so `getCsvStreamResource` returns a `StreamResource` instead of a `GridExporterStreamResource`.
Learnt from: javier-godoy
Repo: FlowingCode/GridExporterAddon PR: 143
File: src/main/java/com/flowingcode/vaadin/addons/gridexporter/GridExporter.java:315-315
Timestamp: 2024-10-08T21:28:24.972Z
Learning: In the `GridExporter` class, CSV exports are not subject to concurrent controls, so `getCsvStreamResource` returns a `StreamResource` instead of a `GridExporterStreamResource`.
📚 Learning: 2024-09-26T18:26:03.345Z
Learnt from: javier-godoy
Repo: FlowingCode/GridExporterAddon PR: 143
File: src/main/java/com/flowingcode/vaadin/addons/gridexporter/GridExporter.java:315-315
Timestamp: 2024-09-26T18:26:03.345Z
Learning: In the `GridExporter` class, CSV exports are not subject to concurrent controls, so `getCsvStreamResource` returns a `StreamResource` instead of a `GridExporterStreamResource`.
Applied to files:
src/main/java/com/flowingcode/vaadin/addons/gridexporter/ConcurrentStreamResourceWriter.javasrc/main/java/com/flowingcode/vaadin/addons/gridexporter/ConcurrentOperationBase.javasrc/main/java/com/flowingcode/vaadin/addons/gridexporter/GridExporter.javasrc/main/java/com/flowingcode/vaadin/addons/gridexporter/ConcurrentDownloadHandler.java
📚 Learning: 2024-10-28T17:35:31.077Z
Learnt from: javier-godoy
Repo: FlowingCode/GridExporterAddon PR: 154
File: src/main/java/com/flowingcode/vaadin/addons/gridexporter/BaseStreamResourceWriter.java:98-103
Timestamp: 2024-10-28T17:35:31.077Z
Learning: `BaseStreamResourceWriter` and its extending classes are not public API currently, but the return type must be refactored as we are going to expose `BaseStreamResourceWriter` in the near future.
Applied to files:
src/main/java/com/flowingcode/vaadin/addons/gridexporter/ConcurrentStreamResourceWriter.java
📚 Learning: 2024-07-27T22:14:37.039Z
Learnt from: javier-godoy
Repo: FlowingCode/GridExporterAddon PR: 119
File: src/main/java/com/flowingcode/vaadin/addons/gridexporter/CsvStreamResourceWriter.java:77-93
Timestamp: 2024-07-27T22:14:37.039Z
Learning: User javier-godoy requested to open a follow-up GitHub issue to discuss resource management in the refactored code of CsvStreamResourceWriter, highlighting the absence of try-with-resources in the previous implementation.
Applied to files:
src/main/java/com/flowingcode/vaadin/addons/gridexporter/ConcurrentStreamResourceWriter.java
📚 Learning: 2024-10-08T21:28:24.972Z
Learnt from: javier-godoy
Repo: FlowingCode/GridExporterAddon PR: 125
File: src/main/java/com/flowingcode/vaadin/addons/gridexporter/GridExporter.java:338-402
Timestamp: 2024-10-08T21:28:24.972Z
Learning: Zero or negative costs in the `setConcurrentDownloadCost` method do not require any permits, allowing such downloads to bypass the semaphore mechanism.
Applied to files:
src/main/java/com/flowingcode/vaadin/addons/gridexporter/ConcurrentStreamResourceWriter.javasrc/main/java/com/flowingcode/vaadin/addons/gridexporter/ConcurrentOperationBase.javasrc/main/java/com/flowingcode/vaadin/addons/gridexporter/ConcurrentDownloadHandler.java
🧬 Code graph analysis (1)
src/main/java/com/flowingcode/vaadin/addons/gridexporter/GridExporter.java (1)
src/main/java/com/flowingcode/vaadin/addons/gridexporter/GridExporterConcurrentSettings.java (1)
GridExporterConcurrentSettings(37-123)
🪛 GitHub Check: SonarCloud Code Analysis
src/main/java/com/flowingcode/vaadin/addons/gridexporter/GridExporter.java
[warning] 319-319: Do not forget to remove this deprecated code someday.
[warning] 375-375: Do not forget to remove this deprecated code someday.
[warning] 402-402: Do not forget to remove this deprecated code someday.
[warning] 361-361: Do not forget to remove this deprecated code someday.
[warning] 388-388: Do not forget to remove this deprecated code someday.
[warning] 347-347: Do not forget to remove this deprecated code someday.
[warning] 333-333: Do not forget to remove this deprecated code someday.
[failure] 1043-1043: Remove usage of generic wildcard type.
src/main/java/com/flowingcode/vaadin/addons/gridexporter/ConcurrentDownloadHandler.java
[warning] 24-24: Remove this unused import 'com.vaadin.flow.server.VaadinSession'.
🔇 Additional comments (11)
src/main/java/com/flowingcode/vaadin/addons/gridexporter/ConcurrentDownloadHandler.java (1)
38-77: LGTM!The class structure and delegation pattern are well-designed. The
handleDownloadRequestmethod correctly wraps the delegate's handling within the semaphore-controlledrunWithSemaphoremethod, ensuring consistent concurrency behavior with the existingConcurrentStreamResourceWriter.src/main/java/com/flowingcode/vaadin/addons/gridexporter/ConcurrentStreamResourceWriter.java (1)
38-89: LGTM!The refactoring to extend
ConcurrentOperationBaseis well-executed. The class now properly delegates semaphore logic to the base class while maintaining its public API contract forsetLimit/getLimit. Theacceptmethod correctly usesrunWithSemaphorefor concurrency control.src/main/java/com/flowingcode/vaadin/addons/gridexporter/ConcurrentOperationBase.java (4)
52-81: LGTM!The
ConfigurableSemaphoreimplementation is well-designed. ThesetPermitsmethod correctly handles both increasing and decreasing permits atomically through synchronization, and thetoStringmethod provides useful debugging output.
94-128: LGTM!The
setLimitandgetLimitmethods properly handle edge cases including infinite limits and provide thread-safe access to the semaphore configuration.
130-139: LGTM!The
costToPermitsmethod correctly implements the fixed-point arithmetic for permit calculation. The behavior of returning 0 permits for zero or negative costs (allowing bypass of the semaphore) is consistent with the documented design. Based on learnings, this is the intended behavior.
220-257: LGTM!The
runWithSemaphoremethod has sound concurrency logic:
onAccept()is correctly called before any semaphore interaction (appropriate for UI state management)- Permits are properly released in the inner
finallyblock only when acquiredInterruptedExceptioncorrectly restores the thread's interrupt status- The outer
finallyensuresonFinish()is always calledThe UI attachment check after semaphore acquisition provides a safeguard against processing requests for detached UIs when
failOnUiChangeis enabled.src/main/java/com/flowingcode/vaadin/addons/gridexporter/GridExporter.java (5)
45-46: LGTM!The new
DownloadHandlerimport is correctly added to support the new API.
310-405: LGTM!The deprecation of StreamResource-based methods is properly implemented with:
@Deprecated(since = "2.6.0", forRemoval = true)annotations- Javadoc pointing to the new
DownloadHandleralternatives- Clear indication of removal timeline (version 3.0.0)
This provides a smooth migration path as stated in the PR objectives. The SonarCloud warnings are expected reminders for future cleanup.
406-489: LGTM!The new
DownloadHandlermethods are well-implemented:
- DOCX, PDF, and Excel exports correctly use
makeConcurrentDownloadHandlerfor concurrency control- CSV export correctly uses
StreamResourceWriterAdapterdirectly without concurrency wrapper, consistent with the existing behavior where CSV exports are not subject to concurrent controls (based on learnings)- MIME types are correctly specified for each format
496-500: LGTM!The
makeConcurrentDownloadHandlerhelper properly composes the writer into a concurrent-aware download handler by first adapting it toDownloadHandlerand then wrapping with concurrency control.
226-232: LGTM!Minor formatting adjustment with no semantic changes.
| @@ -816,4 +1039,43 @@ public void setCsvCharset(SerializableSupplier<Charset> charset) { | |||
| csvCharset = charset; | |||
| } | |||
|
|
|||
| private interface GridExporterConcurrentStrategy { | |||
| GridExporter<?> getExporter(); | |||
|
|
|||
| Component getButton(); | |||
|
|
|||
| default float getCost(VaadinSession session) { | |||
There was a problem hiding this comment.
The forComponent method is not accessible from the public API.
The getExcelDownloadHandler() (and similar methods) return DownloadHandler, but forComponent(Component) is defined on the private GridExporterConcurrentDownloadHandler class. This means users cannot call forComponent to associate a button with the download handler, unlike the existing GridExporterStreamResource.forComponent() pattern.
This creates an API asymmetry between the deprecated and new methods. Consider either:
- Making the return type of
getExcelDownloadHandler()etc. return a public type withforComponent, or - Creating a public interface/class that exposes
forComponentwhile implementingDownloadHandler
For comparison, the deprecated API allows:
exporter.getExcelStreamResource().forComponent(button)But the new API doesn't support:
exporter.getExcelDownloadHandler().forComponent(button) // forComponent not visible🤖 Prompt for AI Agents
In src/main/java/com/flowingcode/vaadin/addons/gridexporter/GridExporter.java
around lines 566 to 633, the nested class GridExporterConcurrentDownloadHandler
defines forComponent(Component) but is private so callers receiving a
DownloadHandler can't access it; change the API so callers can chain
forComponent like the deprecated StreamResource API. Fix by extracting a public
interface (e.g. PublicConcurrentDownloadHandler) that extends DownloadHandler
and declares forComponent(Component) (and any needed
GridExporterConcurrentStrategy methods), have
GridExporterConcurrentDownloadHandler implement that interface, and update the
public factory/getter methods (getExcelDownloadHandler() etc.) to return the new
public interface type instead of DownloadHandler; alternatively, make
GridExporterConcurrentDownloadHandler a public/static top-level class with the
forComponent method exposed and update the return types accordingly. Ensure
imports and method signatures are updated so existing callers can call
.forComponent(button).
This PR introduces support for the new DownloadHandler API while maintaining full backward compatibility with existing code. The deprecated StreamResource API methods are preserved but marked for removal in version 3.0.0, allowing users to migrate at their own pace. All export formats (Excel, DOCX, PDF, CSV) now have corresponding DownloadHandler-based methods alongside the existing StreamResource methods.
Closes #179
Summary by CodeRabbit
New Features
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.