feat(adms): add SAP ADMS module with sync/async clients and BDD tests#138
Open
adwitiyasushant wants to merge 49 commits into
Open
feat(adms): add SAP ADMS module with sync/async clients and BDD tests#138adwitiyasushant wants to merge 49 commits into
adwitiyasushant wants to merge 49 commits into
Conversation
|
|
Adds a full-featured ADMS (Attachment Document Management Service) module to the SDK with sync and async clients, IAS X.509 token authentication, OData V4 service support (DocumentService, ConfigurationService, AdminService), and pytest-bdd integration tests with Gherkin scenarios. Also adds shared SDK building blocks consumed by ADMS: IAS token fetcher, mTLS support, async HTTP client, and the ADMS telemetry module entry.
Required by upstream's "Enforce version bump when src/ is modified" CI check.
- Revert global [tool.pytest.ini_options] integration marker description and remove asyncio_mode=auto so the change does not leak into other modules' test runs. - Extract /etc/secrets/appfnd and CLOUD_SDK_CFG as module-level constants in adms/config.py for consistency with the existing _SERVICE_PATH / _ADMIN_SERVICE_PATH constants.
Upstream's data-anonymization PR (SAP#93) also bumped to 0.20.0 — bumping to 0.20.1 to satisfy the version-bump CI check on the rebased branch.
adwitiyasushant
force-pushed
the
contrib/adms
branch
from
2a3f60e to
74a9909
Compare
Required for pytest-asyncio strict mode (the project default after asyncio_mode auto was scoped out). Matches the convention already in use in tests/agentgateway/.
…trings Leftover wording from the dms module template — exception, config, auth and http docstrings now correctly identify themselves as ADMS. No behavioural change.
ArthurTonial
requested changes
May 27, 2026
- Add ADMS placeholder block to .env_integration_tests.example so reviewers know which env vars to set for external/BTP mode. - Correct the env var names in conftest docstring and INTEGRATION_TESTS_ADMS.md: the loader expects CLIENTID / CLIENTSECRET / URL / URI (matching the IAS binding field names used by destination/), not the underscored CLIENT_ID / IAS_URL / SERVICE_URL variants — those would have failed with KeyError. - Add the optional RESOURCE entry (IAS resource URI) the docs were missing. - Rename leftover "DMS Integration Tests" heading to "ADMS Integration Tests".
- Remove unused re-exports from adms/_auth.py (_CC_CACHE_KEY, _GRANT_JWT_BEARER) - Make InMemoryTokenCache thread-safe via threading.Lock - Drop module-specific reference from core/http async client docstring - Delete unused core/http/_batch.py and its tests/BDD scenarios - Remove 5 pattern YAML stubs from docs/adms/patterns/ - Restore underscore names for internal API classes in adms unit tests
Per PR review: integration tests target real service instances, so the ADMS-specific guide duplicates the canonical doc. Merge env vars into docs/INTEGRATION_TESTS.md and delete docs/INTEGRATION_TESTS_ADMS.md.
Per PR review. Local-only test driver — kept on developer machines via .gitignore but no longer shipped with the SDK.
… pattern Per PR review: - Drop the special CLOUD_SDK_ADMS_INTEGRATION_URL block from the workflow. ADMS now flows through the existing CLOUD_SDK_CFG_* loader like every other module. - Rewrite tests/adms/integration/conftest.py to target real ADM instances only. Removes the local HDM Maven auto-start mode (~150 lines), the mock IasTokenFetcher, and the CLOUD_SDK_ADMS_SKIP_IF_UNAVAILABLE flag. - Skip the suite gracefully when load_from_env_or_mount() raises ConfigError because of missing credentials. - Update docs and .env_integration_tests.example to match.
The concurrent-create integration test built coroutines and called asyncio.gather() from sync code, binding the resulting future to the default event loop. The run_async fixture then ran it in a separate loop, raising "future belongs to a different loop" against a real ADM instance. Wrap the gather in an async helper, mirroring the existing cleanup_concurrent_async_relations pattern, so coroutines and gather share the run_async fixture's loop.
…example Per PR review. The 'resource' field example pointed to a personal IAS application name; replace with the generic 'my-adm-app'.
Per PR review. Replace internal-only "Unified Provisioning / UCL" and "BTP Fabric SDK Business Services TRA" references with the generic "BTP service instance" wording suitable for a public SDK. Also fix the stale "DMS" tag on the first line; the module is ADMS.
Per PR review. The defensive untyped parameter and "avoid circular import" comment were incorrect — config.py imports only from core and exceptions, no cycle exists. Add the AdmsConfig import and annotate the parameter properly.
Per PR review. HttpError was imported twice — once bare and once aliased as AdmsHttpError. Since CoreHttpError is already aliased on the line below, the bare HttpError name is unambiguous. Drop the duplicate alias and use HttpError consistently.
Per PR review.
Per PR review. Replace `from sap_cloud_sdk.core.auth._token_cache import TokenCache` with the public re-export `from sap_cloud_sdk.core.auth import TokenCache` in client.py and __init__.py to avoid reaching into the private `_token_cache` module.
- Expand "ADM Document Management Service" to "Advanced Document Management Service" in _models.py docstring (consistent with the rest of the module). - Drop SAP-internal "Unified Provisioning / UCL" wording in user-guide.md; use the public "BTP service instance" phrasing already used in __init__.py.
Per PR review. * PEP 8: rename `mTLSConfig` → `MTLSConfig` and `mTLSStrategy` → `MTLSStrategy`. Public-facing classes now follow the standard PascalCase rule. All callers, BDD scenarios, step definitions, error messages and test class names are updated. * Drop the unused `client: Optional[httpx.AsyncClient] = None` parameter from `MTLSStrategy.apply_to_async_client`. The previous signature documented the argument as "Ignored (kept for API symmetry)" — it was never honoured because httpx does not allow swapping the SSL context on an existing client. The misleading parameter is removed and the docstring rewritten accordingly. Module docstring example and BDD step are updated to call without the argument.
Per PR review. `_CC_CACHE_KEY` and `_GRANT_JWT_BEARER` are private constants of `_ias_fetcher` and should not appear in the public `sap_cloud_sdk.core.auth` namespace. Drop them from the import list and `__all__` in core/auth/__init__.py, and update tests/adms/unit/test_auth.py to import `_CC_CACHE_KEY` directly from the private module (matching the existing pattern in tests/core/unit/auth/test_ias_fetcher.py).
# Conflicts: # pyproject.toml # uv.lock
* tests/adms/integration/test_e2e_async_flow.py — bind narrowed context.client / context.bo_type_id to local variables before the nested `_gather` async closure. ty re-widens optional attributes inside nested closures, so the outer-function asserts no longer apply once the closure references them. Local-variable binding preserves narrowing. * pyproject.toml + uv.lock — bump version 0.21.1 → 0.22.0. Required by check-version-bump CI now that the merge with main has introduced src/ changes (new ADMS module). Minor bump because this adds a new optional module.
betinacosta
reviewed
May 29, 2026
Resolves three conflicts from the parallel agentgateway work landing on main: - .env_integration_tests.example — keep both ADMS and Agent Gateway env var blocks - docs/INTEGRATION_TESTS.md — keep both ADMS and Agent Gateway sections - tests/core/unit/telemetry/test_operation.py — combined operation count (4 agentgateway from upstream + 33 adms from this branch = 124)
Required by the CI version-bump check after merging upstream/main (which already shipped 0.22.0). Bumping minor since this PR still introduces the new ADMS module.
TokenCache is a generic auth utility from sap_cloud_sdk.core.auth and is not ADMS-specific. Re-exporting it from the adms namespace created two import paths for the same class. Users should import it directly from core.auth. Also drops the unused TokenCache import in user-guide.md (only RedisTokenCache was actually used in the example).
ArthurTonial
requested changes
Jun 2, 2026
Member
|
And just a small thing, please, can you update the PR description so it follows the template? |
CSRF tokens have a server-side TTL and may expire between cached fetches. When a mutating request (POST/PATCH/DELETE) returns 403, evict the cached token, re-fetch, and retry once before raising. Also adds a PEP 561 py.typed marker to core/http for downstream type checkers.
…leak AsyncAdmsHttp.with_user_jwt previously constructed a new instance without forwarding the underlying httpx.AsyncClient, allocating a fresh connection pool per user-scoped call. The returned wrapper had no parent close path, so each call leaked a pool — a real problem in handlers that fan out to client.with_user_jwt(jwt).x(). Now the user-scoped wrapper borrows the parent's client and is marked non-owning so closing it is a no-op; the parent retains ownership and closes once. Also expands the sync _get_csrf_token docstring to mirror the async sibling's reasoning about skipping error checks on the bare service-root response.
- Sort the exceptions group of __all__ alphabetically (AdmsError / AdmsOperationError now precede AuthError) for consistency with the surrounding groups. - Replace getattr(config, "resource", None) with config.resource — AdmsConfig is a dataclass with a typed `resource: str | None = None` field, so the attribute is always present.
The async _AsyncJobApi.get_status was hard-coded to DocumentService, so async callers polling a DELETE_USER_DATA job (started against AdminService via start_delete_user_data) hit the wrong path and got a 404. Sync sibling already accepted use_admin_service: bool — restore parity. Add an async unit test that asserts AdminService routing when use_admin_service=True.
User-supplied IDs (job_id, document_type_id, etc.) were f-string interpolated directly into single-quote-wrapped OData entity keys. A value containing a single quote would break the URL or alter query intent — e.g. an ID like ``x'); ...`` could in principle target a different entity than the caller intended. Add ``quote_odata_string_key`` helper (OData V4 §5.1.1.6.2 — single quotes inside string literals must be doubled) and apply it at all 8 call sites in client.py (sync + async). Also adds 4 unit tests covering simple values, embedded quotes, injection neutralisation, and the empty string.
The CSRF token cache (``self._csrf_tokens``) was read, written, and (on the 403-retry path) popped without synchronisation. ``AdmsClient`` is a natural candidate for shared use across threads — ``requests.Session`` is documented thread-safe and users will reuse one client from a thread pool — so the unguarded check-then-act sequence in ``_get_csrf_token`` and the unconditional ``pop`` in the 403-retry path race readers. Sync side: add a ``threading.Lock``; guard the read and the ``setdefault``-based write. The 403 retry path now only evicts the cached token if it still matches the value that 403'd, so it cannot clobber a token a sibling thread just refreshed. Async side: add an ``asyncio.Lock`` with the same pattern for parallel coroutines on a single event loop. The fetch itself is performed *outside* the lock, so the lock blocks only on cheap dict ops. Class docstrings now declare the thread- / coroutine-safety contract explicitly. Tests: two regression tests covering convergence under concurrency and the "evict-only-if-stale" guard.
H3 — IAS ``int(expires_in)`` propagated raw ``ValueError`` / ``TypeError`` when a misbehaving proxy or IAS implementation returned ``"3600"`` (string) or ``null`` for ``expires_in``. Callers using ``except AuthError`` saw the raw built-in exception instead, bypassing retry envelopes. Wrap the conversion and re-raise as ``AuthError`` with a descriptive message. H4 — async ``_get_csrf_token`` had no explicit timeout; behaviour depended on whatever the caller-supplied ``httpx.AsyncClient`` was constructed with, so sync (10s) and async clients could behave very differently under load. Pass an explicit ``_CSRF_FETCH_TIMEOUT_SECONDS`` like the sync sibling. H5 — extract magic ``10`` and ``30`` second timeouts in ADMS ``_http.py`` to named constants (``_CSRF_FETCH_TIMEOUT_SECONDS``, ``_REQUEST_TIMEOUT_SECONDS``) documenting the asymmetry. H8 — ``response_text`` on ``HttpError`` / ``NotFoundError`` carried the *full* response body, which can be 50KB+ HTML from a misconfigured ingress. Truncate consistently to 500 chars (extracted to a named constant) on both the async core client and the sync ADMS HTTP wrapper. Tests: two regression tests pinning the IAS ``expires_in`` envelope for non-integer and null values.
``RedisTokenCache`` swallowed all exceptions silently in ``get`` / ``set`` / ``delete``. When Redis is misconfigured (wrong password, TLS mismatch, ACL denial, network partition), the cache silently degrades to "always miss + always fail-write" — yet the whole reason to enable Redis is to share tokens across pods and avoid IAS thundering-herd. Operators get zero signal that the cache is broken. Log at WARNING with ``exc_info=True`` so the misconfiguration surfaces in operator dashboards, while keeping the cache failures non-fatal. Tests: extended the three existing failure-path tests to assert WARNING records are emitted with the expected method-prefixed message.
Fourteen ``to_odata_dict`` methods on public dataclasses (in ``__all__``) were undocumented — readers had to read the body to understand the OData payload contract. Add a one-line docstring to each. ``DraftActivateInput`` gets a slightly longer note since it extends the parent and adds an extra optional field.
M1 — ``create_client`` and ``create_async_client`` wrapped any ``Exception`` in ``ClientCreationError``. Because ``load_from_env_or_mount`` already maps real config failures to ``ConfigError`` and explicit ``ValueError`` paths re-raise unchanged, the broad except only ever caught internal SDK bugs (``KeyError``, ``AttributeError``, ``TypeError``) and reported them to callers as "client creation failed", masking the real fault. Drop the catch-all so genuine bugs surface as themselves. The ``ClientCreationError`` symbol remains exported from ``sap_cloud_sdk.adms.exceptions`` for any caller still catching it, but the factories no longer raise it. Update the test that asserted the wrapping behaviour to assert pass-through instead. M2 — ``_BindingData.validate`` iterated a hard-coded list of field names via ``getattr(self, f)``. Since the list is static and matches the dataclass shape, switch to direct attribute access (a list of ``(name, value)`` tuples) — both clearer and friendlier to type checkers.
…Strategy Private-key material was previously cleaned up only via __del__, which is best-effort: held references, GC delays, or interpreter shutdown can postpone or skip cleanup. On long-lived containers with shared /tmp mounts (sidecar containers, host-bind mounts), private-key bytes can linger longer than necessary. - Add close() that deletes tracked temp files; idempotent and safe to call multiple times. - Add __enter__/__exit__ so the strategy works as a context manager, giving deterministic cleanup on both happy and exception paths. - Document the lifecycle in the class docstring; recommend the context-manager form. - Keep __del__ as a best-effort safety net; log at DEBUG when it runs without an explicit close() so operators can spot non-deterministic cleanup if it matters. Tests cover explicit close, idempotency, context-manager exit (happy and exception), and reuse-after-close.
…O/CC cache isolation - AsyncHttpClient: assert __aexit__ runs aclose when the body raises; assert real httpx.AsyncClient.aclose is safe to call twice (the protocol can result in double-cleanup in caller code). - AsyncAdmsHttp: same exception-path coverage; assert aclose is idempotent on the owned client. - IasTokenFetcher: assert interleaving get_token (cached) and exchange_token (not cached) keeps the two grant types isolated — no cache-key collision and exactly one client_credentials IAS hit across the sequence. Guards the OBO privilege boundary against a future regression where someone adds caching to exchange_token.
ArthurTonial
reviewed
Jun 3, 2026
Reviewer ArthurTonial flagged AllowedDomainID as missing the OData escape that H1 added. Audit found the same pattern in 27 sites: AllowedDomainID, DocumentRelationID, and DocumentTypeBOTypeMapID are all Edm.Guid keys (per their docstrings — "Primary key UUID"), interpolated into the URL unquoted. The string-key escape isn't the right tool for Edm.Guid: OData V4 §5.1.1.6.2 reserves single-quoted literals for Edm.String, and SAP CAP rejects a quoted Guid as a type mismatch. Injection protection on Guid keys must come from validation, not escaping. - Add quote_odata_guid_key() that parses with uuid.UUID and raises ValueError on malformed input. Returns the canonical lowercase 8-4-4-4-12 form, unquoted, ready to drop into a key segment. - Apply across all 27 unquoted-Guid call sites (sync + async variants of DocumentRelation, AllowedDomain, DocumentTypeBOTypeMap paths). - Update unit-test fixtures to use real UUIDs where they pass through the new validator (placeholders like "rel-1" / "ad-uuid-1" now fail validation, as they should). Verified: 269 unit tests pass, all 17 integration tests still pass against the live tenant — wire format is unchanged.
# Conflicts: # pyproject.toml # uv.lock
- Bump project version per the CI version-bump check (src/ changed, so the version must move; 0.23.1 → 0.23.2). - Reformat the adms client import block to multi-line form so it satisfies `ruff format` line-length rules.
NicoleMGomes
reviewed
Jun 3, 2026
Per reviewer feedback, the contribution should be self-contained inside the adms/ package. Relocate IAS auth, token cache, and async HTTP modules into adms/, remove the orphaned MTLSStrategy that was never wired into ADMS, and revert unrelated drift in core/ (PEP 563 tolerance in secret_resolver, the auditlog conftest fail->skip change). - Move src/sap_cloud_sdk/core/auth/_ias_fetcher.py -> adms/_ias_fetcher.py - Move src/sap_cloud_sdk/core/auth/_token_cache.py -> adms/_token_cache.py - Move src/sap_cloud_sdk/core/http/_async_client.py -> adms/_async_http.py - Move corresponding unit tests into tests/adms/unit/ - Delete core/auth/_mtls.py (orphan — never imported by ADMS) - Delete tests/core/unit/bdd/ (BDD duplicates of unit-tested behaviour) - Drop `from __future__ import annotations` from adms/config.py so the reverted secret_resolver.py keeps working without the PEP 563 workaround - Update all imports + sphinx :class: references in adms/ docstrings and the user-guide example
Per reviewer feedback (NicoleMGomes, PR SAP#138): after the relocation refactor moved IasTokenFetcher into the adms package, the _auth wrapper (config adaptation + AuthError conversion) became pure boilerplate wrapping a sibling class. Inline the adaptation into _ias_fetcher (constructor now takes AdmsConfig directly) and drop the duplicate AuthError by importing from adms.exceptions. Consolidates two overlapping test files into test_ias_fetcher.py.
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
4 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.
Description
Adds the ADMS (Advanced Document Management Service) module to the
SAP Cloud SDK for Python.
Core components
AdmsClient(sync) andAsyncAdmsClient(async) with sub-namespacesfor
documents,relations,jobs, and configuration readscreate_client()/create_async_client()factories that load IASbindings from a mounted secret volume with environment-variable fallback
X509_GENERATED) client-credentials authentication, withoptional
resourcescoping for ADM application audience bindingDocumentService,AdminService, andConfigurationServicepathsFeatures
(
PENDING/CLEAN/INFECTED)DocumentRelationlifecycle including draft create / validate /activate / discard
business-object node types
ZIP_DOWNLOADandDELETE_USER_DATAjob orchestration viaAdminServiceSupporting infrastructure (in
core/)IasTokenFetcher—client_credentials(cached) andjwt-beareron-behalf-of (not cached, by design) grant flows
AdmsHttp/AsyncAdmsHttp— typed HTTP helpers with OData errormapping, X-CSRF-Token fetch-and-carry (per service root), and
thread- / coroutine-safe CSRF caching
MTLSStrategy— X.509 mTLS for services usingaccessStrategy: sap:cmp-mtls:v1, with explicitclose()andcontext-manager lifecycle
TokenCacheinterface withInMemoryTokenCacheandRedisTokenCache(multi-pod thundering-herd mitigation)py.typedmarkers oncore.auth,core.http,admsRelated Issue
N/A — this is a new module addition with no tracked issue.
Type of Change
How to Test
Expected: all unit tests pass.
uv run ruff check src/sap_cloud_sdk/
uv run ty check --python-version 3.11 src/
Expected: clean.
Set the standard ADMS env vars (or mount the secret volume):
CLOUD_SDK_CFG_ADMS_DEFAULT_URL (IAS tenant URL)
CLOUD_SDK_CFG_ADMS_DEFAULT_URI (ADM service URL)
CLOUD_SDK_CFG_ADMS_DEFAULT_CLIENTID
CLOUD_SDK_CFG_ADMS_DEFAULT_CLIENTSECRET
CLOUD_SDK_CFG_ADMS_DEFAULT_RESOURCE (optional)
Then:
uv run python -m pytest tests/adms/integration/ -m integration -v
Expected: all 17 BDD scenarios pass — sync + async document/relation
CRUD, draft flow, scan-state, concurrent creates, and 404 paths.
Checklist
Breaking Changes
None. This PR is purely additive — it introduces a new adms package and adds helpers under core.auth / core.http without modifying any existing public API.
Additional Notes