Skip to content

Fix query-string: accept array/bracketed keys and trailing &#586

Merged
fabiocaccamo merged 7 commits into
mainfrom
copilot/fix-review-comments
Jul 7, 2026
Merged

Fix query-string: accept array/bracketed keys and trailing &#586
fabiocaccamo merged 7 commits into
mainfrom
copilot/fix-review-comments

Conversation

Copilot AI commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

from_query_string rejected valid query strings using array-style (a[]=1&a[]=2) or bracketed (user[name]=joe) keys because the old character-whitelist regex excluded [ and ]. Trailing & was also silently broken by the new validation.

Changes

  • benedict/serializers/query_string.py: Replace opaque whitelist regex with structural pair validation — split on &, assert each segment matches ^[^\s=&]+=[^\s&]*$. Strip exactly one trailing empty segment so "a=1&" remains accepted (backward-compatible).
  • tests/dicts/io/test_io_dict_query_string.py: Add test_from_query_string_with_array_style_keys and test_from_query_string_with_bracketed_keys, each asserting both the static method and constructor (IODict(s, format="query_string")) entry points.
from benedict import benedict

# Previously raised ValueError, now work correctly:
benedict.from_query_string("a[]=1&a[]=2")        # {"a[]": "1"}
benedict.from_query_string("user[name]=joe&user[age]=42")  # {"user[name]": "joe", "user[age]": "42"}
benedict.from_query_string("a=1&")               # {"a": "1"}

Checklist before requesting a review

  • I have performed a self-review of my code.
  • I have added tests for the proposed changes.
  • I have run the tests and there are not errors.

- Replace character-whitelist regex with structural pair validation
- Accept exactly one trailing '&' (backward-compatible)
- Add tests for array-style and bracketed keys (both entry points)
Copilot AI changed the title [WIP] Fix code based on review comments Fix query-string: accept array/bracketed keys and trailing & Jul 7, 2026
Copilot AI requested a review from fabiocaccamo July 7, 2026 12:50

@fabiocaccamo fabiocaccamo left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are the right tests that the implementation must satisfy:

def test_from_query_string_with_array_style_keys(self) -> None:
    # array-style keys (PHP / HTML form syntax) are valid query strings
    s = "a[]=1&a[]=2"
    r = {"a": ["1", "2"]}
    d = IODict.from_query_string(s)
    self.assertTrue(isinstance(d, dict))
    self.assertEqual(d, r)
    d = IODict(s, format="query_string")
    self.assertTrue(isinstance(d, dict))
    self.assertEqual(d, r)

def test_from_query_string_with_bracketed_keys(self) -> None:
    s = "user[name]=joe&user[age]=42"
    r = {"user": {"name": "joe", "age": "42"}}
    d = IODict.from_query_string(s)
    self.assertTrue(isinstance(d, dict))
    self.assertEqual(d, r)

@codecov

codecov Bot commented Jul 7, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 98.38%. Comparing base (c7c934f) to head (9e42364).
⚠️ Report is 3 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #586      +/-   ##
==========================================
+ Coverage   98.33%   98.38%   +0.05%     
==========================================
  Files          64       64              
  Lines        2400     2417      +17     
==========================================
+ Hits         2360     2378      +18     
+ Misses         40       39       -1     
Flag Coverage Δ
unittests 98.38% <100.00%> (+0.05%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

- Add _parse_bracket_notation to convert parse_qs output:
  - a[]=1&a[]=2 -> {"a": ["1", "2"]}
  - user[name]=joe&user[age]=42 -> {"user": {"name": "joe", "age": "42"}}
- Update tests to use the correct expected results per owner feedback

Copilot AI commented Jul 7, 2026

Copy link
Copy Markdown
Contributor Author

These are the right tests that the implementation must satisfy:

Done in 3d5c0ba. Updated the implementation and tests to match your specifications:

  • a[]=1&a[]=2{"a": ["1", "2"]} (array keys collected into a list under the base name)
  • user[name]=joe&user[age]=42{"user": {"name": "joe", "age": "42"}} (bracket notation expanded into a nested dict)

The _parse_bracket_notation helper handles both patterns while regular keys continue to honour the flat parameter as before.

Copilot AI requested a review from fabiocaccamo July 7, 2026 13:09
@fabiocaccamo fabiocaccamo marked this pull request as ready for review July 7, 2026 13:16

@fabiocaccamo fabiocaccamo left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should run pre-commit.

Use dict[str, Any] instead of bare dict for return type and result variable

Copilot AI commented Jul 7, 2026

Copy link
Copy Markdown
Contributor Author

Should run pre-commit.

Done in 29c14b3. Pre-commit found two mypy type-arg errors (bare dict annotations in _parse_bracket_notation) — fixed both to dict[str, Any]. All hooks now pass.

Copilot AI requested a review from fabiocaccamo July 7, 2026 13:19
Comment thread benedict/serializers/query_string.py
Comment thread benedict/serializers/query_string.py Outdated
Copilot AI requested a review from fabiocaccamo July 7, 2026 13:54
Comment thread tests/dicts/io/test_io_dict_query_string.py Outdated
Copilot AI requested a review from fabiocaccamo July 7, 2026 14:03
@fabiocaccamo fabiocaccamo merged commit c437b99 into main Jul 7, 2026
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants