🎉 Source Amazon Seller Partner: add replication end date to config #13059
Conversation
github-actions
bot
added
the
area/documentation
…azon-seller-partner-end-date
There was a problem hiding this comment.
Thank you for the contribution @ganpatagarwal
I've two main concerns:
- I don't think it's good practice to ask users to manually set the value of this new field to
default. I don't think your new fields requires a default, the connector should just handle aNonevalue if the user does not set this field. - Setting a replication end date for an incremental streaùs mode sounds a bit awkward to me. What do you think about checking the stream sync mode and set the replication end date only when the sync mode is full refresh?
| "pattern": "(^[0-9]{4}-[0-9]{2}-[0-9]{2}T[0-9]{2}:[0-9]{2}:[0-9]{2}Z$|default)", | ||
| "examples": ["default", "2017-02-25T00:00:00Z"], | ||
| "type": "string", | ||
| "default": "default" |
There was a problem hiding this comment.
I don't think you need to set a default, I'd suggest you remove the default value and handle it as a None value in the config parsing logic. Not required config option default to None if they are not set by the user.
There was a problem hiding this comment.
None is much better than default 👍
Changes done
| @@ -41,6 +41,7 @@ def __init__( | |||
| url_base: str, | |||
| aws_signature: AWSSignature, | |||
| replication_start_date: str, | |||
| replication_end_date: str, | |||
There was a problem hiding this comment.
| replication_end_date: str, | |
| replication_end_date: Optional[str], |
There was a problem hiding this comment.
Changes are done !!
| @@ -149,6 +161,7 @@ def __init__( | |||
| url_base: str, | |||
| aws_signature: AWSSignature, | |||
| replication_start_date: str, | |||
| replication_end_date: str, | |||
There was a problem hiding this comment.
| replication_end_date: str, | |
| replication_end_date: Optional[str], |
There was a problem hiding this comment.
Changes are done !!
| "reportType": self.name, | ||
| "marketplaceIds": [self.marketplace_id], | ||
| "dataStartTime": replication_start_date.strftime(DATE_TIME_FORMAT), | ||
| } | ||
|
|
||
| if is_valid_replication_end_date(self._replication_end_date): | ||
| params["dataEndTime"] = self._replication_end_date | ||
| params["dataStartTime"] = self._replication_start_date |
There was a problem hiding this comment.
| params["dataStartTime"] = self._replication_start_date |
Why are you setting the dataStartTime again? It's already set above.
There was a problem hiding this comment.
replication_start_date = max(pendulum.parse(self._replication_start_date), pendulum.now("utc").subtract(days=90))
With above logic, any start date older than 90 days would be overwritten.
If an user puts a start & end date older than 90 days, we would want to honour that.
There was a problem hiding this comment.
Ok, could you please add a comment in the code to explain this logic?
| return params | ||
|
|
||
| def parse_response(self, response: requests.Response, stream_state: Mapping[str, Any], **kwargs) -> Iterable[Mapping]: | ||
| yield from response.json().get(self.data_field, {}).get("shippingLabels", []) | ||
|
|
||
|
|
||
| def is_valid_replication_end_date(custom_date: str) -> bool: |
There was a problem hiding this comment.
I don't think this function should exist, the regex pattern in the spec.json should already guarantee a valid input.
If you still think this is required, please call this function once in the __init__ of the stream class, avoid calling it multiple times.
There was a problem hiding this comment.
totally agree!!
have removed it
|
/test connector=connectors/source-amazon-seller-partner
|
|
I'm requesting a review from @misteryeo as this PR is touching the |
cleanup based on review comments
…azon-seller-partner-end-date
There was a problem hiding this comment.
Thanks @alafanechere for the review.
I have tried to incorporate the suggested changes. PTAL
| "pattern": "(^[0-9]{4}-[0-9]{2}-[0-9]{2}T[0-9]{2}:[0-9]{2}:[0-9]{2}Z$|default)", | ||
| "examples": ["default", "2017-02-25T00:00:00Z"], | ||
| "type": "string", | ||
| "default": "default" |
There was a problem hiding this comment.
None is much better than default 👍
Changes done
| @@ -41,6 +41,7 @@ def __init__( | |||
| url_base: str, | |||
| aws_signature: AWSSignature, | |||
| replication_start_date: str, | |||
| replication_end_date: str, | |||
There was a problem hiding this comment.
Changes are done !!
| @@ -149,6 +161,7 @@ def __init__( | |||
| url_base: str, | |||
| aws_signature: AWSSignature, | |||
| replication_start_date: str, | |||
| replication_end_date: str, | |||
There was a problem hiding this comment.
Changes are done !!
| "reportType": self.name, | ||
| "marketplaceIds": [self.marketplace_id], | ||
| "dataStartTime": replication_start_date.strftime(DATE_TIME_FORMAT), | ||
| } | ||
|
|
||
| if is_valid_replication_end_date(self._replication_end_date): | ||
| params["dataEndTime"] = self._replication_end_date | ||
| params["dataStartTime"] = self._replication_start_date |
There was a problem hiding this comment.
replication_start_date = max(pendulum.parse(self._replication_start_date), pendulum.now("utc").subtract(days=90))
With above logic, any start date older than 90 days would be overwritten.
If an user puts a start & end date older than 90 days, we would want to honour that.
| return params | ||
|
|
||
| def parse_response(self, response: requests.Response, stream_state: Mapping[str, Any], **kwargs) -> Iterable[Mapping]: | ||
| yield from response.json().get(self.data_field, {}).get("shippingLabels", []) | ||
|
|
||
|
|
||
| def is_valid_replication_end_date(custom_date: str) -> bool: |
There was a problem hiding this comment.
totally agree!!
have removed it
| params.update({"createdBefore": pendulum.now("utc").strftime(self.time_format)}) | ||
| end_date = pendulum.now("utc").strftime(self.time_format) | ||
| if self._replication_end_date: | ||
| end_date = self._replication_end_date | ||
| params.update({self.replication_end_date_field: end_date}) |
There was a problem hiding this comment.
On this stream the replication_start_date_field might be automatically set compared to now because The date range to search must not be more than 7 days.
I think you might also need to change the computation of the replication_start_date_field in the __init__ if _replication_end_date is set.
There was a problem hiding this comment.
After giving a closer look, I doubt that this API ever worked on Airbyte.
We are not setting createdAfter request parameter, which is required
https://sellingpartnerapi-na.amazon.com/vendor/directFulfillment/shipping/v1/shippingLabels?createdBefore=2022-05-24T10%3A26%3A22Z
self.replication_start_date_field = max(...)
This code is overwriting the value createdAfter, which should have been used to set request params.
We should move the logic to calculate start date to request_params(...) along with calculation and setting end date
Please let me know if my observation is correct and will make the necessary changes.
There was a problem hiding this comment.
I have made the changes. Please review and share your thoughts.
|
/test connector=connectors/source-amazon-seller-partner
|
|
@ganpatagarwal thank you for the changes you made. I made a final change request above. |
|
/test connector=connectors/source-amazon-seller-partner
Build PassedTest summary info: |
There was a problem hiding this comment.
Thank you for the change @ganpatagarwal . I made an additional minor change to the pattern to copy the existing pattern on start date.
|
/publish connector=connectors/source-amazon-seller-partner
|
What
*With a custom replication end date, users will be able to fetch data for a specified period.
*It will also help in batching the fetches while pulling report for a longer duration ,say 3 years.
How
*Most of the SP APIs support a parameter to provide end date, e.g “dataEndTime”, “LastUpdatedBefore”, etc.
*When a user provides a valid date time against
End Date(replication_end_date), it would be used with the APIs.*
End Date(replication_end_date)is an optional config option with a default value ofdefault. When default value is present, it would be treated as no end date is provided by user, and no specific end date parameter would get added to API calls.Recommended reading order
🚨 User Impact 🚨
No user impact, as
End Date(replication_end_date)config is optional. Also, default value makes sure to not include any end-date parameter.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 exampleTests
Unit
Integration
Acceptance