Extendable database_consistency

[toydestroyer]

Hey everyone! 👋

I have a feature request.

Problem 1: Compatibility with strong_migrations

Our to-do list is quite long, and we're solving problems in batches. For some problems (like missing NOT NULL constraints or foreign keys), the autofix feature automatically generates a migration file for us.

However, we're also using the strong_migrations gem, which complains about the autogenerated migration files. For instance, to add a NOT NULL constraint, we need 2 migration files:

class SetSomeColumnNotNull < ActiveRecord::Migration[7.0]
  def change
    add_check_constraint :users, "some_column IS NOT NULL", name: "users_some_column_null", validate: false
  end
end

class ValidateSomeColumnNotNull < ActiveRecord::Migration[7.0]
  def change
    validate_check_constraint :users, name: "users_some_column_null"
    change_column_null :users, :some_column, false
    remove_check_constraint :users, name: "users_some_column_null"
  end
end

Instead of the one being generated:

class SetSomeColumnNotNull < ActiveRecord::Migration[7.0]
  def change
    change_column_null :users, :some_column, false
  end
end

Problem 2: Custom checkers

We have some ideas for checkers that are very specific to our internal conventions, and making them public doesn't make sense. We also have WIP checkers that are not ready to be merged yet (like enum one) but can internally benefit us a lot.

Proposed Solution

Allow users to extend checkers/writers with custom ones similar to RuboCop's approach:

require:
  # extend with gem
  - rubocop-performance
  # extend with lib
  - './lib/rubocop/cop/custom/some_custom_cop.rb'

Looking forward to your suggestions and feedback. Thank you!

[djezzzl]

Hi @toydestroyer,

This is a great idea! I'm so happy your team is leveraging the gem so deep that you create custom checkers.

I like the feature, but I have a question.

Would we support adding such through database_consistency.yml, or would it be better to have it with a ruby configuration?

DatabaseConsistency.configure do
  add_checkers CustomChecker1, CustomChecker2
  add_writers SimpleWriter1, AutofixWriter1
end

How would those - rubocop-performance or ./lib/....some_custom_cop.rb look like?

Something like this?

class CustomChecker < DatabaseConsistency::Checkers::ColumnChecker
  ...
end

DatabaseConsistency.add_checker(CustomChecker)

P.S. Do you have any suggestions for problem one? strong_migrations is fantastic, but I don't know how to adapt to that, so we don't need to duplicate their code.

[toydestroyer]

Would we support adding such through database_consistency.yml, or would it be better to have it with a ruby configuration?

I don't have a strong opinion here. Yaml is neater, and this is what rubocop is doing (here's how).

Rubocop rules are self-contained. You do checks, reporting, and autocorrections in the same class (example).

Rubocop does have a registry and register new rules upon inheritance.

But in the case of DatabaseConsistency gem, we need to register the checker, simple writer, error slug (?), and sometimes autofix.

I'll think more about the topic this week. For now, I want to share my findings and thoughts and hear your thoughts as well @djezzzl.

P.S. Do you have any suggestions for problem one? strong_migrations is fantastic, but I don't know how to adapt to that, so we don't need to duplicate their code.

This definitely should be a separate gem that overrides autofix writers. We don't need to duplicate their code, we just need to generate migration files that don't violate strong_migrrations' recommendations.

[djezzzl]

Yaml is neater, and this is what rubocop is doing

I don't see any issues with doing the same.

register new rules upon inheritance.

Excellent, we can do the same 👍

we need to register the checker, simple writer, error slug (?), and sometimes auto fix.

We may have some logic to ensure that everything (or minimal requirement) is provided, but my main idea was to avoid being bound to any of the writers (TODO/Simple/Autofix, etc.). But I understand if there is none provided, how would the consumer know something needs to be added to make it fully work? We may start with good guidance or additional "code" that validates custom classes if they have everything/something to be written.

I'll think more about the topic this week.

Please share once you foresee some vision; I like the idea and want to help you and your team leverage the gem as much as possible.

This definitely should be a separate gem that overrides autofix writers. We don't need to duplicate their code

Agree; let's let strong_migrations do its job.

we just need to generate migration files that don't violate strong_migrrations' recommendations.

Now, I'm kind of confused. How can we write good migrations without "duplicating the strong_recommendations"? It would require us to maintain and support all recent changes, and I wonder if we should do this.

I agree that strong_migrations is helpful. However, some projects don't need the gem's complexity, for example, when the zero downtime policy can be ignored. Therefore, we may emphasize and recommend using strong_migrations somewhere (as a comment in generated migrations, maybe), but we shouldn't create complex migration initially.

What do you think, @toydestroyer?

[toydestroyer]

Now, I'm kind of confused. How can we write good migrations without "duplicating the strong_recommendations"?

Sorry for the confusion, let me clarify. I meant there should be a gem called database_consistency-strong_migrations that doesn't add any new checkers but only overwrites existing Autofix writers to comply with strong_migrations' recommendations.

It would require us to maintain and support all recent changes, and I wonder if we should do this.

You personally shouldn't do anything as long as database_consistency provides an API to extend itself similarly to that some of the rubocop extensions are not maintained by the rubocop core team.

Also, maintenance shouldn't be a big issue because there are not many changes in how you write safe migrations. For example, last time there was a change in how you safely add NOT NULL constraint in PostgreSQL 12. So the reasons to update the database_consistency-strong_migrations gem would be:

• database_consistency adds new autofix
• strong_migrations changes its recommendations.

However, some projects don't need the gem's complexity

In that case, they just use pure database_consistency without database_consistency-strong_migrations extension. Similarly, people who don't use RSpec just don't include rubocop-rspec in their rubocop config

[toydestroyer]

We may have some logic to ensure that everything (or minimal requirement) is provided, but my main idea was to avoid being bound to any of the writers (TODO/Simple/Autofix, etc.). But I understand if there is none provided, how would the consumer know something needs to be added to make it fully work? We may start with good guidance or additional "code" that validates custom classes if they have everything/something to be written.

I have a feeling that "simple writer" should be replaced by the concept of "formatter". This is something RSpec does. Take a look at the progress and documentation formatters for example.

The idea is that the checker should report all the information anyone might need to render the result. And if you think about it, TodoWriter is already like this because, unlike SimpleWriter, it knows how to generate a todo item for any checker.

In order to make this happen, we need to move the template from a checker-specific simple writer to the checker itself, remove all checker-specific writers, and make SimpleWriter read the template from a checker.

Once again, this opens an opportunity for extensions (rspec_junit_formatter as an example of such an extension for RSpec). Like, what if I want to generate the report in an HTML file? I just do DatabaseConsistency::Writers::HTMLWriter < BaseWriter in my project and then run

bundle exec database_consistency --writer HTMLWriter

How does this sound to you @djezzzl?