Open up for extensibility

[voltechs]

Hi @pat!

I'm such a huge fan of Combustion; I was working on a new project and was looking to leverage Combustion for it.

Sadly I may be at an impasse. I'm trying to use the Snowflake DB, through a few different gems and configs. All in vein though, it would seem, as Combustion seems to be pretty stubborn on what databases it supports.

That is to say, without a substantial amount of "hacking", combustion has it's list of supported databases pretty locked down, unavailable for modification or extension from end users/developers.

Specifically: https://github.com/pat/combustion/blob/8916f6bec7f1a3de6baa7a15670f3b171d2238ba/lib/combustion/database/reset.rb#L9-L16 and https://github.com/pat/combustion/blob/8916f6bec7f1a3de6baa7a15670f3b171d2238ba/lib/combustion/database/reset.rb#L52-L60

Any thoughts on making that a little less rigid and available for others to append/modify as they might see fit?

Thanks for the consideration in advance 🙂

[pat]

Hi @voltechs - thanks for getting in touch, and it's great to know you find Combustion so helpful!

Also: I'm very keen to make additional database support possible. I suspect the freeze is there due to Rubocop, but disabling that rule sounds like a fair move to me. Did you want to try using a local copy of Combustion, remove that freeze call on OPERATOR_PATTERNS and then add Snowflake into the hash and see if that works? If so, you're welcome to submit a PR, or otherwise I can make the adjustment - I just want to be sure it does what you need before publishing a new release :)

[voltechs]

Hi @pat, that would definitely be a step in the right direction. Of course, OPERATOR_PATTERNS is still a const, and so adding to it would at the very least produce a warning and maybe not the best approach in the end. Perhaps a class variable that one could append to at some point in the initialization process?

class Combustion::Database::Reset
  # ...
  @@operator_patterns = {
    Combustion::Databases::MySQL      => [/mysql/],
    Combustion::Databases::PostgreSQL => [/postgres/, /postgis/],
    Combustion::Databases::SQLite     => [/sqlite/],
    Combustion::Databases::SQLServer  => [/sqlserver/],
    Combustion::Databases::Oracle     => %w[ oci oracle ],
    Combustion::Databases::Firebird   => %w[ firebird ]
  }
  # ...
  def operator_class(adapter)
    klass = nil
    @@operator_patterns do |operator, keys|
      klass = operator if keys.any? { |key| adapter[key] }
    end
    return klass if klass

    raise UnsupportedDatabase, "Unsupported database type: #{adapter}"
  end
end

Also, off-topic: would it make sense to simplify the operator_class method as:

  def operator_class(adapter)
    @@operator_patterns do |operator, keys|
      return operator if keys.any? { |key| adapter[key] }
    end
    raise UnsupportedDatabase, "Unsupported database type: #{adapter}"
  end

Especially after opening up the hash to modification, there's a chance of traversing the whole hash even after finding a match.

[pat]

Given we can rely on ActiveSupport being present, perhaps cattr_reader could be used instead?

cattr_reader :operator_patterns, default: {
  Combustion::Databases::MySQL      => [/mysql/],
  Combustion::Databases::PostgreSQL => [/postgres/, /postgis/],
  Combustion::Databases::SQLite     => [/sqlite/],
  Combustion::Databases::SQLServer  => [/sqlserver/],
  Combustion::Databases::Oracle     => %w[ oci oracle ],
  Combustion::Databases::Firebird   => %w[ firebird ]
}

Your simplification of operator_class looks good to me 👍🏻

Also, I'm wondering if this attribute, the operator_class method, and the UnsupportedDatabase definition should be shifted to Combustion::Database, with the method becoming a class-level concern, and the error class being renamed to UnsupportedDatabaseError? If you'd rather hold off on that for now, I can look at doing that refactoring myself - but more than happy for you to sort it out if you'd like! :)

[voltechs]

Sounds good. I'll try to take a stab at creating a PR here when I've got a few spare cycles!