-
Notifications
You must be signed in to change notification settings - Fork 494
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
Make SessionToken on QueryRequestOptions public #979
Make SessionToken on QueryRequestOptions public #979
Conversation
/azp run |
@@ -138,7 +138,7 @@ public class QueryRequestOptions : RequestOptions | |||
/// | |||
/// </para> | |||
/// </remarks> | |||
internal string SessionToken { get; set; } | |||
public string SessionToken { get; set; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a note to the changelog
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I've done this now, let me know if it's not correct
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even though this is made public we need to wire it through the query stack right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From my understanding this is already done on the line 164:
RequestOptions.SetSessionToken(request, this.SessionToken);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. I just want to make sure what the story will be. If a user does multiple point writes, then what session token will they send for the query?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good point but also a different question.
SessionToken would be same as what is used for any ReadItemAsync.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there anything else I need to change, or are we all good now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope we are good to ship it. Thanks @dpgregory for your contributions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great! No worries, it wasn't much!
/azp run |
1 similar comment
/azp run |
/AzurePipelines run |
/azp run |
/AzurePipelines help |
/AzurePipelines run |
/azp run |
* Fix feed iterator for mocking * Updated changelog * Added method back * Fixed type casting * Removed additional internal abstract types. Added UT * Removed unused variable * Updated changelog * Fixed tests
changelog.md
Outdated
@@ -9,6 +9,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 | |||
|
|||
## <a name="3.4.1"/> [3.4.1](https://www.nuget.org/packages/Microsoft.Azure.Cosmos/3.4.1) - 2019-11-06 | |||
|
|||
### Added | |||
|
|||
- [#853](https://github.com/Azure/azure-cosmos-dotnet-v3/pull/979) Make SessionToken on QueryRequestOptions public. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please move this above 3.4.1 under the Unreleased tag. 3.4.1 is already released
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for all the azp comments. It turns out that Github had a bug that prevented the gates from running on forked repos.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No worries. Sorry, I think I've got it in the right place now. This is my first contribution to open source project, so it's a learning experience. I thought it best to try and fix my own issue though.
/azp run |
Azure Pipelines successfully started running 2 pipeline(s). |
/azp run |
Azure Pipelines successfully started running 2 pipeline(s). |
Is there anything else I need to do on this, or is it just waiting for someone to be free to look at it? |
Thanks alot @dpgregory for your contributions. @bchong95 , @sboshra please take a look. |
/azp run |
Azure Pipelines successfully started running 2 pipeline(s). |
/azp run |
Azure Pipelines successfully started running 2 pipeline(s). |
* Make SessionToken on QueryRequestOptions public * Fix mocking for FeedIterator and Response classes (#978) * Fix feed iterator for mocking * Updated changelog * Added method back * Fixed type casting * Removed additional internal abstract types. Added UT * Removed unused variable * Updated changelog * Fixed tests * Updating package (#975) * Update azure-pipelines-official.yml (#970) * Update changelog with 979 * Move line in changelog to correct location * Remove added in change log to correct location
Description
Made SessionToken public on QueryRequestOptions to allow querying of multiple documents with SessionToken.
Type of change
Closing issues
Closes #973