solr reindexing through events #873
Conversation
|
@milanmajchrak |
dspace-api/src/main/java/org/dspace/authorize/ResourcePolicyServiceImpl.java
Show resolved
Hide resolved
dspace-api/src/main/java/org/dspace/authorize/ResourcePolicyServiceImpl.java
Outdated
Show resolved
Hide resolved
dspace-oai/src/main/java/org/dspace/event/OAIIndexEventConsumer.java
Outdated
Show resolved
Hide resolved
dspace-oai/src/main/java/org/dspace/event/OAIIndexEventConsumer.java
Outdated
Show resolved
Hide resolved
dspace-oai/src/main/java/org/dspace/event/OAIIndexEventConsumer.java
Outdated
Show resolved
Hide resolved
dspace-api/src/main/java/org/dspace/authorize/ResourcePolicyServiceImpl.java
Outdated
Show resolved
Hide resolved
|
@milanmajchrak I added test to test scenatious. |
dspace-api/src/main/java/org/dspace/authorize/ResourcePolicyServiceImpl.java
Outdated
Show resolved
Hide resolved
dspace-api/src/main/java/org/dspace/authorize/ResourcePolicyServiceImpl.java
Outdated
Show resolved
Hide resolved
dspace-api/src/main/java/org/dspace/authorize/ResourcePolicyServiceImpl.java
Show resolved
Hide resolved
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughThis pull request refactors and standardizes the handling of resource policies, event logging, and authorization across the codebase. Major changes include consolidating event logging via a new method in the resource policy service, updating method signatures to pass user and group parameters directly, and standardizing null checks. The dependency on SolrOAIReindexer has been removed from several repository classes, and configuration files have been updated to support new event consumers and dependency exclusions. Additional improvements enhance error handling in Sword v2 integration tests and adjust submission form metadata fields. Changes
Sequence Diagram(s)sequenceDiagram
participant Service as ResourcePolicyServiceImpl
participant Context as DSpace Context
participant DSO as DSpaceObject (Item)
participant Event as Event Logger
Service->>+Service: addEventModify(context, dso)
alt dso is an Item
Service->>Event: Create MODIFY event
Service->>Context: Register event in context
else
Service->>Context: Skip event creation
end
sequenceDiagram
participant Client
participant DepositManager as CollectionDepositManagerDSpace
participant Logger
Client->>DepositManager: createNew(depositData)
alt SwordAuthException occurs
DepositManager->>Logger: Log SwordAuthException
DepositManager-->>Client: Throw SwordAuthException
else
DepositManager->>Client: Process deposit normally
end
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
dspace-oai/src/main/java/org/dspace/event/OAIIndexEventConsumer.java (1)
157-169:⚠️ Potential issueRemove duplicated code block to avoid compile or runtime issues.
These lines appear to repeat the exception-handling and finalization logic already included above (lines 141-149). Keeping both copies may lead to compiler errors or unexpected behavior.
- throw e; - } finally { - if (Objects.nonNull(anonymousContext)) { - anonymousContext.complete(); - } - } - } - - public void finish(Context ctx) throws Exception { - // No-op - } -} + // Remove the duplicated lines (157-169) if they are artifacts from a merge or snippet error
🧹 Nitpick comments (14)
.github/workflows/trigger-builds.yml (1)
19-29: Enhance Shell Script Robustness in Build TriggeringThe current script block successfully authenticates with the GitHub CLI, fetches remote branches, and iterates over branches matching the
customer/pattern to trigger their builds.Points to consider:
- Error Handling: If no matching branches are found, the loop silently does nothing. It could be useful to add a check for an empty branch list or log a message indicating that no customer branches were detected.
- Quoting Variables: To prevent potential issues with branch names containing special characters or spaces, consider quoting the branch variable when used in commands.
Below is a suggested diff snippet that incorporates these improvements:
- git fetch --prune origin # Ensure remote refs are fetched - BRANCHES=$(git ls-remote --heads origin | awk -F'/' '{print $3"/"$4}' | grep '^customer/') - for branch in $(echo "$BRANCHES" | sed -e 's/[\[\]"]//g' -e 's/,/\n/g'); do - echo "Triggering build for branch: $branch" - gh workflow run build.yml --ref $branch - done + set -e # Exit on any error + git fetch --prune origin # Ensure remote refs are fetched + BRANCHES=$(git ls-remote --heads origin | awk -F'/' '{print $3"/"$4}' | grep '^customer/') + if [ -z "$BRANCHES" ]; then + echo "No customer branches found to trigger." + else + for branch in $(echo "$BRANCHES" | sed -e 's/[\[\]"]//g' -e 's/,/\n/g'); do + echo "Triggering build for branch: $branch" + gh workflow run build.yml --ref "$branch" + done + fiThese changes will make the script more robust and clearer in its intent.
dspace-api/src/main/resources/org/dspace/storage/rdbms/sqlmigration/h2/V7.6_2023.09.28__enforce_group_or_eperson_for_resourcepolicy.sql (1)
9-9: Cleaning up invalid resource policies.The SQL statement removes resource policies that don't have an associated EPerson or Group, which is a good preparatory step before enforcing the constraint. Consider adding a comment explaining the significance of this cleanup for future maintainers.
-DELETE FROM ResourcePolicy WHERE eperson_id is null and epersongroup_id is null; +-- Remove orphaned resource policies with no associated user or group +DELETE FROM ResourcePolicy WHERE eperson_id is null and epersongroup_id is null;dspace/config/clarin-dspace.cfg (1)
308-308: Uncommented metadata editing configurationThe line for allowing metadata editing has been uncommented but left empty. This appears to be preparation for future configuration.
Consider adding a comment explaining the intended use of this configuration and why it's currently empty.
dspace-oai/src/main/java/org/dspace/event/OAIIndexEventConsumer.java (3)
36-38: Consider removing or documenting this empty initialize() method.This method is currently empty and does not appear to be overridden. If it's not required by an interface contract or used by downstream subclasses, you can remove it to reduce boilerplate code.
47-100: Refactor nested ifs to improve maintainability.The large block of conditional checks in the
consumemethod makes the code difficult to read and maintain. Consider extracting smaller logic units (e.g., separate methods for handling items, bundles, and communities) or using a strategy pattern to reduce the complexity.
114-150: Consider verifying the new application context creation logic.Creating a new
AnnotationConfigApplicationContextfor indexing each time may introduce unnecessary overhead. If performance or resource usage becomes a concern, consider reusing an existing context, or deferring initialization until truly needed.dspace-oai/src/main/java/org/dspace/xoai/app/XOAI.java (3)
112-118: Consider lazy initialization for the Spring application context.Performing the
new AnnotationConfigApplicationContextin a static initializer or instance initializer might increase startup overhead. If performance or memory usage is a concern, consider lazy-loading these beans on demand.
734-744: Centralize or rename isTest() logic for clarity.Hardcoding
"jdbc:h2:mem:test"for detection can be brittle. Consider a configuration property or a dedicated environment check for test mode, especially if future test setups differ.
755-774: Include the original exception when re-throwing for better stack traces.Currently, the code discards the original exception cause when throwing a new
RuntimeException. Consider retaining it:- throw new RuntimeException("Cannot reindex the item with ID: " + item.getID() + " because: " - + e.getMessage()); + throw new RuntimeException("Cannot reindex the item with ID: " + item.getID(), e);This allows upstream handlers to accurately capture the root cause.
dspace-server-webapp/src/test/java/org/dspace/app/sword2/Swordv2IT.java (2)
93-104: Configuration overrides for SWORDv2 in the @before method.Setting the SWORDv2 properties under test conditions is helpful to align integration tests with the local environment. Consider verifying that these config overrides do not inadvertently persist in other test classes, especially if parallel test execution is enabled.
189-229: depositItemWithEmbargo - Tests item upload with embargo.This test ensures that a zipped embargoed item is accepted and returns HTTP 201 with the correct ATOM entry content type. Optionally, consider verifying the embargo policy details in the resulting item, but this is a good start.
dspace-server-webapp/src/test/java/org/dspace/app/rest/ResourcePolicyRestRepositoryIT.java (1)
1048-1106: Consider extending negative tests.
This new test method for creating a policy with a group appears correct. As a minor improvement, consider adding negative or edge-case scenarios (e.g., invalid group references).dspace-api/src/main/java/org/dspace/authorize/ResourcePolicyServiceImpl.java (2)
306-312: Consider refactoring for DRY.
Repeatedly checkingif (r.getdSpaceObject() instanceof Item)could be consolidated into a helper method (e.g.,addEventModify(context, dSpaceObject)). This reduces duplication and centralizes event logic.
374-377: Repetitive item-event check.
We could reuse the same helper functionality here to streamline event firing forItemobjects.
🛑 Comments failed to post (2)
dspace-server-webapp/pom.xml (1)
483-489: 💡 Verification agent
🧩 Analysis chain
Centralized json-smart version management.
Good approach to define a specific version of
json-smartas a direct dependency after excluding it from multiple other dependencies. This resolves version conflicts between nimbus-jose-jwt and json-path.However, it's worth verifying that 2.5.1 is the latest secure version.
🏁 Script executed:
#!/bin/bash # Check if 2.5.1 is the latest version of json-smart and if there are any security advisories # Check Maven Central for the latest version echo "Latest json-smart version in Maven Central:" curl -s "https://search.maven.org/solrsearch/select?q=g:net.minidev+AND+a:json-smart&rows=20&wt=json" | jq -r '.response.docs[0].latestVersion' # Check for CVEs related to json-smart echo -e "\nChecking for security vulnerabilities in json-smart:" curl -s "https://services.nvd.nist.gov/rest/json/cves/2.0?keywordSearch=json-smart" | jq '.vulnerabilities[] | {cve: .cve.id, description: .cve.descriptions[0].value}'Length of output: 1263
Action Required: Update json-smart Dependency Version
The current dependency version (2.5.1) in
dspace-server-webapp/pom.xmlis outdated and vulnerable. Verification indicates that:
- Maven Central now lists 2.5.2 as the latest version.
- Security advisories (e.g., CVE-2024-57699) confirm that versions up to 2.5.1 are susceptible to a DoS vulnerability.
Recommendations:
- Update the dependency version to 2.5.2.
- Verify that the new version resolves the identified security issue without breaking dependency exclusions.
Diff Snippet Update:
<!-- Specify the version of json-smart we want to use. This resolves version conflicts and addresses a known security vulnerability. --> <dependency> <groupId>net.minidev</groupId> <artifactId>json-smart</artifactId> <version>2.5.2</version> </dependency>dspace-api/src/main/java/org/dspace/content/packager/AbstractMETSIngester.java (1)
765-773:
⚠️ Potential issueImproved policy handling during bitstream crosswalk operations.
This change preserves bitstream policies during the crosswalk process by storing them before the operation and reapplying them afterward. This ensures that authorization settings aren't lost when metadata is updated through the crosswalk operation.
Great enhancement that fixes a potential issue where bitstream permissions could be lost during package ingestion. The policies are now properly preserved and restored after the crosswalk operation.
Problem description
Modify the reindexing of an item by event.
Summary by CodeRabbit
New Features
Refactoring & Improvements
UI & Configuration