Apply updates to unrestricted external tables#1536
Apply updates to unrestricted external tables#1536samuelbray32 wants to merge 3 commits intomasterfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request fixes a bug where _resolve_external_table was attempting to update restricted external tables in DataJoint, which raises a DataJointError. The solution introduces tracking of unrestricted table objects separately so updates can be applied to the correct unrestricted tables instead of the restricted query results.
Changes:
- Modified
_resolve_external_tableto maintain a parallel list (table_to_update) of unrestricted table objects alongside the existing list of restricted tables (to_updates) - Applied database updates to unrestricted tables instead of restricted ones, resolving the DataJointError from issue #1535
- Minor formatting changes: removed empty line after imports and repositioned
noqacomment
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def _resolve_external_table( | ||
| filepath: str, file_name: str, location: str = "analysis" | ||
| ): | ||
| """Function to resolve database vs. file property discrepancies. | ||
|
|
||
| WARNING: This should only be used when editing file metadata. Can violate | ||
| data integrity if improperly used. | ||
|
|
||
| Parameters | ||
| ---------- | ||
| filepath : str | ||
| abs path to the file to edit | ||
| file_name : str | ||
| name of the file to edit | ||
| location : str, optional | ||
| which external table the file is in, current options are | ||
| ["analysis", "raw], by default "analysis" | ||
| """ | ||
| from spyglass.common import LabMember | ||
| from spyglass.common.common_nwbfile import AnalysisRegistry | ||
| from spyglass.common.common_nwbfile import schema as common_schema | ||
|
|
||
| LabMember().check_admin_privilege( | ||
| error_message="Please contact database admin to edit database checksums" | ||
| ) | ||
|
|
||
| file_restr = f"filepath LIKE '%{file_name}'" | ||
|
|
||
| to_updates = [] | ||
| table_to_update = [] | ||
| if location == "analysis": # Update for each custom Analysis external | ||
| for external in AnalysisRegistry().get_externals(): | ||
| restr_external = external & file_restr | ||
| if not bool(restr_external): | ||
| continue | ||
| if len(restr_external) > 1: | ||
| raise ValueError( | ||
| "Multiple entries found in external table for file: " | ||
| + f"{file_name}, cannot resolve." | ||
| ) | ||
| to_updates.append(restr_external) | ||
| table_to_update.append(external) | ||
|
|
||
| elif location == "raw": | ||
| restr_external = common_schema.external["raw"] & file_restr | ||
| to_updates.append(restr_external) | ||
| table_to_update.append(common_schema.external["raw"]) | ||
|
|
||
| if not to_updates: | ||
| logger.warning( | ||
| f"No entries found in external tables for file: {file_name}" | ||
| ) | ||
| return | ||
|
|
||
| update_vals = dict( | ||
| size=Path(filepath).stat().st_size, | ||
| contents_hash=dj.hash.uuid_from_file(filepath), | ||
| ) | ||
| for to_update in to_updates: | ||
| for to_update, table in zip(to_updates, table_to_update): | ||
| key = to_update.fetch1() | ||
| key.update(update_vals) | ||
| to_update.update1(key) | ||
| table.update1(key) | ||
|
|
There was a problem hiding this comment.
The _resolve_external_table function lacks test coverage. Given that this function handles critical database updates and has been the source of a bug (as per issue #1535), adding tests would help prevent regressions. The test file tests/utils/test_dj_helper_fn.py exists but only contains a single deprecation test. Consider adding tests for both the "analysis" and "raw" location branches, including edge cases like empty results, multiple entries, and successful updates.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
|
@samuelbray32 I've opened a new pull request, #1543, to work on those changes. Once the pull request is ready, I'll request review from you. |
Description
Fixes #1535
Checklist:
CITATION.cffaltersnippet for release notes.CHANGELOG.mdwith PR number and description.