TDL-26523: Upgrade salesforce API version to v61 #184

prijendev · 2024-10-08T06:37:44Z

Description of change

Upgrading current salesforce API version 52.0 to 61.0
Following are the key changes
- A total of 258 new objects have been introduced in version 61.0.
- One object that is available in version 52.0 has been removed in version 61.0.
  - CustomObjectUserLicenseMetrics
- The following streams are currently available as INCREMENTAL but are not functioning correctly INCREMENTAL. Therefore, we will mark them as FULL_TABLE in the next release.
  - PermissionSetEventStore
  - ListViewEvent
  - IdentityProviderEventStore
  - ApiEvent
  - BulkApiResultEventStore
  - IdentityVerificationEvent
  - 'LoginAsEvent
- We do not have access to extract data for the following streams. It is mentioned here that objects associated with Data.com may throw errors on request.
  - DatacloudAddress
  - DatacloudDandBCompany
- We will skip the following streams for both REST and BULK type sync, as they are query-restricted objects.
  Although the following streams are listed as supported for Bulk API in the current version of the tap, they are not functioning as expected with the Bulk sync. So, will skip these streams in Bulk Sync
  - WorkStepStatus
  - FieldSecurityClassification
  - WorkOrderStatus
  - ShiftStatusWorkOrderLineItemStatus

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

prijendev · 2024-10-08T06:39:15Z

.circleci/config.yml

@@ -62,29 +62,25 @@ jobs:
          path: test_output/report.xml
      - store_artifacts:
          path: htmlcov
-  run_integration_tests:


In the parallel run, tests are failing. Switching them to run in a serialized manner.

cngpowered · 2024-10-10T11:35:06Z

We will skip the following streams for both REST and BULK type sync, as they are query-restricted objects.
Although the following streams are listed as supported for Bulk API in the current version of the tap, they are not functioning as expected with the Bulk sync. So, will skip these streams in Bulk Sync
WorkStepStatus
FieldSecurityClassification
WorkOrderStatus
ShiftStatusWorkOrderLineItemStatus

In the current version of the tap, are these streams able to extract records?

cngpowered · 2024-10-10T11:37:32Z

tap_salesforce/salesforce/__init__.py

@@ -111,6 +112,12 @@
                                           'FlowVariableView',
                                           'AppTabMember',
                                           'ColorDefinition',
+                                           'DatacloudDandBCompany', # Not filterable without a criteria.


Not filterable without a criteria

Is the comment Correct here?

This is an error message I have added over here. General comment is already written on top of this list

cngpowered · 2024-10-10T11:39:29Z

tap_salesforce/salesforce/__init__.py

@@ -235,7 +243,7 @@ def __init__(self,
        self.rest_requests_attempted = 0
        self.jobs_completed = 0
        self.login_timer = None
-        self.data_url = "{}/services/data/v52.0/{}"
+        self.data_url = "{}/services/data/v61.0/{}"


define a constant for this version number and utilize it in the tap

I think constant is not required here because It is just used for a single time in this module.

cngpowered · 2024-10-10T11:39:55Z

tap_salesforce/salesforce/bulk.py

@@ -41,7 +41,7 @@ def find_parent(stream):

 class Bulk():

-    bulk_url = "{}/services/async/52.0/{}"
+    bulk_url = "{}/services/async/61.0/{}"


define a constant for this version number and utilize it in the tap

I think constant is not required here because It is just used for a single time in this module.

cngpowered · 2024-10-10T11:40:21Z

tap_salesforce/salesforce/rest.py

@@ -28,7 +28,7 @@ def _query_recur(
            end_date=None,
            retries=MAX_RETRIES):
        params = {"q": query}
-        url = "{}/services/data/v52.0/queryAll".format(self.sf.instance_url)
+        url = "{}/services/data/v61.0/queryAll".format(self.sf.instance_url)


define a constant for this version number and utilize it in the tap

I think constant is not required here because It is just used for a single time in this module.

since this version is referenced multiple times, create a single constant where it can be referenced in all the modules.

Done, I have added a constant for the API version and utilized it for all other places.

cngpowered · 2024-10-10T11:42:54Z

tests/base.py

+            'IdentityProviderEventStore': default_full,  # new
+            'IdentityVerificationEvent': default_full,  # new


please provide link to the documentation of the streams that have changed replication methods

There is no documentation link for replication of these objects. Only when we run sync mode will we be able to know this behavior.

cngpowered · 2024-10-10T13:56:31Z

Please add a changelog and version bump in the PR

…//github.com/singer-io/tap-salesforce into TDL-26523/upgrade-api-version-from-52-to-v61

luandy64 · 2024-10-10T15:47:34Z

tap_salesforce/salesforce/__init__.py

@@ -199,6 +206,7 @@ def field_to_property_schema(field, mdata): # pylint:disable=too-many-branches

    return property_schema, mdata

+#pylint: disable=too-many-positional-arguments


I was skimming this PR and thought it was weird to disable a linter for the whole class. So I just wanted to note that this class level disable is to fix things for just two methods.

Here's a pylint run after deleting this line:

$ pylint tap_salesforce -d 'broad-except,broad-exception-caught,broad-exception-raised,chained-comparison,empty-docstring,fixme,invalid-name,line-too-long,missing-class-docstring,missing-function-docstring,missing-module-docstring,no-else-raise,no-else-return,too-few-public-methods,too-many-arguments,too-many-branches,too-many-lines,too-many-locals,ungrouped-imports,wrong-spelling-in-comment,wrong-spelling-in-docstring,consider-using-f-string,logging-fstring-interpolation,wrong-import-order,stop-iteration-return,logging-format-interpolation' ************* Module tap_salesforce.salesforce tap_salesforce/salesforce/__init__.py:211:4: R0917: Too many positional arguments (12/5) (too-many-positional-arguments) tap_salesforce/salesforce/__init__.py:292:4: R0917: Too many positional arguments (7/5) (too-many-positional-arguments) ------------------------------------------------------------------- Your code has been rated at 9.98/10 (previous run: 10.00/10, -0.02)

And here's the changes to get this back to a 10/10 rating

diff --git a/tap_salesforce/salesforce/__init__.py b/tap_salesforce/salesforce/__init__.py index f144b53..2a334cc 100644 --- a/tap_salesforce/salesforce/__init__.py +++ b/tap_salesforce/salesforce/__init__.py @@ -206,9 +206,8 @@ def field_to_property_schema(field, mdata): # pylint:disable=too-many-branches return property_schema, mdata -#pylint: disable=too-many-positional-arguments class Salesforce(): - # pylint: disable=too-many-instance-attributes,too-many-arguments + # pylint: disable=too-many-instance-attributes,too-many-arguments,too-many-positional-arguments def __init__(self, refresh_token=None, token=None, @@ -284,7 +283,7 @@ class Salesforce(): self.quota_percent_per_run) raise TapSalesforceQuotaExceededException(partial_message) - # pylint: disable=too-many-arguments + # pylint: disable=too-many-arguments,too-many-positional-arguments @backoff.on_exception(backoff.expo, (requests.exceptions.ConnectionError, requests.exceptions.Timeout), max_tries=10,

Maybe we should open a separate issue to clean up some of these inline disables. For example, too-many-arguments appears in $PYLINT_DISABLE_LIST and doesn't need to be inlined anymore

Moved these class level inline pylint disables to function level. Created separate ticket to clean these inline disables - TDL-26624

cngpowered · 2024-10-29T05:59:49Z

setup.py

@@ -3,7 +3,7 @@
 from setuptools import setup

 setup(name='tap-salesforce',
-      version='2.0.1',


This P.R qualify's for a minor version release, since we are changing a lot of things.
I will suggest to use 2.1.0 over 2.0.1

Agree. I have updated it

TDL-26523: Upgrade API version to v61

00002d6

prijendev commented Oct 8, 2024

View reviewed changes

prijendev added 2 commits October 9, 2024 12:03

Update sync canary test

1a27653

Skip EventLogFile from tests

0122f81

cngpowered reviewed Oct 10, 2024

View reviewed changes

prijendev added 2 commits October 10, 2024 19:28

Update changelog md

3c629c7

Merge branch 'TDL-26523/upgrade-api-version-from-52-to-v61' of https:…

ac9d892

…//github.com/singer-io/tap-salesforce into TDL-26523/upgrade-api-version-from-52-to-v61

prijendev requested a review from cngpowered October 10, 2024 13:59

luandy64 reviewed Oct 10, 2024

View reviewed changes

prijendev added 9 commits October 11, 2024 09:56

Define constant for api version and pylint fix

516a726

Update Bulk API

d537582

Consider some streams forcefully as FULL_TABLE

c946e96

Skip black listed objects during sync mode

77ec1ac

Update tap tester for newly added streams

822f450

Fix pylint issue

d11b7a2

Update tap-tester

a948f2f

Fix replication-key issue

dcd2fa5

Fix forced-replication-methods things

620de2d

cngpowered reviewed Oct 29, 2024

View reviewed changes

Update release minor version

2d14802

cngpowered approved these changes Oct 29, 2024

View reviewed changes

prijendev added 3 commits October 29, 2024 18:02

Fix sync canary tests

f52fd14

Fix sync canary tests

be341a9

Fix sync canary test

18c22d2

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TDL-26523: Upgrade salesforce API version to v61 #184

TDL-26523: Upgrade salesforce API version to v61 #184

prijendev commented Oct 8, 2024 •

edited

Loading

prijendev Oct 8, 2024

cngpowered commented Oct 10, 2024

cngpowered Oct 10, 2024

prijendev Oct 10, 2024

cngpowered Oct 10, 2024

prijendev Oct 10, 2024 •

edited

Loading

cngpowered Oct 10, 2024

prijendev Oct 10, 2024 •

edited

Loading

cngpowered Oct 10, 2024

prijendev Oct 10, 2024

cngpowered Oct 10, 2024

prijendev Oct 11, 2024

cngpowered Oct 10, 2024

prijendev Oct 10, 2024

cngpowered commented Oct 10, 2024

luandy64 Oct 10, 2024

prijendev Oct 11, 2024

cngpowered Oct 29, 2024

prijendev Oct 29, 2024

		'IdentityProviderEventStore': default_full, # new
		'IdentityVerificationEvent': default_full, # new

		@@ -199,6 +206,7 @@ def field_to_property_schema(field, mdata): # pylint:disable=too-many-branches

		return property_schema, mdata

		#pylint: disable=too-many-positional-arguments

TDL-26523: Upgrade salesforce API version to v61 #184

Are you sure you want to change the base?

TDL-26523: Upgrade salesforce API version to v61 #184

Conversation

prijendev commented Oct 8, 2024 • edited Loading

Description of change

QA steps

Risks

Rollback steps

AI generated code

Choose a reason for hiding this comment

cngpowered commented Oct 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prijendev Oct 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prijendev Oct 10, 2024 • edited Loading

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

cngpowered commented Oct 10, 2024

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 commented Oct 8, 2024 •

edited

Loading

prijendev Oct 10, 2024 •

edited

Loading

prijendev Oct 10, 2024 •

edited

Loading