{"action":"created","issue":{"url":"https://api.github.com/repos/rails/rails/issues/33616","repository_url":"https://api.github.com/repos/rails/rails","labels_url":"https://api.github.com/repos/rails/rails/issues/33616/labels{/name}","comments_url":"https://api.github.com/repos/rails/rails/issues/33616/comments","events_url":"https://api.github.com/repos/rails/rails/issues/33616/events","html_url":"https://github.com/rails/rails/pull/33616","id":350525706,"node_id":"MDExOlB1bGxSZXF1ZXN0MjA4MzU2MzU4","number":33616,"title":"Improve handling of querying and loading data with ignored_columns","user":{"login":"jasonroelofs","id":3223,"node_id":"MDQ6VXNlcjMyMjM=","avatar_url":"https://avatars0.githubusercontent.com/u/3223?v=4","gravatar_id":"","url":"https://api.github.com/users/jasonroelofs","html_url":"https://github.com/jasonroelofs","followers_url":"https://api.github.com/users/jasonroelofs/followers","following_url":"https://api.github.com/users/jasonroelofs/following{/other_user}","gists_url":"https://api.github.com/users/jasonroelofs/gists{/gist_id}","starred_url":"https://api.github.com/users/jasonroelofs/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/jasonroelofs/subscriptions","organizations_url":"https://api.github.com/users/jasonroelofs/orgs","repos_url":"https://api.github.com/users/jasonroelofs/repos","events_url":"https://api.github.com/users/jasonroelofs/events{/privacy}","received_events_url":"https://api.github.com/users/jasonroelofs/received_events","type":"User","site_admin":false},"labels":[{"id":107191,"node_id":"MDU6TGFiZWwxMDcxOTE=","url":"https://api.github.com/repos/rails/rails/labels/activerecord","name":"activerecord","color":"0b02e1","default":false}],"state":"open","locked":false,"assignee":{"login":"eileencodes","id":1080678,"node_id":"MDQ6VXNlcjEwODA2Nzg=","avatar_url":"https://avatars0.githubusercontent.com/u/1080678?v=4","gravatar_id":"","url":"https://api.github.com/users/eileencodes","html_url":"https://github.com/eileencodes","followers_url":"https://api.github.com/users/eileencodes/followers","following_url":"https://api.github.com/users/eileencodes/following{/other_user}","gists_url":"https://api.github.com/users/eileencodes/gists{/gist_id}","starred_url":"https://api.github.com/users/eileencodes/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/eileencodes/subscriptions","organizations_url":"https://api.github.com/users/eileencodes/orgs","repos_url":"https://api.github.com/users/eileencodes/repos","events_url":"https://api.github.com/users/eileencodes/events{/privacy}","received_events_url":"https://api.github.com/users/eileencodes/received_events","type":"User","site_admin":true},"assignees":[{"login":"eileencodes","id":1080678,"node_id":"MDQ6VXNlcjEwODA2Nzg=","avatar_url":"https://avatars0.githubusercontent.com/u/1080678?v=4","gravatar_id":"","url":"https://api.github.com/users/eileencodes","html_url":"https://github.com/eileencodes","followers_url":"https://api.github.com/users/eileencodes/followers","following_url":"https://api.github.com/users/eileencodes/following{/other_user}","gists_url":"https://api.github.com/users/eileencodes/gists{/gist_id}","starred_url":"https://api.github.com/users/eileencodes/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/eileencodes/subscriptions","organizations_url":"https://api.github.com/users/eileencodes/orgs","repos_url":"https://api.github.com/users/eileencodes/repos","events_url":"https://api.github.com/users/eileencodes/events{/privacy}","received_events_url":"https://api.github.com/users/eileencodes/received_events","type":"User","site_admin":true}],"milestone":null,"comments":4,"created_at":"2018-08-14T17:33:19Z","updated_at":"2018-08-14T18:38:22Z","closed_at":null,"author_association":"NONE","pull_request":{"url":"https://api.github.com/repos/rails/rails/pulls/33616","html_url":"https://github.com/rails/rails/pull/33616","diff_url":"https://github.com/rails/rails/pull/33616.diff","patch_url":"https://github.com/rails/rails/pull/33616.patch"},"body":"There are multiple situations that led to investigating how\r\n`ignored_columns` works, how it interacts with querying and how to make\r\nit better in more situations.\r\n\r\nThe first problem is that the use of `ignored_columns` in a class was\r\nbreaking migrations that ran after the removal of a column in that class\r\n(a column that was not added to `ignored_columns`). This turned out to\r\nbe because when `ignored_columns` has entries, AR was exploding the full\r\nlist of known columns in the query (including the column that was just\r\nremoved , instead of letting `Arel.star` just return the database's known\r\nlist. While this is solvable by adding `reset_column_information` in the\r\nmigration between column removal and the failing code, this research led\r\nto our next concern.\r\n\r\nOur second concern is the performance effects of splatting out the full\r\ncolumn list for every query for a model with `ignored_columns` set. We\r\nhaven't done explicit performance testing but processing a list of\r\n`arel_columns` is definitely slower than a simple `Arel.star`, and this\r\ncould easily add up for high traffic sites.\r\n\r\nAs such, we wanted to find a way to support `ignored_columns` properly,\r\nincluding keeping the correct `reload` functionality, without resorting\r\nto a full set of columns for every query.\r\n\r\nThis commit contains two changes to fix the above concerns.\r\n\r\n* When a select set is explicitly given, remove the columns that are in\r\nthe `ignored_columns` list.\r\n* When loading a record from the database, ignore the attributes that\r\nare in the `ignored_columns` list.\r\n\r\n/cc @eileencodes @tenderlove "},"comment":{"url":"https://api.github.com/repos/rails/rails/issues/comments/412973804","html_url":"https://github.com/rails/rails/pull/33616#issuecomment-412973804","issue_url":"https://api.github.com/repos/rails/rails/issues/33616","id":412973804,"node_id":"MDEyOklzc3VlQ29tbWVudDQxMjk3MzgwNA==","user":{"login":"matthewd","id":1034,"node_id":"MDQ6VXNlcjEwMzQ=","avatar_url":"https://avatars1.githubusercontent.com/u/1034?v=4","gravatar_id":"","url":"https://api.github.com/users/matthewd","html_url":"https://github.com/matthewd","followers_url":"https://api.github.com/users/matthewd/followers","following_url":"https://api.github.com/users/matthewd/following{/other_user}","gists_url":"https://api.github.com/users/matthewd/gists{/gist_id}","starred_url":"https://api.github.com/users/matthewd/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/matthewd/subscriptions","organizations_url":"https://api.github.com/users/matthewd/orgs","repos_url":"https://api.github.com/users/matthewd/repos","events_url":"https://api.github.com/users/matthewd/events{/privacy}","received_events_url":"https://api.github.com/users/matthewd/received_events","type":"User","site_admin":false},"created_at":"2018-08-14T18:38:22Z","updated_at":"2018-08-14T18:38:22Z","author_association":"MEMBER","body":"I do think this option is currently doing double duty.. sometimes to mean \"don't mention this\" and others to mean \"don't pull these values over the wire\". Maybe it's worth splitting it internally into two names, so they can be overridden individually?\r\n\r\nI don't think the explicit removal from select is desirable… IMO if you've gone out of your way to ask for it, we should give it to you -- specific [parameters on a query] beats general [method on a model].\r\n\r\nHow far could we get by extracting a\r\n```ruby\r\ndef select_all_columns(table)\r\n  if ignored_columns.any?\r\n    column_names.map { |field| arel_attribute(field, table) }\r\n  else\r\n    table[Arel.star]\r\n  end\r\nend\r\n```\r\n\r\nfrom `build_select`?\r\n\r\nThat'd keep `ignored_columns` doing its existing double duty by default, but would allow the projection to be overridden separately (e.g., to force it back to Arel.star)."}}
{"actor":{"display_login":"matthewd"}}