Fixed #1617 - The specified SyncGatewaySession value with /_replicate REST API is not used. #1618
Conversation
This file contains hidden or 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
added 3 commits
April 10, 2017 17:46
…ieJar, all cookies should be stored into the CookieJar.
pasin
reviewed
Apr 13, 2017
| // NOTE: In case that the path of URL is not end with `/`, | ||
| // last segment of a path is not stored as a path of Cookie. | ||
| Cookie cookie = Cookie.parse(HttpUrl.get(remote), cookieString); | ||
| if (cookie != null && replicationInternal != null) { |
Contributor
There was a problem hiding this comment.
I think we don't need to check if the replicationInternal is null here as it will not be null?
Contributor
There was a problem hiding this comment.
Does it also need to remove the Cookies from the requestHeadersParams as well? I don't know the behavior when the cookies are in both headers and cookie jar.
Author
There was a problem hiding this comment.
for comment 1
- replicationInternal should not be null. So I will remove this.
for comment 2
- Cookie's in Cookie jar always overwrite if it exists. However, passed cookie value in the headers does not includes expirers, domain, and path. So it could send two cookies in the final request header. As you advise, I feel it is better to remove from headers Map object.
Author
|
@pasin applied feedback from you. Please review this PR again? |
hideki
pushed a commit
to couchbase/couchbase-lite-android
that referenced
this pull request
Apr 13, 2017
pasin
reviewed
Apr 14, 2017
|
|
||
| private void storeCookiesIntoCookieJar(Map<String, Object> requestHeadersParam) { | ||
| try { | ||
| if (requestHeadersParam.containsKey("Cookie") && requestHeadersParam.get("Cookie") instanceof String) { |
Contributor
There was a problem hiding this comment.
I think requestHeadersParam can be null so null check would be required.
Author
|
@pasin can you re-review? |
hideki
pushed a commit
to couchbase/couchbase-lite-android
that referenced
this pull request
Apr 24, 2017
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.
Please refer https://github.com/couchbase/couchbase-lite-java-core/issues/1617#issue-220851080 for details.