feat(redesign-new-filter-url-params): adds new url params format#3636
feat(redesign-new-filter-url-params): adds new url params format#3636brian-mecha wants to merge 11 commits into
Conversation
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
📝 WalkthroughWalkthroughThe PR replaces the indexed Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
docs/developer_guides/dynamic_filters.md (1)
34-67:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate the
Column Filtersection to match the new parameter contract.Line 66 still states matching on
filter_{i}_column, which conflicts with the new format documented in Lines 34–60. Please update that paragraph to describef_{query_param}+op_{query_param}(and optionally note legacy fallback explicitly).🤖 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 `@docs/developer_guides/dynamic_filters.md` around lines 34 - 67, The "Column Filter" section describes how the ColumnFilter class processes filters using the old format with filter_{i}_column, which conflicts with the new recommended format documented earlier (f_{column_name} and op_{column_name}). Update the "Column Filter" section paragraph to describe how the ColumnFilter class works with the new parameter format f_{query_param} and op_{query_param} instead of filter_{i}_column, and optionally note that the old format is deprecated or supported as a legacy fallback for backwards compatibility.apps/web/dynamic_filters/datastructures.py (1)
57-83:⚠️ Potential issue | 🟠 Major | ⚡ Quick winLegacy
filter_{i}_*URLs are not preserved end-to-end during migration.The shared root cause is that legacy query keys are no longer fully decoded into active filters across backend + UI hydration paths, so existing bookmarks/saved links can silently stop applying filters.
apps/web/dynamic_filters/datastructures.py#L57-L83: add legacyfilter_{i}_column/operator/valueparsing path (or dual parser) inFilterParamsso old links still produceColumnFilterData.templates/filters/_filter_component_alpine.html#L229-L325: add legacy query-string hydration fallback inloadFiltersFromQueryStringso old URLs populatethis.filterData.filters.templates/filters/_filter_component_alpine.html#L511-L523: add old-format fallback for date-range label hydration to avoid UI desync on legacy links.🤖 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 `@apps/web/dynamic_filters/datastructures.py` around lines 57 - 83, The legacy filter_{i}_column/operator/value URL format is not being parsed, causing old bookmarks and saved links to lose their filters during migration. In apps/web/dynamic_filters/datastructures.py (lines 57-83), add parsing logic in the FilterParams class to extract and convert legacy filter_{i}_* parameters into ColumnFilterData objects alongside the new f_* format parsing, handling filter_{i}_column, filter_{i}_operator, and filter_{i}_value parameter groups. In templates/filters/_filter_component_alpine.html (lines 229-325), add a legacy query-string hydration fallback in the loadFiltersFromQueryString function to detect and parse legacy filter_{i}_* parameters when the new f_* format is not present, populating this.filterData.filters accordingly. In templates/filters/_filter_component_alpine.html (lines 511-523), add an old-format fallback within the date-range label hydration logic to correctly parse and display filter labels for legacy query strings to prevent UI desync on legacy links.
🧹 Nitpick comments (1)
apps/web/tests/test_datastructures.py (1)
16-33: ⚡ Quick winParametrize normalization cases with readable IDs.
This block is a good fit for
pytest.mark.parametrize(..., id="...")instead of sequential inline cases/comments.As per coding guidelines, "
**/*test*.py: Preferpytest.mark.parametrizefor tests over enumerated data; give each case a readable ID withpytest.param(..., id=\"...\")rather than inline comments."🤖 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 `@apps/web/tests/test_datastructures.py` around lines 16 - 33, The test function test_column_filter_data_list_normalization contains multiple inline test cases with comments instead of using pytest.mark.parametrize. Refactor this function by adding a `@pytest.mark.parametrize` decorator above it that defines the test cases (column, operator, value input, and expected output) with readable IDs such as "json_array_input", "tilde_separated_values", "single_value", and "quoted_strings_with_tildes". Update the function signature to accept these parametrized arguments, then replace the multiple inline ColumnFilterData assertions with a single assertion that uses the parametrized values.Source: Coding guidelines
🤖 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 `@apps/web/tests/test_datastructures.py`:
- Around line 35-48: A regression test for the legacy filter parameter format is
missing. Add a new test function (following the same pattern as
test_filter_params_parse_new_format) that validates parsing of the old format
using filter_{i}_column, filter_{i}_operator, and filter_{i}_value parameters.
Construct a QueryDict with legacy-format parameters (e.g., filter_0_column,
filter_0_operator, filter_0_value) and verify that the FilterParams object
parses them correctly and produces ColumnFilterData objects with the same
column, operator, and value attributes as the new f_*/op_* format test expects,
ensuring backward compatibility across both parameter formats.
In `@templates/filters/_filter_component_alpine.html`:
- Around line 271-279: The filterIndex computation on line 277 subtracts
newFormatColumns.length, which counts all detected f_* keys regardless of
whether they have a corresponding op_* pair. When a malformed f_* key exists
without its op_* counterpart, this causes the filterIndex calculation to be
incorrect and skip valid filters. Fix this by counting only the valid filter
pairs (those where both urlParams.has(`f_${columnName}`) and
urlParams.has(opKey) are true) and using that count instead of
newFormatColumns.length in the filterIndex subtraction.
- Around line 206-227: The parseCSVTildeValue function uses simple quote
toggling on each `"` character, which is incompatible with RFC 4180 CSV
semantics where quotes inside quoted fields are escaped by doubling them (`""`).
Update parseCSVTildeValue to properly handle doubled quotes as escape sequences
rather than quote toggles, so that a value like `"foo""bar"` correctly
represents `foo"bar`. Additionally, update the serializer (around line 332) to
escape quotes by doubling them (`""`) instead of using backslash escaping
(`\"`), ensuring round-trip compatibility with the Python backend's
csv.reader/csv.writer implementation.
---
Outside diff comments:
In `@apps/web/dynamic_filters/datastructures.py`:
- Around line 57-83: The legacy filter_{i}_column/operator/value URL format is
not being parsed, causing old bookmarks and saved links to lose their filters
during migration. In apps/web/dynamic_filters/datastructures.py (lines 57-83),
add parsing logic in the FilterParams class to extract and convert legacy
filter_{i}_* parameters into ColumnFilterData objects alongside the new f_*
format parsing, handling filter_{i}_column, filter_{i}_operator, and
filter_{i}_value parameter groups. In
templates/filters/_filter_component_alpine.html (lines 229-325), add a legacy
query-string hydration fallback in the loadFiltersFromQueryString function to
detect and parse legacy filter_{i}_* parameters when the new f_* format is not
present, populating this.filterData.filters accordingly. In
templates/filters/_filter_component_alpine.html (lines 511-523), add an
old-format fallback within the date-range label hydration logic to correctly
parse and display filter labels for legacy query strings to prevent UI desync on
legacy links.
In `@docs/developer_guides/dynamic_filters.md`:
- Around line 34-67: The "Column Filter" section describes how the ColumnFilter
class processes filters using the old format with filter_{i}_column, which
conflicts with the new recommended format documented earlier (f_{column_name}
and op_{column_name}). Update the "Column Filter" section paragraph to describe
how the ColumnFilter class works with the new parameter format f_{query_param}
and op_{query_param} instead of filter_{i}_column, and optionally note that the
old format is deprecated or supported as a legacy fallback for backwards
compatibility.
---
Nitpick comments:
In `@apps/web/tests/test_datastructures.py`:
- Around line 16-33: The test function
test_column_filter_data_list_normalization contains multiple inline test cases
with comments instead of using pytest.mark.parametrize. Refactor this function
by adding a `@pytest.mark.parametrize` decorator above it that defines the test
cases (column, operator, value input, and expected output) with readable IDs
such as "json_array_input", "tilde_separated_values", "single_value", and
"quoted_strings_with_tildes". Update the function signature to accept these
parametrized arguments, then replace the multiple inline ColumnFilterData
assertions with a single assertion that uses the parametrized values.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: f5a473c3-181b-40f1-abf3-6b75946cf6aa
📒 Files selected for processing (5)
apps/experiments/tests/test_filters.pyapps/web/dynamic_filters/datastructures.pyapps/web/tests/test_datastructures.pydocs/developer_guides/dynamic_filters.mdtemplates/filters/_filter_component_alpine.html
| def test_filter_params_parse_new_format(): | ||
| """Test parsing new format (f_* and op_*).""" | ||
| query_dict = QueryDict("f_tags=tag1~tag2&op_tags=any%20of&f_status=active&op_status=equals") | ||
| params = FilterParams(query_dict) | ||
|
|
||
| assert len(params.filters) == 2 | ||
| assert params.get("tags").column == "tags" | ||
| assert params.get("tags").operator == "any of" | ||
| assert params.get("tags").value == '["tag1", "tag2"]' | ||
|
|
||
| assert params.get("status").column == "status" | ||
| assert params.get("status").operator == "equals" | ||
| assert params.get("status").value == "active" | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add a legacy-format regression test for backward compatibility.
The suite validates f_*/op_*, but doesn’t assert parsing of filter_{i}_column/operator/value. Given the migration requirement, add a regression test that old params still normalize into equivalent ColumnFilterData.
🤖 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 `@apps/web/tests/test_datastructures.py` around lines 35 - 48, A regression
test for the legacy filter parameter format is missing. Add a new test function
(following the same pattern as test_filter_params_parse_new_format) that
validates parsing of the old format using filter_{i}_column,
filter_{i}_operator, and filter_{i}_value parameters. Construct a QueryDict with
legacy-format parameters (e.g., filter_0_column, filter_0_operator,
filter_0_value) and verify that the FilterParams object parses them correctly
and produces ColumnFilterData objects with the same column, operator, and value
attributes as the new f_*/op_* format test expects, ensuring backward
compatibility across both parameter formats.
| parseCSVTildeValue(csvString) { | ||
| // Parse tilde-separated values, respecting quoted strings | ||
| const values = []; | ||
| let current = ''; | ||
| let inQuotes = false; | ||
|
|
||
| for (let i = 0; i < csvString.length; i++) { | ||
| const char = csvString[i]; | ||
| if (char === '"') { | ||
| inQuotes = !inQuotes; | ||
| } else if (char === '~' && !inQuotes) { | ||
| values.push(current.replace(/^"|"$/g, '')); | ||
| current = ''; | ||
| } else { | ||
| current += char; | ||
| } | ||
| } | ||
| if (current) { | ||
| values.push(current.replace(/^"|"$/g, '')); | ||
| } | ||
| return values; | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and examine the file containing the code
find . -name "_filter_component_alpine.html" -type fRepository: dimagi/open-chat-studio
Length of output: 117
🏁 Script executed:
# Check the actual implementation at the specified lines
if [ -f "templates/filters/_filter_component_alpine.html" ]; then
sed -n '206,227p' "templates/filters/_filter_component_alpine.html"
fiRepository: dimagi/open-chat-studio
Length of output: 731
🏁 Script executed:
# Also check the serializeCSVTildeValues function around lines 327-336
if [ -f "templates/filters/_filter_component_alpine.html" ]; then
sed -n '327,336p' "templates/filters/_filter_component_alpine.html"
fiRepository: dimagi/open-chat-studio
Length of output: 409
🏁 Script executed:
# Check the Python backend implementation mentioned in the comment
find . -path "*apps/web/dynamic_filters/datastructures.py" -type fRepository: dimagi/open-chat-studio
Length of output: 112
🏁 Script executed:
# Search for any usage of these parsing functions to understand the data flow
rg "parseCSVTildeValue|serializeCSVTildeValues" -A 3 -B 3Repository: dimagi/open-chat-studio
Length of output: 3277
🏁 Script executed:
# Examine the Python backend CSV handling
sed -n '1,50p' "./apps/web/dynamic_filters/datastructures.py"Repository: dimagi/open-chat-studio
Length of output: 1742
🏁 Script executed:
# Search for csv usage in the Python file
rg "csv\.|reader|writer" "./apps/web/dynamic_filters/datastructures.py" -A 5 -B 5Repository: dimagi/open-chat-studio
Length of output: 1386
🏁 Script executed:
# Check how the filters are processed in the backend
rg "f_" "./apps/web/dynamic_filters/datastructures.py" -A 3 -B 3 | head -60Repository: dimagi/open-chat-studio
Length of output: 990
🏁 Script executed:
# Look at the entire datastructures.py to understand the parsing logic
wc -l "./apps/web/dynamic_filters/datastructures.py"Repository: dimagi/open-chat-studio
Length of output: 116
🏁 Script executed:
# Read the datastructures.py file to understand the CSV handling
cat -n "./apps/web/dynamic_filters/datastructures.py"Repository: dimagi/open-chat-studio
Length of output: 5150
CSV tilde parsing/serialization is not round-trip safe for quoted values.
The JavaScript parser (lines 214–216) toggles quote state on every " character, while the Python backend uses csv.reader/csv.writer with RFC 4180 semantics, which uses doubled quotes ("") to escape quotes inside quoted fields. The serializer escapes quotes as \" (line 332), which is also incompatible with RFC CSV. This causes data corruption when values contain quotes and tildes.
Example: A Python-serialized value "foo""bar"~baz (where foo"bar is the first field) is incorrectly parsed by the JavaScript parser because the quote-toggle logic desynchronizes at the doubled quote.
The proposed fix correctly implements RFC CSV quoting with doubled quotes for both parsing and serialization.
🤖 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 `@templates/filters/_filter_component_alpine.html` around lines 206 - 227, The
parseCSVTildeValue function uses simple quote toggling on each `"` character,
which is incompatible with RFC 4180 CSV semantics where quotes inside quoted
fields are escaped by doubling them (`""`). Update parseCSVTildeValue to
properly handle doubled quotes as escape sequences rather than quote toggles, so
that a value like `"foo""bar"` correctly represents `foo"bar`. Additionally,
update the serializer (around line 332) to escape quotes by doubling them (`""`)
instead of using backslash escaping (`\"`), ensuring round-trip compatibility
with the Python backend's csv.reader/csv.writer implementation.
| for (const columnName of Array.from(newFormatColumns).sort()) { | ||
| const opKey = `op_${columnName}`; | ||
| if (urlParams.has(`f_${columnName}`) && urlParams.has(opKey)) { | ||
| const valueParam = urlParams.get(`f_${columnName}`); | ||
| const operatorParam = urlParams.get(opKey); | ||
|
|
||
| const filterIndex = Object.keys(filters).length - Array.from(newFormatColumns).length + newFormatIndex; | ||
| if (filterIndex >= 0 && filterIndex < filters.length) { | ||
| const filter = filters[filterIndex]; |
There was a problem hiding this comment.
Index computation can discard valid filters when partial f_* params are present.
Line 277 subtracts newFormatColumns.length (all detected f_* keys), not the count of valid f_+op_ pairs. If one malformed f_* key exists, valid filters can get a negative filterIndex and be skipped.
🤖 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 `@templates/filters/_filter_component_alpine.html` around lines 271 - 279, The
filterIndex computation on line 277 subtracts newFormatColumns.length, which
counts all detected f_* keys regardless of whether they have a corresponding
op_* pair. When a malformed f_* key exists without its op_* counterpart, this
causes the filterIndex calculation to be incorrect and skip valid filters. Fix
this by counting only the valid filter pairs (those where both
urlParams.has(`f_${columnName}`) and urlParams.has(opKey) are true) and using
that count instead of newFormatColumns.length in the filterIndex subtraction.
…ps://github.com/brian-mecha/open-chat-studio into brianmecha-feat/re-design-filter-url-parameters
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
…tial f_* params are present.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
SmittieC
left a comment
SmittieC
left a comment
There was a problem hiding this comment.
Generally looks good to me, thanks! I do think we need a data migration to update saved filters so they use the new format
| def from_request(cls, request) -> Self: | ||
| query_params = request.GET | ||
| if not any(key.startswith("filter_") for key in query_params): | ||
| if not any(key.startswith(("filter_", "f_")) for key in query_params): |
There was a problem hiding this comment.
I think we can drop the filter_ entry
| loadFiltersFromQueryString(queryString) { | ||
| const urlParams = new URLSearchParams(queryString); | ||
| const filters = []; | ||
| const newFormatColumns = new Set(); |
There was a problem hiding this comment.
nit: references to the "old" format only creates unnecessary clutter that needs cleanup down the line. You can just call this columns
| @@ -287,10 +359,15 @@ | |||
| params.delete(key); | |||
There was a problem hiding this comment.
any reason to keep this part around?
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
SmittieC
left a comment
SmittieC
left a comment
There was a problem hiding this comment.
Almost there. We just need to update FilterSet.filter_query_string to the new format as well
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
|
||
|
|
||
| def migrate_saved_filters(apps, schema_editor): | ||
| DashboardFilter = apps.get_model("dashboard", "DashboardFilter") |
There was a problem hiding this comment.
DashboarFilter doesn't use the same filter params that FilterSet does, so you should only be updating FilterSet in here. (We want to make the dashboard use the same at some time)
|
A few more things beyond my inline comments worth looking into (credit to fable 5):
|
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| for filter_obj in DashboardFilter.objects.all(): | ||
| if not isinstance(filter_obj.filter_data, dict): | ||
| FilterSet = apps.get_model("filters", "FilterSet") | ||
| for filter_set in FilterSet.objects.all(): |
There was a problem hiding this comment.
For data migrations where we iterate like this, prefer bulk fetching them instead. See an example here. Same with migrate_auto_population_filter_query_strings
| if not params.filters: | ||
| continue | ||
|
|
||
| rule.filter_query_string = params.to_query() |
There was a problem hiding this comment.
The string conversion is still needed, no?
There was a problem hiding this comment.
@brian-mecha this question still stands. This doesn't seem to do a proper conversion?
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
Note This comment was written by Claude (on behalf of @SmittieC) after reviewing the branch. JSON-array filter values are misinterpreted by the new normalizer, breaking two in-tree filter producers.
Most of the currently failing filter tests ( Suggested fix: either convert both producers to the new tilde format, or make |
|
Note This comment was written by Claude (on behalf of @SmittieC) after reviewing the branch. Two more issues, both consequences of legacy 1. The evaluations data migration is a no-op — saved auto-population rules are never migrated.
params = FilterParams(QueryDict(raw_query))
...
if not params.filters:
continueBut Suggested fix: give the migration its own self-contained legacy parser instead of importing live app code (this is exactly why migrations shouldn't depend on application code — the app code moved on before the migration ran). Alternatively, restoring legacy parsing in 2. "Create dataset" still generates legacy params the backend now ignores.
params.set(`filter_${filterIndex}_column`, 'experiment');
params.set(`filter_${filterIndex}_operator`, 'any of');
params.set(`filter_${filterIndex}_value`, JSON.stringify([experimentId]));The backend discards these, so the dataset-creation flow silently loses its experiment scoping. This needs converting to |
There was a problem hiding this comment.
Gates Failed
(4 files with Bumpy Road Ahead, Deep, Nested Complexity)
(4 files with Complex Method)
Our agent can fix these. Install it.
Gates Passed
Reason for failure
| Enforce critical code health rules | Violations | Code Health Impact | |
|---|---|---|---|
| 0016_migrate_auto_population_filter_query_strings.py | 2 critical rules | 8.43 | Suppress |
| filter_format.py | 2 critical rules | 8.78 | Suppress |
| datastructures.py | 2 critical rules | 9.54 → 8.55 | Suppress |
| 0002_migrate_saved_filters_format.py | 1 critical rule | 9.54 | Suppress |
| Enforce advisory code health rules | Violations | Code Health Impact | |
|---|---|---|---|
| 0016_migrate_auto_population_filter_query_strings.py | 1 advisory rule | 8.43 | Suppress |
| filter_format.py | 1 advisory rule | 8.78 | Suppress |
| datastructures.py | 1 advisory rule | 9.54 → 8.55 | Suppress |
| 0002_migrate_saved_filters_format.py | 1 advisory rule | 9.54 | Suppress |
Quality Gate Profile: Clean Code Collective
Install CodeScene MCP: safeguard and uplift AI-generated code. Catch issues early with our IDE extension and CLI tool.
| @model_validator(mode="after") | ||
| def _normalize_list_value(self) -> Self: | ||
| """Wrap bare strings in a JSON array for operators that expect lists.""" | ||
| """Normalize list-based filter values to JSON arrays for operators that expect lists.""" |
There was a problem hiding this comment.
❌ New issue: Complex Method
_normalize_list_value has a cyclomatic complexity of 9, threshold = 9
| @model_validator(mode="after") | ||
| def _normalize_list_value(self) -> Self: | ||
| """Wrap bare strings in a JSON array for operators that expect lists.""" | ||
| """Normalize list-based filter values to JSON arrays for operators that expect lists.""" |
There was a problem hiding this comment.
❌ New issue: Deep, Nested Complexity
_normalize_list_value has a nested complexity depth of 4, threshold = 4
| def _convert_legacy_filter_query_string_to_new_format(raw_query: str) -> str: | ||
| query_params = QueryDict(raw_query) | ||
| legacy_filter_data = { | ||
| key: values[0] if len(values) == 1 else values for key, values in query_params.lists() | ||
| } | ||
|
|
||
| legacy_filters = [] | ||
| for key, value in legacy_filter_data.items(): | ||
| match = re.fullmatch(r"filter_(\d+)_((?:column|operator|value))", key) | ||
| if not match: | ||
| continue | ||
|
|
||
| index = int(match.group(1)) | ||
| field = match.group(2) | ||
|
|
||
| while len(legacy_filters) <= index: | ||
| legacy_filters.append({}) | ||
|
|
||
| legacy_filters[index][field] = value | ||
|
|
||
| converted = {} | ||
| for legacy_filter in legacy_filters: | ||
| column_name = legacy_filter.get("column") | ||
| if not column_name: | ||
| continue | ||
|
|
||
| if "value" in legacy_filter: | ||
| value = legacy_filter["value"] | ||
| if isinstance(value, str): | ||
| try: | ||
| parsed = json.loads(value) | ||
| except (TypeError, json.JSONDecodeError): | ||
| parsed = None | ||
|
|
||
| if isinstance(parsed, list): | ||
| value = "~".join(str(item) for item in parsed) | ||
| elif value is not None and not isinstance(value, str): | ||
| value = str(value) | ||
| elif isinstance(value, list): | ||
| value = "~".join(str(item) for item in value) | ||
| elif value is not None: | ||
| value = str(value) | ||
|
|
||
| converted[f"f_{column_name}"] = value | ||
|
|
||
| if "operator" in legacy_filter: | ||
| converted[f"op_{column_name}"] = legacy_filter["operator"] | ||
|
|
||
| return urlencode(converted) |
There was a problem hiding this comment.
❌ New issue: Complex Method
_convert_legacy_filter_query_string_to_new_format has a cyclomatic complexity of 18, threshold = 9
| def migrate_auto_population_filter_query_strings(apps, schema_editor): | ||
| DatasetAutoPopulationRule = apps.get_model("evaluations", "DatasetAutoPopulationRule") | ||
| batch_size = 500 | ||
| rules_to_update = [] | ||
|
|
||
| for rule in DatasetAutoPopulationRule.objects.all().iterator(chunk_size=batch_size): | ||
| raw_query = rule.filter_query_string or "" | ||
| if not raw_query: | ||
| continue | ||
|
|
||
| if raw_query.startswith(("f_", "op_")): | ||
| continue | ||
|
|
||
| converted_query_string = _convert_legacy_filter_query_string_to_new_format(raw_query) | ||
| if converted_query_string and converted_query_string != raw_query: | ||
| rule.filter_query_string = converted_query_string | ||
| rules_to_update.append(rule) | ||
| if len(rules_to_update) >= batch_size: | ||
| DatasetAutoPopulationRule.objects.bulk_update(rules_to_update, ["filter_query_string"]) | ||
| rules_to_update.clear() | ||
|
|
||
| if rules_to_update: | ||
| DatasetAutoPopulationRule.objects.bulk_update(rules_to_update, ["filter_query_string"]) |
There was a problem hiding this comment.
❌ New issue: Complex Method
migrate_auto_population_filter_query_strings has a cyclomatic complexity of 9, threshold = 9
| def _convert_legacy_filter_query_string_to_new_format(raw_query: str) -> str: | ||
| query_params = QueryDict(raw_query) | ||
| legacy_filter_data = { | ||
| key: values[0] if len(values) == 1 else values for key, values in query_params.lists() | ||
| } | ||
|
|
||
| legacy_filters = [] | ||
| for key, value in legacy_filter_data.items(): | ||
| match = re.fullmatch(r"filter_(\d+)_((?:column|operator|value))", key) | ||
| if not match: | ||
| continue | ||
|
|
||
| index = int(match.group(1)) | ||
| field = match.group(2) | ||
|
|
||
| while len(legacy_filters) <= index: | ||
| legacy_filters.append({}) | ||
|
|
||
| legacy_filters[index][field] = value | ||
|
|
||
| converted = {} | ||
| for legacy_filter in legacy_filters: | ||
| column_name = legacy_filter.get("column") | ||
| if not column_name: | ||
| continue | ||
|
|
||
| if "value" in legacy_filter: | ||
| value = legacy_filter["value"] | ||
| if isinstance(value, str): | ||
| try: | ||
| parsed = json.loads(value) | ||
| except (TypeError, json.JSONDecodeError): | ||
| parsed = None | ||
|
|
||
| if isinstance(parsed, list): | ||
| value = "~".join(str(item) for item in parsed) | ||
| elif value is not None and not isinstance(value, str): | ||
| value = str(value) | ||
| elif isinstance(value, list): | ||
| value = "~".join(str(item) for item in value) | ||
| elif value is not None: | ||
| value = str(value) | ||
|
|
||
| converted[f"f_{column_name}"] = value | ||
|
|
||
| if "operator" in legacy_filter: | ||
| converted[f"op_{column_name}"] = legacy_filter["operator"] | ||
|
|
||
| return urlencode(converted) |
There was a problem hiding this comment.
❌ New issue: Bumpy Road Ahead
_convert_legacy_filter_query_string_to_new_format has 3 blocks with nested conditional logic. Any nesting of 2 or deeper is considered. Threshold is 2 blocks per function
| def migrate_auto_population_filter_query_strings(apps, schema_editor): | ||
| DatasetAutoPopulationRule = apps.get_model("evaluations", "DatasetAutoPopulationRule") | ||
| batch_size = 500 | ||
| rules_to_update = [] | ||
|
|
||
| for rule in DatasetAutoPopulationRule.objects.all().iterator(chunk_size=batch_size): | ||
| raw_query = rule.filter_query_string or "" | ||
| if not raw_query: | ||
| continue | ||
|
|
||
| if raw_query.startswith(("f_", "op_")): | ||
| continue | ||
|
|
||
| converted_query_string = _convert_legacy_filter_query_string_to_new_format(raw_query) | ||
| if converted_query_string and converted_query_string != raw_query: | ||
| rule.filter_query_string = converted_query_string | ||
| rules_to_update.append(rule) | ||
| if len(rules_to_update) >= batch_size: | ||
| DatasetAutoPopulationRule.objects.bulk_update(rules_to_update, ["filter_query_string"]) | ||
| rules_to_update.clear() | ||
|
|
||
| if rules_to_update: | ||
| DatasetAutoPopulationRule.objects.bulk_update(rules_to_update, ["filter_query_string"]) |
There was a problem hiding this comment.
❌ New issue: Bumpy Road Ahead
migrate_auto_population_filter_query_strings has 2 blocks with nested conditional logic. Any nesting of 2 or deeper is considered. Threshold is 2 blocks per function
| def _convert_legacy_filter_query_string_to_new_format(raw_query: str) -> str: | ||
| query_params = QueryDict(raw_query) | ||
| legacy_filter_data = { | ||
| key: values[0] if len(values) == 1 else values for key, values in query_params.lists() | ||
| } | ||
|
|
||
| legacy_filters = [] | ||
| for key, value in legacy_filter_data.items(): | ||
| match = re.fullmatch(r"filter_(\d+)_((?:column|operator|value))", key) | ||
| if not match: | ||
| continue | ||
|
|
||
| index = int(match.group(1)) | ||
| field = match.group(2) | ||
|
|
||
| while len(legacy_filters) <= index: | ||
| legacy_filters.append({}) | ||
|
|
||
| legacy_filters[index][field] = value | ||
|
|
||
| converted = {} | ||
| for legacy_filter in legacy_filters: | ||
| column_name = legacy_filter.get("column") | ||
| if not column_name: | ||
| continue | ||
|
|
||
| if "value" in legacy_filter: | ||
| value = legacy_filter["value"] | ||
| if isinstance(value, str): | ||
| try: | ||
| parsed = json.loads(value) | ||
| except (TypeError, json.JSONDecodeError): | ||
| parsed = None | ||
|
|
||
| if isinstance(parsed, list): | ||
| value = "~".join(str(item) for item in parsed) | ||
| elif value is not None and not isinstance(value, str): | ||
| value = str(value) | ||
| elif isinstance(value, list): | ||
| value = "~".join(str(item) for item in value) | ||
| elif value is not None: | ||
| value = str(value) | ||
|
|
||
| converted[f"f_{column_name}"] = value | ||
|
|
||
| if "operator" in legacy_filter: | ||
| converted[f"op_{column_name}"] = legacy_filter["operator"] | ||
|
|
||
| return urlencode(converted) |
There was a problem hiding this comment.
❌ New issue: Deep, Nested Complexity
_convert_legacy_filter_query_string_to_new_format has a nested complexity depth of 4, threshold = 4
2 participants
Product Description
This new URL parameters format uses more native URL parameters rather than JSON encoding and does not separate the column name and value. The operator is kept separate to avoid repetition when there are multiple values being filtered
Technical Description
This change introduces a new URL filter parameter format designed to reduce URL length and improve readability. The previous filter_N_column, filter_N_operator, and filter_N_value structure has been replaced with column-based parameters using the format f_{column} for values and op_{column} for operators.
The new format supports both single and multi-value filters, with multiple values represented as a ~-separated list. Values containing the separator character are enclosed in double quotes. The implementation maintains backward compatibility by supporting both legacy and new filter formats during the migration period.
Old format
New format
Demo
Docs and Changelog
Resolves 2023