TDL-19319 Make replication key as automatic inclusion #195

prijendev · 2022-06-07T06:17:35Z

Description of change

Replaced replication key of deals stream with property_hs_lastmodifieddate field.
Updated replication method of deals stream in the tap to INCREMENTAL.
Updated replication key in the base.py of tap-tester.
Updated automatic_field and start_date tap tester test case.

Manual QA steps

Risks

Rollback steps

revert this branch

* Added versionTimestamp in contacts stream. * Updated bookmark test case. * Updated comments. * Updated replication method of contacts stream.

* Initial commit for request_timeout * Updated unit test case error * Removed path of unittest file * Updated config file * Updated readme file

prijendev · 2022-06-07T06:53:33Z

tap_hubspot/__init__.py

@@ -599,14 +599,23 @@ def sync_companies(STATE, ctx):
 def has_selected_custom_field(mdata):
    top_level_custom_props = [x for x in mdata if len(x) == 2 and 'property_' in x[1]]
    for prop in top_level_custom_props:
-        if mdata.get(prop, {}).get('selected') == True:
+        if (mdata.get(prop, {}).get('selected') == True) or (mdata.get(prop, {}).get('inclusion') == "automatic"):


Return True if the custom field is automatic.

@prijendev can you add this statement as the Code comment

Added comment in the code.

tap_hubspot/__init__.py

karanpanchal-crest · 2022-06-07T10:14:31Z

tap_hubspot/__init__.py

@@ -599,14 +599,23 @@ def sync_companies(STATE, ctx):
 def has_selected_custom_field(mdata):
    top_level_custom_props = [x for x in mdata if len(x) == 2 and 'property_' in x[1]]
    for prop in top_level_custom_props:
-        if mdata.get(prop, {}).get('selected') == True:
+        if (mdata.get(prop, {}).get('selected') == True) or (mdata.get(prop, {}).get('inclusion') == "automatic"):


@prijendev can you add this statement as the Code comment

kspeer825 · 2022-06-08T13:05:12Z

tests/base.py

@@ -105,7 +105,7 @@ def expected_metadata(self):  # DOCS_BUG https://stitchdata.atlassian.net/browse
            "deals": {
                self.PRIMARY_KEYS: {"dealId"},
                self.REPLICATION_METHOD: self.INCREMENTAL,
-                self.REPLICATION_KEYS: {"hs_lastmodifieddate"},
+                self.REPLICATION_KEYS: {"property_hs_lastmodifieddate"},


Should this include the property_ prefix from the denesting method? None of the other replicaiton keys have this. And I think it will break every customer's connection that is replicating this stream.

Currently, hs_lastmodifieddate is the bookmark key for the deals stream. hs_lastmodifieddate is available in the properties field at the nested level.
For example, the response structure:

{ "dealId": 1, "properties": { "hs_lastmodifieddate": 1653590080602 } }

As hs_lastmodifieddate is not available at the 1st level, it can not be marked as automatic inclusion and we can not select that particular field even. Also, tap includes all nested fields of the properties field as custom fields in the schema by appending the prefix property_ along with each field.

For example, the schema of the deals stream looks like as below,

{ "dealId": {"type": ["null", "integer"]}, "properties": {"type": "object", "properties": { "hs_lastmodifieddate": {}, "hs_likelihood_to_close": {} } }, "property_hs_lastmodifieddate": {}, "property_hs_likelihood_to_close": {} }

That's why we are just marking property_hs_lastmodifieddate as the inclusion of automatic. So, I am not sure in which case it will break the customer's connection because we have not removed or updated any fields.

I believe this nested structure is true of the other streams as well, but companies for example denests the key and does not prefix property_ onto the key. I think it will break downstream for customers because the value of the replication key is changing.

@kspeer825 We have changed the bookmark key just for the deals stream. This change will not affect any other streams.
If bookmark(last_modifieddate) is already available for the deals stream, then tap will use only that value for the first time. (#196 )
After completing sync, the tap will write an updated bookmark in the new bookmark key(i.e. property_lastmodified_date).
We haven't removed the existing bookmark as well, so in case we need to revert back, we can able to.
After sync, the bookmark will look like the below.

"bookmarks": { "stream_id_1": { "old_bookmark": "2022-04-08T00:00:00", "current_bookmark": "2022-04-15T00:00:00" } }

We have incorporated this change to fix the customer's issue. So, @KrisPersonal shall we plan an alpha release first for the particular customer?

…c-inclusion

kspeer825 · 2022-06-28T14:55:32Z

tests/test_hubspot_automatic_fields.py

@@ -69,7 +69,7 @@ def test_run(self):
                expected_keys = self.expected_automatic_fields().get(stream)

                # BUG_TDL-9939 https://jira.talendforge.org/browse/TDL-9939 Replication keys are not included as an automatic field for these streams
-                if stream in {'companies', 'deals', 'subscription_changes', 'email_events'}:
+                if stream in {'companies', 'subscription_changes', 'email_events'}:


Can we make this change for the other streams with replication keys that are missing automatic inclusion?

@kspeer825 Yes, the replication key is not automatic for companies, subscription_changes, and email_events streams.

The companies stream has the same issue(link) of the replication key as the deals stream. So, can we create a card to resolve this issue and work on that?

subscription_changes and email_events streams are retrieving records in the date window. These streams are saving the start date of the last window as a bookmark value in startTimestamp field.
email_events stream has a created field in the API response and subscription_changes has a timestamp field in the response that we may use as a replication key for the respective stream. But, we are not sure whether filter parameters(Start timestamp, End timestamp) are following these keys or not as we can't find any doc stating the same, so we asked a question for the same in the community(link).
Possible solutions:

We can add a new startTimestamp field in the record explicitly and mark that field as an automatic to keep current behavior as is.

Update the bookmark key for email_events and subscription_changes as created and timestamp respectively if we got a positive reply from the support.

Sounds good

Are you able to verify the usage of these keys by exploratory testing? If you have a record before the StartTimestamp, another record after the EndTimestamp, and another record between those two I think you'd answer that question. I supposed you'd actually need two more records at either of those fence posts to see if the API treats either inclusively.

As per discussion with @cosimon , we concluded to make a change in the replication key just for the deals stream and company stream as this problem is for 2 streams only. We are not making any changes in subscription_changes and email_events as of now.

…c-inclusion

…ddate field.

https://github.com/singer-io/tap-hubspot into TDL-19319-make-replication-key-as-automatic-inclusion

kspeer825

Looks good 👍

* Clearing bookmark for existing connection. * Updated bookmark logic to utilize older bookmark key. * Updated logic to keep older bookmark key in the state. * Added unittest cases. * Updated unit test cases. * Resolved review comments.

…c-inclusion

* fixed pylint issues TDL-15447 * changing methods to functions since there is no-self-use * pylint issues * more pylint issues * added pylint disable comment for some scenarios * remove duplicate-code from config.yaml * pylint comment for test_bookmarks * remove duplicate code * removed wrong-import-order

* Qa/save test artifacts (singer-io#201) * add logging and save test logs as artifacts * name should be static method so it plays well with BaseCase Co-authored-by: kspeer <kspeer@stitchdata.com> * skip fields missing from schema for contact_lists (singer-io#202) Co-authored-by: kspeer <kspeer@stitchdata.com> * add limitExempt to all fields test workaround (singer-io#203) Co-authored-by: kspeer <kspeer@stitchdata.com> * TDL-19319 Make replication key as automatic inclusion (singer-io#195) * TDL 19241 add version timestamp in contacts (singer-io#191) * Added versionTimestamp in contacts stream. * Updated bookmark test case. * Updated comments. * Updated replication method of contacts stream. * TDL 16686 Implement request timeout (singer-io#177) * Initial commit for request_timeout * Updated unit test case error * Removed path of unittest file * Updated config file * Updated readme file * Initial commit. * Updated interruptes sync offset test case * Resolved comments. * Replaced replication key of deals stream with property_hs_lastmodifieddate field. * Updated discover test case. * Updated ingterrupted_sync test case. * Utilize bookmark of existing connection (singer-io#196) * Clearing bookmark for existing connection. * Updated bookmark logic to utilize older bookmark key. * Updated logic to keep older bookmark key in the state. * Added unittest cases. * Updated unit test cases. * Resolved review comments. * Updated get_start to use old bookmark for companies stream method. * Tdl 15447 fix pylint issues (singer-io#204) * fixed pylint issues TDL-15447 * changing methods to functions since there is no-self-use * pylint issues * more pylint issues * added pylint disable comment for some scenarios * remove duplicate-code from config.yaml * pylint comment for test_bookmarks * remove duplicate code * removed wrong-import-order Co-authored-by: kethan1122 <105211331+kethan1122@users.noreply.github.com> --------- Co-authored-by: Kyle Speer <54034650+kspeer825@users.noreply.github.com> Co-authored-by: kspeer <kspeer@stitchdata.com> Co-authored-by: Prijen Khokhani <88327452+prijendev@users.noreply.github.com> Co-authored-by: kethan1122 <105211331+kethan1122@users.noreply.github.com>

prijendev and others added 4 commits June 6, 2022 14:41

TDL 19241 add version timestamp in contacts (#191)

752a177

* Added versionTimestamp in contacts stream. * Updated bookmark test case. * Updated comments. * Updated replication method of contacts stream.

TDL 16686 Implement request timeout (#177)

5842f35

* Initial commit for request_timeout * Updated unit test case error * Removed path of unittest file * Updated config file * Updated readme file

Merge remote-tracking branch 'origin/master' into crest-master

a2e2718

Initial commit.

a8e4f09

prijendev changed the title ~~Initial commit.~~ TDL-19319 Make replication key as automatic inclusion Jun 7, 2022

prijendev commented Jun 7, 2022

View reviewed changes

Updated interruptes sync offset test case

1cabf60

prijendev requested review from dbshah1212, namrata270998 and savan-chovatiya June 7, 2022 10:02

dbshah1212 approved these changes Jun 7, 2022

View reviewed changes

prijendev requested a review from karanpanchal-crest June 7, 2022 10:06

NevilParikh14 reviewed Jun 7, 2022

View reviewed changes

tap_hubspot/__init__.py Outdated Show resolved Hide resolved

namrata270998 approved these changes Jun 7, 2022

View reviewed changes

karanpanchal-crest suggested changes Jun 7, 2022

View reviewed changes

Resolved comments.

1de4991

prijendev requested a review from NevilParikh14 June 7, 2022 12:51

karanpanchal-crest approved these changes Jun 7, 2022

View reviewed changes

NevilParikh14 approved these changes Jun 7, 2022

View reviewed changes

kspeer825 reviewed Jun 8, 2022

View reviewed changes

prijendev requested a review from kspeer825 June 8, 2022 13:47

savan-chovatiya approved these changes Jun 13, 2022

View reviewed changes

dbshah1212 requested a review from KrisPersonal June 15, 2022 05:36

prijendev changed the base branch from crest-master to master June 15, 2022 06:41

Merge branch 'master' into TDL-19319-make-replication-key-as-automati…

8a52b06

…c-inclusion

kspeer825 reviewed Jun 28, 2022

View reviewed changes

prijendev requested a review from kspeer825 June 29, 2022 14:42

prijendev and others added 3 commits July 1, 2022 16:40

Merge branch 'master' into TDL-19319-make-replication-key-as-automati…

82f7e80

…c-inclusion

Replaced replication key of deals stream with property_hs_lastmodifie…

2f13bd2

…ddate field.

Merge branch 'TDL-19319-make-replication-key-as-automatic-inclusion' of

9d797ce

https://github.com/singer-io/tap-hubspot into TDL-19319-make-replication-key-as-automatic-inclusion

prijendev added 2 commits July 1, 2022 17:43

Updated discover test case.

91bc9e2

Updated ingterrupted_sync test case.

c833659

kspeer825 approved these changes Jul 13, 2022

View reviewed changes

prijendev and others added 3 commits July 29, 2022 11:16

Utilize bookmark of existing connection (#196)

b7985ed

* Clearing bookmark for existing connection. * Updated bookmark logic to utilize older bookmark key. * Updated logic to keep older bookmark key in the state. * Added unittest cases. * Updated unit test cases. * Resolved review comments.

Merge branch 'master' into TDL-19319-make-replication-key-as-automati…

918bdcf

…c-inclusion

Updated get_start to use old bookmark for companies stream method.

68bcff7

dbshah1212 requested a review from cosimon August 10, 2022 09:32

cosimon approved these changes Sep 6, 2022

View reviewed changes

prijendev and others added 2 commits September 8, 2022 20:44

Merge branch 'master' into TDL-19319-make-replication-key-as-automati…

4e5b818

…c-inclusion

kethan1122 merged commit 681bc8f into master Sep 13, 2022

HarrisonMarcRose deleted the TDL-19319-make-replication-key-as-automatic-inclusion branch October 20, 2022 15:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TDL-19319 Make replication key as automatic inclusion #195

TDL-19319 Make replication key as automatic inclusion #195

prijendev commented Jun 7, 2022

prijendev Jun 7, 2022

karanpanchal-crest Jun 7, 2022

prijendev Jun 7, 2022

karanpanchal-crest Jun 7, 2022

kspeer825 Jun 8, 2022

prijendev Jun 8, 2022

kspeer825 Jun 15, 2022

prijendev Jun 16, 2022

kspeer825 Jun 28, 2022

prijendev Jun 29, 2022

kspeer825 Jul 8, 2022

prijendev Jul 11, 2022 •

edited

Loading

kspeer825 left a comment

TDL-19319 Make replication key as automatic inclusion #195

TDL-19319 Make replication key as automatic inclusion #195

Conversation

prijendev commented Jun 7, 2022

Description of change

Manual QA steps

Risks

Rollback steps

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prijendev Jul 11, 2022 • edited Loading

Choose a reason for hiding this comment

kspeer825 left a comment

Choose a reason for hiding this comment

prijendev Jul 11, 2022 •

edited

Loading