SAC-31984: Code Refactor

[satyendra101]

Description of change

• Added synthetic key processedUntil as a replicationKeyto keep track of stream window processed till datetime.
• Used singer specific get_bookmark and write_bookmark functionalities.
• Added REPLICATION_METHOD, REPLICATION_KEYS, and REQUIRES metadata to applicable streams.
• Added contact-cache prefill and stream ordering to handle the dependency of call_log/messages on contacts.

Details in JIRA: https://qlik-dev.atlassian.net/browse/SAC-31984

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

[Copilot]

🟡 Changes recommended

Bookmark advancement/state persistence logic can cause repeated reprocessing and loss of progress on interruption, and should be corrected before merge.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR refactors the tap’s replication/state handling to align with Singer bookmark conventions, introduces a synthetic replication key (processedUntil) for windowed incremental streams, and adds dependency-aware syncing via a prefilled contacts cache.

Changes:

• Add REPLICATION_METHOD / REPLICATION_KEYS / REQUIRES metadata on streams and mark replication-key fields as automatic in catalog metadata.
• Switch windowed streams to Singer get_bookmark / write_bookmark and add state migration from legacy last_record.
• Prefill contacts cache (and order streams) so extension-based streams (call_log, messages) can run when contacts isn’t selected.

File summaries

File	Description
tests/unittests/test_streams.py	Adds assertions for stream replication metadata.
tests/unittests/test_streams_sync.py	Updates sync unit tests for bookmark behavior and record counting.
tests/unittests/test_streams_advanced.py	Adds tests for contacts cache fill and updated period/bookmark behavior.
tests/unittests/test_state.py	Adds tests for bookmark migration and updated bookmark key usage.
tests/unittests/test_main.py	Adds tests for currently_syncing and contacts-cache prefill behavior.
tests/unittests/test_contact_base_stream_contacts.py	Updates mocks to align with new return values from sync methods.
tests/test_sync.py	Removes save_state patching to match new state-writing approach.
tests/test_start_date.py	Removes save_state patching to match new state-writing approach.
tests/test_bookmark.py	Updates assertions from last_record to processedUntil.
tests/test_all_fields.py	Removes save_state patching to match new state-writing approach.
tests/base.py	Updates expected catalog metadata for replication method/keys.
tap_ringcentral/streams/messages.py	Declares incremental replication metadata and contacts dependency.
tap_ringcentral/streams/contacts.py	Adds fill_cache() to load extension IDs without emitting records.
tap_ringcentral/streams/company_call_log.py	Adds incremental replication metadata and bookmark-writing in period sync.
tap_ringcentral/streams/call_log.py	Declares incremental replication metadata and contacts dependency.
tap_ringcentral/streams/base.py	Refactors windowed sync to Singer bookmark APIs and returns synced record counts.
tap_ringcentral/state.py	Adds legacy bookmark migration + deprecates old state helpers in favor of Singer APIs.
tap_ringcentral/schemas/messages.json	Expands schema nullability and adds processedUntil.
tap_ringcentral/schemas/contacts.json	Expands schema nullability.
tap_ringcentral/schemas/company_call_log.json	Expands schema nullability and adds processedUntil.
tap_ringcentral/schemas/call_log.json	Expands schema nullability and adds processedUntil.
tap_ringcentral/schema.py	Marks replication key fields as inclusion=automatic in metadata.
tap_ringcentral/init.py	Adds contacts-cache prefill, dependency-based stream ordering, and currently_syncing updates.

Review details

Suppressed comments (1)

tap_ringcentral/init.py:101

• do_sync() clears currently_syncing only on the success path. If a stream raises, the state can be left with a stale currently_syncing value (and the state written at line 83), which can confuse resuming/monitoring. Clear it in a finally so state is consistent even on errors.

            # Track currently syncing stream
            singer.set_currently_syncing(self.state, stream_name)
            singer.write_state(self.state)
            LOGGER.info('Currently syncing: %s', stream_name)

            stream_obj = self.available_streams[stream_name](
                        self.config, self.state, stream_to_sync, self.client
                    )
            try:
                stream_obj.state = self.state
                stream_obj.sync()
                self.state = stream_obj.state

                # Clear currently_syncing after successful sync
                singer.set_currently_syncing(self.state, None)
                singer.write_state(self.state)
            except Exception as e:
                LOGGER.error(str(e))
                LOGGER.error('Failed to sync endpoint {}, moving on!'
                             .format(stream_obj.TABLE))
                raise e

• Files reviewed: 23/23 changed files
• Comments generated: 4
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[RushiT0122]

Findings

High: A forbidden extension can be permanently skipped after a partial sync

In base.py:181-215, sync_data_for_extension() catches RingCentralForbiddenError, logs it, and returns 0. The surrounding period then advances the bookmark whenever any other extension produced records:

For example:

if records_synced > 0:
    write_bookmark(..., date.isoformat())

1. Extension A returns records successfully.
2. Extension B returns 403.
3. The period bookmark advances.
4. On the next run, the tap starts after that period and never retries Extension B’s missed data.

This causes silent data loss when access differs between extensions or an extension temporarily returns 403. The code should retain the previous bookmark, fail the period, or record/retry incomplete extensions rather than advancing the shared bookmark after a partial result. The current tests only cover a single forbidden extension, not a mixed success/forbidden batch.

Medium: Extension-based streams can silently succeed with no data when contacts access fails

contacts.py:22-40 catches a forbidden contacts-directory request and returns without signaling failure. The runner then continues syncing dependent streams after init.py:36-45. Since the cache is empty, those streams emit no records and do not advance their bookmarks.

If the dependent endpoint is accessible while the contacts directory is not, the run can appear successful while silently extracting nothing from call_log or messages. The runner should either exclude dependent streams during discovery, fail them explicitly, or propagate the contacts access failure.

Low: README still describes changed streams as full-table

The PR changes call_log, company_call_log, and messages to incremental replication, but the stream documentation still describes other affected stream behavior as full-table/no-bookmark, notably:

• README.md:34-41
• README.md:62-69
• README.md:112-118

The documentation should reflect the new processedUntil bookmark and incremental behavior where applicable.

[satyendra101]

Findings

High: A forbidden extension can be permanently skipped after a partial sync

In base.py:181-215, sync_data_for_extension() catches RingCentralForbiddenError, logs it, and returns 0. The surrounding period then advances the bookmark whenever any other extension produced records:

For example:

if records_synced > 0:
    write_bookmark(..., date.isoformat())

1. Extension A returns records successfully.
2. Extension B returns 403.
3. The period bookmark advances.
4. On the next run, the tap starts after that period and never retries Extension B’s missed data.

This causes silent data loss when access differs between extensions or an extension temporarily returns 403. The code should retain the previous bookmark, fail the period, or record/retry incomplete extensions rather than advancing the shared bookmark after a partial result. The current tests only cover a single forbidden extension, not a mixed success/forbidden batch.

Medium: Extension-based streams can silently succeed with no data when contacts access fails

contacts.py:22-40 catches a forbidden contacts-directory request and returns without signaling failure. The runner then continues syncing dependent streams after init.py:36-45. Since the cache is empty, those streams emit no records and do not advance their bookmarks.

If the dependent endpoint is accessible while the contacts directory is not, the run can appear successful while silently extracting nothing from call_log or messages. The runner should either exclude dependent streams during discovery, fail them explicitly, or propagate the contacts access failure.

Low: README still describes changed streams as full-table

The PR changes call_log, company_call_log, and messages to incremental replication, but the stream documentation still describes other affected stream behavior as full-table/no-bookmark, notably:

• README.md:34-41
• README.md:62-69
• README.md:112-118

The documentation should reflect the new processedUntil bookmark and incremental behavior where applicable.

Below are my findings/changes for each comment.

1. High Finding

• Context:

  • The current behavior (advance the bookmark if any extension in the period produced records) is already an improvement over the pre-PR code, which advanced the bookmark unconditionally even when zero records were synced across the board. This PR's guard narrows that blast radius to "only skip advancing when the entire period yields nothing."

  • Why "never advance on any partial failure" is risky:

    RingCentral 403s are almost always permission-boundary errors, not transient failures — they don't resolve on their own.
    If a single extension is permanently unauthorized, blocking the bookmark for the whole stream means it never advances again.
    Every subsequent run would then re-fetch the entire history for every other successfully-synced extension, since the local sync window is always computed from the last persisted bookmark.
    That trades a narrow, bounded gap (one extension, one window) for unbounded reprocessing and growing API usage/rate-limit risk — a worse outcome for the common case of a permanently-restricted extension.

2. Medium finding

• Changes are added to exclude the dependent stream if contacts stream is not accessible. Also the exception is not swallowed rather raised the caller to decide.

3. README change

• Added Replication strategy section in readme.