RSA4c+d #212
RSA4c+d #212
Conversation
withakay
changed the base branch from
RSA4a+b
to
feature/RSC7b+RSA4a+RSA4c1+RSA4c3
withakay
changed the base branch from
feature/RSC7b+RSA4a+RSA4c1+RSA4c3
to
RSA4a+b
| } | ||
| else if (newState.State != Realtime.ConnectionState.Failed) | ||
| { | ||
| await SetState(new ConnectionFailedState(this, ex.ErrorInfo, Logger)); |
There was a problem hiding this comment.
Heads up: some parts of the spec might be reworded and lead to a change of this behavior. At the moment it's just a discussion, so this is fine.
| should transition to the FAILED state, with the connection errorReason should be set to | ||
| the ErrorInfo (or where there is none, as for a 403 authUrl response with no body, an ErrorInfo with code 40300 and an appropriate message) | ||
| */ | ||
|
|
There was a problem hiding this comment.
While putting this in code might make it unreadable pretty quickly, I think it's more than ok to put it in the description of the PR. Future us will have a way to know what was the wording of the spec item when it was implemented.
| await realtimeClient.WaitForState(ConnectionState.Connected); | ||
| realtimeClient.Connection.On(change => | ||
| { | ||
| // this callback should not be called |
There was a problem hiding this comment.
If I am reading this correctly, there should be a fail statement here. No?
There was a problem hiding this comment.
@cesare the failure is on line 353 (await tca.Task).Should().BeFalse(context);
src/IO.Ably.Shared/AblyAuth.cs
Outdated
| @@ -213,9 +213,17 @@ private void SetAuthMethod() | |||
| } | |||
| catch (Exception ex) when (shouldCatch) | |||
| { | |||
| HttpStatusCode? statusCode = null; | |||
| int code = 80019; | |||
| if (ex is AblyException ablyException) | |||
There was a problem hiding this comment.
I don't think this implements the requirement. The outer error has code 80019 and it wraps the original error as cause. I don't know why this references 40300.
There was a problem hiding this comment.
@paddybyers
This is to comply with RSA4d, could you expand on what you think is wrong with this please?
If a request by a realtime client to an authUrl results in an HTTP 403 response, or any of an authUrl request, an authCallback, or a request to Ably to exchange a TokenRequest for a TokenDetails result in an ErrorInfo with statusCode 403, then the client library should transition to the FAILED state, with the connection errorReason should be set to the ErrorInfo (or where there is none, as for a 403 authUrl response with no body, an ErrorInfo with code 40300 and an appropriate message)
There was a problem hiding this comment.
Where are you getting that text from?
The requirement I'm looking at is in https://github.com/ably/docs/blob/integration-spec-1-1/content/client-lib-development-guide/features.textile
The required check for RSA4d is statusCode == 403, not code == xx. And either way once you have the inner error you generate the ErrorInfo to return with code = 80019, statusCode = <inner err>.statusCode, cause = <inner err>. The difference when RSA4d applies is in the Realtime case you transition to failed.
There was a problem hiding this comment.
That text is from the from the Google Sheet titled "Ably .NET Realtime Client Library Spec - v1.1"
There was a problem hiding this comment.
@paddybyers fixed by 1d1d88a & 7572963
# Conflicts: # src/IO.Ably.Shared/IO.Ably.Shared.projitems
… pass up 401 (per Java lib)
| reconnectAwaiter.SetCompleted(); | ||
| }); | ||
| }); | ||
| await Task.Delay(1000); |
There was a problem hiding this comment.
There are points where some internal events are raised and if I recall it was possible to hit the next line before they had been handled, so this delay allow for that. So yes, somewhat magical.
Changes and tests to cover RSA4c (1,2,3) and RSA4d