Conversation
|
@mbarlow12 I'm assuming you'll want to make a bunch of changes on this. Feel free to take over the branch, but I'd prefer if we keep the signature on the new POST (i.e. takes an array of resource IDs, returns a Resource object). |
There was a problem hiding this comment.
Pull request overview
This pull request adds a new /resources/merge endpoint to the API that implements stub functionality for merging multiple resources. The endpoint accepts a list of resource IDs and repoints resources[1:] to resources[0] as the canonical target. This is explicitly noted as a springboard for future entity-linkage service development and intentionally lacks automated tests.
Changes:
- Added POST
/resources/mergeendpoint with MergeRequest model for accepting resource IDs - Implemented
merge_resourcesfunction in resource_actions that updates repointed_id field on resources - Added seed data (2 resources, 2 sources, 2 memberships) to support merge testing scenarios
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 10 comments.
| File | Description |
|---|---|
| deployments/api/src/stitch/api/routers/resources.py | Adds merge endpoint with validation, logging, and error handling wrapper around resource_actions.merge_resources |
| deployments/api/src/stitch/api/db/resource_actions.py | Implements merge_resources function that validates resource existence and updates repointed_id fields |
| deployments/api/src/stitch/api/db/init_job.py | Adds test/seed data for merge scenarios: "Merge Target" and "Merge Consumed" resources with associated sources |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| session: AsyncSession, | ||
| user: CurrentUser, | ||
| ids: Sequence[int], |
There was a problem hiding this comment.
The user parameter is accepted but never used in the merge_resources function. If this is intended for future authorization checks or audit logging (e.g., updating last_updated_by_id on modified resources), consider adding a TODO comment. Otherwise, the parameter can be removed to reduce confusion.
There was a problem hiding this comment.
@mbarlow12 my local IDE kept warning me about this too (User isn't consumed in the resource_actions). I'm guessing that when we start actually using permissions to filter the reesources, that's when we'll start using it?
| if not ids: | ||
| raise HTTPException(status_code=400, detail="No resource IDs provided.") | ||
| # preserve order but drop duplicates | ||
| unique_ids = list(dict.fromkeys(ids)) | ||
| if len(unique_ids) < 2: | ||
| raise HTTPException( | ||
| status_code=400, detail="Provide at least 2 unique resource IDs." | ||
| ) |
There was a problem hiding this comment.
The validation logic in lines 171-178 duplicates the validation already performed in the router endpoint (lines 58-61 of resources.py). Since the endpoint already validates that there are at least 2 unique resource IDs before calling this function, this redundant validation can be removed. The check for empty IDs list at line 171-172 is also unnecessary since the endpoint guarantees the list is non-empty after deduplication.
| if not ids: | |
| raise HTTPException(status_code=400, detail="No resource IDs provided.") | |
| # preserve order but drop duplicates | |
| unique_ids = list(dict.fromkeys(ids)) | |
| if len(unique_ids) < 2: | |
| raise HTTPException( | |
| status_code=400, detail="Provide at least 2 unique resource IDs." | |
| ) | |
| # preserve order but drop duplicates | |
| unique_ids = list(dict.fromkeys(ids)) |
| except HTTPException: | ||
| raise | ||
| except Exception as exc: | ||
| logger.exception("Error while merging resources %s: %s", unique_ids, exc) |
There was a problem hiding this comment.
The logger.exception call already includes the full exception traceback and the exception message automatically. Including the exception in the log message string as well (with %s, exc) results in duplicate exception information in the logs. Consider removing exc from the message format and just using: logger.exception("Error while merging resources %s", unique_ids)
| logger.exception("Error while merging resources %s: %s", unique_ids, exc) | |
| logger.exception("Error while merging resources %s", unique_ids) |
|
|
||
| logger.info( | ||
| "Merge requested by user=%s for resource_ids=%s", | ||
| getattr(user, "sub", "<anon>"), |
There was a problem hiding this comment.
The use of getattr(user, "sub", "") is unnecessary. According to the User entity definition (entities.py line 167), the 'sub' field is a required string field (not optional). The user parameter is typed as CurrentUser which is a User entity with sub always present. This can be simplified to just user.sub.
| getattr(user, "sub", "<anon>"), | |
| user.sub, |
|
@mbarlow12 Copilot brought up a bunch of issues with my implementation (which all seem fair), but I'm probably not the best person to figure out what the "right" answer is. You mentioned in meeting yesterday wanting to use what's already available in I'm fine if you want to either take over this PR, or close it. From a design perspective, I'd like to have a merge endpoint at |
|
@AlexAxthelm need to merge the ogsi model branch. But hopefully this'll be cleaner once that's done. |
|
@AlexAxthelm had to assign you instead of requesting review since you initiated the PR. Should be good to go. |
Adds a
/resources/mergeendpoint to API.Note: This has not added any automated tests, and exists primarily as a springboard for adding an entity-linkage service.
Confirmed via API that:
Testing can come in a follow up PR