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

fix(DataStore): dataStore cannot connect to model's sync subscriptions (AWS_LAMBDA auth type) #3550

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

MuniekMg
Copy link
Contributor

@MuniekMg MuniekMg commented Mar 5, 2024

Bug: #3549

General Checklist

  • Added new tests to cover change, if needed
  • Build succeeds with all target using Swift Package Manager
  • All unit tests pass
  • All integration tests pass
  • PR title conforms to conventional commit style

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@MuniekMg MuniekMg requested a review from a team as a code owner March 5, 2024 15:54
@lawmicha
Copy link
Member

lawmicha commented Mar 6, 2024

Hi @MuniekMg, thanks for opening this PR. I believe some AppSync backends were provisioned without the filter input available on subscriptions, which is why client side filtering with the sync expression was done on all subscription events coming back instead of translating the sync expression to the filter input for the subscription request.

Most likely we will have to introduce a new configuration property on DataStoreConfiguration such as subscriptionFiltering: Bool to allow developers to opt into this feature. We'll discuss internally and get back to you

@MuniekMg
Copy link
Contributor Author

MuniekMg commented Mar 7, 2024

Hi @lawmicha,

Most likely we will have to introduce a new configuration property on DataStoreConfiguration such as subscriptionFiltering: Bool to allow developers to opt into this feature. We'll discuss internally and get back to you

I have added DataStoreConfiguration.subscriptionFiltering in the last commit. What do you think?

[EDIT]
oh.... I just realized that you said "We'll discuss internally and get back to you"... ok, so maybe ignore my last commit ^^

@lawmicha
Copy link
Member

lawmicha commented Mar 7, 2024

@MuniekMg, I editted my message to mention i'll check with the team, maybe you missed it since I had editted sometime later, sorry for the extra work! I was able to check with the team and found out the approach that Amplify JS library is taking.

  1. We attempt the subscription with the filter and catch ther error
  2. Handle the specific "RTF" error as a retry without the filter for backwards compatibility.

References
https://github.com/aws-amplify/amplify-js/blob/main/packages/datastore/src/sync/processors/subscription.ts#L467-L492

https://github.com/aws-amplify/amplify-js/blob/main/packages/datastore/src/sync/processors/subscription.ts#L677-L682C10

I think the approach of adding a new configuration flag will require documentation and developers to know about it. This feature was launched in April 2022 so all new customers would have to set the flag which would be a pretty poor DX. Retrying without the filter for the customers with the old provisioning appears to be the performance trade-off we are willing to make.

@MuniekMg
Copy link
Contributor Author

MuniekMg commented Mar 8, 2024

@lawmicha I have removed my last commit and created a new one with a possible solution (work in progress) for retrying subscription requests (e.g. without a filter), but it looks a little messy, and I'm not sure whether I should continue this approach.
Could you maybe help me choose a better solution?

(commit e70a3f5)

@lawmicha
Copy link
Member

lawmicha commented Apr 5, 2024

@lawmicha I have removed my last commit and created a new one with a possible solution (work in progress) for retrying subscription requests (e.g. without a filter), but it looks a little messy, and I'm not sure whether I should continue this approach. Could you maybe help me choose a better solution?

(commit e70a3f5)

Sorry for the delay, we're working on migrating the Amplify.API API calls to the async await version, which will drastically change the outer components like the RetryableGraphQLSubscriptionOperation (cc: @5d) We should consider this use case when designing the new retryable mechanism. The code appears to be on the right track, I'll leave some comments in line so we don't lose track of any necessary changes.

}

// TODO: - How to distinguish errors?
// TODO: - Handle other errors
Copy link
Member

Choose a reason for hiding this comment

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


// Just to be sure that endless retry won't happen
filterLimitRetried = true
maxRetries += 1
Copy link
Member

Choose a reason for hiding this comment

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

is maxRetries updated here to account for the attempt being incremented for the RTF retry case? So the overall attempt to maxRetries counts will continue to represent the number of auth types to attempt? (maxRetries is set to the number of auth types to attempt in multi-auth rules scenarios).

@MuniekMg
Copy link
Contributor Author

MuniekMg commented Apr 11, 2024

Sorry for the delay, we're working on migrating the Amplify.API API calls to the async await version, which will drastically change the outer components like the RetryableGraphQLSubscriptionOperation (cc: @5d) We should consider this use case when designing the new retryable mechanism. The code appears to be on the right track, I'll leave some comments in line so we don't lose track of any necessary changes.

@lawmicha No problem :) I have just finished implementing the same logic like in JS implementation (reconnecting in case of any RTF error) and added more tests (commit e0398e4). It would be great if You could merge those changes while waiting for your new implementation.

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.

2 participants