Skip to content

Added access check in discovery, updated unittests and integration base test#19

Open
rsaha-qlik wants to merge 7 commits into
SAC-28857-add-new-metadatafrom
SAC-30945-discovery-auth-perm
Open

Added access check in discovery, updated unittests and integration base test#19
rsaha-qlik wants to merge 7 commits into
SAC-28857-add-new-metadatafrom
SAC-30945-discovery-auth-perm

Conversation

@rsaha-qlik

@rsaha-qlik rsaha-qlik commented Jun 8, 2026

Copy link
Copy Markdown

Description of change

Summary

Added discovery-time access checks to tap-mailshake so that streams the API key cannot access are excluded from the catalog rather than causing a runtime error.

Changes

tap_mailshake/discover.py

  • Added check_stream_access(client, stream_name, stream_config) -> bool that probes each top-level stream endpoint with perPage: 1 and returns False on MailshakeInvalidApiKeyError / MailshakeNotAuthorizedError
  • Updated discover() to accept a client argument
  • Implemented a two-pass approach: top-level streams are probed first; child streams (e.g. recipients) are included only if their parent (campaigns) is accessible — child streams are never probed directly
  • Logs a warning for each excluded stream
  • Raises an exception if no streams are accessible

tap_mailshake/__init__.py

  • Updated do_discover() to accept and pass client to discover(client)

tests/unittests/test_discover.py

  • Rewrote all existing tests to patch check_stream_access and pass a MagicMock client
  • Added new tests:
    • test_inaccessible_stream_excluded
    • test_child_stream_excluded_when_parent_inaccessible
    • test_child_stream_included_when_parent_accessible
    • test_child_stream_not_probed_directly
    • test_warning_logged_for_excluded_stream
    • test_all_inaccessible_raises_exception

tests/unittests/test_sync.py

  • Fixed TestWriteSchema tests to patch check_stream_access and pass a MagicMock client to discover()

tests/_mock_tap_tester.py

  • Updated _run_check_mode() to build a lightweight MailshakeClient (bypassing __init__) and call discover(client) with check_stream_access patched to True, matching the pattern used in run_sync

Testing

python -m pytest tests/unittests/ -v

Manual QA steps

Risks

Rollback steps

  • revert this branch

AI generated code

https://internal.qlik.dev/general/ways-of-working/code-reviews/#guidelines-for-ai-generated-code

  • this PR has been written with the help of GitHub Copilot or another generative AI tool

@rsaha-qlik rsaha-qlik changed the title Sac 30945 discovery auth perm Added access check in discovery, updated unittests and integration base test Jun 8, 2026
@rsaha-qlik rsaha-qlik changed the base branch from gl-master to master June 8, 2026 05:50
@rsaha-qlik rsaha-qlik changed the base branch from master to gl-master June 8, 2026 05:51
@rsaha-qlik rsaha-qlik requested a review from Copilot June 8, 2026 05:51

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR adds discovery-time permission probing so streams that the configured Mailshake API key cannot access are excluded from the discovered catalog (instead of failing later during sync).

Changes:

  • Added check_stream_access() and updated discover() to probe top-level stream endpoints and exclude inaccessible streams, while only including child streams when their parent is accessible.
  • Updated CLI discovery wiring (do_discover) to pass a constructed client into discover(client).
  • Reworked unit tests and the mock integration harness to accommodate the new discover(client) signature and access-check behavior.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
tap_mailshake/discover.py Introduces access probing and filters catalog streams based on permissions.
tap_mailshake/__init__.py Passes the client through to discovery from the CLI entrypoint.
tests/unittests/test_discover.py Adds/updates unit tests for access-check and stream exclusion behavior.
tests/unittests/test_sync.py Updates schema-writing tests to call discover(client) with access-check patched.
tests/_mock_tap_tester.py Updates mock “check mode” to call discover(client) under patched access-checks.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread tap_mailshake/discover.py
Comment thread tests/unittests/test_discover.py Outdated
Comment thread tests/_mock_tap_tester.py Outdated

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.

@RushiT0122 RushiT0122 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are skipping ERROR Error status_code = 401 while check_access() at the beginning of tap execution. Ideally we should throw exception immediately if we don't valid permissions.

@RushiT0122 RushiT0122 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Standard metadata changes missing which is already handled in the PR#15.

@RushiT0122 RushiT0122 changed the base branch from gl-master to SAC-28857-add-new-metadata June 11, 2026 12:13
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.

4 participants