Suggested code improvements

[JasonLandbridge]

Hi there,

While making my #50 PR, I thought of some improvements that might be interesting:

1. Capitalize all Table and Column names
   • This is consistent with the SQL scripts provided by Quartz.Net
2. Provide prefix and schema parameters with default values for UseSqlite(), UseMySQL() etc

public static class QuartzModelBuilderSQLiteExtensions  
{  
  public static QuartzModelBuilder UseSqlite(
   this QuartzModelBuilder builder,
   string prefix = "QRTZ_",
   string schema = "")
  {  
    builder.UseEntityTypeConfigurations(context => { ... })
  }

Because then the setup becomes even easier and cleaner

public class SQLiteIntegrationDbContext : DbContext  
{   
  protected override void OnModelCreating(ModelBuilder modelBuilder)  
  {  
    modelBuilder.AddQuartz(builder => builder.UseSqlite());  
    // OR
    modelBuilder.AddQuartz(builder => builder.UseSqlite("qrtz_", "quartz"));  

    // Instead of
    // modelBuilder.AddQuartz(builder => builder  
    //  .UseSqlite()  
    //  .UsePrefix("qrtz_")  
    //  .UseNoSchema());
  }  
}

3. Store all the naming as Attributes in the Entities which then don't have to keep being configured for every database type in the EntityConfigurations

// [Table("CALENDARS")] => Can't use this as we need to dynamically add an optional schema
public class QuartzCalendar  
{  
  // Which is why we define it here
  public static readonly string TableName = "CALENDARS";  

  [Column("SCHED_NAME", Order = 0)]  
  public string SchedulerName { get; set; } = null!;  

  [Column("CALENDAR_NAME", Order = 1)]  
  public string CalendarName { get; set; } = null!;  

  [Column("CALENDAR", Order = 2)]  
  public byte[] Calendar { get; set; } = null!;  
}

And then the configuration for every database type becomes:

public class QuartzCalendarEntityTypeConfiguration : IEntityTypeConfiguration<QuartzCalendar>  
{  
  private readonly string prefix;  
  private readonly string schema;  
  // prefix and schema don't need to be nullable anymore since it's always guaranteed to get a value
  public QuartzCalendarEntityTypeConfiguration(string prefix, string schema)  
  {  
    this.prefix = prefix;  
    this.schema = schema;  
  }  

  public void Configure(EntityTypeBuilder<QuartzCalendar> builder)  
  {  
    // This is why we cant use [Table("CALENDARS")]
    builder.ToTable(prefix + QuartzCalendar.TableName, schema);  

    builder.HasKey(x => new { x.SchedulerName, x.CalendarName });  

    builder.Property(x => x.SchedulerName)  
      .HasColumnType("text")  
      .IsRequired();  

    builder.Property(x => x.CalendarName)  
      .HasColumnType("text")  
      .IsRequired();  

    builder.Property(x => x.Calendar)  
      .HasColumnType("bytea")  
      .IsRequired();  
  }  
}

4. Clean-up the AppAny.Quartz.EntityFrameworkCore.Migrations.Tests project where each database type gets its own folder.

5. I added a integration test that creates a Quartz scheduler with the dbcontext provided as a jobstore SQLiteIntegrationDbContext_IntegrationTests.cs. Maybe do this for the other Database types as well?

   @sergeyshaykhullin and @ZaoralJ, What do you guys think?

[sergeyshaykhullin]

1. PostgreSQL scripts are not capitalized
2. I am ok with this shortcut for Use* methods
3. It will not work, because each provider has it's own column names configured by separate entity type configurations on shared C# models + table prefixes
4. Seems like a good refactoring, because of 4 db provider implementations
5. Yes, this test is handy and can help with future schema changes

[ZaoralJ]

Hi,

1. Let's keep object names in EntityConfiguration for each implementation, not super DRY but we can follow names from Quartz.NET scripts as is
2. Fine with me
3. see 1., TableName prop has nothing to do with entity
4. ok
5. super good idea, it will test if Quartz will really work, it has build in schema check. I find out yesterday when use SQL implementation where I set some column not null by mistake and it failed on startup

[JasonLandbridge]

Thanks for the insights!

1. Okay I see there are differences, then maybe let's agree on the following rule: "Follow the naming conventions for tables and column names that are given in each SQL script for each database" and then hard code those in the entity configuration. So for SQLite its uppercase and for PostgreSQL its lowercase
2. Nice!
3. Yes, going with number 1 prevents this one
4. Awesome!
5. Even more awesome!

I think my #50 should wait for #49 and then I can create a seperate PR for the agreed proposals?

[ZaoralJ]

Another improvement can be to use https://github.com/Deffiss/testenvironment-docker or similar for integration tests and use non standard ports to avoid collisions on dev machines

[sergeyshaykhullin]

Another improvement can be to use https://github.com/Deffiss/testenvironment-docker or similar for integration tests and use non standard ports to avoid collisions on dev machines

Yes, but testenvironment has no mysql support right now

[sergeyshaykhullin]

@JasonLandbridge Does all suggestions completed in https://github.com/appany/AppAny.Quartz.EntityFrameworkCore.Migrations/pull/50?

[JasonLandbridge]

Only 4 and 5 are left

4. Clean-up the AppAny.Quartz.EntityFrameworkCore.Migrations.Tests project where each database type gets its own folder.

5. I added a integration test that creates a Quartz scheduler with the dbcontext provided as a jobstore SQLiteIntegrationDbContext_IntegrationTests.cs. Maybe do this for the other Database types as well?

4 needs a bit more love and 5 requires setting up some docker containers to do some actual real database testing. Maybe someone can else pick that up?

[JasonLandbridge]

This might be a good starting point: https://dotnet.testcontainers.org/ (video about it)

[ZaoralJ]

Another improvement can be to use https://github.com/Deffiss/testenvironment-docker or similar for integration tests and use non standard ports to avoid collisions on dev machines

Yes, but testenvironment has no mysql support right now

It's actually support MariaDB which is technically MySQL as OSS. I've checked source code and it's using MySqlConnector for db access

[ZaoralJ]

I've created new PR #56 to improve Integration tests using https://github.com/Deffiss/testenvironment-docker

[ZaoralJ]

I've created new PR #56 to improve Integration tests using https://github.com/Deffiss/testenvironment-docker

[sergeyshaykhullin]

@ZaoralJ Thank you for your contribution!

Merged

[sergeyshaykhullin]

Seems like there are no more actionable tasks

If i missed something please open new granular issues with project improvements