🎉 Source Shopify: migrate products, product images and product variants to GraphQL BULK

[bazarnov]

What

Resolving:

• https://github.com/airbytehq/airbyte-internal-issues/issues/7353
• https://github.com/airbytehq/airbyte-internal-issues/issues/7354
• https://github.com/airbytehq/airbyte-internal-issues/issues/7355

How

• migrated Products, Product Images and Product Variants to Shopify GQL BULK
• removed the parent dependency (Product) for the product images and product variants streams, they should be independent to have the proper migration process
• extended the query.py (BULK Queries module) with 3 more classes for Product, ProductImage and ProductVariant queries
• changed the references for the stream.py to make the migrated streams inherit from the IncrementalShopifyGraphQlBulkStream
• added 3 test cases to cover the change
• fixed the tests that were affected by the change

User Impact

There is a BREAKING CHANGE for the product variants stream STATE:

Before:

"id": 112233

Now:

"updated_at": "2024-01-01T01:01:01+00:00"

Can this PR be safely reverted and rolled back?

• YES 💚,

Because the old STATE for id based values is still there for the STATE object, we can safely roll back when something is wrong.

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
airbyte-docs	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	May 14, 2024 8:38am

[bazarnov]

@katmarkham Could you please take a look at the user-facing message about the breaking change? Thank you.

[bazarnov]

The pre-release version has been published successfully: 2.1.0-dev.805cd2979d
Action run: https://github.com/airbytehq/airbyte/actions/runs/9014252053

[maxi297]

I feel uneasy to accept those live testing results without explaining those gaps:

1adee9d6-d8b1-4425-85a7-e13aebf7331e and 80be7c99-7a87-41e9-bae9-b0e61e7be4c4

The HTTP requests are still all the same i.e. I still don't see any request to products.json in the control version which makes me wonder if the stream is tested at all... Can you explain why we don't see HTTP requests to /products.json

d82df6a0-1b94-4608-9e63-e9f0d57ec652

It says product_variants stream: records with primary key in target & control whose values differ but the diff does not highlight this change. I would also expect to see this discrepancy in 81a5acbb-4374-497d-9af2-297ea8e7f6a7 and a7d4dcfe-39bb-4803-8e49-18ae07e838b2. Do we know why this change is not picked up by the live testing?

[bazarnov]

Because of the inability to test using live-testing tool (permissions issues), the manual changes report was conducted.

CHANGES (REGRESSION) TEST PLAN

1. Make common CONFIG
2. Make common CATALOG with isolated target (affected) stream
3. Run-Test each affected stream against the Test Criteria. Affected streams: Products, Product Images, Product Variants

COMMANDS:

FULL-REFRESH

## CONTROL

docker run --rm -v $(pwd)/secrets:/secrets -v $(pwd)/integration_tests:/integration_tests airbyte/source-shopify:latest read --config /secrets/config.json --catalog /integration_tests/configured_catalog.json --debug > control.json

## TARGET

docker run --rm -v $(pwd)/secrets:/secrets -v $(pwd)/integration_tests:/integration_tests airbyte/source-shopify:dev read --config /secrets/config.json --catalog /integration_tests/configured_catalog.json --debug > target.json

INCREMENTAL

## CONTROL

docker run --rm -v $(pwd)/secrets:/secrets -v $(pwd)/integration_tests:/integration_tests airbyte/source-shopify:latest read --config /secrets/config.json --catalog /integration_tests/configured_catalog.json --state /integration_tests/control_state.json --debug > control_inc.json

## TARGET

docker run --rm -v $(pwd)/secrets:/secrets -v $(pwd)/integration_tests:/integration_tests airbyte/source-shopify:dev read --config /secrets/config.json --catalog /integration_tests/configured_catalog.json --state /integration_tests/target_state.json --debug > target_inc.json

Test Criteria:

1. Outgoing API request example:

   • 1.1) CONTROL
   • 1.2) TARGET
   • Explain the diff

2. Sync mode: Full-Refresh

   • 2.1) Number of records:

     • 2.1.1) CONTROL
     • 2.1.2) TARGET
     • Explain the diff

   • 2.2) STATE comparison:

     • 2.2.1) CONTROL
     • 2.2.2) TARGET
     • Explain the diff

   • 2.3) Missing records:

     • 2.3.1) CONTROL
     • 2.3.2) TARGET
     • Explain the diff

3. Sync mode: Incremental

   • 3.1) Number of records:

     • 3.1.1) CONTROL
     • 3.1.2) TARGET
     • Explain the diff

   • 3.2) STATE comparison:

     • 3.2.1) CONTROL
     • 3.2.2) TARGET
     • Explain the diff

   • 3.3) Missing records:

     • 3.3.1) CONTROL
     • 3.3.2) TARGET
     • Explain the diff

4. Record example:

   • 4.1) CONTROL
   • 4.2) TARGET

5. Schema changes:

   • 5.1) Added fields
   • 5.2) Changes types
   • 5.3) Removed fields
   • Explain the diff

6. Is this a breaking change:

   • [] Yes
   • [] No

Test Results ( PII - free ):

• Products stream
• Product Images stream
• Product Variants stream

cc @maxi297

[maxi297]

Shit Baz! Big thumbs up on documenting the changes for this PR. I really appreciate it as it help review and go back to those changes so that we understand why it was done like that. Kudos!

Based on the report:

The difference in records is explained by the fact we don't have the possibility to fetch the deleted records, using GraphQL. The deleted records, are available using REST only.

Should we add that to the migration notes?

[maxi297]

I think position is also missing based on the output you provided, right?

[bazarnov]

It's not present in the record for the TARGET version, but schema didn't change. I'll add this mention.

[bazarnov]

Corrected.

[matiasferrin]

Hello, position is not missing but it always returns null values. Could you please investigate?

[maxi297]

Based on the sample you provided, the control version has field image_id while the target has image. Is this expected? Should we document this as well?

[bazarnov]

No, added the handling of that, we basically need to have image_id at the root level, but graphql nests it in "image":{"image_id": "gid://shopify/ProductImage/123456"}. We don't have this populated for our sandbox, therefore it was Null for the data and wasn't processed by far. Thanks for the notice!

Corrected. Updated the dedicated unit_test to reflect this and updated the branch.

[loris]

@bazarnov @maxi297 quick question: since there are breaking changes, it should have been included in a 3.0 release no? I can't find how Airbyte Cloud handles connector upgrade, is this automated?

[maxi297]

Hi @loris ! The breaking changes were introduced as part of 2.1.0. As this is a breaking change, the opt-in mechanism on cloud should kick in and you should see a prompt asking you to upgrade before 2024-06-10

[loris]

@maxi297 Thanks for the clarification, I'm in discussion with cloud support because this is clearly not working as expected. In our production workspace, connections have been upgraded to 2.2 without any prompt (and thus broke our pipeline), while on our other workspaces, connections are still running Shopify 2.0 connector and we don't see any Upgrade prompt popping up.

[loris]

@maxi297 Actually I might have found the issue:

      2.1.0:
        message:
          "This upgrade changes the `Products`, `Product Images` and `Product Variants` streams to use `Shopify GraphQL BULK`.
          More details here: https://github.com/airbytehq/airbyte/pull/37767."
        upgradeDeadline: "2024-06-10"
        scopedImpact:
          - scopeType: stream
            impactedScopes: ["product_variants"]

The breaking change message only states product_variants in the impactedScopes, while products was also clearly impacted. Our Airbyte connections only needed to sync products (as the variants where part of that stream too). But with the change, we lost the variant data and were not informed of the breaking change because the impactedScopes did not included products, does this seem right?

[bazarnov]

@loris This message included the breaking change that breaks the source sync operations only, the info about the data was re-arranged based on the API changes are not included in this message.

In the migration guide here: https://github.com/airbytehq/airbyte/blob/e9e6cf3778caf804bc5d433ccda1530de1373a6d/docs/integrations/sources/shopify-migrations.md which comes with this release, we included all the details about the actual data change we discovered. Have you had a chance to observe this in the UI?

Hope that answers your question and thank you for your feedback.

[loris]

Very clear!

Have you had a chance to observe this in the UI?

This is the part which is confusing to me: No, I did not observe anything in the UI. I also have a workspace where Shopify connector are still running the 2.0 version and did not see any Upgrade banner. I have the email/webhook notifications set up, but nothing there too.
We are waiting for a reply of Airbyte Cloud support and will let you know

[maxi297]

This information will be helpful for us a well as maybe they is a gap we are not aware related to the opt-in mechanism. Please, keep us up to date on this one

[loris]

@bazarnov Hi, out of curiosity, is this normal to have hard-coded updated_at values here?

[bazarnov]

For the 'docstring' query example, I believe - yes.

Does this look wrong to you?
Should we have dynamic docstring for such cases?

If you can contribute to have the examples like this constructed dynamically, we would appreciate such improvements.

Thank you for your question, and feel free to add more.