RCORE-2222 Remove 308 redirect support from App/AppUser#7996
RCORE-2222 Remove 308 redirect support from App/AppUser#7996michael-wb merged 10 commits intomasterfrom
Conversation
danieltabacaru
left a comment
danieltabacaru
left a comment
There was a problem hiding this comment.
Is this reverting to the state before supporting redirection?
Technically yes - the redirection is being removed since there is already support for handling 301/308 redirections in the HTTP transport supplied by the SDK and Core will never actually see a 301/308 response. The only exception to this is the CPP SDK, and I am working on that now so it supports redirections as well. |
|
|
Unfortunately not, I created realm/realm-cpp#239 for adding redirection support in the CPP SDK. |
Pull Request Test Coverage Report for Build michael.wilkersonbarker_1371Details
💛 - Coveralls |
| request = std::move(request)](const Response& response) mutable { | ||
| self->check_for_redirect_response(std::move(request), response, std::move(completion)); | ||
| request_ref, | ||
| [completion = std::move(completion), request = std::move(request)](const Response& response) mutable { |
There was a problem hiding this comment.
do we need to capture self here to keep App alive for the duration of the request, or is the contract here that the completion is responsible for keeping the App alive?
There was a problem hiding this comment.
The completion callback provided to do_request() captures an App shared pointer, which will ensure the app sticks around until the callback is called.
The only exception is when the original request is re-sent after successfully refreshing the user access token after the request sent by do_authenticated_request() initially failed.
I will update this call and add a statement to do_request() regarding ensuring the callback contains an App shared ptr instance in the captures.
| request = std::move(request)](const Response& response) mutable { | ||
| self->check_for_redirect_response(std::move(request), response, std::move(completion)); | ||
| request_ref, | ||
| [completion = std::move(completion), request = std::move(request)](const Response& response) mutable { |
There was a problem hiding this comment.
Do we need to capture self here to keep the App alive?
There was a problem hiding this comment.
This function is only called from do_request(), so the completion callback will have a reference to to App, like do_request() does.
michael-wb
force-pushed
the
mwb/remove-308-support
branch
from
ac5d7f2 to
8a753e5
Compare
3 participants
What, How & Why?
Removes the 301/308 http redirection support from the app services support in
App. The redirection tests have been removed in PR #7994.Fixes #7942
☑️ ToDos
[ ] 🚦 Tests (or not relevant)[ ] C-API, if public C++ API changed[ ]bindgen/spec.yml, if public C++ API changed