Source Amazon Seller Partner: GET_SELLER_FEEDBACK_DATA normalize header field names #9212
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
area/connectors
test results |
Closed
alafanechere
suggested changes
Dec 30, 2021
There was a problem hiding this comment.
Thank @lizdeika for the fixes!
I have a stability concern with the workaround you found. Let me know what you think about my suggestions.
I confirm that acceptance tests are passing in our CI, and I regret that our sandbox account does not have enough data to be able to detect the bug you found with "real-life" data. Maybe we can share on Slack some suggestions on how we could set this up to be more robust.
| # and skip the original header with next() | ||
| @staticmethod | ||
| def parse_document(document): | ||
| reader = csv.DictReader(StringIO(document), delimiter="\t", fieldnames=SellerFeedbackReports.NORMALIZED_FIELD_NAMES) |
There was a problem hiding this comment.
This is a working workaround but not 100% safe if Amazon changes the structure of the CSV file. And I'm not sure that this API has versioning that could allow us to avoid these breaking changes.
Do you think you could raise an error if the length of NORMALIZED_FIELD_NAMES is different from the number of columns in the CSV file?
Do you have access to all the localized values? It could make the normalization safer by applying a mapping between localized and normalized values.
There was a problem hiding this comment.
Yes with amazon seller partner apis there is no certainty at all :D
Agreed with error raising on column count mismatch.
But mapping values is almost impossible, values are random, are not guaranteed to be the same tomorrow and I do not have access to all possible marketplaces to get them anyway.
There was a problem hiding this comment.
@alafanechere added header field count error raising on count mismatch
…_FEEDBACK_DATA-normalize-headers-field-names
alafanechere
approved these changes
Jan 3, 2022
|
Thank you @lizdeika for the error handling and this nice fix. Just merged! |
lizdeika
deleted the
amazon-seller-partner-GET_SELLER_FEEDBACK_DATA-normalize-headers-field-names
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What
Examples:
A1PA6795UKMFR9DE:A1RKKUPIHCS9HSES:A1AM78C64UM0Y8MX:etc.
How
Luckily columns come in the same order in all marketplaces.
We pass fieldnames to DictReader so we always have the same header field names.
Fix date formats
Recommended reading order
🚨 User Impact 🚨
Pre-merge Checklist
Updating a connector
Community member or Airbyter
airbyte_secret./gradlew :airbyte-integrations:connectors:<name>:integrationTest.README.mdbootstrap.md. See description and examplesdocs/integrations/<source or destination>/<name>.mdincluding changelog. See changelog exampleAirbyter
If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.
/test connector=connectors/<name>command is passing./publishcommand described here