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.

Sign up for GitHub

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

Jump to bottom

Cosmos DB for Events #61

Open

lizard-boy wants to merge 4 commits into main from cosmosevents

lizard-boy commented Oct 19, 2024 •

edited by greptile-apps bot

Loading

Type of change

- [ ] Bug fix
- [ ] New feature development
- [ ] Tech debt (refactoring, code cleanup, dependency upgrades, etc)
- [ ] Build/deploy pipeline (DevOps)
- [ ] Other

Objective

Code changes

file.ext: Description of what was changed and why

Before you submit

Please check for formatting errors (dotnet format --verify-no-changes) (required)
If making database changes - make sure you also update Entity Framework queries and/or migrations
Please add unit tests where it makes sense to do so (encouraged but not required)
If this change requires a documentation update - notify the documentation team
If this change has particular deployment requirements - notify the DevOps team

Greptile Summary

This pull request introduces Cosmos DB support for event handling in the server application, focusing on the implementation of a new EventItem class and EventRepository for Cosmos DB integration.

Added src/Core/Models/Data/EventItem.cs with a new EventItem class for Cosmos DB event data handling
Introduced src/Core/Repositories/Cosmos/EventRepository.cs for managing events in Cosmos DB
Modified src/EventsProcessor/AzureQueueHostedService.cs to support Cosmos DB alongside existing TableStorage
Updated src/SharedWeb/Utilities/ServiceCollectionExtensions.cs to register Cosmos DB EventRepository

kspearrin and others added 4 commits

January 23, 2024 10:51


          events repo using cosmos db

36f417d


          partition key note

854fdbf


          Merge branch 'main' into cosmosevents

94852bd


          Merge branch 'main' into cosmosevents

0a7da87

greptile-apps bot reviewed

View reviewed changes

greptile-apps bot left a comment

_{4 file(s) reviewed, 11 comment(s)}
_{Edit PR Review Bot Settings | Greptile}

src/Core/Models/Data/EventItem.cs

+              public class EventItem : IEvent
+              {
+                  public EventItem() {}

greptile-apps bot Oct 19, 2024

style: Consider removing the empty constructor if it's not explicitly needed

src/Core/Models/Data/EventItem.cs

+                  }
+                  [JsonPropertyName("id")]
+                  public string Id { get; set; }

greptile-apps bot Oct 19, 2024

style: Consider making Id property init-only to prevent modification after creation

src/Core/Models/Data/EventItem.cs

+                  [JsonPropertyName("saId")]
+                  public Guid? ServiceAccountId { get; set; }
+                  [JsonPropertyName("domain")]
+                  public string DomainName { get; set; }

greptile-apps bot Oct 19, 2024

style: DomainName should be nullable (string?) for consistency with other properties

src/Core/Repositories/Cosmos/EventRepository.cs

+                  private readonly Container _container;
+                  public EventRepository(GlobalSettings globalSettings)
+                      : this("TODO")

greptile-apps bot Oct 19, 2024

logic: Replace 'TODO' with actual connection string retrieval from globalSettings

src/Core/Repositories/Cosmos/EventRepository.cs

Comment on lines +36 to +37

		_database = _client.GetDatabase("events");
		_container = _database.GetContainer("events");

greptile-apps bot Oct 19, 2024

style: Consider using configuration for database and container names instead of hardcoding

src/Core/Repositories/Cosmos/EventRepository.cs

Comment on lines +102 to +103

		// TODO: How should we handle the partition yet? Perhaps something like table storage did with
		// orgId, userId, providerId

greptile-apps bot Oct 19, 2024

logic: Implement a proper partitioning strategy before production use

src/Core/Repositories/Cosmos/EventRepository.cs

Comment on lines +111 to +120

+                  public Task CreateManyAsync(IEnumerable<IEvent> events)
+                  {
+                      // ref: https://learn.microsoft.com/en-us/azure/cosmos-db/nosql/tutorial-dotnet-bulk-import
+                      var tasks = new List<Task>();
+                      foreach (var e in events)
+                      {
+                          tasks.Add(CreateAsync(e));
+                      }
+                      return Task.WhenAll(tasks);
+                  }

greptile-apps bot Oct 19, 2024

style: Consider using Cosmos DB bulk operations for better performance when creating multiple events

src/Core/Repositories/Cosmos/EventRepository.cs

Comment on lines +139 to +149

+                      var result = new PagedResult<IEvent>();
+                      while (iterator.HasMoreResults)
+                      {
+                          var response = await iterator.ReadNextAsync();
+                          result.Data.AddRange(response);
+                          if (response.Count > 0)
+                          {
+                              result.ContinuationToken = response.ContinuationToken;
+                              break;
+                          }
+                      }

greptile-apps bot Oct 19, 2024

logic: This pagination implementation may not handle large result sets efficiently. Consider implementing server-side pagination

src/EventsProcessor/AzureQueueHostedService.cs

@@ @@ -54,12 +55,15 @@ public void Dispose() @@
                   private async Task ExecuteAsync(CancellationToken cancellationToken)
                   {
                       var storageConnectionString = _configuration["azureStorageConnectionString"];
+                      var cosmosConnectionString = _configuration["cosmosConnectionString"];

greptile-apps bot Oct 19, 2024

style: Consider using a more descriptive configuration key, such as 'cosmosDbConnectionString' for clarity

src/EventsProcessor/AzureQueueHostedService.cs

Comment on lines +64 to +66

+                      IEventRepository repo = string.IsNullOrWhiteSpace(cosmosConnectionString) ?
+                          new Core.Repositories.TableStorage.EventRepository(storageConnectionString) :
+                          new Core.Repositories.Cosmos.EventRepository(cosmosConnectionString);

greptile-apps bot Oct 19, 2024

style: This logic might be better placed in a factory method or dependency injection setup

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet