[Port to dtq-dev] Issue 1343: add PUT and DELETE endpoint methods to ClarinLicenseLabel REST repository#1325
Open
kosarko wants to merge 2 commits into
Open
[Port to dtq-dev] Issue 1343: add PUT and DELETE endpoint methods to ClarinLicenseLabel REST repository#1325kosarko wants to merge 2 commits into
kosarko wants to merge 2 commits into
Conversation
… REST repository (#1357) * Issue 1343: add PUT and DELETE endpoint methods to ClarinLicenseLabelRest repository * resolve Copilot comments * fixed PUT request in ClarinLicenseLabelRestRepository * added check for Clarin License Label -> Label string to be shorter that 5 characters * implement coorrect put method in ClarinLicenseLabelRestRepository * add constraints to license_label table: made label UNIQUE, make license_label not deletable when used in clarin licenses * fixed IT test failure * change order of deleting objects in test cleanup(): delete license objects before license_label objects (to satisfy license_label constraints) * resolve PR Copilot comments, fixed failing ClarinWorkspaceItemRestRepositoryIT * prevent creating duplicate Clarin License Labels in REST API * not necessary to trim label twice * minor fixes, suggested by Copilot * Rename SQL migration files to use today's date (2026.06.01) --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> (cherry picked from commit b041c90)
…abel arg
The backport added a `label` String parameter to the createClarinLicense
test helper but left six 4-arg call sites unchanged, breaking testCompile
in dspace-server-webapp. Pass a label at each remaining call site
("lbl"; "lbl1"/"lbl2" for the paired-license test) to match the helper's
new signature, preserving the previously hard-coded "lbl" behaviour.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe PR adds label-based lookup capability for ClarinLicense and ClarinLicenseLabel entities across the data access, REST, and test layers. It introduces ChangesClarinLicense findByLabel and REST Validation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
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: 5
🧹 Nitpick comments (6)
dspace-api/src/main/java/org/dspace/content/dao/impl/clarin/ClarinLicenseLabelDAOImpl.java (1)
34-35: ⚡ Quick winAdd method Javadoc on the new public override.
For overridden public methods, a brief
/** {@inheritdoc} */is sufficient to satisfy documentation requirements.As per coding guidelines, "Provide Javadoc comments for all public classes and methods".
🤖 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/main/java/org/dspace/content/dao/impl/clarin/ClarinLicenseLabelDAOImpl.java` around lines 34 - 35, Add a Javadoc comment to the public overridden method findByLabel in class ClarinLicenseLabelDAOImpl: place a brief /** {`@inheritDoc`} */ directly above the method signature of public ClarinLicenseLabel findByLabel(Context context, String label) throws SQLException to satisfy the coding guideline requiring Javadoc on public methods.Source: Coding guidelines
dspace-api/src/main/java/org/dspace/content/dao/clarin/ClarinLicenseLabelDAO.java (1)
26-26: ⚡ Quick winDocument the new public DAO contract with Javadoc.
Please add method-level Javadoc for
findByLabel(...)to match API documentation requirements.As per coding guidelines, "Provide Javadoc comments for all public classes and methods".
🤖 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/main/java/org/dspace/content/dao/clarin/ClarinLicenseLabelDAO.java` at line 26, Add Javadoc for the public DAO method findByLabel in ClarinLicenseLabelDAO: describe the purpose (find a ClarinLicenseLabel by its label), document parameters (Context context, String label), document return value (ClarinLicenseLabel or null if not found), and list thrown exceptions (throws SQLException). Place the Javadoc immediately above the method declaration in the ClarinLicenseLabelDAO interface and use standard tags (`@param`, `@return`, `@throws`) to satisfy the API documentation requirement.Source: Coding guidelines
dspace-api/src/main/java/org/dspace/content/clarin/ClarinLicenseLabelServiceImpl.java (1)
80-83: ⚡ Quick winAdd Javadoc for the new public service method.
Please add method documentation (or
{@inheritdoc}) forfindByLabel(...).As per coding guidelines, "Provide Javadoc comments for all public classes and methods".
🤖 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/main/java/org/dspace/content/clarin/ClarinLicenseLabelServiceImpl.java` around lines 80 - 83, Add Javadoc to the public service method findByLabel in ClarinLicenseLabelServiceImpl: update the method declaration for public ClarinLicenseLabel findByLabel(Context context, String label) to include a Javadoc block (or {`@inheritDoc`}) describing the purpose, parameters (context, label), return value, and SQLException thrown; reference the DAO call clarinLicenseLabelDAO.findByLabel in the comment if helpful to explain behavior.Source: Coding guidelines
dspace-api/src/main/java/org/dspace/content/dao/clarin/ClarinLicenseDAO.java (1)
31-31: ⚡ Quick winAdd Javadoc to the new public DAO method.
findByLabel(...)is public API surface and should be documented for contract clarity.Suggested update
+ /** + * Find CLARIN licenses by associated label. + * + * `@param` context current DSpace context + * `@param` label license label + * `@return` matching licenses + * `@throws` SQLException if database access fails + */ List<ClarinLicense> findByLabel(Context context, String label) throws SQLException;As per coding guidelines, "Provide Javadoc comments for all public classes and methods".
🤖 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/main/java/org/dspace/content/dao/clarin/ClarinLicenseDAO.java` at line 31, Add Javadoc for the new public DAO method findByLabel in the ClarinLicenseDAO interface: document the method's purpose (lookup licenses by label), describe each parameter (Context context, String label), state the return value (List<ClarinLicense> matching the label, possibly empty), and document the SQLException thrown; ensure the Javadoc appears above the method signature in ClarinLicenseDAO and follows project tag conventions (`@param`, `@return`, `@throws`).Source: Coding guidelines
dspace-api/src/main/java/org/dspace/content/clarin/ClarinLicenseServiceImpl.java (1)
101-104: ⚡ Quick winDocument
findByLabel(...)with Javadoc.This new public method should include method-level Javadoc (or
{@inheritdoc}) to keep public API docs complete.As per coding guidelines, "Provide Javadoc comments for all public classes and methods".
🤖 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/main/java/org/dspace/content/clarin/ClarinLicenseServiceImpl.java` around lines 101 - 104, Add method-level Javadoc to the public method findByLabel in ClarinLicenseServiceImpl: either add a descriptive Javadoc block explaining purpose, parameters (Context context, String label), return value (List<ClarinLicense>), and thrown SQLException, or use {`@inheritDoc`} if the interface already documents it; ensure the Javadoc appears immediately above the findByLabel method and references the parameters and exception so the public API documentation remains complete.Source: Coding guidelines
dspace-server-webapp/src/main/java/org/dspace/app/rest/repository/ClarinLicenseLabelRestRepository.java (1)
170-180: 💤 Low valueAdd Javadoc for the private helper method.
While less critical than public methods, documenting the validation logic in
checkLabelAndTitlewould improve maintainability.📝 Suggested Javadoc
+ /** + * Validates that the label and title are not blank and that the label length + * does not exceed MAX_LABEL_LENGTH. + * + * `@param` clarinLicenseLabelRest the REST object to validate + * `@throws` DSpaceBadRequestException if validation fails + */ private void checkLabelAndTitle(ClarinLicenseLabelRest clarinLicenseLabelRest) {🤖 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-server-webapp/src/main/java/org/dspace/app/rest/repository/ClarinLicenseLabelRestRepository.java` around lines 170 - 180, Add Javadoc to the private helper checkLabelAndTitle in ClarinLicenseLabelRestRepository describing its purpose (validate non-null/empty trimmed label and title), the behavior (throws DSpaceBadRequestException on missing/blank fields or when label exceeds MAX_LABEL_LENGTH), and note the trimming of label via Optional and the use of MAX_LABEL_LENGTH; keep the comment concise and place it immediately above the checkLabelAndTitle method.
🤖 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/content/dao/impl/clarin/ClarinLicenseDAOImpl.java`:
- Around line 62-76: The public method findByLabel in ClarinLicenseDAOImpl is
missing Javadoc; add a standard Javadoc block above the method explaining the
purpose of findByLabel(Context context, String label), describing the parameters
(context and label), the return value (List<ClarinLicense>), and the thrown
SQLException, and include any behavioural notes (e.g., matching semantics or
null/empty handling) so the documentation meets project guidelines and documents
the contract of the overridden DAO method.
In
`@dspace-api/src/main/resources/org/dspace/storage/rdbms/sqlmigration/h2/V7.6_2026.06.01__license_label_constraint.sql`:
- Around line 17-18: The migration unconditionally drops/adds the UNIQUE
constraint on license_label.label which will fail if duplicate labels exist;
before adding license_label_label_unique run a preflight duplicate check (SELECT
label, COUNT(*) ... HAVING COUNT(*) > 1) and implement a deterministic
dedup/upsert step to remove or merge duplicates (e.g. keep the earliest id or
newest timestamp) for rows with the same label, then re-run the ALTER TABLE to
add the UNIQUE constraint; reference the table name license_label and the
constraint name license_label_label_unique when adding the preflight check and
deterministic cleanup migration.
In
`@dspace-api/src/test/java/org/dspace/builder/util/AbstractBuilderCleanupUtil.java`:
- Line 20: The cleanup map in AbstractBuilderCleanupUtil now includes
ClarinLicenseBuilder but is missing ClarinLicenseLabelBuilder, causing FK
deletion order issues; update the cleanup map in the AbstractBuilderCleanupUtil
class to add ClarinLicenseLabelBuilder immediately after the existing
ClarinLicenseBuilder entry so that ClarinLicenseBuilder (licenses) are removed
before ClarinLicenseLabelBuilder (labels), ensuring deletions obey the
foreign-key constraint referenced by ClarinLicenseLabelRestRepository; locate
the map population code (the entries involving ClarinLicenseBuilder) and insert
the ClarinLicenseLabelBuilder entry right after it.
In
`@dspace-server-webapp/src/main/java/org/dspace/app/rest/repository/ClarinLicenseLabelRestRepository.java`:
- Around line 112-148: Add a Javadoc comment to the public method put in
ClarinLicenseLabelRestRepository: document the purpose of the endpoint, describe
each parameter (Context context, HttpServletRequest request, String apiCategory,
String model, Integer id, JsonNode jsonNode), explain the return value
(ClarinLicenseLabelRest via converter.toRest), and list thrown exceptions
(SQLException, AuthorizeException, DSpaceBadRequestException,
ClarinLicenseLabelNotFoundException). Keep the description concise and mention
relevant behavior such as parsing with objectMapper.treeToValue, validation via
checkLabelAndTitle, uniqueness check via clarinLicenseLabelService.findByLabel,
and that the entity is updated with updateClarinLicenseLabel and
clarinLicenseLabelService.update.
- Around line 150-168: Add Javadoc to the public method delete(Context context,
Integer id) in ClarinLicenseLabelRestRepository: document the purpose of the
method (deletes a ClarinLicenseLabel by id), describe parameters `@param` context
and `@param` id, and list thrown exceptions with `@throws` for AuthorizeException,
ClarinLicenseLabelNotFoundException, DSpaceBadRequestException and any
runtime/SQL-related exceptions that can propagate (e.g., RuntimeException
wrapping SQLException); also include a brief note about the
`@PreAuthorize`("hasAuthority('ADMIN')") security requirement. Ensure the Javadoc
sits immediately above the delete method signature.
---
Nitpick comments:
In
`@dspace-api/src/main/java/org/dspace/content/clarin/ClarinLicenseLabelServiceImpl.java`:
- Around line 80-83: Add Javadoc to the public service method findByLabel in
ClarinLicenseLabelServiceImpl: update the method declaration for public
ClarinLicenseLabel findByLabel(Context context, String label) to include a
Javadoc block (or {`@inheritDoc`}) describing the purpose, parameters (context,
label), return value, and SQLException thrown; reference the DAO call
clarinLicenseLabelDAO.findByLabel in the comment if helpful to explain behavior.
In
`@dspace-api/src/main/java/org/dspace/content/clarin/ClarinLicenseServiceImpl.java`:
- Around line 101-104: Add method-level Javadoc to the public method findByLabel
in ClarinLicenseServiceImpl: either add a descriptive Javadoc block explaining
purpose, parameters (Context context, String label), return value
(List<ClarinLicense>), and thrown SQLException, or use {`@inheritDoc`} if the
interface already documents it; ensure the Javadoc appears immediately above the
findByLabel method and references the parameters and exception so the public API
documentation remains complete.
In
`@dspace-api/src/main/java/org/dspace/content/dao/clarin/ClarinLicenseDAO.java`:
- Line 31: Add Javadoc for the new public DAO method findByLabel in the
ClarinLicenseDAO interface: document the method's purpose (lookup licenses by
label), describe each parameter (Context context, String label), state the
return value (List<ClarinLicense> matching the label, possibly empty), and
document the SQLException thrown; ensure the Javadoc appears above the method
signature in ClarinLicenseDAO and follows project tag conventions (`@param`,
`@return`, `@throws`).
In
`@dspace-api/src/main/java/org/dspace/content/dao/clarin/ClarinLicenseLabelDAO.java`:
- Line 26: Add Javadoc for the public DAO method findByLabel in
ClarinLicenseLabelDAO: describe the purpose (find a ClarinLicenseLabel by its
label), document parameters (Context context, String label), document return
value (ClarinLicenseLabel or null if not found), and list thrown exceptions
(throws SQLException). Place the Javadoc immediately above the method
declaration in the ClarinLicenseLabelDAO interface and use standard tags
(`@param`, `@return`, `@throws`) to satisfy the API documentation requirement.
In
`@dspace-api/src/main/java/org/dspace/content/dao/impl/clarin/ClarinLicenseLabelDAOImpl.java`:
- Around line 34-35: Add a Javadoc comment to the public overridden method
findByLabel in class ClarinLicenseLabelDAOImpl: place a brief /** {`@inheritDoc`}
*/ directly above the method signature of public ClarinLicenseLabel
findByLabel(Context context, String label) throws SQLException to satisfy the
coding guideline requiring Javadoc on public methods.
In
`@dspace-server-webapp/src/main/java/org/dspace/app/rest/repository/ClarinLicenseLabelRestRepository.java`:
- Around line 170-180: Add Javadoc to the private helper checkLabelAndTitle in
ClarinLicenseLabelRestRepository describing its purpose (validate non-null/empty
trimmed label and title), the behavior (throws DSpaceBadRequestException on
missing/blank fields or when label exceeds MAX_LABEL_LENGTH), and note the
trimming of label via Optional and the use of MAX_LABEL_LENGTH; keep the comment
concise and place it immediately above the checkLabelAndTitle method.
🪄 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: 22405db3-31ea-40ca-aa5e-3395ae46e0db
📒 Files selected for processing (16)
dspace-api/src/main/java/org/dspace/content/clarin/ClarinLicenseLabelServiceImpl.javadspace-api/src/main/java/org/dspace/content/clarin/ClarinLicenseServiceImpl.javadspace-api/src/main/java/org/dspace/content/dao/clarin/ClarinLicenseDAO.javadspace-api/src/main/java/org/dspace/content/dao/clarin/ClarinLicenseLabelDAO.javadspace-api/src/main/java/org/dspace/content/dao/impl/clarin/ClarinLicenseDAOImpl.javadspace-api/src/main/java/org/dspace/content/dao/impl/clarin/ClarinLicenseLabelDAOImpl.javadspace-api/src/main/java/org/dspace/content/service/clarin/ClarinLicenseLabelService.javadspace-api/src/main/java/org/dspace/content/service/clarin/ClarinLicenseService.javadspace-api/src/main/resources/org/dspace/storage/rdbms/sqlmigration/h2/V7.6_2026.06.01__license_label_constraint.sqldspace-api/src/main/resources/org/dspace/storage/rdbms/sqlmigration/postgres/V7.6_2026.06.01__license_label_constraint.sqldspace-api/src/test/java/org/dspace/builder/util/AbstractBuilderCleanupUtil.javadspace-server-webapp/src/main/java/org/dspace/app/rest/exception/ClarinLicenseLabelNotFoundException.javadspace-server-webapp/src/main/java/org/dspace/app/rest/repository/ClarinLicenseLabelRestRepository.javadspace-server-webapp/src/test/java/org/dspace/app/rest/ClarinLicenseLabelRestRepositoryIT.javadspace-server-webapp/src/test/java/org/dspace/app/rest/ClarinWorkspaceItemRestRepositoryIT.javadspace-server-webapp/src/test/java/org/dspace/app/rest/ProvenanceServiceIT.java
Comment on lines
+62
to
+76
| @Override | ||
| public List<ClarinLicense> findByLabel(Context context, String label) throws SQLException { | ||
| CriteriaBuilder criteriaBuilder = getCriteriaBuilder(context); | ||
| CriteriaQuery<ClarinLicense> criteriaQuery = getCriteriaQuery(criteriaBuilder, ClarinLicense.class); | ||
| Root<ClarinLicense> clarinLicenseRoot = criteriaQuery.from(ClarinLicense.class); | ||
|
|
||
| SetJoin<ClarinLicense, ClarinLicenseLabel> labelJoin = | ||
| clarinLicenseRoot.joinSet(ClarinLicense_.CLARIN_LICENSE_LABELS); | ||
|
|
||
| Predicate labelPredicate = criteriaBuilder.equal(labelJoin.get(ClarinLicenseLabel_.LABEL), label); | ||
|
|
||
| criteriaQuery.select(clarinLicenseRoot).where(labelPredicate); | ||
|
|
||
| return list(context, criteriaQuery, false, ClarinLicense.class, -1, -1); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add Javadoc for the public findByLabel method.
The new findByLabel method is public and overrides a DAO interface method, but lacks Javadoc documentation. As per coding guidelines, all public methods in Java should have Javadoc comments explaining parameters, return values, and exceptions.
📝 Suggested Javadoc
+ /**
+ * Find all Clarin Licenses that contain the specified license label.
+ *
+ * `@param` context DSpace context object
+ * `@param` label the license label to search for
+ * `@return` List of Clarin Licenses containing the specified label
+ * `@throws` SQLException if database error
+ */
`@Override`
public List<ClarinLicense> findByLabel(Context context, String label) throws SQLException {🤖 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/main/java/org/dspace/content/dao/impl/clarin/ClarinLicenseDAOImpl.java`
around lines 62 - 76, The public method findByLabel in ClarinLicenseDAOImpl is
missing Javadoc; add a standard Javadoc block above the method explaining the
purpose of findByLabel(Context context, String label), describing the parameters
(context and label), the return value (List<ClarinLicense>), and the thrown
SQLException, and include any behavioural notes (e.g., matching semantics or
null/empty handling) so the documentation meets project guidelines and documents
the contract of the overridden DAO method.
Source: Coding guidelines
Comment on lines
+17
to
+18
| ALTER TABLE license_label DROP CONSTRAINT IF EXISTS license_label_label_unique; | ||
| ALTER TABLE license_label ADD CONSTRAINT license_label_label_unique UNIQUE(label); |
There was a problem hiding this comment.
Guard the unique-constraint migration with duplicate-data handling.
Adding UNIQUE(label) can fail upgrades on instances that already contain duplicate license_label.label values. Add a preceding cleanup/preflight step (or deterministic dedup migration) before enforcing the constraint.
Example preflight query
SELECT label, COUNT(*)
FROM license_label
GROUP BY label
HAVING COUNT(*) > 1;🤖 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/main/resources/org/dspace/storage/rdbms/sqlmigration/h2/V7.6_2026.06.01__license_label_constraint.sql`
around lines 17 - 18, The migration unconditionally drops/adds the UNIQUE
constraint on license_label.label which will fail if duplicate labels exist;
before adding license_label_label_unique run a preflight duplicate check (SELECT
label, COUNT(*) ... HAVING COUNT(*) > 1) and implement a deterministic
dedup/upsert step to remove or merge duplicates (e.g. keep the earliest id or
newest timestamp) for rows with the same label, then re-run the ALTER TABLE to
add the UNIQUE constraint; reference the table name license_label and the
constraint name license_label_label_unique when adding the preflight check and
deterministic cleanup migration.
| import org.dspace.builder.BitstreamFormatBuilder; | ||
| import org.dspace.builder.BundleBuilder; | ||
| import org.dspace.builder.ClaimedTaskBuilder; | ||
| import org.dspace.builder.ClarinLicenseBuilder; |
There was a problem hiding this comment.
ClarinLicenseLabelBuilder is missing from the cleanup order.
ClarinLicenseBuilder has been added to the cleanup map, but ClarinLicenseLabelBuilder is absent. Based on the REST repository's delete guard (lines 159-163 in ClarinLicenseLabelRestRepository.java), licenses reference labels, creating a foreign key dependency. The cleanup order must delete licenses before labels to avoid constraint violations.
Add ClarinLicenseLabelBuilder to the map after line 89 so licenses are cleaned up first:
🔧 Suggested fix
import org.dspace.builder.ClarinLicenseBuilder;
+import org.dspace.builder.ClarinLicenseLabelBuilder;
import org.dspace.builder.CollectionBuilder; map.put(ClarinLicenseBuilder.class.getName(), new ArrayList<>());
+ map.put(ClarinLicenseLabelBuilder.class.getName(), new ArrayList<>());
}Also applies to: 89-89
🤖 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/builder/util/AbstractBuilderCleanupUtil.java`
at line 20, The cleanup map in AbstractBuilderCleanupUtil now includes
ClarinLicenseBuilder but is missing ClarinLicenseLabelBuilder, causing FK
deletion order issues; update the cleanup map in the AbstractBuilderCleanupUtil
class to add ClarinLicenseLabelBuilder immediately after the existing
ClarinLicenseBuilder entry so that ClarinLicenseBuilder (licenses) are removed
before ClarinLicenseLabelBuilder (labels), ensuring deletions obey the
foreign-key constraint referenced by ClarinLicenseLabelRestRepository; locate
the map population code (the entries involving ClarinLicenseBuilder) and insert
the ClarinLicenseLabelBuilder entry right after it.
Comment on lines
+112
to
+148
| @Override | ||
| @PreAuthorize("hasAuthority('ADMIN')") | ||
| public ClarinLicenseLabelRest put(Context context, | ||
| HttpServletRequest request, | ||
| String apiCategory, | ||
| String model, | ||
| Integer id, | ||
| JsonNode jsonNode) throws SQLException, AuthorizeException { | ||
| ClarinLicenseLabel clarinLicenseLabel = clarinLicenseLabelService.find(context, id); | ||
| if (Objects.isNull(clarinLicenseLabel)) { | ||
| throw new ClarinLicenseLabelNotFoundException("Clarin License Label with id " + id + " was not found"); | ||
| } | ||
|
|
||
| // parse request body | ||
| ClarinLicenseLabelRest clarinLicenseLabelRest; | ||
| try { | ||
| clarinLicenseLabelRest = objectMapper.treeToValue(jsonNode, ClarinLicenseLabelRest.class); | ||
| } catch (IOException excIO) { | ||
| throw new DSpaceBadRequestException("error parsing request body", excIO); | ||
| } | ||
|
|
||
| checkLabelAndTitle(clarinLicenseLabelRest); | ||
|
|
||
| ClarinLicenseLabel clarinLicenseLabelWithSameLabel = clarinLicenseLabelService.findByLabel(context, | ||
| clarinLicenseLabelRest.getLabel().trim()); | ||
| if (clarinLicenseLabelWithSameLabel != null && !clarinLicenseLabelWithSameLabel.getID().equals(id)) { | ||
| throw new DSpaceBadRequestException("Clarin License Label with label " + clarinLicenseLabelRest.getLabel() + | ||
| " already exists"); | ||
| } | ||
|
|
||
| clarinLicenseLabel.setId(id); | ||
| updateClarinLicenseLabel(clarinLicenseLabel, clarinLicenseLabelRest); | ||
|
|
||
| clarinLicenseLabelService.update(context, clarinLicenseLabel); | ||
|
|
||
| return converter.toRest(clarinLicenseLabel, utils.obtainProjection()); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add Javadoc for the public put method.
The put method is a public REST endpoint handler but lacks Javadoc documentation. As per coding guidelines, all public methods in Java should have Javadoc comments explaining parameters, return values, and exceptions.
📝 Suggested Javadoc
+ /**
+ * Update a Clarin License Label by ID.
+ *
+ * `@param` context DSpace context object
+ * `@param` request HTTP servlet request
+ * `@param` apiCategory API category
+ * `@param` model model name
+ * `@param` id ID of the license label to update (from path)
+ * `@param` jsonNode request body containing updated fields
+ * `@return` updated Clarin License Label REST object
+ * `@throws` SQLException if database error
+ * `@throws` AuthorizeException if user is not authorized
+ */
`@Override`
`@PreAuthorize`("hasAuthority('ADMIN')")
public ClarinLicenseLabelRest put(Context context,🤖 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-server-webapp/src/main/java/org/dspace/app/rest/repository/ClarinLicenseLabelRestRepository.java`
around lines 112 - 148, Add a Javadoc comment to the public method put in
ClarinLicenseLabelRestRepository: document the purpose of the endpoint, describe
each parameter (Context context, HttpServletRequest request, String apiCategory,
String model, Integer id, JsonNode jsonNode), explain the return value
(ClarinLicenseLabelRest via converter.toRest), and list thrown exceptions
(SQLException, AuthorizeException, DSpaceBadRequestException,
ClarinLicenseLabelNotFoundException). Keep the description concise and mention
relevant behavior such as parsing with objectMapper.treeToValue, validation via
checkLabelAndTitle, uniqueness check via clarinLicenseLabelService.findByLabel,
and that the entity is updated with updateClarinLicenseLabel and
clarinLicenseLabelService.update.
Source: Coding guidelines
Comment on lines
+150
to
+168
| @Override | ||
| @PreAuthorize("hasAuthority('ADMIN')") | ||
| public void delete(Context context, Integer id) throws AuthorizeException { | ||
| ClarinLicenseLabel clarinLicenseLabel; | ||
| try { | ||
| clarinLicenseLabel = clarinLicenseLabelService.find(context, id); | ||
| if (Objects.isNull(clarinLicenseLabel)) { | ||
| throw new ClarinLicenseLabelNotFoundException("Clarin License Label with id " + id + " was not found"); | ||
| } | ||
| List<ClarinLicense> licenses = clarinLicenseService.findByLabel(context, clarinLicenseLabel.getLabel()); | ||
| if (!licenses.isEmpty()) { | ||
| throw new DSpaceBadRequestException("Clarin License Label " + clarinLicenseLabel.getLabel() + | ||
| " is in use and cannot be deleted"); | ||
| } | ||
| clarinLicenseLabelService.delete(context, clarinLicenseLabel); | ||
| } catch (SQLException e) { | ||
| throw new RuntimeException(e.getMessage(), e); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add Javadoc for the public delete method.
The delete method is a public REST endpoint handler but lacks Javadoc documentation. As per coding guidelines, all public methods in Java should have Javadoc comments explaining parameters and exceptions.
📝 Suggested Javadoc
+ /**
+ * Delete a Clarin License Label by ID.
+ * Deletion is rejected if the label is in use by any Clarin License.
+ *
+ * `@param` context DSpace context object
+ * `@param` id ID of the license label to delete
+ * `@throws` AuthorizeException if user is not authorized
+ */
`@Override`
`@PreAuthorize`("hasAuthority('ADMIN')")
public void delete(Context context, Integer id) throws AuthorizeException {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| @Override | |
| @PreAuthorize("hasAuthority('ADMIN')") | |
| public void delete(Context context, Integer id) throws AuthorizeException { | |
| ClarinLicenseLabel clarinLicenseLabel; | |
| try { | |
| clarinLicenseLabel = clarinLicenseLabelService.find(context, id); | |
| if (Objects.isNull(clarinLicenseLabel)) { | |
| throw new ClarinLicenseLabelNotFoundException("Clarin License Label with id " + id + " was not found"); | |
| } | |
| List<ClarinLicense> licenses = clarinLicenseService.findByLabel(context, clarinLicenseLabel.getLabel()); | |
| if (!licenses.isEmpty()) { | |
| throw new DSpaceBadRequestException("Clarin License Label " + clarinLicenseLabel.getLabel() + | |
| " is in use and cannot be deleted"); | |
| } | |
| clarinLicenseLabelService.delete(context, clarinLicenseLabel); | |
| } catch (SQLException e) { | |
| throw new RuntimeException(e.getMessage(), e); | |
| } | |
| } | |
| /** | |
| * Delete a Clarin License Label by ID. | |
| * Deletion is rejected if the label is in use by any Clarin License. | |
| * | |
| * `@param` context DSpace context object | |
| * `@param` id ID of the license label to delete | |
| * `@throws` AuthorizeException if user is not authorized | |
| */ | |
| `@Override` | |
| `@PreAuthorize`("hasAuthority('ADMIN')") | |
| public void delete(Context context, Integer id) throws AuthorizeException { | |
| ClarinLicenseLabel clarinLicenseLabel; | |
| try { | |
| clarinLicenseLabel = clarinLicenseLabelService.find(context, id); | |
| if (Objects.isNull(clarinLicenseLabel)) { | |
| throw new ClarinLicenseLabelNotFoundException("Clarin License Label with id " + id + " was not found"); | |
| } | |
| List<ClarinLicense> licenses = clarinLicenseService.findByLabel(context, clarinLicenseLabel.getLabel()); | |
| if (!licenses.isEmpty()) { | |
| throw new DSpaceBadRequestException("Clarin License Label " + clarinLicenseLabel.getLabel() + | |
| " is in use and cannot be deleted"); | |
| } | |
| clarinLicenseLabelService.delete(context, clarinLicenseLabel); | |
| } catch (SQLException e) { | |
| throw new RuntimeException(e.getMessage(), e); | |
| } | |
| } |
🤖 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-server-webapp/src/main/java/org/dspace/app/rest/repository/ClarinLicenseLabelRestRepository.java`
around lines 150 - 168, Add Javadoc to the public method delete(Context context,
Integer id) in ClarinLicenseLabelRestRepository: document the purpose of the
method (deletes a ClarinLicenseLabel by id), describe parameters `@param` context
and `@param` id, and list thrown exceptions with `@throws` for AuthorizeException,
ClarinLicenseLabelNotFoundException, DSpaceBadRequestException and any
runtime/SQL-related exceptions that can propagate (e.g., RuntimeException
wrapping SQLException); also include a brief note about the
`@PreAuthorize`("hasAuthority('ADMIN')") security requirement. Ensure the Javadoc
sits immediately above the delete method signature.
Source: Coding guidelines
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Port of ufal#1357 by @kuchtiak-ufal to
dtq-dev.Summary by CodeRabbit
New Features
Bug Fixes