UFAL/download-bitstreams-as-separated-api-calling #969
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds APP_CONFIG injection to the legacy bitstream redirect guard and makes its redirect namespace-aware; updates CLARIN files section to produce a brace-expansion multi-file curl command using a new bitstream base URL pattern. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor U as User
participant R as Router
participant G as legacyBitstreamURLRedirectGuard
participant S as Bitstream Service
participant C as APP_CONFIG
participant SH as ServerHardRedirectService
U->>R: Navigate to legacy bitstream URL
R->>G: Activate guard
G->>S: Fetch bitstream by UUID
S-->>G: RemoteData (success / error)
alt Success
G->>C: Read appConfig.ui.nameSpace
Note right of G: Sanitize nameSpace\n(nameSpace?.replace(/\/$/, '') || '')
G->>SH: getCurrentOrigin()
Note right of G: Build redirectUrl: new URL(nameSpace + `/bitstreams/${uuid}/download`, origin).href
G->>SH: redirect(redirectUrl, 301)
G-->>R: return false
else Not found / Error
G-->>R: return UrlTree -> PAGE_NOT_FOUND_PATH
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Pre-merge checks (2 passed, 1 warning)❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Poem
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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). (3)
✨ Finishing Touches
🧪 Generate unit tests
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: 1
🧹 Nitpick comments (1)
src/app/bitstream-page/legacy-bitstream-url-redirect.guard.ts (1)
51-55: Add focused tests for namespace permutations.
Please add guard tests covering ui.nameSpace of: "", "/repository", "repository", "/repository/", and an absolute-like value ("http://x") to confirm sanitization and final URL.I can draft the spec with mocks for BitstreamDataService and HardRedirectService if helpful.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/app/bitstream-page/legacy-bitstream-url-redirect.guard.ts(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/app/bitstream-page/legacy-bitstream-url-redirect.guard.ts (1)
src/config/app-config.interface.ts (2)
AppConfig(71-71)APP_CONFIG(72-72)
⏰ 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). (3)
- GitHub Check: dspace-angular / docker-build (linux/amd64, ubuntu-latest, true)
- GitHub Check: tests (18.x)
- GitHub Check: tests (20.x)
🔇 Additional comments (3)
src/app/bitstream-page/legacy-bitstream-url-redirect.guard.ts (3)
12-12: LGTM: config token import is correct.
Import path and symbols look right for AppConfig injection.
34-34: LGTM: inject(APP_CONFIG) usage in functional guard.
Consistent with existing DI pattern in this guard.
54-54: Confirm 301 usage policy.
Do we want 301 in all environments, or only in production/SSR? If not universal, gate via env flag or HardRedirectService behavior.
Paurikova2
changed the title
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a bug in legacy bitstream URL redirects where redirects were missing the configured UI namespace prefix, causing broken downloads. It also updates the curl command generation to use individual bitstream downloads instead of a bulk zip approach.
- Fixes legacy bitstream URL redirects to properly include UI namespace prefix
- Updates curl command generation to use individual file downloads instead of zip
- Adds debug logging for redirect URL generation
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/app/item-page/clarin-files-section/clarin-files-section.component.ts | Updates curl command generation to use individual bitstream downloads instead of bulk zip |
| src/app/bitstream-page/legacy-bitstream-url-redirect.guard.ts | Fixes redirect URL to include configured UI namespace prefix and adds debug logging |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
src/app/item-page/clarin-files-section/clarin-files-section.component.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/app/item-page/clarin-files-section/clarin-files-section.component.ts (2)
110-114: Brace expansion is brittle with commas/braces in filenames and shell portability.Brace lists break on commas/braces in filenames and don’t work in all shells (e.g., Windows cmd). A safer alternative is multiple URLs in one curl command.
Alternative:
const urls = this.listOfFiles.value.map((file, index) => `${baseUrl}/${(file as any).sequenceId ?? index}/${encodeURIComponent(file.name)}` ); this.command = `curl -L -OJ ${urls.join(' ')} -k`;
110-114: Avoid -k by default; make it config-driven or omit in production.-k disables TLS verification and shouldn’t be encouraged broadly.
Example:
const insecure = this.appConfig?.ui?.allowSelfSigned === true ? ' -k' : ''; this.command = `curl -L -OJ ${baseUrl}{${fileNamesFormatted}}${insecure}`;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/app/item-page/clarin-files-section/clarin-files-section.component.ts(1 hunks)
⏰ 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). (3)
- GitHub Check: dspace-angular / docker-build (linux/amd64, ubuntu-latest, true)
- GitHub Check: tests (18.x)
- GitHub Check: tests (20.x)
🔇 Additional comments (1)
src/app/item-page/clarin-files-section/clarin-files-section.component.ts (1)
110-114: Remove sequenceId refactor suggestion.MetadataBitstreamdoes not define asequenceIdproperty, so usingfile.sequenceIdwon’t compile; keeping the current index-based mapping is correct.Likely an incorrect or invalid review comment.
src/app/item-page/clarin-files-section/clarin-files-section.component.ts
Show resolved
Hide resolved
Problem description
When a bitstream is downloaded by name (e.g., https://dev-5.pc:8443/repository/bitstream/123456789/2-5947/0/test.txt), it redirects to https://dev-5.pc:8443/bitstreams/525188bc-7d2e-4ed6-a6d7-1a4b031b67b7/download, but the /repository prefix is missing in the redirected URL.
Summary by CodeRabbit
Bug Fixes
New Features