Strict params - raise on unknown param provided to san functions#202
Merged
Conversation
📝 WalkthroughWalkthroughThe PR introduces strict keyword argument validation for API functions ( Changes
Sequence DiagramsequenceDiagram
participant User
participant APIFunction as API Function<br/>(san.get, etc.)
participant Validator as validate_kwargs
participant Config as ApiConfig
participant Error as SanError
User->>APIFunction: Call with **kwargs
APIFunction->>Validator: validate_kwargs(func_name, kwargs)
Validator->>Config: Check strict_kwargs setting
Config-->>Validator: strict_kwargs value
alt strict_kwargs == True
Validator->>Validator: Check for unsupported kwargs
alt Unsupported kwargs found
Validator->>Error: Raise SanError with details
Error-->>User: Exception
else All kwargs supported
Validator-->>APIFunction: Validation passed
APIFunction->>APIFunction: Continue processing
APIFunction-->>User: Return results
end
else strict_kwargs == False
Validator-->>APIFunction: No validation
APIFunction->>APIFunction: Continue processing
APIFunction-->>User: Return results
end
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested Reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![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
♻️ Duplicate comments (1)
README.md (1)
148-151:⚠️ Potential issue | 🟡 MinorVersion string and covered surface.
Two items here, already raised against the corresponding source files:
- The version tag (
v0.13.0) disagrees with the0.14.0bump insetup.py.Batch.getalso raises on unknown kwargs but isn't listed.Also worth documenting (even briefly) the undocumented
ApiConfig.strict_kwargs = Falseescape hatch mentioned in the PR description, so users hitting an erroneous rejection have a supported recovery path without reading source.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 148 - 151, Update the README note to match the actual package version and covered APIs: change the version tag from `v0.13.0` to `v0.14.0`, add `Batch.get` (in addition to `AsyncBatch.get`) and `Batch.get_many` to the list of methods that now raise `SanError` on unknown kwargs, and briefly document the escape hatch by mentioning `ApiConfig.strict_kwargs = False` as a supported way to bypass strict keyword validation; reference the README note text, the `Batch.get`/`Batch.get_many` and `AsyncBatch.get`/`.get_many` symbols, and `ApiConfig.strict_kwargs` so maintainers can locate and update the exact lines.
🧹 Nitpick comments (3)
san/sanbase_graphql.py (1)
290-297: LGTM, with one subtle consideration aroundkwargs.pop.The
isinstance(value, bool)check correctly rejects ints/strings (bool is a subclass of int, so order matters — this is right).One thing to confirm:
_only_finalized_data_arg_helpermutates the incomingkwargsdict viapop. Current callers (get_metric_timeseries_data,get_metric_timeseries_data_per_slug) receivekwargsvia**kwargsunpacking, so each call gets a fresh dict — safe. If a future refactor ever passes a shared dict by reference (e.g., caching/retry logic), the second call would silently seeNone. Consider either documenting the mutating contract in the helper's name/docstring or reading without popping and lettingtransform_query_argsdrop it. Non-blocking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@san/sanbase_graphql.py` around lines 290 - 297, The helper _only_finalized_data_arg_helper currently mutates the incoming kwargs by using kwargs.pop which is safe now because callers get_metric_timeseries_data and get_metric_timeseries_data_per_slug pass **kwargs (fresh dicts) but could lead to subtle bugs if a shared dict is ever reused; either document the mutation clearly in the helper name/docstring (e.g., note it removes "only_finalized_data") or change the helper to read without popping (use kwargs.get("only_finalized_data", None)) and let transform_query_args or the caller handle removal, updating references to _only_finalized_data_arg_helper accordingly.san/tests/test_get.py (1)
446-455: Usemonkeypatchto restorestrict_kwargsinstead of try/finally.The manual try/finally works, but
monkeypatch.setattr(san.ApiConfig, "strict_kwargs", False)would make the intent clearer and guarantee restoration even ifApiConfig.strict_kwargshad been left in a non-default state by an earlier test failure.Proposed refactor
`@patch`("san.transport.requests.Session.post") -def test_strict_kwargs_disabled_allows_unknown(mock, test_response): +def test_strict_kwargs_disabled_allows_unknown(mock, test_response, monkeypatch): api_call_result = {"query_0": {"timeseriesDataJson": []}} mock.return_value = test_response(status_code=200, data=deepcopy(api_call_result)) - san.ApiConfig.strict_kwargs = False - try: - san.get("price_usd", slug="bitcoin", from_date="2026-01-01", to_date="2026-01-02", vlkajsdsakd=123) - finally: - san.ApiConfig.strict_kwargs = True + monkeypatch.setattr(san.ApiConfig, "strict_kwargs", False) + san.get("price_usd", slug="bitcoin", from_date="2026-01-01", to_date="2026-01-02", vlkajsdsakd=123)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@san/tests/test_get.py` around lines 446 - 455, Replace the manual try/finally that toggles san.ApiConfig.strict_kwargs in test_strict_kwargs_disabled_allows_unknown with monkeypatch.setattr to both set and ensure restoration; update the test signature to accept the pytest monkeypatch fixture, call monkeypatch.setattr(san.ApiConfig, "strict_kwargs", False) before invoking san.get("price_usd", ...), and remove the try/finally so the fixture guarantees restoration even if earlier tests left a non-default state.san/param_validation.py (1)
39-48: Minor: error message wording mismatch between message and "Supported parameters" label.The message says
"received unsupported parameter(s):"(with(s)) but"Supported parameters:"(no(s)). Purely cosmetic — tests match only on the"unsupported parameter"substring so this is safe to leave. Also consider sorting theunsupportedlist for determinism (already done viasorted(...)on line 36 ✓).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@san/param_validation.py` around lines 39 - 48, The error message is inconsistent: it uses "received unsupported parameter(s):" but "Supported parameters:"; update the string inside the SanError raised in the block that references func_name, unsupported and _PUBLIC_KWARGS so the wording is consistent (e.g., change "Supported parameters:" to "Supported parameter(s):" or remove "(s)" after "parameter" to make both plural), keeping the same .format substitutions (func=func_name, bad=", ".join(unsupported), ok=", ".join(sorted(_PUBLIC_KWARGS))) and preserve the deterministic sorting of unsupported if needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@san/batch.py`:
- Line 15: The README note about strict kwargs validation currently mentions
san.get, san.get_many, and AsyncBatch.get/ .get_many but omits that Batch.get
also calls validate_kwargs("Batch.get", kwargs) and will raise on unknown
kwargs; update the README text to either (a) add Batch.get to the list of
request entry points that perform strict validation, or (b) reword the note to
state that "all request entry points (san.get, san.get_many, Batch.get,
AsyncBatch.get/ get_many) validate kwargs via validate_kwargs" so users see
Batch.get's behavior when upgrading mixed codebases.
In `@san/param_validation.py`:
- Around line 4-36: The validator currently allows the internal-only "idx"
because validate_kwargs subtracts _SUPPORTED_KWARGS instead of the public-safe
set; change the membership check in validate_kwargs to compute unsupported =
sorted(set(kwargs) - _PUBLIC_KWARGS) so external callers are validated against
the public parameter set (keep ApiConfig.strict_kwargs logic and the existing
error message behavior that references supported params).
In `@setup.py`:
- Line 9: The package version in setup.py is set to "0.14.0" but README.md still
refers to the strict-kwargs change as v0.13.0; update one of them to keep
versions consistent — either change the README note (the text mentioning
"v0.13.0" around the strict-kwargs explanation) to "v0.14.0" or revert the
version field in setup.py (the version= assignment) to "0.13.0" so both files
match.
---
Duplicate comments:
In `@README.md`:
- Around line 148-151: Update the README note to match the actual package
version and covered APIs: change the version tag from `v0.13.0` to `v0.14.0`,
add `Batch.get` (in addition to `AsyncBatch.get`) and `Batch.get_many` to the
list of methods that now raise `SanError` on unknown kwargs, and briefly
document the escape hatch by mentioning `ApiConfig.strict_kwargs = False` as a
supported way to bypass strict keyword validation; reference the README note
text, the `Batch.get`/`Batch.get_many` and `AsyncBatch.get`/`.get_many` symbols,
and `ApiConfig.strict_kwargs` so maintainers can locate and update the exact
lines.
---
Nitpick comments:
In `@san/param_validation.py`:
- Around line 39-48: The error message is inconsistent: it uses "received
unsupported parameter(s):" but "Supported parameters:"; update the string inside
the SanError raised in the block that references func_name, unsupported and
_PUBLIC_KWARGS so the wording is consistent (e.g., change "Supported
parameters:" to "Supported parameter(s):" or remove "(s)" after "parameter" to
make both plural), keeping the same .format substitutions (func=func_name,
bad=", ".join(unsupported), ok=", ".join(sorted(_PUBLIC_KWARGS))) and preserve
the deterministic sorting of unsupported if needed.
In `@san/sanbase_graphql.py`:
- Around line 290-297: The helper _only_finalized_data_arg_helper currently
mutates the incoming kwargs by using kwargs.pop which is safe now because
callers get_metric_timeseries_data and get_metric_timeseries_data_per_slug pass
**kwargs (fresh dicts) but could lead to subtle bugs if a shared dict is ever
reused; either document the mutation clearly in the helper name/docstring (e.g.,
note it removes "only_finalized_data") or change the helper to read without
popping (use kwargs.get("only_finalized_data", None)) and let
transform_query_args or the caller handle removal, updating references to
_only_finalized_data_arg_helper accordingly.
In `@san/tests/test_get.py`:
- Around line 446-455: Replace the manual try/finally that toggles
san.ApiConfig.strict_kwargs in test_strict_kwargs_disabled_allows_unknown with
monkeypatch.setattr to both set and ensure restoration; update the test
signature to accept the pytest monkeypatch fixture, call
monkeypatch.setattr(san.ApiConfig, "strict_kwargs", False) before invoking
san.get("price_usd", ...), and remove the try/finally so the fixture guarantees
restoration even if earlier tests left a non-default state.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: e273da09-c334-4511-ab3d-62a925fc8b81
📒 Files selected for processing (10)
README.mdsan/api_config.pysan/async_batch.pysan/batch.pysan/get.pysan/get_many.pysan/param_validation.pysan/sanbase_graphql.pysan/tests/test_get.pysetup.py
createMonster
approved these changes
Apr 17, 2026
Contributor
createMonster
left a comment
createMonster
left a comment
There was a problem hiding this comment.
LGTM!
In the future maybe we can centralize kwarg validation in SanClient entrypoints instead of duplicating it across san.get / get_many / Batch / AsyncBatch
Draft that I had in mind: #200
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
2 participants
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
arguments instead of silently ignoring them, so typos and params from newer sanpy versions
surface immediately.
behavior.
Summary by CodeRabbit
Release Notes v0.14.0
New Features
only_finalized_dataparameter to limit returned metric data to finalized time-series points only.Changes