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

Changed product.external_id to string #13

Merged
merged 2 commits into from
Nov 17, 2021
Merged

Conversation

ericdigr
Copy link
Contributor

@ericdigr ericdigr commented Nov 16, 2021

Description of change

product.external_id is defined as a string in the BigCommerce v2 API documentation. This request is related to issue #14, where external_id's in the BigCommerce source are strings and the insert fails for these orders.

The BigCommerce API docs which indicate the external_id is a string [(https://developer.bigcommerce.com/api-reference/store-management/orders/orders/createanorder)]

Screenshot of the API doc:
https://share.getcloudapp.com/v1ujPmGe

Manual QA steps

Verify string external_ids are being passed through the Tap

Risks

Rollback steps

  • revert this branch

@cmerrick
Copy link
Contributor

Hi @ericdigr, thanks for your contribution!

In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes.

@Maynkmohta
Copy link

https://share.getcloudapp.com/v1ujPmGe
documentation --- https://developer.bigcommerce.com/api-reference/store-management/orders/orders/createanorder
looks like it is not defined correctly in our tap code as per documentation

@Maynkmohta
Copy link

we are using stitch and we have tons of similar errors — error logs

https://share.getcloudapp.com/bLudleG7

Copy link
Contributor

@dmosorast dmosorast left a comment

Choose a reason for hiding this comment

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

Just one change, also @ericdigr in order to merge this, I will need you to sign the CLA first. Thanks for contributing!

@@ -442,7 +442,7 @@
}
},
"external_id": {
"$ref": "type-integer.json"
"$ref": "type-string.json"
Copy link
Contributor

Choose a reason for hiding this comment

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

I did some digging, and it looks like this specific issue isn't widespread. Given that this is pretty impactful, since the tap seems to silently drop all data that doesn't match the schema, I'm willing to merge it, as long as the change is backwards compatible. In this case, that would mean making the schema a fallback from most specific to least specific, e.g., something like {"type": ["integer", "string", "null"]}.

If you could make that change, and show redacted logs of the tap extracting data successfully (to ensure the JSON schema syntax is correct), then I would be willing to merge this change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dmosorast changes made to row 444 with fallback

@cmerrick
Copy link
Contributor

You did it @ericdigr!

Thank you for signing the Singer Contribution License Agreement.

@dmosorast dmosorast merged commit 64f86f1 into singer-io:master Nov 17, 2021
@dmosorast dmosorast mentioned this pull request Nov 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants