Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Source Hubspot: fix for property_history stream #13596

Merged

Conversation

davydov-d
Copy link
Collaborator

@davydov-d davydov-d commented Jun 8, 2022

What

PopertyHistory stream did not emit any records
https://github.com/airbytehq/oncall/issues/259

How

  • First of all, the API response did not return the versions field we use to yield records from. That's because propertyMode was not explicitly set - now fixed. Method list, which was probably designed for it, was never used, so I dropped it.
  • Also, GET param property was for some reason named properties and the values were passed separated with commas instead of &. Now fixed, it allows us to get more detailed response.
  • Finally, set of stream properties was unavailable because the entity attribute was not set. Now fixed.
  • When querying the API, streams that do have properties provided by that same API, unconditionally were splitting them into multiple queries - no matter how long the url length was. Now it's done only in case the URL length exceeds the allowed length.
  • After splitting the properties into multiple requests, the records were grouped by the primary key no matter whether the stream had PK or not. Now it's done only if the stream has a PK and the records should not be denormalized (multiple records are yielded from a single response entity - introduced denormalize_records attribute for that).
  • For some unknown reason the cursor_field of property_history used to be of type date-time (although originally this field is of type timestamp in the API) what led to buggy behavior when reading incrementally. Rolled back the field type, changed the schema.
  • Fixed unit tests
  • Updated integration tests

@github-actions github-actions bot added area/connectors Connector related issues area/documentation Improvements or additions to documentation labels Jun 8, 2022
@davydov-d
Copy link
Collaborator Author

davydov-d commented Jun 8, 2022

/test connector=connectors/source-hubspot

🕑 connectors/source-hubspot https://github.com/airbytehq/airbyte/actions/runs/2460617779
❌ connectors/source-hubspot https://github.com/airbytehq/airbyte/actions/runs/2460617779
🐛 https://gradle.com/s/xi6ixbpejrgoc

Build Failed

Test summary info:

=========================== short test summary info ============================
FAILED test_core.py::TestBasicRead::test_read[inputs1] - AssertionError: All ...
=================== 1 failed, 29 passed in 183.45s (0:03:03) ===================

@codecov
Copy link

codecov bot commented Jun 8, 2022

Codecov Report

❗ No coverage uploaded for pull request base (master@9a4582a). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master   #13596   +/-   ##
=========================================
  Coverage          ?   90.57%           
=========================================
  Files             ?        5           
  Lines             ?      912           
  Branches          ?        0           
=========================================
  Hits              ?      826           
  Misses            ?       86           
  Partials          ?        0           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9a4582a...0817fae. Read the comment docs.

@davydov-d
Copy link
Collaborator Author

davydov-d commented Jun 8, 2022

/test connector=connectors/source-hubspot

🕑 connectors/source-hubspot https://github.com/airbytehq/airbyte/actions/runs/2460709143
✅ connectors/source-hubspot https://github.com/airbytehq/airbyte/actions/runs/2460709143
Python tests coverage:

Name                                                 Stmts   Miss  Cover
------------------------------------------------------------------------
source_acceptance_test/utils/__init__.py                 6      0   100%
source_acceptance_test/tests/__init__.py                 4      0   100%
source_acceptance_test/__init__.py                       2      0   100%
source_acceptance_test/tests/test_full_refresh.py       52      2    96%
source_acceptance_test/utils/asserts.py                 37      2    95%
source_acceptance_test/config.py                        77      6    92%
source_acceptance_test/utils/json_schema_helper.py     105     13    88%
source_acceptance_test/tests/test_incremental.py       121     25    79%
source_acceptance_test/utils/common.py                  80     17    79%
source_acceptance_test/tests/test_core.py              294    106    64%
source_acceptance_test/utils/compare.py                 62     23    63%
source_acceptance_test/base.py                          10      4    60%
source_acceptance_test/utils/connector_runner.py       110     48    56%
------------------------------------------------------------------------
TOTAL                                                  960    246    74%
Name                         Stmts   Miss  Cover
------------------------------------------------
source_hubspot/errors.py         6      0   100%
source_hubspot/__init__.py       2      0   100%
source_hubspot/streams.py      776     62    92%
source_hubspot/helpers.py       30      3    90%
source_hubspot/source.py        97     21    78%
------------------------------------------------
TOTAL                          911     86    91%

Build Passed

Test summary info:

All Passed

@davydov-d davydov-d self-assigned this Jun 8, 2022
@davydov-d davydov-d requested review from erica-airbyte, jnr0790, Phlair and girarda and removed request for grubberr, midavadim and jnr0790 June 8, 2022 14:18
Copy link
Contributor

@girarda girarda left a comment

Choose a reason for hiding this comment

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

thank you @davydov-d this is great!

@davydov-d
Copy link
Collaborator Author

davydov-d commented Jun 9, 2022

/test connector=connectors/source-hubspot

🕑 connectors/source-hubspot https://github.com/airbytehq/airbyte/actions/runs/2466334037
✅ connectors/source-hubspot https://github.com/airbytehq/airbyte/actions/runs/2466334037
Python tests coverage:

Name                                                 Stmts   Miss  Cover
------------------------------------------------------------------------
source_acceptance_test/utils/__init__.py                 6      0   100%
source_acceptance_test/tests/__init__.py                 4      0   100%
source_acceptance_test/__init__.py                       2      0   100%
source_acceptance_test/tests/test_full_refresh.py       52      2    96%
source_acceptance_test/utils/asserts.py                 37      2    95%
source_acceptance_test/config.py                        77      6    92%
source_acceptance_test/utils/json_schema_helper.py     105     13    88%
source_acceptance_test/tests/test_incremental.py       121     25    79%
source_acceptance_test/utils/common.py                  80     17    79%
source_acceptance_test/tests/test_core.py              294    106    64%
source_acceptance_test/utils/compare.py                 62     23    63%
source_acceptance_test/base.py                          10      4    60%
source_acceptance_test/utils/connector_runner.py       110     48    56%
------------------------------------------------------------------------
TOTAL                                                  960    246    74%
Name                         Stmts   Miss  Cover
------------------------------------------------
source_hubspot/errors.py         6      0   100%
source_hubspot/__init__.py       2      0   100%
source_hubspot/streams.py      776     62    92%
source_hubspot/helpers.py       31      3    90%
source_hubspot/source.py        97     21    78%
------------------------------------------------
TOTAL                          912     86    91%

Build Passed

Test summary info:

All Passed

@davydov-d
Copy link
Collaborator Author

davydov-d commented Jun 9, 2022

/publish connector=connectors/source-hubspot

🕑 connectors/source-hubspot https://github.com/airbytehq/airbyte/actions/runs/2466388428
🚀 Successfully published connectors/source-hubspot
🚀 Auto-bumped version for connectors/source-hubspot
✅ connectors/source-hubspot https://github.com/airbytehq/airbyte/actions/runs/2466388428

@octavia-squidington-iii octavia-squidington-iii temporarily deployed to more-secrets June 9, 2022 06:47 Inactive
@davydov-d davydov-d merged commit 922bf46 into master Jun 9, 2022
@davydov-d davydov-d deleted the ddavydov/#259-oncall-hubspot-fix-property-history-stream branch June 9, 2022 07:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/connectors Connector related issues area/documentation Improvements or additions to documentation connectors/source/hubspot
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants