Issue 1297 fix: don't create additional item handle on item import#1298
Issue 1297 fix: don't create additional item handle on item import#1298kuchtiak-ufal wants to merge 5 commits intoclarin-v7from
Conversation
| * @return handle file contents or null if doesn't exist | ||
| */ | ||
| protected String processHandleFile(Context c, Item i, String path, String filename) { | ||
| protected String processHandleFile(String path, String filename) { |
There was a problem hiding this comment.
These two parameters are not used.
| public WorkspaceItem create(Context context, Collection collection, boolean template) | ||
| throws AuthorizeException, SQLException; | ||
|
|
||
| default WorkspaceItem create(Context context, Collection collection, UUID uuid, boolean template) |
There was a problem hiding this comment.
Why the default method? Is there more than one implementation of the WorkspaceItemService interface?
If there are other implementations they already have a public WorkspaceItem create(Context context, Collection collection, UUID uuid, boolean template), otherwise they wouldn't be an implementaion of the old interface, no?
There was a problem hiding this comment.
This method is used in other classes.
Because I've added new method, this "default" method keeps the binary compatibility for code written against old version of this interface.
There was a problem hiding this comment.
I know what you mean.
The default method should be the new one, rather than the old one.
But, I asked AI, and the response was this:
Question:
I have Java interface with one method. I also have one implementation class. Can I add additional method to interface and implement it in implementation class, and transform the original interface method to default ?
Response:
✅ Yes, you CAN do this: Add new method + make original method default
Here is the pattern:
✔ Before
public interface MyService {
void process();
}
public class MyServiceImpl implements MyService {
@Override
public void process() {
// existing code
}
}
✔ After (Backward-Compatible)
Interface
public interface MyService {
// Original method becomes default
default void process() {
// The default implementation calls the new method
process("default");
}
// New method with the new parameter
void process(String param);
}
Implementation class
public class MyServiceImpl implements MyService {
@Override
public void process(String param) {
System.out.println("Processing with param = " + param);
}
}
✔ Why this works
- Existing callers do not need to change
Calls like:
myService.process();
still work — the interface now provides a default implementation.
-
New callers can use the new method
myService.process("custom parameter"); -
The implementation class stays clean
You only implement the new method.
- This is a standard Java extension technique
Java interfaces evolve like this all the time (e.g., collections API).
✔ When this is a good solution
You control the interface
You need backward compatibility
You want future callers to use the new parameter
You’re using Java 8+
⚠ Important Notes
This only works if all existing implementations can accept a reasonable default behavior.
If your interface has multiple implementations, all of them will inherit the default method unless they override it.
👍 Summary
Yes, you can:
✔ Add a new method with parameters
✔ Convert the old method to default
✔ Call the new method from the default implementation
This is a clean, safe, industry-standard solution for evolving Java interfaces.
There was a problem hiding this comment.
Finally change the default method to abstract, and do the tricks in implementation class:
- implemented the new method
- implement also the old method by delegating to new method
There was a problem hiding this comment.
Pull request overview
This PR fixes issue #1297 by preventing the creation of duplicate handles during item import. When a handle is provided in a handle file during SAF (Simple Archive Format) import, the system now reads this handle before creating the workspace item and skips automatic handle registration if one is found.
Key changes:
- Modified the import workflow to read handle files before workspace item creation rather than after
- Added a new parameter to control identifier registration during workspace item creation
- Added comprehensive test coverage for importing items with existing handles
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
dspace-api/src/main/java/org/dspace/app/itemimport/ItemImportServiceImpl.java |
Core fix: reads handle file early and passes flag to skip identifier registration if handle exists; refactored to remove duplicate handle processing code |
dspace-api/src/main/java/org/dspace/content/service/WorkspaceItemService.java |
Added new parameter registerIdentifiers to control identifier registration; includes default method for backward compatibility |
dspace-api/src/main/java/org/dspace/content/WorkspaceItemServiceImpl.java |
Implemented the conditional identifier registration based on the new parameter |
dspace-api/src/main/java/org/dspace/content/InstallItemServiceImpl.java |
Minor optimization: moved filter initialization inside conditional block |
dspace-api/src/test/java/org/dspace/app/itemimport/ItemImportCLIIT.java |
Added test for importing with existing handle; refactored to use helper method for finding items |
dspace-api/src/test/resources/org/dspace/app/itemimport/handle |
Test resource file containing sample handle for testing |
dspace-api/src/test/java/org/dspace/curate/ItemHandleCheckerIT.java |
Updated test handle URL (appears unrelated to main fix) |
dspace-api/src/test/data/dspaceFolder/config/local.cfg |
Added user registration configuration (appears unrelated to main fix) |
dspace-api/src/test/java/org/dspace/curate/ItemHandleCheckerIT.java
Outdated
Show resolved
Hide resolved
dspace-api/src/main/java/org/dspace/content/service/WorkspaceItemService.java
Outdated
Show resolved
Hide resolved
dspace-api/src/test/java/org/dspace/app/itemimport/ItemImportCLIIT.java
Outdated
Show resolved
Hide resolved
| try { | ||
| if (suppliedHandle == null) { | ||
| // Get map of filters to use for identifier types. | ||
| Map<Class<? extends Identifier>, Filter> filters = FilterUtils.getIdentifierFilters(false); |
There was a problem hiding this comment.
This change is slightly unrelated, but improves the performance: filters are only obtained when needed (when suppliedHandle is null)
|
looks good. Won't merge (will wait for dataquest-dev#1207), but will deploy an image (https://github.com/ufal/clarin-dspace/actions/runs/20138463466) based on this to finish the imports |
This is the fix for #1297
Solution is to get handle first from handle file, and if the handle exists do not register additional one for workspace item.