- 
                Notifications
    
You must be signed in to change notification settings  - Fork 3.3k
 
Add Cosmos db manual session token management #37027
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
base: main
Are you sure you want to change the base?
Conversation
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.
A few questions from me still
| /// any release. You should only use it directly in your code with extreme caution and knowing that | ||
| /// doing so can result in application failures when updating to a new Entity Framework Core release. | ||
| /// </summary> | ||
| public class SessionTokenStorage : ISessionTokenStorage | 
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.
Wondering if this needs to be a wrapper and have an SessionTokenStorageInternal.
Also wondering about naming: CosmosSessionTokenStorage?
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 now think that this API might be somewhat clunky to use. Perhaps this should just implement IReadOnlyDictionary<string, string> and that's what GetSessionTokens returns. AddSessionTokens(IReadOnlyDictionary<string, string>) will merge in the passed values and that's when you'll check container names (when the tokens are set internally we know that the containers are correct). I think this will make the common case of just moving all tokens from one instance to another easier and since it's not a special type it would also be simpler to (de)serialize in the app. This also avoids the potential ambiguity of the API using the default container name.
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.
If this implements IReadOnlyDictionary then the values in the reference the user receives will change after using a query or savechanges. Preventing them from having to call GetSessionTokens multiple times, but it might be confusing that it doesn't return a snapshot. Thoughts on this?
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.
You are right, returning a snapshot would be less confusing. And it's also unlikely that developers would need to call GetSessionTokens frequently on the same context instance.
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.
Moved to returning a snapshot
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've kept the ambiguity, as I personally feel most users will use a single container per DbContext, and working with the SetSessionTokens api becomes a bit of a hassle having to always specify the default container name. Let me know if you still think this is a bad idea
| } | ||
| 
               | 
          ||
| // @TODO: Is this the right place for this? | ||
| annotations.Add(CosmosAnnotationNames.ContainerNames, GetContainerNames(model)); | 
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.
In SessionTokenStorage I want to check whether the container exists if a user is appending or adding a session token. It felt expensive to do GetContainerNames every DbContext instantiation, while I felt it could be a property of the model. However, I am not sure if annotations are the right way to add this data to the model for the cosmos provider. I am also not sure what would be the best place to calculate this metadata and set the annotation. Doing this this way fails CompiledModelCosmosTests and I am not sure if there is actually something wrong or the test should change and if so how.
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.
@AndriySvyryd or @roji Could you provide some guidance on this?
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.
With the above proposed design you shouldn't need this
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.
Ahh I see what you mean now. Updated this to do the check in the Extension method and removed the annotations
        
          
                src/EFCore.Cosmos/Query/Internal/CosmosQueryCompilationContextFactory.cs
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                ...Core.Cosmos/Query/Internal/CosmosShapedQueryCompilingExpressionVisitor.QueryingEnumerable.cs
              
                Outdated
          
            Show resolved
            Hide resolved
        
      Register CosmosDatabaseWrapper as IResettableService
| <value>The property '{propertyType} {structuralType}.{property}' has element type '{elementType}', which requires a value converter. Elements types requiring value converters are not currently supported with the Azure Cosmos DB database provider.</value> | ||
| </data> | ||
| <data name="EnableManualSessionTokenManagement" xml:space="preserve"> | ||
| <value>Enable manual session token management using CosmosDbContextOptionsBuilder.ManualSessionTokenManagementEnabled to use this method.</value> | 
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.
| <value>Enable manual session token management using CosmosDbContextOptionsBuilder.ManualSessionTokenManagementEnabled to use this method.</value> | |
| <value>Enable manual session token management using 'options.ManualSessionTokenManagementEnabled' to use this method.</value> | 
Allows the user to retrieve session tokens from reads and writes done by a dbcontext and set session tokens to use for reads.
Adds a GetSessionTokens() extension method to DatabaseFacade which returns session tokens per container.
Adds a AppendSessionTokens() extension method to DatabaseFacade which allows appending session tokens per container.
Adds GetSessionToken() and AppendSessionToken() for the default container.
Adds option ManualSessionTokenManagementEnabled to CosmosDbContextOptionsBuilder
Implements: #36504