Meta data additon forced replcation key by romilqlik · Pull Request #49 · singer-io/tap-eloqua

romilqlik · 2025-10-03T12:10:31Z

Description of change

What changed

Added forced-replication-method metadata to all discovered streams in tap_eloqua/discover.py
- Streams with an UpdatedAt (or updatedAt) property are set to INCREMENTAL
- All other streams are set to FULL_TABLE
Renamed local metadata variable to md_list to resolve naming conflict with the imported singer.metadata module (aliased as mdata)
Bumped Python version from 3.9 to 3.12 in CircleCI config
Bumped dependencies: backoff==2.2.1, requests==2.33.1, pendulum==3.2.0, singer-python==6.8.0

Tests added

Unit tests (tests/unittests/):
Integration tests (tests/):

CI changes (`.circleci/config.yml`)

Added Pylint step (pylint tap_eloqua --disable=C,R,W) — enforces zero error/warning-level lint issues
Added Unit Tests step — runs tests/unittests/ in isolation using the tap's own virtualenv
Separated Integration Tests step to use the tap-tester virtualenv (required for Stitch test framework)

Manual QA steps

Run python -m unittest discover -s tests/unittests -p 'test_*.py' and confirm all unit tests pass
Run discovery and confirm output catalog contains forced-replication-method in root metadata for each stream
Confirm INCREMENTAL streams (e.g. accounts, contacts, campaigns) have UpdatedAt/updatedAt in their schema
Confirm FULL_TABLE streams (e.g. visitors, activity streams) do not

Risks

Changing forced-replication-method metadata is a catalog-level change; downstream targets or orchestrators that read this field may change sync behavior on next run
Schema tightening (adding explicit replication method) may require a catalog reset for existing connections

Rollback steps

Revert this branch
Existing connections may need to re-run discovery to restore previous catalog state

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

RushiT0122 · 2025-10-03T15:50:33Z

-        metadata = field_metadata[stream_name]
+
+        # existing field-level metadata from schema.py
+        md_list = field_metadata[stream_name]


What was the need to change the variable name from metadata to md_list?

@RushiT0122

Renamed the local metadata variable to md_list because it conflicted with the Singer metadata module.
To avoid this naming clash, the Singer module is now aliased as mdata, ensuring all metadata operations (write, to_map, etc.) behave correctly.

Also added the required top-level metadata fields — inclusion and forced-replication-method — to align with Singer catalog specifications and client expectations.

This needs unit test.

We don't have any test cases from the beginning for this tap. we have incorporate unittest. is there any reference. for this kind of scenario.

Added test cases. for this change. please check

Copilot

Pull request overview

This PR adds forced replication method metadata to Eloqua tap streams, automatically setting INCREMENTAL for streams with an UpdatedAt property and FULL_TABLE otherwise.

Key Changes:

Modified the discover function to detect UpdatedAt properties and set appropriate forced-replication-method metadata
Added comprehensive test coverage to verify the metadata is correctly applied to all discovered streams

Reviewed changes

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

File	Description
tap_eloqua/discover.py	Adds logic to detect UpdatedAt property and write forced-replication-method metadata to stream catalog entries
test/test_discovery_metadata.py	Adds unit test verifying that forced-replication-method metadata is correctly set based on schema properties

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

Copilot · 2025-11-28T12:46:55Z

        schema = Schema.from_dict(schema_dict)
-        metadata = field_metadata[stream_name]
+        properties = schema_dict.get("properties", {})
+        replication_key = "updatedAt" if "UpdatedAt" in properties else None


The replication_key is set to lowercase 'updatedAt' but the check is for 'UpdatedAt' with capital letters. This inconsistency could cause issues if the replication_key value is used elsewhere in the codebase expecting the correct casing.

Suggested change

replication_key = "updatedAt" if "UpdatedAt" in properties else None

replication_key = "UpdatedAt" if "UpdatedAt" in properties else None

Copilot · 2025-11-28T12:46:55Z

+            # compute expected replication method from schema properties
+            schema_props = s.schema.to_dict().get("properties", {})
+            expected_rep_method = "INCREMENTAL" if "UpdatedAt" in schema_props else "FULL_TABLE"
+
+            self.assertEqual(root.get("forced-replication-method"),
+                             expected_rep_method,
+                             msg=f"forced-replication-method mismatch for stream {s.stream}")


The test duplicates the logic from the discover function (checking for 'UpdatedAt' property). Consider testing against known stream fixtures with expected outcomes rather than replicating the implementation logic, which makes the test less effective at catching bugs.

Suggested change

# compute expected replication method from schema properties

schema_props = s.schema.to_dict().get("properties", {})

expected_rep_method = "INCREMENTAL" if "UpdatedAt" in schema_props else "FULL_TABLE"

self.assertEqual(root.get("forced-replication-method"),

expected_rep_method,

msg=f"forced-replication-method mismatch for stream {s.stream}")

# Use known expected replication methods for each stream

expected_rep_methods = {

"Contacts": "INCREMENTAL",

"Accounts": "FULL_TABLE",

"Activities": "INCREMENTAL",

# Add other streams and their expected replication methods here

}

stream_name = s.stream

self.assertIn(stream_name, expected_rep_methods, msg=f"Unknown stream {stream_name} in test fixture")

expected_rep_method = expected_rep_methods[stream_name]

self.assertEqual(root.get("forced-replication-method"),

expected_rep_method,

msg=f"forced-replication-method mismatch for stream {stream_name}")

This comment is consider. and made the changes. @RushiT0122

Copilot

Pull request overview

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

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

Copilot · 2025-11-28T14:02:13Z

        schema = Schema.from_dict(schema_dict)
-        metadata = field_metadata[stream_name]
+        properties = schema_dict.get("properties", {})
+        replication_key = next((k for k in properties if k.lower() == "updatedat"), None)


The case-insensitive comparison k.lower() == 'updatedat' will fail if the property key is 'updatedAt' (camelCase). This logic assumes all property keys are lowercase, but the comparison converts them to lowercase for matching. Consider verifying that property keys in the schema are actually lowercase, or adjust the logic to handle camelCase properly (e.g., comparing against both 'updatedat' and 'updatedAt').

Suggested change

replication_key = next((k for k in properties if k.lower() == "updatedat"), None)

# Accept both "updatedAt" (camelCase) and "updatedat" (lowercase) as replication keys

replication_key = next(

(k for k in properties if k.lower() == "updatedat" or k == "updatedAt"),

None

)

I think this comment is valid but you should explain inconsistency in the replication name and why this is handled like in the comment.

I will not hold the PR for this but please add the appropriate comment before merging.

* upgrade python and added unittests * Update tests/test_sync.py Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Update tests/test_sync.py Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * review comments * resolve review comments * remove out of scope changes * request lib to latest * fix review comments * resolve review comments * resolve review comments * updated changelog --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Co-authored-by: Ranit Saha <ranit.saha@globallogic.com>

atttiwari · 2026-04-20T06:55:53Z


+## [1.4.0] 2026-04-08
+- Added replication method into metadata [#49](https://github.com/singer-io/tap-eloqua/pull/49)
+- Bump backoff, requests, pendulum, singer-python version [#50](https://github.com/singer-io/tap-eloqua/pull/50)


also update for adding unittests and integration tests

atttiwari · 2026-04-20T06:57:47Z

          command: |
            source /usr/local/share/virtualenvs/tap-tester/bin/activate
-            stitch-validate-json /usr/local/share/virtualenvs/tap-eloqua/lib/python3.9/site-packages/tap_eloqua/schemas/*.json
+            stitch-validate-json /usr/local/share/virtualenvs/tap-eloqua/lib/python3.12/site-packages/tap_eloqua/schemas/*.json


please also add pylint, unittest test validation in config file.

Meta data additon forced replcation key

4546bfe

RushiT0122 requested changes Oct 3, 2025

View reviewed changes

Pagadala Narender added 3 commits November 28, 2025 14:41

removing inclusion

c397e96

changes for the dynamic method

05a751d

added unittest case

0b47da5

RushiT0122 requested review from Copilot and narenderqlik November 28, 2025 12:45

Copilot AI reviewed Nov 28, 2025

View reviewed changes

changes for the test cases

7103c0c

RushiT0122 requested a review from Copilot November 28, 2025 14:01

Copilot AI reviewed Nov 28, 2025

View reviewed changes

RushiT0122 approved these changes Nov 28, 2025

View reviewed changes

Pagadala Narender and others added 2 commits December 1, 2025 08:00

changes for the version release details

69f41f3

atttiwari reviewed Apr 20, 2026

View reviewed changes

ranitsaha-gl added 3 commits April 20, 2026 15:51

update circleci, changelog

fcf75df

added pylint

55008eb

Addressed pylint issue

f21a71e

atttiwari approved these changes Apr 20, 2026

View reviewed changes

	replication_key = "updatedAt" if "UpdatedAt" in properties else None
	replication_key = "UpdatedAt" if "UpdatedAt" in properties else None

-            # compute expected replication method from schema properties
-            schema_props = s.schema.to_dict().get("properties", {})
-            expected_rep_method = "INCREMENTAL" if "UpdatedAt" in schema_props else "FULL_TABLE"
-            self.assertEqual(root.get("forced-replication-method"),
-                             expected_rep_method,
-                             msg=f"forced-replication-method mismatch for stream {s.stream}")
+            # Use known expected replication methods for each stream
+            expected_rep_methods = {
+                "Contacts": "INCREMENTAL",
+                "Accounts": "FULL_TABLE",
+                "Activities": "INCREMENTAL",
+                # Add other streams and their expected replication methods here
+            }
+            stream_name = s.stream
+            self.assertIn(stream_name, expected_rep_methods, msg=f"Unknown stream {stream_name} in test fixture")
+            expected_rep_method = expected_rep_methods[stream_name]
+            self.assertEqual(root.get("forced-replication-method"),
+                             expected_rep_method,
+                             msg=f"forced-replication-method mismatch for stream {stream_name}")

Conversation

romilqlik commented Oct 3, 2025 • edited by rsaha-qlik Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description of change

What changed

Tests added

CI changes (.circleci/config.yml)

Manual QA steps

Risks

Rollback steps

Rollback steps

AI generated code

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Nov 28, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 28, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Nov 28, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

romilqlik commented Oct 3, 2025 •

edited by rsaha-qlik

Loading

CI changes (`.circleci/config.yml`)