Skip to content

feat(dpi_ng/consent): add Data Privacy Integration NextGen Consent service integration to sdk#199

Open
Ashwith-N wants to merge 33 commits into
SAP:mainfrom
Ashwith-N:main
Open

feat(dpi_ng/consent): add Data Privacy Integration NextGen Consent service integration to sdk#199
Ashwith-N wants to merge 33 commits into
SAP:mainfrom
Ashwith-N:main

Conversation

@Ashwith-N

@Ashwith-N Ashwith-N commented Jul 6, 2026

Copy link
Copy Markdown

Disclaimer: Do not include SAP-internal or customer-specific information in this PR (e.g. internal system URLs, customer names, tenant IDs, or confidential configurations). This is a public repository.

Description

To introduce the sap_cloud_sdk.core.dpi_ng.consent capability to the SAP Cloud SDK for Python. It provides a unified API for managing consents, purposes, templates, retention rules, and configuration reference data exposed by the DPI NextGen Consent Repository OData V4 service.

Key components included:

  • Client Factory: A public create_client() function with a typed ConsentClient API
  • Authentication strategies: BearerTokenAuth, ClientCredentialsAuth, and ClientCertificateAuth (mTLS)
  • Request/Result Models: Typed DTOs for consent operations
  • Service layer: Five service attributes (consents, purposes, templates, retention, configuration) each covering full CRUD and lifecycle operations against the respective OData endpoints.
  • Exception hierarchy: DPINGError base with typed subclasses for 401, 403, 400/422, 404, 409, and other OData errors.
  • Documentation: Module guidance in user-guide.md.
  • Test coverage: Unit tests and BDD integration tests under tests/core/unit/dpi_ng and tests/core/integration/dpi_ng/consent

Related Issue

No pre-existing issue - this is a new feature contribution.

Type of Change

Please check the relevant option:

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update
  • Code refactoring
  • Dependency update

How to Test

  1. Install dependencies: uv sync --all-extras
  2. Configure credentials per docs/INTEGRATION_TESTS.md
  3. Run unit tests: uv run pytest tests/core/unit/dpi_ng/ -v
  4. Run integration tests: uv run pytest tests/core/integration/dpi_ng/consent/ -v
  5. Expected result: all tests pass

Checklist

Before submitting your PR, please review and check the following:

  • I have read the Contributing Guidelines
  • I have verified that my changes solve the issue
  • I have added/updated automated tests to cover my changes
  • All tests pass locally
  • I have verified that my code follows the Code Guidelines
  • I have updated documentation (if applicable)
  • I have added type hints for all public APIs
  • My code does not contain sensitive information (credentials, tokens, etc.)
  • I have followed Conventional Commits for commit messages

Breaking Changes

If this PR introduces breaking changes, please describe:

  • What breaks
    None.
  • Migration path for users
    No migration required.
  • Alternative approaches considered
    N/A

Additional Notes

This feature follows the existing SDK module structure described in CONTRIBUTING.md and aligns with the integration test setup documented in INTEGRATION_TESTS.md. It is intended as an additive capability and does not change existing public APIs.

AI-assisted code: The logger.info("Invoked …") / logger.info("Exiting …") instrumentation lines throughout the dpi_ng module were written with the assistance of Claude (Anthropic). Disclosure per the SAP AI contribution guideline.

Dependency note: python-odata>=0.7.0 was added as a runtime dependency. The package was checked against the OSV vulnerability database prior to inclusion - no known CVEs found as of the time of this PR.

@Ashwith-N
Ashwith-N requested a review from a team as a code owner July 6, 2026 07:39
@cla-assistant

cla-assistant Bot commented Jul 6, 2026

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

Comment thread tests/core/unit/dpi_ng/consent/unit/test_config.py Fixed
Ashwith-N added 26 commits July 14, 2026 13:59
@Ashwith-N

Copy link
Copy Markdown
Author

PR Review: #199 feat(dpi_ng/consent): add Data Privacy Integration NextGen Consent service integration to sdk

Author: Ashwith-N   Branch: Ashwith-N:mainSAP:main
Verdict: ⚠️ Needs Minor Work
Summary: A comprehensive, well-structured new module that passes all CI checks; three structural/process gaps need addressing before merge.


A: Process & Compliance

# Criterion Status Finding
A1 PR template complete All sections filled; all 9 checklist items ticked; 3 type boxes checked; 5-step How to Test present
A2 Conventional Commits 32 commits all match type(scope): description; one commit (chore: bump version to 0.29.0...) omits the scope but is valid per CC spec and passes Commit Validation CI
A3 Issue linked ⚠️ PR body says "N/A" — no Closes #N/Fixes #N/Resolves #N. Acceptable for a brand-new feature with no pre-existing issue, but the template requires an explicit reference
A4 AI-generated code disclosure ⚠️ Every method in dpi_ng source files carries logger.info("Invoked X.__init__") / logger.info("Exiting X.__init__") entry/exit traces that are absent from every other module in the repo — a strong signal of AI-assisted generation. No disclosure in PR description as required by the SAP AI contribution guideline. If AI tooling was used, a disclosure sentence is required.

B: Security & Sensitive Data

# Criterion Status Finding
B1 No sensitive data in code All URLs are placeholder-form; no real credentials, tenant IDs, or customer identifiers
B2 No sensitive data in PR body PR body uses only generic placeholder references

C: Code Quality

# Criterion Status Finding
C1 CI checks passing All 8 required checks pass: Code Quality Checks, Tests & Coverage, Build & Package, Commit Validation, Check Version Bump, Analyze (python), CodeQL, REUSE Compliance Check
C2 Version bump src/ files changed → pyproject.toml bumped 0.35.0 → 0.36.0
C3 Type hints Full annotations on all public functions/methods; py.typed present at src/sap_cloud_sdk/core/dpi_ng/consent/py.typed
C4 No hardcoded values Service path defined as a dataclass field default in consent/config.py, not inline
C5 Import organization ⚠️ ConsentClient.__init__ uses a lazy import (from .client import _ConsentODataClient) without a comment explaining the circular-import reason — consent/__init__.py line ~82
C6 Naming conventions All enum values follow SCREAMING_SNAKE_CASE = "snake_case_value"; file names are snake_case; private symbols use leading underscore (_ConsentODataClient, _OAuth2Flow)
C7 No unused code No unused imports or dead methods identified
C8 No unjustified new dependencies ⚠️ python-odata>=0.7.0 added as a runtime dependency — the choice is reasonable, but no CVE check or version justification is noted in the PR. responses>=0.25 added as dev dep for HTTP mocking — standard and justified
C9 Proto code freshness N/A — no .proto files changed

D: API & Design

# Criterion Status Finding
D1 API future-proofing create_client() factory present; ConsentConfig dataclass; BaseCapabilityConfig ABC for future DPI NG sub-services; auth via strategy classes
D2 Public API hygiene __all__ carefully curated in both dpi_ng/__init__.py and dpi_ng/consent/__init__.py; internal helpers prefixed _; _ConsentODataClient not exported
D3 Breaking changes marked N/A — purely additive; no existing public APIs changed
D4 Pagination & tenant filtering OData filter passed as query param; tenant routing via x-tenant-id header for mTLS; consistent with existing patterns
D5 Telemetry instrumentation Module.DPI_NG in telemetry/module.py; 90+ operations added to telemetry/operation.py; @record_metrics on create_client(); _telemetry_source plumbed through ConsentClient.__init__ and all five service classes

E: Tests & Documentation

# Criterion Status Finding
E1 Tests added/updated Unit tests under tests/core/unit/dpi_ng/consent/; BDD integration tests under tests/core/integration/dpi_ng/consent/; all new env vars documented in .env_integration_tests.example and docs/INTEGRATION_TESTS.md
E2 Documentation quality Full user-guide.md with overview, auth options, config table, API examples; all public methods have Google-style docstrings; README.md and INTEGRATION_TESTS.md updated
E3 Module structure compliance ⚠️ Two deviations from required layout: (1) load_from_env_or_mount() is absent from consent/config.pydestination, print, and adms all provide this factory; (2) models are in dtos/ + entities/ subpackages rather than a single _models.py — reasonable for OData complexity but deviates from the spec

⚠️ Non-Blocking Suggestions

  • [A4] If AI tooling (Copilot, Claude, ChatGPT, etc.) was used, add a one-sentence disclosure to the PR description per the SAP AI contribution guideline. The logger.info("Invoked …") / logger.info("Exiting …") trace in every dpi_ng method is unique to this module and strongly suggests AI assistance.
  • [C5] Add a short comment on the lazy import explaining why — e.g. # deferred import to break circular dependency: consent/__init__ → services → client
  • [C8] Note in the PR (or a commit message) that python-odata was checked for known CVEs before being added as a runtime dependency.
  • [E3] Consider adding load_from_env_or_mount() to ConsentConfig (or BaseCapabilityConfig) to match the env-var/mount pattern used by other SDK modules.

✅ Things Done Well

  • All CI checks pass cleanly, including CodeQL and ruff/ty quality gates.
  • Telemetry instrumentation is exceptionally thorough — 90+ operations defined, _telemetry_source plumbed through all five service layers.
  • The shared dpi_ng/ base layer (auth.py, config.py, exceptions.py, odata_client.py) is cleanly abstracted for future DPI NG sub-services.
  • Three auth strategies (Bearer, ClientCredentials, mTLS) with proper validation; tenant_id mTLS requirement enforced at config construction time.
  • __all__ exports carefully curated in every __init__.py; internal symbols consistently prefixed _.
  • Context manager support (__enter__/__exit__) on ConsentClient follows the established SDK pattern.
  • Documentation is unusually complete — user guide, docstrings, integration test guide, and example env file all updated.

Review generated by Claude Code PR review skill

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