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

Add support for specifying comments for tables, columns, and indexes in database itself #22911

Merged
merged 1 commit into from Apr 17, 2016
Merged

Add support for specifying comments for tables, columns, and indexes in database itself #22911

merged 1 commit into from Apr 17, 2016

Conversation

Envek
Copy link
Contributor

@Envek Envek commented Jan 4, 2016

This pull request adds support for specifying comments for database objects in migrations, storing them inside database, and dumping them into db/schema.rb file for PostgreSQL and MySQL.

create_table "pages", force: :cascade, comment: 'Arbitrary content pages' do |t|
  # ...
  t.string "path",    comment: "Path fragment of page URL used for routing"
  t.string "locale",  comment: "RFC 3066 locale code of website language section"
  t.index ["locale", "path"], name: 'page_uri_index' comment: "Main index used to lookup page by it's URI."
  t.text   "content", comment: "HTML code of page"
  # ...
end

In my opinion it is really useful feature, allowing people to create documented data model right from Rails migrations. New people can look for tables, columns, and indexes meaning right in db/schema.rb (or right in model files with gems like @ctran's annotate_models) and DBA can view that information directly from MySQL Workbench, PgAdmin III, DBeaver or anything else. Afterwards any existing data modelling tool can be used to generate data model documentation (such as ER diagrams and table structure) automatically with minimal editing.

For example in my work recently was situation when we were need to send documentation to customer and database schema was part of it. We lost a day helping writers to make this docs as we explained tables and table columns meaning only in our internal wiki (and it was incomplete because everyone always forgets to edit it when writing a migration) and writers came to us with autogenerated docs from our staging database. And there was a lot of manual docs merging and investigation.

Among built in adapters only PostgreSQL and MySQL supports it (SQLite does not). Also oracle_enhanced adapter supports comments with exactly same syntax (actually I follow it's syntax). May be @rsim, @yahonda, or @cdinger will also review and say whether I should change something for ease of integration of oracle_enhanced implementation with my implementation.

Comment implementation in RDBMS differs heavily:

Here ActiveRecord takes all dirty work and hides all differences between implementations. I think this is awesome.

Unfortunately I'm not really experienced in ActiveRecord hacking and most of the work I have done blindly, just by inserting code chunks here and there and seeing what errors will occur and whether tests will pass. Is there any docs about ActiveRecord internals and architecture? I would be glad to read them. Please review carefully and tell me what I should change. I think this PR should be considered as Work In Progress, hence it already do it's job well.

Also I think that note about importance of schema documentation should be added to guides, but can't imagine good place and good words for it (and my English really isn't good enough for guides yet). Any suggestions are welcome!

@rails-bot
Copy link

rails-bot commented Jan 4, 2016

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @rafaelfranca (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

PostgreSQLColumn.new(name, default, sql_type_metadata, null, default_function, collation, comment)
end

def table_comment(table_name)
Copy link
Member

@rafaelfranca rafaelfranca Jan 6, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

# :nodoc:

@rafaelfranca
Copy link
Member

rafaelfranca commented Jan 6, 2016

It looks good to me. I made some minor comments. Make sure you rebased and squased your commits and ping me when done.

@Envek
Copy link
Contributor Author

Envek commented Jan 6, 2016

@rafaelfranca thanks for review. Fixed your notes, squashed, and rebased.

@@ -703,6 +713,9 @@ def table_options(table_name)

# strip AUTO_INCREMENT
raw_table_options.sub(/(ENGINE=\w+)(?: AUTO_INCREMENT=\d+)/, '\1')
Copy link
Contributor

@rsim rsim Jan 6, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sub should be changed to sub! as now this is not yet the return value and will be discarded otherwise.

Copy link
Contributor Author

@Envek Envek Jan 6, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, you're right! Fixed.

@Envek
Copy link
Contributor Author

Envek commented Jan 7, 2016

@rafaelfranca is there anything left for me to change? I rebased and squashed.

require 'cases/helper'
require 'support/schema_dumping_helper'

class PostgresqlCommentTest < ActiveRecord::PostgreSQLTestCase
Copy link
Member

@matthewd matthewd Jan 7, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks a lot like the MySQL equivalent. Do we have precedent for that? Shouldn't this be a generic test, which just gets skipped on sqlite3?

Copy link
Contributor Author

@Envek Envek Jan 8, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@matthewd is there example of such a generic test with exceptions for some adapters in ActiveRecord?

I have tried to make a single test here: Envek@0db3d7b#diff-b905e321f988197140400a200a951ec0

But for some reason these tests doesn't run for MySQL:

➜  activerecord  ARCONN=mysql2 bundle exec ruby -Itest test/cases/comment_test.rb
Using mysql2
Run options: --seed 20982

# Running:



Finished in 0.001537s, 0.0000 runs/s, 0.0000 assertions/s.

0 runs, 0 assertions, 0 failures, 0 errors, 0 skips

ActiveRecord::Base.connection.supports_comments? at Envek@0db3d7b#diff-b905e321f988197140400a200a951ec0R29 returns true

PostgreSQL is fine:

➜  activerecord  ARCONN=postgresql bundle exec ruby -Itest test/cases/comment_test.rb
Using postgresql
Run options: --seed 21355

# Running:

......

Finished in 1.961782s, 3.0584 runs/s, 11.7240 assertions/s.

6 runs, 23 assertions, 0 failures, 0 errors, 0 skips

Copy link
Member

@matthewd matthewd Jan 8, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're still inheriting from PostgreSQLTestCase.

See pretty much all the uses of current_adapter? for relevant examples. (Notably, I think we generally don't use supports_*? methods to decide whether the tests should run, because an incorrect result could cause the tests to be skipped instead of reporting the problem.)

Copy link
Contributor Author

@Envek Envek Jan 8, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uh, seems like I was really sleepy this morning. Sorry for wasteing your time. Fixed!

@Envek
Copy link
Contributor Author

Envek commented Jan 8, 2016

@rafaelfranca, @matthewd, I merged the tests into generic one, squashed and rebased.

@Envek
Copy link
Contributor Author

Envek commented Feb 11, 2016

Does anything prevents this PR from being merged? :-)

@connection.drop_table 'commenteds', if_exists: true
end

if %i[ Mysql2Adapter PostgreSQLAdapter ].any? { |a| current_adapter?(a) }
Copy link
Member

@kamipo kamipo Feb 11, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if current_adapter?(:Mysql2Adapter, :PostgreSQLAdapter)

@Envek
Copy link
Contributor Author

Envek commented Feb 12, 2016

@kamipo, thanks for review. Fixed your notes.

Anything else?

:options_include_default?, :supports_indexes_in_create?, :supports_foreign_keys?, :foreign_key_options
delegate :quote_column_name, :quote_table_name, :quote_default_expression, :quote, :type_to_sql,
:options_include_default?, :supports_indexes_in_create?, :supports_foreign_keys?, :foreign_key_options,
:supports_comments?, :supports_comments_in_create?, to: :@conn
Copy link
Member

@kamipo kamipo Feb 12, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:supports_comments_in_create? is unused in SchemaCreation. And I think :supports_comments? is not needed.

Copy link
Contributor Author

@Envek Envek Feb 25, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed :supports_comments_in_create? and :supports_comments? from SchemaCreation.

@dsounded
Copy link
Contributor

dsounded commented Feb 23, 2016

+1 for this

@Envek
Copy link
Contributor Author

Envek commented Feb 25, 2016

@kamipo thank you for review and sorry for the delay. Fixed your notes.

Is there anything that I should do?

@kou
Copy link
Contributor

kou commented Mar 17, 2016

I want this feature.

It seems that there are some conflicts with the current master.

@Envek
Copy link
Contributor Author

Envek commented Mar 19, 2016

@kou, thank you for support. There were a lot of merge conflicts this time (usually it's only CHANGELOG.md). I rebased on top of current master and detailed commit message.

@kamipo, @rafaelfranca, what do you think?

Comments are specified in migrations, stored in database itself (in its schema),
and dumped into db/schema.rb file.

This allows to generate good documentation and explain columns and tables' purpose
to everyone from new developers to database administrators.

For PostgreSQL and MySQL only. SQLite does not support comments at the moment.

See docs for PostgreSQL: http://www.postgresql.org/docs/current/static/sql-comment.html
See docs for MySQL: http://dev.mysql.com/doc/refman/5.7/en/create-table.html
@Envek
Copy link
Contributor Author

Envek commented Apr 16, 2016

@rafaelfranca Is there any chance to get it into 5.0?

@jeremy jeremy merged commit c690b9c into rails:master Apr 17, 2016
1 check passed
jeremy added a commit that referenced this pull request Apr 17, 2016
Add support for specifying comments for tables, columns, and indexes in database itself
@Envek Envek deleted the database_comments branch Apr 17, 2016
@Envek
Copy link
Contributor Author

Envek commented Apr 17, 2016

@jeremy thank you for merging!
Feel free to ping me if anyone will find any issues with comments inside the database. I will do my best to fix it.

@jeremy jeremy added this to the 5.0.0 milestone Apr 17, 2016
@jeremy
Copy link
Member

jeremy commented Apr 17, 2016

Thank you @Envek! Very nice work, and thorough. I have a couple of tweaks coming.

(Could be neat to explore: adding default comments in migrations so tables/columns/indexes indicate which migration added them.)

@jeremy
Copy link
Member

jeremy commented Apr 17, 2016

Looks like we could support this for SQLite, too, since it records the SQL comments used in CREATE TABLE statement: http://stackoverflow.com/questions/6460671/sqlite-schema-information-metadata/6617764#6617764

@Envek
Copy link
Contributor Author

Envek commented Apr 17, 2016

As I can see from the links, SQLite only saves CREATE statements as they were typed into SQLite and have no syntax for accessing or changing comments programmatically. So adding SQLite support requires to parse whole SQL statement and extracting SQL language comments (-- these ones) from it. It may be very cumbersome in case when database wasn't created by Rails.

@jeremy
Copy link
Member

jeremy commented Apr 17, 2016

Oops, missed linking http://stackoverflow.com/questions/7426205/sqlite-adding-comments-to-tables-and-columns?lq=1 above too.

We parse the SQL statement already to extract collation info. It's a little easier because we fetch columns first using PRAGMA table_info(…). That means we can use simple regexps to extract collation info per column. We could do the same to extract an optional trailing SQL comment per column, too.

Ditto for a table comment. If we have a -- sql comment that appears before the first ( or after the last ), then it's a table comment 😁

https://github.com/rails/rails/blob/master/activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb#L531-L568

@vipulnsward
Copy link
Member

vipulnsward commented Apr 17, 2016

@jeremy can you share and example for how "adding default comments in migrations" would look like.

@jeremy
Copy link
Member

jeremy commented Apr 17, 2016

@vipulnsward Think of "git blame" but for your database schema. We'd see which migration last touched a table, column, or index.

Maybe not the best use of comments, but a fun idea to explore. This could also be tracked in a separate metadata table, after all, but that wouldn't be visible in DBA tools.

For example,

# db/migrate/20160417165529_create_foobars.rb
# class CreateFoobars
create_table :foobars do |t|
  t.string :name, index: true
end

could result in a comment: migration.filename on the table, column, and index.

@vipulnsward
Copy link
Member

vipulnsward commented Apr 18, 2016

Got it. I am going to explore this.

@marnen
Copy link

marnen commented Apr 27, 2016

@jeremy:

Think of "git blame" but for your database schema. We'd see which migration last touched a table, column, or index.

Why not just use git blame itself for that on your schema file, then? Or are you thinking that you want to see this at migration granularity, not commit granularity.

@jeremy
Copy link
Member

jeremy commented Apr 29, 2016

Yeah, that's the fun part to explore, @marnen. A db comment indicates what's actually live in the database (authoritative source) vs. what's committed to the codebase (may be ahead of database). @vipulnsward implemented to try it out in #24622, but indeed it weighed a bit on the noisier side of helpful.

jeremy pushed a commit that referenced this pull request Apr 30, 2016
Refactor of #22911.

Signed-off-by: Jeremy Daer <jeremydaer@gmail.com>
@marnen
Copy link

marnen commented Apr 30, 2016

@jeremy Since the schema_migrations table already indicates the current version of the DB, and that can already be cross-referenced to the schema file and migrations, why go to the extra trouble?

@jeremy
Copy link
Member

jeremy commented Apr 30, 2016

Sure - why? For the fun of exploring an idea and using a new feature
available to us. It's easy to poke holes in it. We already did. It was fun
😊
On Fri, Apr 29, 2016 at 21:23 Marnen Laibow-Koser notifications@github.com
wrote:

@jeremy https://github.com/jeremy Since the schema_migrations table
already indicates the current version of the DB, and that can already be
cross-referenced to the schema file and migrations, why go to the extra
trouble?


You are receiving this because you were mentioned.

Reply to this email directly or view it on GitHub
#22911 (comment)

bogdanvlviv added a commit to bogdanvlviv/rails that referenced this pull request Oct 4, 2017
unasuke added a commit to unasuke/rails that referenced this pull request Dec 15, 2018
Enabled specify comment of database objects is introduced at rails#22911.
But for table comment, support only in the create_table clause,
so cannot add or modify comment for the existing table.
But add (modify) table comment is helpful to develop an app that
getting complex as time proceeds, or maintaining a historic app.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet