Initial commit for EF db seeder #53
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
6 file(s) reviewed, 8 comment(s)
Edit PR Review Bot Settings | Greptile
| { | ||
| private readonly string _connectionString; | ||
| private readonly string _databaseProvider; | ||
| private readonly ILogger<EFDBSeeder> _logger; |
There was a problem hiding this comment.
style: ILogger is declared but never used
| var factory = new DatabaseContextFactory(); | ||
| using (var context = factory.CreateDbContext(new[] { _connectionString })) |
There was a problem hiding this comment.
logic: DatabaseContextFactory is not using the _databaseProvider. Ensure it's correctly configured for different database types
|
|
||
|
|
||
|
|
||
| return true; |
There was a problem hiding this comment.
logic: This always returns true, even if an exception is thrown. Consider returning false on exceptions
| .RuleFor(u => u.Email, (f, u) => f.Internet.Email(u.Name)) | ||
| .RuleFor(u => u.EmailVerified, f => f.Random.Bool(0.9f)) | ||
| .RuleFor(u => u.SecurityStamp, f => Guid.NewGuid().ToString()) | ||
| .RuleFor(u => u.ApiKey, f => Guid.NewGuid().ToString("N").Substring(0, 30)) |
There was a problem hiding this comment.
style: ApiKey generation might not be secure. Consider using a cryptographically secure method
| services.AddDbContext<DatabaseContext>(options => | ||
| options.UseSqlServer(connectionString)); |
There was a problem hiding this comment.
style: This hardcodes SQL Server. Consider using a provider-agnostic approach to support multiple database types.
| var services = new ServiceCollection(); | ||
| services.AddDataProtection(); | ||
| services.AddDbContext<DatabaseContext>(options => | ||
| options.UseSqlServer(connectionString)); | ||
| var serviceProvider = services.BuildServiceProvider(); |
There was a problem hiding this comment.
style: ServiceCollection is created and immediately disposed. Consider passing IServiceProvider as a parameter for better testability and reusability.
| public void Execute( | ||
|
|
||
| [Operand(Description = "Database provider (mssql, mysql, postgres, sqlite).")] string databaseProvider, | ||
| [Operand(Description = "Database connection string.")] string ConnectionString |
There was a problem hiding this comment.
style: ConnectionString parameter name should be camelCase for consistency
| var seeder = new EFDBSeeder(databaseConnectionString, databaseProvider); | ||
| bool success; | ||
|
|
||
| success = seeder.SeedDatabase(); // Change this line |
There was a problem hiding this comment.
style: Remove the comment '// Change this line' as it's no longer relevant
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-5985
📔 Objective
Use EF to create CLI for seeding databases for the specific provider
📸 Screenshots
⏰ Reminders before review
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changesGreptile Summary
This pull request introduces a new database seeding functionality using Entity Framework for the Bitwarden server project.
util/DBSeeder/EFDBSeeder.cswith methods to seed users and ciphers using Bogus for fake data generationutil/DBSeeder/Factories.cswith aDatabaseContextFactoryfor creatingDatabaseContextinstancesutil/EFDBSeederUtility/Program.csfor executing the seeding processDBSeeder.csprojandEFDBSeederUtility.csprojwith necessary dependencies and referencesbitwarden-server.slnto include the new DBSeeder and EFDBSeederUtility projects