[Port to dtq-dev] Issue 1364: tgz file preview fix#1338
Conversation
* Issue 1364: tgz file preview fix * resolve MR comments + fixed test * test cleanup * Update help message for force preview option * get rid of the password requirement on FilePreview script --------- Co-authored-by: Ondřej Košarko <kosarko@ufal.mff.cuni.cz> (cherry picked from commit 00a2a37)
|
Warning Review limit reached
More reviews will be available in 10 minutes and 25 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe PR fixes Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 2
🧹 Nitpick comments (1)
dspace-api/src/test/java/org/dspace/scripts/filepreview/FilePreviewIT.java (1)
247-247: ⚡ Quick winAvoid exact info-log count assertions in helper; they’re brittle.
hasSize(previewGenerationExpected ? 3 : 2)tightly couples tests to incidental logging and causes noisy failures (same pattern also appears intestPreviewWithForce, Line 209). Prefer asserting required messages only.Suggested refactor
- assertThat(messages, hasSize(previewGenerationExpected ? 3 : 2)); assertThat(messages, hasItem(containsString("Generate the file previews for the specified item with " + "the given UUID: " + item.getID()))); assertThat(messages, hasItem(containsString("Authentication by user: " + user.getEmail())));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@dspace-api/src/test/java/org/dspace/scripts/filepreview/FilePreviewIT.java` at line 247, The assertion using hasSize(previewGenerationExpected ? 3 : 2) creates brittle tests that are tightly coupled to incidental logging details and cause noisy failures when logging behavior changes. This pattern appears in both the assertion at line 247 and in the testPreviewWithForce method at line 209. Instead of asserting exact message counts, refactor both locations to assert only the presence of required/essential log messages using containsInAnyOrder or hasItems, which will make tests resilient to incidental logging changes and focus on behavior that actually matters.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@dspace-api/src/main/java/org/dspace/scripts/filepreview/FilePreview.java`:
- Around line 206-209: The getEperson() method does not validate that
ePersonService.find() returned a non-null result, allowing null to be returned
and subsequently dereferenced at the call site, causing an NPE. After the
ePersonService.find(context, getEpersonIdentifier()) call in getEperson(), add a
null check and throw an AuthenticationException with a clear error message if
the result is null. This ensures fail-fast behavior and prevents null pointer
exceptions in downstream code that depends on getEperson() returning a valid
EPerson instance.
- Around line 89-90: The validation check in the condition at lines 89-90 only
validates that epersonMail is not null, but does not reject blank or
whitespace-only strings. Replace the null check with a blank-safe validation
method (such as StringUtils.isBlank()) to ensure that empty strings are also
rejected at parse-time when no eperson identifier is provided. This will enforce
a strict CLI contract and provide immediate error messaging instead of allowing
blank input to proceed and fail later.
---
Nitpick comments:
In `@dspace-api/src/test/java/org/dspace/scripts/filepreview/FilePreviewIT.java`:
- Line 247: The assertion using hasSize(previewGenerationExpected ? 3 : 2)
creates brittle tests that are tightly coupled to incidental logging details and
cause noisy failures when logging behavior changes. This pattern appears in both
the assertion at line 247 and in the testPreviewWithForce method at line 209.
Instead of asserting exact message counts, refactor both locations to assert
only the presence of required/essential log messages using containsInAnyOrder or
hasItems, which will make tests resilient to incidental logging changes and
focus on behavior that actually matters.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 69f90be9-f6ca-4374-84b5-013336e05bd7
📒 Files selected for processing (5)
dspace-api/src/main/java/org/dspace/content/PreviewContentServiceImpl.javadspace-api/src/main/java/org/dspace/scripts/filepreview/FilePreview.javadspace-api/src/main/java/org/dspace/scripts/filepreview/FilePreviewConfiguration.javadspace-api/src/test/java/org/dspace/scripts/filepreview/FilePreviewIT.javadspace-server-webapp/src/test/java/org/dspace/app/rest/PreviewContentServiceImplIT.java
There was a problem hiding this comment.
Pull request overview
Ports Issue 1364 fix to dtq-dev by improving .tgz preview detection, adding a --force option to regenerate previews, and adjusting the file-preview script to run as an EPerson resolved by email (without password verification).
Changes:
- Add
.tgzhandling to gzip/tar preview extraction logic. - Add
-f/--forceto delete existing preview content and regenerate it. - Update CLI script + integration tests to use email-only EPerson resolution (remove password option/usage).
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| dspace-server-webapp/src/test/java/org/dspace/app/rest/PreviewContentServiceImplIT.java | Adds a new integration test bitstream for .tgz with gzip MIME type and exercises preview generation. |
| dspace-api/src/test/java/org/dspace/scripts/filepreview/FilePreviewIT.java | Updates script ITs for email-only invocation and adds --force test coverage. |
| dspace-api/src/main/java/org/dspace/scripts/filepreview/FilePreviewConfiguration.java | Adds --force option and removes password option from CLI configuration. |
| dspace-api/src/main/java/org/dspace/scripts/filepreview/FilePreview.java | Implements --force regeneration behavior and switches authentication resolution to EPerson lookup by email/identifier. |
| dspace-api/src/main/java/org/dspace/content/PreviewContentServiceImpl.java | Treats .tgz filenames as tar+gzip when processing gzip content. |
Port of ufal#1372 by @kuchtiak-ufal to
dtq-dev.Summary by CodeRabbit
New Features
--forceflag to regenerate file previews even when previews already exist.tgzcompressed archive files in preview generationTests
.tgzfile preview handling