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

Open up for extensibility #129

Open
voltechs opened this issue Jun 9, 2022 · 4 comments
Open

Open up for extensibility #129

voltechs opened this issue Jun 9, 2022 · 4 comments

Comments

@voltechs
Copy link

voltechs commented Jun 9, 2022

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:

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 ]
}.freeze

and
def operator_class(adapter)
klass = nil
OPERATOR_PATTERNS.each do |operator, keys|
klass = operator if keys.any? { |key| adapter[key] }
end
return klass if klass
raise UnsupportedDatabase, "Unsupported database type: #{adapter}"
end

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
Copy link
Owner

pat commented Jun 9, 2022

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
Copy link
Author

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
Copy link
Owner

pat commented Jun 16, 2022

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
Copy link
Author

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

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

No branches or pull requests

2 participants