aboutsummaryrefslogtreecommitdiffstats
Commit message (Collapse)AuthorAgeFilesLines
* Remove unused `DepthFirst` visitorRyuta Kamizono2019-06-157-518/+7
| | | | | | | | | | | | | We only use `ToSQL` visitors in the our codebase, do not use `DepthFirst` and `Dot` visitors. The `DepthFirst` visitor (which was introduced at c86c37e5f) is used to traverse an Arel (partial) ast with depth first. Is there any worth to keep that undocumented feature with much code and test cases. This removes that unused `DepthFirst` code and test cases.
* Merge pull request #36483 from kamipo/no_allocation_to_sql_visitRyuta Kamizono2019-06-154-33/+34
|\ | | | | No allocation `Arel::Visitors::ToSql#visit`
| * No allocation `Arel::Visitors::ToSql#visit`Ryuta Kamizono2019-06-154-33/+34
|/ | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | Each `visit o, collector` allocates one extra array due to receiving args by splat array. https://github.com/rails/rails/blob/2c3332cc4c0fa77dbe2e13e8a792f80fbd8f4ad3/activerecord/lib/arel/visitors/visitor.rb#L27-L29 Currently 1,000 times `User.where(id: 1).to_sql` allocates 13,000 arrays in `visitor.accept`. This avoids receiving args by splat array, it makes `visitor.accept` no array allocation. ```ruby ObjectSpace::AllocationTracer.setup(%i{path line type}) pp ObjectSpace::AllocationTracer.trace { 1_000.times { User.where(id: 1).to_sql } }.select { |k, _| k[2] == :T_ARRAY && k[0].end_with?("visitor.rb", "to_sql.rb") } ``` Before (2c3332cc4c0fa77dbe2e13e8a792f80fbd8f4ad3): ``` {["~/rails/activerecord/lib/arel/visitors/to_sql.rb", 18, :T_ARRAY]=>[1000, 0, 0, 0, 0, 0], ["~/rails/activerecord/lib/active_record/connection_adapters/determine_if_preparable_visitor.rb", 11, :T_ARRAY]=>[1000, 0, 0, 0, 0, 0], ["~/rails/activerecord/lib/arel/visitors/visitor.rb", 12, :T_ARRAY]=>[1000, 0, 0, 0, 0, 0], ["~/rails/activerecord/lib/arel/visitors/to_sql.rb", 788, :T_ARRAY]=>[3000, 0, 0, 0, 0, 0], ["~/rails/activerecord/lib/arel/visitors/to_sql.rb", 794, :T_ARRAY]=>[3000, 0, 0, 0, 0, 0], ["~/rails/activerecord/lib/arel/visitors/to_sql.rb", 156, :T_ARRAY]=>[1000, 0, 0, 0, 0, 0], ["~/rails/activerecord/lib/arel/visitors/to_sql.rb", 443, :T_ARRAY]=>[1000, 0, 0, 0, 0, 0], ["~/rails/activerecord/lib/arel/visitors/to_sql.rb", 603, :T_ARRAY]=>[1000, 0, 0, 0, 0, 0], ["~/rails/activerecord/lib/arel/visitors/to_sql.rb", 611, :T_ARRAY]=>[1000, 0, 0, 0, 0, 0]} ``` After (this change): ``` {} ```
* :golf:Akira Matsuda2019-06-151-6/+2
|
* Should find last created recordRyuta Kamizono2019-06-151-3/+3
| | | | | | | Tables in tests are not always empty so `klass.first` does not always find last created record. Fixes #36479.
* Ensure to reset actually used `@connection.schema_migration`'s table nameRyuta Kamizono2019-06-151-4/+4
| | | | https://buildkite.com/rails/rails/builds/61744#f12cc6cf-7458-4131-917a-9735615f6259/999-1010
* Fix `test_schema_names` to include "hint_plan" schemaRyuta Kamizono2019-06-151-1/+5
|
* Merge pull request #36469 from ↵Eileen M. Uchitelle2019-06-148-49/+111
|\ | | | | | | | | eileencodes/move-while_preventing_writes-to-handler Move while_preventing_writes from conn to handler
| * Move while_preventing_writes from conn to handlereileencodes2019-06-148-49/+111
|/ | | | | | | | | | | | | | | | | If we put the `while_preventing_writes` on the connection then the middleware that sends reads to the primary and ensures they can't write will not work. The `while_preventing_writes` will only be applied to the connection which it's called on - which in the case of the middleware is Ar::Base. This worked fine if you called it directly like `OtherDbConn.connection.while_preventing_writes` but Rails didn't have a way of knowing you wanted to call it on all the connections. The change here moves the `while_preventing_writes` method from the connection to the handler so that it can block writes to all queries for that handler. This will apply to all the connections associated with that handler.
* Merge pull request #36488 from eileencodes/update-multi-db-docsEileen M. Uchitelle2019-06-141-1/+20
|\ | | | | Update multi-db docs
| * Update multi-db docseileencodes2019-06-141-1/+20
|/ | | | | | * Add note about schema cache * Add note about opening too many connections * Improve headers in caveats section
* Merge pull request #36439 from ↵Eileen M. Uchitelle2019-06-1417-150/+412
|\ | | | | | | | | eileencodes/move-schema-migration-to-migration-context Move SchemaMigration to migration_context
| * Move SchemaMigration to migration_contexteileencodes2019-06-1417-150/+412
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | This PR moves the `schema_migration` to `migration_context` so that we can access the `schema_migration` per connection. This does not change behavior of the SchemaMigration if you are using one database. This also does not change behavior of any public APIs. `Migrator` is private as is `MigrationContext` so we can change these as needed. We now need to pass a `schema_migration` to `Migrator` so that we can run migrations on the right connection outside the context of a rake task. The bugs this fixes were discovered while debugging the issues around the SchemaCache on initialization with multiple database. It was clear that `get_all_versions` wouldn't work without these changes outside the context of a rake task (because in the rake task we establish a connection and change AR::Base.connection to the db we're running on). Because the `SchemaCache` relies on the `SchemaMigration` information we need to make sure we store it per-connection rather than on ActiveRecord::Base. [Eileen M. Uchitelle & Aaron Patterson]
* | Merge pull request #36484 from ↵Ryuta Kamizono2019-06-141-3/+3
|\ \ | |/ |/| | | | | albertoalmagro/alberto/reverse-column-is-reversible [ci skip] Update docs as `remove_column` can be reversed
| * [ci skip] Update docs as `remove_column` can be reversedAlberto Almagro2019-06-141-3/+3
| | | | | | | | | | As `remove_column` can be reversed when a type is provided this example was not accurate anymore.
* | Fix rubocop violationsRyuta Kamizono2019-06-142-3/+2
| |
* | Merge pull request #36478 from kamipo/allocation_on_demand_in_transactionRyuta Kamizono2019-06-141-20/+35
|\ \ | | | | | | Allocation on demand in transactions
| * | Allocation on demand in transactionsRyuta Kamizono2019-06-141-20/+35
|/ / | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | Currently 1,000 transactions creates 10,000 objects regardless whether it is necessary or not. This makes allocation on demand in transactions, now 1,000 transactions creates required 5,000 objects only by default. ```ruby ObjectSpace::AllocationTracer.setup(%i{path line type}) pp ObjectSpace::AllocationTracer.trace { 1_000.times { User.create } }.select { |k, _| k[0].end_with?("transaction.rb") } ``` Before (95d038f): ``` {["~/rails/activerecord/lib/active_record/connection_adapters/abstract/transaction.rb", 209, :T_HASH]=>[1000, 0, 715, 0, 1, 0], ["~/rails/activerecord/lib/active_record/connection_adapters/abstract/transaction.rb", 210, :T_OBJECT]=>[1000, 0, 715, 0, 1, 0], ["~/rails/activerecord/lib/active_record/connection_adapters/abstract/transaction.rb", 210, :T_HASH]=>[1000, 0, 715, 0, 1, 0], ["~/rails/activerecord/lib/active_record/connection_adapters/abstract/transaction.rb", 80, :T_OBJECT]=>[1000, 0, 715, 0, 1, 0], ["~/rails/activerecord/lib/active_record/connection_adapters/abstract/transaction.rb", 8, :T_ARRAY]=>[1000, 0, 715, 0, 1, 0], ["~/rails/activerecord/lib/active_record/connection_adapters/abstract/transaction.rb", 81, :T_ARRAY]=>[1000, 0, 715, 0, 1, 0], ["~/rails/activerecord/lib/active_record/connection_adapters/abstract/transaction.rb", 289, :T_STRING]=>[1000, 0, 714, 0, 1, 0], ["~/rails/activerecord/lib/active_record/connection_adapters/abstract/transaction.rb", 116, :T_ARRAY]=>[1000, 0, 714, 0, 1, 0], ["~/rails/activerecord/lib/active_record/connection_adapters/abstract/transaction.rb", 120, :T_ARRAY]=>[1000, 0, 714, 0, 1, 0], ["~/rails/activerecord/lib/active_record/connection_adapters/abstract/transaction.rb", 121, :T_HASH]=>[1000, 0, 714, 0, 1, 0]} ``` After (this change): ``` {["~/rails/activerecord/lib/active_record/connection_adapters/abstract/transaction.rb", 213, :T_HASH]=>[1000, 0, 739, 0, 1, 0], ["~/rails/activerecord/lib/active_record/connection_adapters/abstract/transaction.rb", 214, :T_OBJECT]=>[1000, 0, 739, 0, 1, 0], ["~/rails/activerecord/lib/active_record/connection_adapters/abstract/transaction.rb", 214, :T_HASH]=>[1000, 0, 739, 0, 1, 0], ["~/rails/activerecord/lib/active_record/connection_adapters/abstract/transaction.rb", 81, :T_OBJECT]=>[1000, 0, 739, 0, 1, 0], ["~/rails/activerecord/lib/active_record/connection_adapters/abstract/transaction.rb", 304, :T_STRING]=>[1000, 0, 738, 0, 1, 0]} ```
* | Merge pull request #36477 from albertoalmagro/alberto/button-to-default-pathRafael França2019-06-131-1/+1
|\ \ | | | | | | [ci skip] Use default path in button_to documentation
| * | [ci skip] Use default path in button_to documentationAlberto Almagro2019-06-131-1/+1
| |/ | | | | | | | | | | | | | | | | | | This is really a nit pick, but as this is the framework's documentation I think it should follow standards as many times as possible to avoid confusion in new users. If we were using `resources :articles` in routes. which is what scaffold adds, the generated helper would be `new_article_path` instead of `new_articles_path`.
* | Merge pull request #36437 from sudara/fix_programmatic_clicks_with_data_remoteGannon McGibbon2019-06-132-2/+16
|\ \ | |/ |/| Fix programmatic clicks with data-remote
| * Ensure non-mouse/programmatic clicks work with data-remoteSudara2019-06-132-2/+16
| |
* | Make ActiveRecord `ConnectionPool.connections` thread-safe. (#36473)jeffdoering2019-06-133-1/+44
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | * Make ActiveRecord `ConnectionPool.connections` thread-safe. ConnectionPool documentation is clear on the need to synchronize access to @connections but also states that public methods do not require synchronization. Existing code exposed @connections directly via attr_reader. The fix uses synchronize() to lock @connections then returns a copy to the caller using Array.dup(). Includes comments on the connections method that thread-safe access to the connections array does not imply thread-safety of accessing methods on the actual connections. Adds a test-case that modifies the pool using a supported method in one thread while a second thread accesses pool.connections. The test fails without this patch. Fixes #36465. * Update activerecord/test/cases/connection_pool_test.rb [jeffdoering + Rafael Mendonça França]
* | Merge pull request #36466 from wbnns/update-missing-create-action-screenshotRafael França2019-06-131-0/+0
|\ \ | | | | | | images/getting_started: Update screenshot for missing action
| * | [ci skip] images/getting_started: Update screenshot for missing actionWill Binns2019-06-121-0/+0
| | |
* | | Merge pull request #36388 from joelhawksley/actionview-componentAaron Patterson2019-06-134-1/+72
|\ \ \ | | | | | | | | Introduce ActionView::Component
| * | | `RenderingHelper` supports rendering objects that `respond_to?` `:render_in`Joel Hawksley2019-06-124-1/+72
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | Co-authored-by: Natasha Umer <natashau@github.com> Co-authored-by: Aaron Patterson <tenderlove@github.com> Co-authored-by: Shawn Allen <shawnbot@github.com> Co-authored-by: Emily Plummer <emplums@github.com> Co-authored-by: Diana Mounter <broccolini@github.com> Co-authored-by: John Hawthorn <jhawthorn@github.com> Co-authored-by: Nathan Herald <myobie@github.com> Co-authored-by: Zaid Zawaideh <zawaideh@github.com> Co-authored-by: Zach Ahn <engineering@zachahn.com>
| * | | Merge pull request #2 from rails/masterJoel Hawksley2019-06-0365-293/+486
| |\ \ \ | | | | | | | | | | merge master
* | | | | Ensure to reset migration version after testing migrationRyuta Kamizono2019-06-131-2/+2
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | "schema_migrations" table may be hard dropped before, so the reset migration version should be done in ensure block. https://buildkite.com/rails/rails/builds/61697#18d6f3ac-2257-4f4b-8efc-4010464c4d9a/999-1011
* | | | | Reset migration version before testing migrationRyuta Kamizono2019-06-131-0/+2
| | | | | | | | | | | | | | | | | | | | https://buildkite.com/rails/rails/builds/61695#373bb1a7-677f-49ec-95e7-a92467fefd60/1076-1084
* | | | | Avoid implicit rollback when testing migrationRyuta Kamizono2019-06-131-0/+2
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | "schema_migrations" is hard dropped by some existing tests, so testing migration in using transactional tests may cause implicit creation and rollback "schema_migrations" table, it makes migration tests flaky. https://buildkite.com/rails/rails/builds/61692#42383249-30be-4508-b1fb-a7bb27600c8e/999-1010 https://buildkite.com/rails/rails/builds/61694#6e462ad3-41d8-4e26-95ce-728495b0ac64/999-1010
* | | | | Merge pull request #36472 from kamipo/empty_line_only_before_access_modifierRyuta Kamizono2019-06-13434-514/+8
|\ \ \ \ \ | | | | | | | | | | | | Enable `Layout/EmptyLinesAroundAccessModifier` cop
| * | | | | Enable `Layout/EmptyLinesAroundAccessModifier` copRyuta Kamizono2019-06-13434-514/+8
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | We sometimes say "✂️ newline after `private`" in a code review (e.g. https://github.com/rails/rails/pull/18546#discussion_r23188776, https://github.com/rails/rails/pull/34832#discussion_r244847195). Now `Layout/EmptyLinesAroundAccessModifier` cop have new enforced style `EnforcedStyle: only_before` (https://github.com/rubocop-hq/rubocop/pull/7059). That cop and enforced style will reduce the our code review cost.
* | | | | | Remove duplicated `table_exists?`Ryuta Kamizono2019-06-132-8/+0
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | `table_exists?` is already exist in `ModelSchema`. https://github.com/rails/rails/blob/5cab344494c340ea82a35b46efa06b94f0b7730b/activerecord/lib/active_record/model_schema.rb#L339-L341
* | | | | | Don't `drop_table` before schema cache testsRyuta Kamizono2019-06-131-1/+2
| | | | | | | | | | | | | | | | | | | | | | | | Since 5cab34449, `drop_table` clears schema cache.
* | | | | | Clear schema cache when a table is created/dropped/renamedRyuta Kamizono2019-06-136-4/+18
|/ / / / / | | | | | | | | | | | | | | | Otherwise `Model.table_exists?` returns the staled cache result.
* | | | | Merge pull request #36470 from lucasprag/activesupport/missing-require-digestRafael França2019-06-121-0/+3
|\ \ \ \ \ | |_|_|/ / |/| | | | Add missing file to require digest/uuid on active_support core ext
| * | | | Add missing file to require digest/uuid on active_support core extensionsLucas Arantes2019-06-121-0/+3
|/ / / /
* | | | Merge pull request #36447 from holyketzer/masterRafael França2019-06-111-1/+1
|\ \ \ \ | | | | | | | | | | ActionCable: optimise logger.debug calling
| * | | | ActionCable: optimize logger.debug callingAlex Emelyanov2019-06-081-1/+1
| | | | |
* | | | | Merge pull request #36440 from malept/multi-db-abort_if_pending_migrations-taskEileen M. Uchitelle2019-06-113-1/+55
|\ \ \ \ \ | | | | | | | | | | | | Add support for multiple databases to `rails db:abort_if_pending_migrations`
| * | | | | Convert the db:abort_if_pending_migrations task to be multi-DB awareMark Lee2019-06-103-1/+55
| | | | | |
* | | | | | Merge pull request #36448 from ↵Ryuta Kamizono2019-06-117-42/+67
|\ \ \ \ \ \ | |/ / / / / |/| | | | | | | | | | | | | | | | | kamipo/allow_column_name_with_simple_function_call Allow column name with function (e.g. `length(title)`) as safe SQL string
| * | | | | Allow column name with function (e.g. `length(title)`) as safe SQL stringRyuta Kamizono2019-06-107-42/+67
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | Currently, almost all "Dangerous query method" warnings are false alarm. As long as almost all the warnings are false alarm, developers think "Let's ignore the warnings by using `Arel.sql()`, it actually is false alarm in practice.", so I think we should effort to reduce false alarm in order to make the warnings valuable. This allows column name with function (e.g. `length(title)`) as safe SQL string, which is very common false alarm pattern, even in the our codebase. Related 6c82b6c99, 6607ecb2a, #36420. Fixes #32995.
* | | | | | All modern adapters returns a numeric value as the result of numeric calculationRyuta Kamizono2019-06-112-10/+2
| | | | | |
* | | | | | Merge pull request #36452 from kojoma/kojoma/update_guidesKasper Timm Hansen2019-06-101-2/+2
|\ \ \ \ \ \ | | | | | | | | | | | | | | [ci skip] Fix rails/command.rb document
| * | | | | | [ci skip] Fix rails/command.rb documentkojoma2019-06-101-2/+2
| | | | | | |
* | | | | | | Bump `webdrivers`yuuji.yaginuma2019-06-101-7/+5
| |/ / / / / |/| | | | | | | | | | | | | | | | | Latest `webdrivers` is necessary to run ujs test.
* | | | | | Allow `column_name AS alias` as safe SQL stringRyuta Kamizono2019-06-105-0/+14
| | | | | |
* | | | | | Refactor `disallow_raw_sql!` to avoid `split(/\s*,\s*/)` to order argsRyuta Kamizono2019-06-096-21/+56
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | `split(/\s*,\s*/)` to order args and then `permit.match?` one by one is much slower than `permit.match?` once.
generated by cgit v1.2.3 (git 2.25.1) at 2024-11-15 23:37:00 +0000