fix: normalize set_indexing mode (lowercase key) and empty target ([]) for Data API#414
Open
erichare wants to merge 3 commits into
Open
fix: normalize set_indexing mode (lowercase key) and empty target ([]) for Data API#414erichare wants to merge 3 commits into
erichare wants to merge 3 commits into
Conversation
CollectionDefinition.set_indexing computed a normalized `_i_target`
(indexing_target or []) but then built the returned definition from the
raw `indexing_target`. As a result, set_indexing("deny") / ("allow")
with no target stored {mode: None} instead of the intended {mode: []},
leaving `_i_target` as dead code.
as_dict() forwards this value verbatim into the request, so the
malformed {"indexing": {"deny": None}} reached the Data API, which
expects a list of paths. Wire the existing _i_target normalization into
the returned definition and add a unit test covering both modes.
set_indexing validates the indexing mode case-insensitively
(`_i_mode = indexing_mode.lower()`) but built the returned definition
from the raw `indexing_mode`. So set_indexing("DENY") / ("Allow")
passed validation yet stored {"DENY": ...} / {"Allow": ...} instead of
the intended lowercase {"deny": ...} / {"allow": ...}.
as_dict() forwards the key verbatim into the request, so the
unexpected-cased key reached the Data API, which expects "allow"/"deny".
Use the normalized `_i_mode` for the stored key and add a unit test
covering case normalization for both modes.
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
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.
Summary
CollectionDefinition.set_indexinghad two related normalization bugs that produced anindexingpayload the Data API does not expect.CollectionDefinition.as_dict()forwardsthe
indexingvalue verbatim (only outerNone/{}entries are filtered), so a malformeddict reaches the wire. The Data API expects
{"allow"|"deny": [<paths>]}— a lowercasemode key mapping to a list of paths (as the attribute's own docstring states).
Bug 1 — target not normalized (value)
It computes a normalized local
_i_target = indexing_target or [], but then builds thereturned definition from the raw
indexing_target:So
_i_targetwas dead code, andset_indexing("deny")/set_indexing("allow")with notarget stored
{mode: None}instead of the intended{mode: []}.Bug 2 — mode not normalized (key)
The mode is validated case-insensitively (
_i_mode = indexing_mode.lower(), checkedagainst
INDEXING_ALLOWED_MODES = {"allow", "deny"}), but the dict is then keyed by theraw
indexing_mode:So
set_indexing("DENY")/set_indexing("Allow")pass validation but store{"DENY": ...}/{"Allow": ...}instead of the lowercase{"deny": ...}/{"allow": ...}the Data API expects.
Fix
Construct the definition from the already-computed normalized locals:
indexing={_i_mode: _i_target}. Both fixes simply activate normalization that was alreadywritten (
.lower()andor []) but never wired into the returned object.Behavior
Before:
After:
Explicitly-provided targets are preserved; only their normalization (key case, empty value)
changes.
Notes
open PR. Same class of latent "normalize-then-discard" defect.
set_indexingconstructor arguments change.Test plan
test_set_indexing_empty_target_normalization—{"deny": []}/{"allow": []}forboth
.indexingand.as_dict(), plus an explicit-target guard.test_set_indexing_mode_case_normalization—"DENY"/"Allow"normalize tolowercase keys for both
.indexingand.as_dict(), including with an explicit target.uv run pytest tests/base/unit/test_collection_options.py— 4 passedruff check+ruff format --check(astrapy + tests) — cleanmypy astrapy tests— no issues found (214 source files)mainsection (covers both normalizations)