Test: start a test suite for migration utils functions#8225
Open
CarolineDenis wants to merge 58 commits into
Open
Test: start a test suite for migration utils functions#8225CarolineDenis wants to merge 58 commits into
CarolineDenis wants to merge 58 commits into
Conversation
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds comprehensive unit tests for migration utilities and management command, extracts discipline resource directory deduplication to a dedicated module, and consolidates migration helper imports. The test suite spans bulk creation, schema operations, default data initialization, deduplication, and tectonic ranks. Integration tests verify the migration command is idempotent across multiple runs without creating duplicates. ChangesMigration Utils Unit Test Suite
Management Command Test Infrastructure and Integration Tests
Deduplication Helper Extraction and Import Updates
Possibly related issues
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Contributor
Author
|
@CodeRabbit review |
Contributor
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (1)
specifyweb/specify/migration_utils/tests/test_helper_0003_cotype_picklist.py (1)
63-63:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winSame issue: passing
_mock_new_parentinstead of a proper apps mock.This line has the same problem as line 36. Use a proper mock apps object.
🤖 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 `@specifyweb/specify/migration_utils/tests/test_helper_0003_cotype_picklist.py` at line 63, The call to create_cotype_splocalecontaineritem is passing mock_container._mock_new_parent as the apps argument, but this should be a proper mock apps object instead. Replace the _mock_new_parent argument with a properly constructed mock apps object (following the same pattern used to fix the similar issue at line 36) to ensure the test passes the correct type of dependency to the function.
🧹 Nitpick comments (1)
specifyweb/specify/migration_utils/tests/test_schema_reader.py (1)
122-122: 💤 Low valueRemove trailing blank line.
The file ends with an extra blank line after the closing of the
if __name__ == '__main__'block.🤖 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 `@specifyweb/specify/migration_utils/tests/test_schema_reader.py` at line 122, There is an extra blank line at the end of the file after the if __name__ == '__main__' block. Remove the trailing blank line at the end of the test_schema_reader.py file to clean up the file formatting.
🤖 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 `@specifyweb/specify/migration_utils/tests/test_bulk_create.py`:
- Around line 46-50: The test is named with "idempotent" but doesn't actually
verify idempotent behavior. After the first assertion where result equals 2, add
a second call to the same function being tested using the same input rows, then
assert that this second call returns 0 to confirm that no duplicate rows are
created. This verifies that the function properly checks for existing rows and
avoids redundant bulk_create operations when called with identical data.
- Around line 16-22: The mock queryset setup in the test does not properly
simulate the chained filter calls used in the real function. The real
implementation calls filter twice in succession before order_by, but the current
mock setup only chains filter once. Modify the mock_qs setup so that the first
filter call returns an object whose filter method can be called again, and that
second filter call returns an object with an order_by method. Specifically,
ensure mock_qs.filter.return_value has its own filter method configured to
return an object that has order_by chained properly, allowing the double filter
pattern to work correctly.
- Around line 11-12: The `@patch` decorator is replacing the actual
bulk_create_splocaleitemstr_idempotent function with a mock, preventing the test
from verifying the real implementation logic. Remove the `@patch` decorator
entirely and instead mock only the internal ORM dependencies (like
model.objects.filter and model.objects.bulk_create) that the actual function
uses. Update the test assertions to verify that the real function correctly
calls these mocked ORM methods with the expected parameters (FK filters,
language filters, and bulk_create calls), rather than asserting on the mock of
the function itself.
In
`@specifyweb/specify/migration_utils/tests/test_helper_0003_cotype_picklist.py`:
- Line 36: The function create_cotype_splocalecontaineritem is being called with
mock_container._mock_new_parent, which is an internal MagicMock attribute and
not a valid apps object with the required get_model method. Create a proper mock
apps object that implements the get_model(app_label, model_name) method as
expected by the function signature, then pass this mock apps object as the
argument to the create_cotype_splocalecontaineritem call instead of the
_mock_new_parent attribute.
- Around line 9-11: The test is patching module-level names that don't actually
exist in helper_0003_cotype_picklist.py, but the real
create_cotype_splocalecontaineritem function dynamically obtains models using
apps.get_model('specify', ModelName). Replace the three `@patch` decorators
targeting Splocalecontainer, Splocalecontaineritem, and Splocaleitemstr with a
single `@patch` for the apps parameter, then configure apps.get_model.side_effect
to return the appropriate mock models based on the model name argument passed to
it. Apply this same pattern to both test methods in the file.
In `@specifyweb/specify/migration_utils/tests/test_schema_reader.py`:
- Around line 28-48: The test assertion for mock_path is incorrect because it
expects the full concatenated path string, but the actual function constructs
the path using chained division operators starting with
Path(settings.SPECIFY_CONFIG_DIR). Update the test to properly mock the Path
chaining behavior by setting up mock_path.return_value to support the
__truediv__ method (which implements the / operator) and chain operations, then
correct the assertion to expect mock_path to be called once with just "/config"
instead of the full path string. The mock path instance should also have
exists() and open() methods properly configured to support the chaining pattern.
In `@specifyweb/specify/migration_utils/tests/test_schema_writer.py`:
- Around line 12-16: The test patches are targeting non-existent module-level
imports in schema_writer, but the real
`update_table_schema_config_with_defaults` function obtains models through
`apps.get_model()` calls, not from direct imports. Instead of patching
`Splocalecontainer`, `Splocalecontaineritem`, `Splocaleitemstr` as if they were
module-level imports, create a mock `apps` object, configure its `get_model()`
method to return the appropriate mock models for each model name (specify,
Splocalecontainer), (specify, Splocalecontaineritem), and (specify,
Splocaleitemstr), and pass this mock as a parameter to the
`update_table_schema_config_with_defaults` function call in the test. Also patch
the `apps` import in the schema_writer module itself.
- Around line 65-72: The test is patching module-level names that the function
`revert_table_schema_config` does not use. Instead, the function obtains models
via `apps.get_model()`, but the test calls
`revert_table_schema_config("TestTable")` without passing an `apps` argument,
causing it to use the real Django registry (global_apps). Remove the three
`@patch` decorators for Splocalecontainer, Splocaleitemstr, and
Splocalecontaineritem. Instead, create a mock `apps` object that is configured
to return the mocked model instances when `get_model()` is called with the
appropriate model names, and pass this mock `apps` object as a parameter to the
`revert_table_schema_config` function call.
---
Duplicate comments:
In
`@specifyweb/specify/migration_utils/tests/test_helper_0003_cotype_picklist.py`:
- Line 63: The call to create_cotype_splocalecontaineritem is passing
mock_container._mock_new_parent as the apps argument, but this should be a
proper mock apps object instead. Replace the _mock_new_parent argument with a
properly constructed mock apps object (following the same pattern used to fix
the similar issue at line 36) to ensure the test passes the correct type of
dependency to the function.
---
Nitpick comments:
In `@specifyweb/specify/migration_utils/tests/test_schema_reader.py`:
- Line 122: There is an extra blank line at the end of the file after the if
__name__ == '__main__' block. Remove the trailing blank line at the end of the
test_schema_reader.py file to clean up the file formatting.
🪄 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 Plus
Run ID: 09e00975-8c37-4bc2-91a7-45fae919b02c
📒 Files selected for processing (5)
specifyweb/specify/migration_utils/tests/__init__.pyspecifyweb/specify/migration_utils/tests/test_bulk_create.pyspecifyweb/specify/migration_utils/tests/test_helper_0003_cotype_picklist.pyspecifyweb/specify/migration_utils/tests/test_schema_reader.pyspecifyweb/specify/migration_utils/tests/test_schema_writer.py
github-project-automation
Bot
moved this from 📋Back Log
to Dev Attention Needed
in General Tester Board
Contributor
Author
|
@CodeRabbit review |
Contributor
✅ Action performedReview finished.
|
Contributor
Author
|
@CodeRabbit full review |
Contributor
✅ Action performedFull review finished. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (4)
specifyweb/specify/migration_utils/tests/test_helper_0039_agent_fields_for_loan_and_gift.py (1)
1-1: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueRemove unused import.
MagicMockis imported but never used in the test.🧹 Proposed fix
-from unittest.mock import patch, MagicMock +from unittest.mock import patch🤖 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 `@specifyweb/specify/migration_utils/tests/test_helper_0039_agent_fields_for_loan_and_gift.py` at line 1, The import statement at the beginning of the test file imports MagicMock from unittest.mock, but MagicMock is never used in the test code. Remove MagicMock from the import statement, leaving only the imports that are actually used in the test.specifyweb/specify/migration_utils/tests/test_helper_0003_cotype_picklist.py (1)
60-61: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winStrengthen assertions for both locale string creations
mock_itemstr.objects.create.assert_called()is too permissive for this path. This scenario should verify both expected creates (label + description), otherwise a partial regression can still pass.Proposed assertion update
- mock_itemstr.objects.create.assert_called() + self.assertEqual(mock_itemstr.objects.create.call_count, 2) + mock_itemstr.objects.create.assert_any_call( + language="en", + itemname=created_item, + text=helper_0003_cotype_picklist.COT_TEXT, + ) + mock_itemstr.objects.create.assert_any_call( + language="en", + itemdesc=created_item, + text=helper_0003_cotype_picklist.COT_TEXT, + )🤖 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 `@specifyweb/specify/migration_utils/tests/test_helper_0003_cotype_picklist.py` around lines 60 - 61, The assertion on mock_itemstr.objects.create is too permissive as it only verifies the method was called at least once. In this test path, both a label and description locale string should be created, so strengthen the assertion in test_helper_0003_cotype_picklist.py to verify that exactly two create calls are made to mock_itemstr.objects.create. Replace assert_called() with assert_called_with() or use assert_has_calls() to ensure both the label and description string creations are verified, preventing partial regressions where only one create occurs instead of the expected two.specifyweb/specify/migration_utils/tests/test_tectonic_ranks.py (1)
130-168: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winUse distinct
root_rankmocks per tree to verify per-tree binding.Right now both
TectonicUnit.objects.createassertions can pass even if the implementation accidentally reuses a single rank object across trees. Using per-callget_or_createresults will make this test catch that regression.Proposed test hardening
- root_rank = MagicMock() - TectonicUnitTreeDefItem.objects.get_or_create.return_value = ( - root_rank, - True, - ) + root_rank1 = MagicMock() + root_rank2 = MagicMock() + TectonicUnitTreeDefItem.objects.get_or_create.side_effect = [ + (root_rank1, True), + (root_rank2, True), + ] @@ TectonicUnit.objects.create.assert_any_call( @@ - definitionitem=root_rank, + definitionitem=root_rank1, ) @@ TectonicUnit.objects.create.assert_any_call( @@ - definitionitem=root_rank, + definitionitem=root_rank2, )🤖 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 `@specifyweb/specify/migration_utils/tests/test_tectonic_ranks.py` around lines 130 - 168, The test currently uses the same root_rank mock object for both trees, which means the test won't catch if the implementation accidentally reuses a single rank across trees instead of creating distinct ones per tree. Create two separate mock objects (for example, root_rank_1 and root_rank_2), configure TectonicUnitTreeDefItem.objects.get_or_create to return different mocks for each call using side_effect (returning tuples like (root_rank_1, True) then (root_rank_2, True)), and then update the two assert_any_call assertions to verify that the correct distinct rank mock is passed to each TectonicUnit.objects.create call for its respective tree.specifyweb/specify/management/commands/tests/app_resource_tests.py (1)
70-75: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAvoid asserting the literal
<lambda>log token.This assertion is brittle and tied to an implementation detail, not behavior. Prefer asserting call order/content semantically (or using a named callable in the command) so harmless refactors don’t break this test.
🤖 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 `@specifyweb/specify/management/commands/tests/app_resource_tests.py` around lines 70 - 75, The test assertion in app_resource_tests.py is brittle because it asserts on the literal string "Running <lambda>..." which depends on implementation details. Either modify the stdout.call_args_list assertion to remove the lambda call check and only verify the meaningful calls like "Running deduplicate_discipline_resource_dirs..." (testing the semantic behavior and call order), or refactor the command code to use a named callable function instead of a lambda function so the log message reflects a stable, descriptive name rather than the implementation detail of using an anonymous lambda.
🤖 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 `@specifyweb/specify/management/commands/tests/cots_tests.py`:
- Around line 7-8: The CotsDatabaseTests class currently inherits from
MigrationCommandTestCase, but since it performs real ORM operations and requires
API/fixture state attributes like self.collection, self.discipline, and
self.collectionobjecttype, it should instead inherit from a database-oriented
test base class. Replace the import statement to import the appropriate database
test base class instead of MigrationCommandTestCase, and then update the
CotsDatabaseTests class definition to inherit from this database-oriented base
class.
In `@specifyweb/specify/management/commands/tests/schema_config_tests.py`:
- Around line 144-149: The test assertion checking that keeper.text equals
"Updated Name" expects update behavior that is not guaranteed by the
bulk_create_splocaleitemstr_idempotent helper function. The helper is designed
only to deduplicate and create objects idempotently, not to update existing
records. Remove the assertion self.assertEqual(keeper.text, "Updated Name") from
the test and keep only the assertions that verify the actual guaranteed
behavior: that created_count is 1 and that the duplicate object no longer exists
in the database.
- Around line 31-61: The names list in this test includes migration function
names that are not currently part of the active fix_schema_config runnable
sequence (some are commented out or disabled upstream). When
patch.object(rkm.usc, name) attempts to patch these non-existent functions, the
test will fail before it can validate the actual ordering behavior. Remove all
function names from the names list that are not currently active in the
fix_schema_config contract, keeping only those that are actually being executed
in the current sequence.
- Around line 199-210: The test is attempting to refresh database objects
(duplicate_name and duplicate_desc) that are actually deleted by the
deduplicate_containeritems_and_strings function, causing a mismatch between
expected test behavior and actual function behavior. Remove the refresh_from_db
calls on duplicate_name and duplicate_desc since these rows no longer exist
after deduplication, and update the assertions to verify that the duplicate
strings are deleted rather than repointed. Ensure the test only checks for the
existence of keeper's names through keeper.names.values_list and confirms that
conflicting duplicate entries are removed from the Splocaleitemstr table.
In `@specifyweb/specify/management/commands/tests/tectonic_tests.py`:
- Around line 6-12: The TectonicDatabaseTests class inherits from plain TestCase
but the test method
test_create_default_tectonic_ranks_creates_chain_and_assigns_discipline uses
self.discipline without initialization, which will cause runtime failures.
Change the inheritance of TectonicDatabaseTests from TestCase to the appropriate
migration database base class that provisions test data and fixtures (including
the discipline instance), so that self.discipline is properly available for use
in the test methods.
In
`@specifyweb/specify/management/commands/tests/test_run_key_migration_functions.py`:
- Around line 44-48: The record_counts() function is counting objects using the
implicit default database alias, but the migration command being tested
(run_key_migration_connection) executes within use_migration_connection()
context which explicitly uses the 'migrations' alias. If these aliases target
different databases, the count comparison will be invalid. Update the
record_counts() function to explicitly route all model.objects.count() queries
to the 'migrations' database alias by using the .using("migrations") method on
each model's queryset to ensure counts are taken from the same database that the
command operates on.
In `@specifyweb/specify/migration_utils/deduplication.py`:
- Around line 179-183: The Exists query in the SpAppResourceDir.objects.filter
is only checking for timestampcreated__lt, which misses rows with identical
timestamps. Modify the filter to also handle the tie-break case where timestamps
are equal but the ID is lower. Add an OR condition (using Q objects) that checks
for either an earlier timestamp OR (equal timestamp AND lower ID) to ensure
duplicates with identical timestamps are properly identified as
earlier_exists=True and deduplicated based on the lower ID value.
In `@specifyweb/specify/migration_utils/tests/test_bulk_create.py`:
- Around line 66-70: The test currently inspects only the last call to
bulk_create using call_args and asserts the length is 2, but since bulk_create
is executed once per FK group (itemname and itemdesc) with one object each, it
will be called twice with one object per call. Replace the assertion on
call_args with assertions using call_args_list to verify all calls to the
bulk_create method. Check that bulk_create was called the expected number of
times and that each individual call in the list contains the correct number of
objects per group.
---
Nitpick comments:
In `@specifyweb/specify/management/commands/tests/app_resource_tests.py`:
- Around line 70-75: The test assertion in app_resource_tests.py is brittle
because it asserts on the literal string "Running <lambda>..." which depends on
implementation details. Either modify the stdout.call_args_list assertion to
remove the lambda call check and only verify the meaningful calls like "Running
deduplicate_discipline_resource_dirs..." (testing the semantic behavior and call
order), or refactor the command code to use a named callable function instead of
a lambda function so the log message reflects a stable, descriptive name rather
than the implementation detail of using an anonymous lambda.
In
`@specifyweb/specify/migration_utils/tests/test_helper_0003_cotype_picklist.py`:
- Around line 60-61: The assertion on mock_itemstr.objects.create is too
permissive as it only verifies the method was called at least once. In this test
path, both a label and description locale string should be created, so
strengthen the assertion in test_helper_0003_cotype_picklist.py to verify that
exactly two create calls are made to mock_itemstr.objects.create. Replace
assert_called() with assert_called_with() or use assert_has_calls() to ensure
both the label and description string creations are verified, preventing partial
regressions where only one create occurs instead of the expected two.
In
`@specifyweb/specify/migration_utils/tests/test_helper_0039_agent_fields_for_loan_and_gift.py`:
- Line 1: The import statement at the beginning of the test file imports
MagicMock from unittest.mock, but MagicMock is never used in the test code.
Remove MagicMock from the import statement, leaving only the imports that are
actually used in the test.
In `@specifyweb/specify/migration_utils/tests/test_tectonic_ranks.py`:
- Around line 130-168: The test currently uses the same root_rank mock object
for both trees, which means the test won't catch if the implementation
accidentally reuses a single rank across trees instead of creating distinct ones
per tree. Create two separate mock objects (for example, root_rank_1 and
root_rank_2), configure TectonicUnitTreeDefItem.objects.get_or_create to return
different mocks for each call using side_effect (returning tuples like
(root_rank_1, True) then (root_rank_2, True)), and then update the two
assert_any_call assertions to verify that the correct distinct rank mock is
passed to each TectonicUnit.objects.create call for its respective tree.
🪄 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 Plus
Run ID: be5667aa-4669-4fcd-9100-220c010d3fff
📒 Files selected for processing (27)
specifyweb/specify/management/commands/run_key_migration_functions.pyspecifyweb/specify/management/commands/tests/__init__.pyspecifyweb/specify/management/commands/tests/app_resource_tests.pyspecifyweb/specify/management/commands/tests/business_rules_tests.pyspecifyweb/specify/management/commands/tests/command_tests.pyspecifyweb/specify/management/commands/tests/cots_tests.pyspecifyweb/specify/management/commands/tests/permissions_tests.pyspecifyweb/specify/management/commands/tests/schema_config_tests.pyspecifyweb/specify/management/commands/tests/tectonic_tests.pyspecifyweb/specify/management/commands/tests/test_deduplicate_discipline.pyspecifyweb/specify/management/commands/tests/test_migration_base.pyspecifyweb/specify/management/commands/tests/test_run_key_migration_functions.pyspecifyweb/specify/management/commands/tests/test_run_key_migration_functions_order.pyspecifyweb/specify/migration_utils/deduplication.pyspecifyweb/specify/migration_utils/migration_helpers/helper_0003_cotype_picklist.pyspecifyweb/specify/migration_utils/migration_helpers/helper_0031_add_default_for_selectseries.pyspecifyweb/specify/migration_utils/tests/__init__.pyspecifyweb/specify/migration_utils/tests/test_bulk_create.pyspecifyweb/specify/migration_utils/tests/test_deduplication.pyspecifyweb/specify/migration_utils/tests/test_default_cots.pyspecifyweb/specify/migration_utils/tests/test_helper_0003_cotype_picklist.pyspecifyweb/specify/migration_utils/tests/test_helper_0031_add_default_for_selectseries.pyspecifyweb/specify/migration_utils/tests/test_helper_0039_agent_fields_for_loan_and_gift.pyspecifyweb/specify/migration_utils/tests/test_schema_reader.pyspecifyweb/specify/migration_utils/tests/test_schema_writer.pyspecifyweb/specify/migration_utils/tests/test_tectonic_ranks.pyspecifyweb/specify/migrations/0031_add_default_for_selectseries.py
💤 Files with no reviewable changes (1)
- specifyweb/specify/migration_utils/migration_helpers/helper_0003_cotype_picklist.py
Comment on lines
+44
to
+48
| def record_counts(): | ||
| return { | ||
| name: model.objects.count() | ||
| for name, model in TRACKED_MODELS.items() | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Inspect command-side DB context and router hooks
rg -n -C3 'def use_migration_connection|use_migration_connection|db_for_read|db_for_write|DATABASE_ROUTERS' specifyweb
# 2) Confirm this test currently uses implicit managers
rg -n -C2 'def record_counts|objects\.count\(|objects\.create\(|call_command\("run_key_migration_functions"' \
specifyweb/specify/management/commands/tests/test_run_key_migration_functions.py
# 3) Inspect DATABASES aliases in settings
fd -i 'settings*.py' | xargs -r rg -n -C2 'DATABASES|["'\'']default["'\'']|["'\'']migrations["'\'']'Repository: specify/specify7
Length of output: 11189
Verify DB alias consistency between test fixture/count helpers and command execution.
run_key_migration_connection() runs inside use_migration_connection() which explicitly routes operations to the 'migrations' database alias. This test creates rows and snapshots counts through implicit .objects managers outside that context, which will route to the default database alias. If these aliases target different backing databases, the idempotency assertion validates the wrong dataset.
Please verify your DATABASES configuration and if needed, pin setup/count helpers to the same alias (for example, .using("migrations")).
🤖 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
`@specifyweb/specify/management/commands/tests/test_run_key_migration_functions.py`
around lines 44 - 48, The record_counts() function is counting objects using the
implicit default database alias, but the migration command being tested
(run_key_migration_connection) executes within use_migration_connection()
context which explicitly uses the 'migrations' alias. If these aliases target
different databases, the count comparison will be invalid. Update the
record_counts() function to explicitly route all model.objects.count() queries
to the 'migrations' database alias by using the .using("migrations") method on
each model's queryset to ensure counts are taken from the same database that the
command operates on.
Comment on lines
+179
to
+183
| earlier_exists=Exists( | ||
| SpAppResourceDir.objects.filter( | ||
| discipline_id=OuterRef('discipline_id'), | ||
| timestampcreated__lt=OuterRef('timestampcreated'), | ||
| **common_filters |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Check dedup query predicate (strict timestamp less-than only):"
rg -n -C3 'timestampcreated__lt=OuterRef' specifyweb/specify/migration_utils/deduplication.py
echo
echo "Check test expectation requiring tie-break on equal timestamp:"
rg -n -C5 'test_deduplicate_discipline_resource_dirs_tie_breaks_on_id|timestampcreated=timestamp' specifyweb/specify/management/commands/tests/test_deduplicate_discipline.pyRepository: specify/specify7
Length of output: 1590
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Check test status ==="
# Find the test and see if there's any indication it's skipped or expected to fail
rg -n -A20 'def test_deduplicate_discipline_resource_dirs_tie_breaks_on_id' specifyweb/specify/management/commands/tests/test_deduplicate_discipline.py
echo
echo "=== Check both query patterns in detail ==="
# Line 90 context
rg -n -B5 -A5 'timestampcreated__lt=OuterRef' specifyweb/specify/migration_utils/deduplication.py | head -30
echo
echo "=== Verify imports in deduplication.py ==="
head -20 specifyweb/specify/migration_utils/deduplication.pyRepository: specify/specify7
Length of output: 2579
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Check for RowNumber usage in deduplication logic ==="
rg -n -B10 -A10 'RowNumber' specifyweb/specify/migration_utils/deduplication.py
echo
echo "=== Full context for line 90 function (Container dedup) ==="
sed -n '80,95p' specifyweb/specify/migration_utils/deduplication.py
echo
echo "=== Check if test has skip/xfail markers ==="
rg -n -B2 'def test_deduplicate_discipline_resource_dirs_tie_breaks_on_id' specifyweb/specify/management/commands/tests/test_deduplicate_discipline.py
echo
echo "=== Check if the SpAppResourceDir function is even called ==="
rg -n 'def deduplicate_discipline_resource_dirs' specifyweb/specify/migration_utils/deduplication.pyRepository: specify/specify7
Length of output: 2611
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Full deduplicate_discipline_resource_dirs function ==="
sed -n '160,190p' specifyweb/specify/migration_utils/deduplication.py
echo
echo "=== Check what happens after finding duplicates ==="
sed -n '160,200p' specifyweb/specify/migration_utils/deduplication.pyRepository: specify/specify7
Length of output: 2446
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Check Container dedup function for comparison ==="
sed -n '75,100p' specifyweb/specify/migration_utils/deduplication.py
echo
echo "=== Look for any post-query tie-breaking logic ==="
rg -n 'rn\s*>|RowNumber|order_by.*id' specifyweb/specify/migration_utils/deduplication.py | head -20
echo
echo "=== Check if there's any additional filtering in deduplicate_discipline_resource_dirs after the query ==="
sed -n '160,195p' specifyweb/specify/migration_utils/deduplication.pyRepository: specify/specify7
Length of output: 3091
Add deterministic tie-break logic for equal timestamps.
The timestampcreated__lt filter at line 182 only catches earlier rows, missing duplicates with identical timestamps. The test test_deduplicate_discipline_resource_dirs_tie_breaks_on_id expects these to be deduplicated by keeping the lower ID, but the current query never identifies them as earlier_exists=True. Add an ID-based tie-break condition for equal timestamps.
Proposed fix
-from django.db.models import Exists, OuterRef
+from django.db.models import Exists, OuterRef, Q
...
earlier_exists=Exists(
SpAppResourceDir.objects.filter(
discipline_id=OuterRef('discipline_id'),
- timestampcreated__lt=OuterRef('timestampcreated'),
+ Q(timestampcreated__lt=OuterRef('timestampcreated'))
+ | Q(
+ timestampcreated=OuterRef('timestampcreated'),
+ id__lt=OuterRef('id'),
+ ),
**common_filters
)
)📝 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
| earlier_exists=Exists( | |
| SpAppResourceDir.objects.filter( | |
| discipline_id=OuterRef('discipline_id'), | |
| timestampcreated__lt=OuterRef('timestampcreated'), | |
| **common_filters | |
| earlier_exists=Exists( | |
| SpAppResourceDir.objects.filter( | |
| discipline_id=OuterRef('discipline_id'), | |
| Q(timestampcreated__lt=OuterRef('timestampcreated')) | |
| | Q( | |
| timestampcreated=OuterRef('timestampcreated'), | |
| id__lt=OuterRef('id'), | |
| ), | |
| **common_filters |
🤖 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 `@specifyweb/specify/migration_utils/deduplication.py` around lines 179 - 183,
The Exists query in the SpAppResourceDir.objects.filter is only checking for
timestampcreated__lt, which misses rows with identical timestamps. Modify the
filter to also handle the tie-break case where timestamps are equal but the ID
is lower. Add an OR condition (using Q objects) that checks for either an
earlier timestamp OR (equal timestamp AND lower ID) to ensure duplicates with
identical timestamps are properly identified as earlier_exists=True and
deduplicated based on the lower ID value.
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
1 participant
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.
Fixes #8224
Checklist
self-explanatory (or properly documented)
specify7/specifyweb/specify/management/commands/run_key_migration_functions.py
Line 50 in ea04665
Testing instructions
Summary by CodeRabbit
New Features
Bug Fixes
selectseriesdefaults are consistently enforced toFalseduring migrations.Tests