feat: implement link update and delete functionality with correspondi…#456
feat: implement link update and delete functionality with correspondi…#456
Conversation
Summary of ChangesHello @robfrank, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request delivers full support for managing user links, allowing them to be updated and deleted. The changes span the entire application stack, from new backend services that enforce ownership and handle persistence, to new API endpoints, and a user-friendly frontend interface with interactive dialogs for these operations. This enhancement significantly improves the user's ability to control their saved links. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
🧪 CI InsightsHere's what we observed from your CI run for 36ff212. 🟢 All jobs passed!But CI Insights is watching 👀 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request successfully implements the update and delete functionality for links, including both backend services and frontend UI components. The changes are well-structured and cover the necessary use cases. I've identified a critical issue in the web layer's permission handling, a high-severity bug in the persistence layer that could lead to data loss, and a few medium-severity issues related to API consistency and frontend performance. Addressing these points will significantly improve the robustness and efficiency of the new features.
| public WebBuilder withLinkManagementController(LinkController linkController) { | ||
| app.before("/api/v1/links/{id}", requireAuthentication); | ||
|
|
||
| // Update link | ||
| app.before("/api/v1/links/{id}", RequirePermission.any(authorizationService, Role.Permissions.UPDATE_OWN_LINKS)); | ||
| app.patch("/api/v1/links/{id}", linkController::updateLink); | ||
|
|
||
| // Delete link | ||
| app.before("/api/v1/links/{id}", RequirePermission.any(authorizationService, Role.Permissions.DELETE_OWN_LINKS)); | ||
| app.delete("/api/v1/links/{id}", linkController::deleteLink); | ||
|
|
||
| return this; | ||
| } |
There was a problem hiding this comment.
The permission checks for updating and deleting links are incorrectly configured. Javalin's app.before(path, handler) applies the handler to all HTTP methods for that path. As a result, a PATCH request to /api/v1/links/{id} will trigger both the UPDATE_OWN_LINKS and DELETE_OWN_LINKS permission checks. This means a user who can update but not delete a link will have their PATCH request denied. The same issue exists for DELETE requests.
The permission checks should be specific to the HTTP method.
public WebBuilder withLinkManagementController(LinkController linkController) {
app.before("/api/v1/links/{id}", requireAuthentication);
app.before("/api/v1/links/{id}", ctx -> {
switch (ctx.method()) {
case PATCH -> RequirePermission.any(authorizationService, Role.Permissions.UPDATE_OWN_LINKS).handle(ctx);
case DELETE -> RequirePermission.any(authorizationService, Role.Permissions.DELETE_OWN_LINKS).handle(ctx);
default -> {
// No-op for other methods
}
}
});
app.patch("/api/v1/links/{id}", linkController::updateLink);
app.delete("/api/v1/links/{id}", linkController::deleteLink);
return this;
}| public Link updateLink(Link link) { | ||
| try { | ||
| database.transaction(() -> { | ||
| database.command( | ||
| "sql", | ||
| """ | ||
| UPDATE Link SET | ||
| url = ?, | ||
| title = ?, | ||
| description = ?, | ||
| extractedAt = ?, | ||
| contentType = ?, | ||
| fullText = ?, | ||
| summary = ?, | ||
| imageUrl = ? | ||
| WHERE id = ? | ||
| """, | ||
| link.url(), | ||
| link.title(), | ||
| link.description(), | ||
| link.extractedAt().truncatedTo(ChronoUnit.SECONDS).format(DateTimeFormatter.ofPattern("yyyy-MM-dd HH:mm:ss")), | ||
| link.contentType(), | ||
| null, // fullText not in Link record yet? Wait, Link record doesn't have | ||
| // fullText/summary/imageUrl. | ||
| null, // But saveLink uses them? | ||
| null, // Let's check saveLink again. | ||
| link.id() | ||
| ); | ||
| }); | ||
| return link; | ||
| } catch (ArcadeDBException e) { | ||
| throw new DatabaseException("Failed to update link: " + link.id(), e); | ||
| } | ||
| } |
There was a problem hiding this comment.
The updateLink method has a few issues:
- It updates fields like
url,extractedAt, andcontentType. Based on theUpdateLinkService, onlytitleanddescriptionshould be mutable. Updating a link's URL is particularly risky. - It explicitly sets
fullText,summary, andimageUrltonull, which will erase any existing data in these columns. The comments in the code suggest there's some confusion about theLinkrecord and the database schema.
A better approach is to only update the fields that are meant to be changed.
public Link updateLink(Link link) {
try {
database.transaction(() -> {
database.command(
"sql",
"""
UPDATE Link SET
title = ?,
description = ?
WHERE id = ?
""",
link.title(),
link.description(),
link.id()
);
});
return link;
} catch (ArcadeDBException e) {
throw new DatabaseException("Failed to update link: " + link.id(), e);
}
}
src/main/java/it/robfrank/linklift/adapter/in/web/LinkController.java
Outdated
Show resolved
Hide resolved
| const handleUpdateLink = async () => { | ||
| if (!linkToEdit) return; | ||
| try { | ||
| await api.updateLink(linkToEdit.id, { title: editTitle, description: editDescription }); | ||
| setEditLinkDialogOpen(false); | ||
| setLinkToEdit(null); | ||
| fetchLinks(); | ||
| } catch (err) { | ||
| console.error("Error updating link:", err); | ||
| setError("Failed to update link."); | ||
| } | ||
| }; |
There was a problem hiding this comment.
After successfully updating a link, you are re-fetching the entire list of links with fetchLinks(). This is inefficient and can be slow if the list is large. The api.updateLink method returns the updated link object. You can use this object to update the link in the local state directly, providing a faster and smoother user experience.
| const handleUpdateLink = async () => { | |
| if (!linkToEdit) return; | |
| try { | |
| await api.updateLink(linkToEdit.id, { title: editTitle, description: editDescription }); | |
| setEditLinkDialogOpen(false); | |
| setLinkToEdit(null); | |
| fetchLinks(); | |
| } catch (err) { | |
| console.error("Error updating link:", err); | |
| setError("Failed to update link."); | |
| } | |
| }; | |
| const handleUpdateLink = async () => { | |
| if (!linkToEdit) return; | |
| try { | |
| const updatedLink = await api.updateLink(linkToEdit.id, { title: editTitle, description: editDescription }); | |
| setLinks((prevLinks) => prevLinks.map((link) => (link.id === updatedLink.id ? updatedLink : link))); | |
| setEditLinkDialogOpen(false); | |
| setLinkToEdit(null); | |
| } catch (err) { | |
| console.error("Error updating link:", err); | |
| setError("Failed to update link."); | |
| } | |
| }; |
| const handleConfirmDelete = async () => { | ||
| if (!linkToDelete) return; | ||
| try { | ||
| await api.deleteLink(linkToDelete.id); | ||
| setDeleteLinkDialogOpen(false); | ||
| setLinkToDelete(null); | ||
| fetchLinks(); | ||
| } catch (err) { | ||
| console.error("Error deleting link:", err); | ||
| setError("Failed to delete link."); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Similar to the update operation, after deleting a link, you re-fetch the entire list. This can be optimized by removing the deleted link from the local state directly.
| const handleConfirmDelete = async () => { | |
| if (!linkToDelete) return; | |
| try { | |
| await api.deleteLink(linkToDelete.id); | |
| setDeleteLinkDialogOpen(false); | |
| setLinkToDelete(null); | |
| fetchLinks(); | |
| } catch (err) { | |
| console.error("Error deleting link:", err); | |
| setError("Failed to delete link."); | |
| } | |
| }; | |
| const handleConfirmDelete = async () => { | |
| if (!linkToDelete) return; | |
| try { | |
| await api.deleteLink(linkToDelete.id); | |
| setLinks((prevLinks) => prevLinks.filter((link) => link.id !== linkToDelete.id)); | |
| setDeleteLinkDialogOpen(false); | |
| setLinkToDelete(null); | |
| } catch (err) { | |
| console.error("Error deleting link:", err); | |
| setError("Failed to delete link."); | |
| } | |
| }; |
Coverage summary from CodacySee diff coverage on Codacy
Coverage variation details
Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: Diff coverage details
Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: See your quality gate settings Change summary preferences |
1 participant
This pull request adds full support for editing and deleting links in the application, covering both backend and frontend components. The backend introduces new use cases and services for updating and deleting links, with proper ownership checks, while the frontend provides new dialogs and actions for users to edit or delete links directly from the link list. The API layer is also extended to support these new operations.
Backend: Link management services and API integration
UpdateLinkUseCaseandDeleteLinkUseCaseinterfaces, with corresponding implementations (UpdateLinkService,DeleteLinkService) that verify link ownership before allowing updates or deletions. [1] [2] [3] [4] [5]LinkControllerto handle PATCH (edit) and DELETE requests for links, returning updated link data or appropriate status codes.Frontend: Edit and delete link UI
LinkList, opening dialogs for editing link details or confirming deletion.API client enhancements
updateLinkanddeleteLinkmethods to interact with the new backend endpoints.Documentation