fix: tolerate unparseable individual records in Okta list responses - #108
Open
jackvaughanjr wants to merge 2 commits into
Open
fix: tolerate unparseable individual records in Okta list responses#108jackvaughanjr wants to merge 2 commits into
jackvaughanjr wants to merge 2 commits into
Conversation
Consolidates every SDK model compatibility patch into one module,
okta_mcp_server.utils.okta_compat, applied at import time.
Covers four defects where a generated Pydantic model is stricter than
the API it describes, each of which aborts an entire response:
A SamlApplicationSettingsSignOn: five required StrictBool fields the
API omits on apps created from OIN catalog templates.
B Policy.embedded: typed Dict[str, Dict[str, Any]], but Okta returns
scalars inside _embedded (e.g. {"resourceType": "APP"}).
C AuthenticatorEnrollmentPolicyAuthenticatorSettings.key: closed enum
missing smart_card_idp, and brittle to any future authenticator.
D UserTypeCondition.exclude/include: required lists where Okta sends
null. Also RiskDetectionTypesPolicyRuleCondition and
UserIdentifierPolicyRuleCondition.
Also migrates the existing LogSecurityContext.user_behaviors patch out
of system_logs.py so all SDK compatibility lives in one place.
Supersedes the inline patches in PR okta#102 (issues okta#100, okta#101).
Upstream: okta/okta-sdk-python#546 (A), #572 (D); okta#100
(B), okta#101 (C), okta#48 (A).
Regression fixtures are synthetic: example.invalid hosts and fake IDs.
The SDK deserializes a list response with a bare per-item comprehension in ApiClient.__deserialize and no per-element guard, so one record that fails Pydantic validation aborts the entire page. For audit and inventory work, partial results with a clear warning beat a hard failure. Installs a per-item tolerant parse at that seam. Successful records are returned in items; unparseable ones are collected in a warnings array carrying the model name, the validation error, and the raw payload, and surfaced through json_response. Warnings are held in a ContextVar so collection is per-request and async-safe. Set OKTA_MCP_STRICT_DESERIALIZATION=1 to restore the current fail-the-whole-page behavior. Patching the SDK seam rather than the individual tool functions means this covers every list endpoint and does not require re-implementing the SDK's snake_case to camelCase query-param mapping, which a per-tool approach would. Relates to okta#48. Stacked on the SDK model compat branch; that commit is included here so the suite runs, and should be reviewed separately.
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.
Addresses the general form of #48.
The problem
ApiClient.__deserializebuilds a list response with a bare comprehension and no per-element guard:One record that fails Pydantic validation raises out through the whole call. The caller gets an error string instead of the page, with no indication that other valid records exist. #107 fixes the four models known to trigger this, but the models live in a pinned dependency generated from a spec that keeps drifting, so the next addition breaks things again.
For audit and inventory work, where the question is "show me everything you can see", partial results with a clear warning are more useful than a hard failure.
Approach
Per-item parsing installed at that seam. Successful records go into
items; unparseable ones are collected into awarningsarray carrying the model name, the validation error, and the raw payload, then surfaced throughjson_response. Warnings live in aContextVar, so collection is per-request and async-safe.OKTA_MCP_STRICT_DESERIALIZATION=1restores the current fail-the-whole-page behavior for callers who want it.Why the SDK seam rather than the tool functions. #64 and #85 both approach this by bypassing the typed client and calling the request executor directly, which forces them to re-implement the SDK's snake_case to camelCase query-param mapping in a local
_camel_case_param()helper. That is a permanent maintenance cost and it only covers applications. Patching__deserializecovers every list endpoint and leaves query-param handling to the SDK.Why not blanket-relax the models. #86 makes every field on
Applicationoptional and setsextra="allow". That does stop the crash, but it also removes the ability to detect a genuinely malformed response. The intent here is targeted patches for known defects (#107) plus a general net for unknown ones (this PR), so validation still means something.Scope is list responses only. Single-object endpoints are covered by the model patches in #107.
Testing
26 tests. The tolerant-path test uses a record broken in a way #107 does not fix, so it cannot pass accidentally. Covers: good records returned alongside a bad one, the warning carrying the raw payload, strict mode restoring the raise, and
json_responseattachingwarningswithout clobbering an existing key.Full suite: 581 passed, up from 529 on
main(26 here, 26 from #107).Note on
total_fetchedIt now means "records successfully returned", which stays consistent with
items. Dropped records are counted inwarnings.has_moreandnext_cursorcome from response headers and are unaffected. Documented increate_paginated_response.