Skip to content
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

Implement extra properties/soft-deleting features #23

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

npavlyk82
Copy link

Resolves #22

@swisslife-bot
Copy link

swisslife-bot commented Feb 7, 2025

CLA assistant check
All committers have signed the CLA.

@npavlyk82 npavlyk82 force-pushed the npav/storeExtraProperties branch 2 times, most recently from e423582 to 05449a7 Compare February 7, 2025 13:33
@@ -23,7 +23,8 @@ public async Task InvokeAsync(
if (context.Result is TPayload result)
{
BewitToken<TPayload> bewit
= await tokenGenerator.GenerateBewitTokenAsync(result, context.RequestAborted);
= await tokenGenerator.GenerateBewitTokenAsync(
result, context.RequestAborted, context.GetBewitTokenExtraProperties());
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cancellationtoken should be the last parameter

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@@ -29,7 +29,7 @@ public async Task InvokeAsync(

BewitToken<string> bewit =
await tokenGenerator.GenerateBewitTokenAsync(
uri.PathAndQuery, context.RequestAborted);
uri.PathAndQuery, context.RequestAborted, context.GetBewitTokenExtraProperties());
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cancellationToken should be the last parameter

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

}

public async ValueTask InsertOneAsync(
Token token, CancellationToken cancellationToken)
{
if (token.ExtraProperties != null)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should not create indexes in CRUD methods, as this doubles our calls to the mongoDB.
If the extra properties can not be prediced on startup, we should skip index creations for them altogether.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would leave it as is. The MongoDB.Driver is smart enough:

  1. it makes a call on GetCollection and caches all index info on the client.
  2. It doesn't make an additional call if we try to create an already existing index.
  3. _collection.Indexes.CreateOne doesn't create the index immediately; it will be done on the server side asynchronously.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have changed the logic. The indexes can be created for values of primitive types. The complex type values will be sterilized to json string

@@ -38,24 +41,51 @@ public MongoNonceRepository(IMongoDatabase database, MongoNonceOptions options)

_collection.Indexes.CreateOne(new CreateIndexModel<Token>(
Builders<Token>.IndexKeys.Ascending(nameof(IdentifiableToken.Identifier))));

_collection.Indexes.CreateOne(new CreateIndexModel<Token>(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

compound index can be removed (nonce being already unique)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed

@npavlyk82 npavlyk82 force-pushed the npav/storeExtraProperties branch 2 times, most recently from 9811d7c to 14ad842 Compare February 10, 2025 10:40
@npavlyk82 npavlyk82 requested a review from gmiserez February 10, 2025 10:42
@npavlyk82 npavlyk82 force-pushed the npav/storeExtraProperties branch from 14ad842 to 2e39409 Compare February 10, 2025 14:51
gmiserez
gmiserez previously approved these changes Feb 11, 2025
gmiserez
gmiserez previously approved these changes Feb 11, 2025
gmiserez
gmiserez previously approved these changes Feb 11, 2025
gmiserez
gmiserez previously approved these changes Feb 11, 2025
gmiserez
gmiserez previously approved these changes Feb 12, 2025
@npavlyk82 npavlyk82 force-pushed the npav/storeExtraProperties branch from 4cfb51a to aae8ab7 Compare February 12, 2025 14:55
@glucaci glucaci closed this Feb 18, 2025
@glucaci glucaci reopened this Feb 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Auditing logs is needed.
4 participants