File content list includes ambiguous column fid in Group By statement

Just happened to be working on another issue and saw this in my e-mail. I reproduced this issue again.

Full postgresql.log

2014-08-24 15:43:09 UTC ERROR:  column reference "fid" is ambiguous at character 587
2014-08-24 15:43:09 UTC STATEMENT:  SELECT file_managed.fid AS fid, file_managed.filename AS file_managed_filename, file_managed.uri AS file_managed_uri, file_managed.filemime AS file_managed_filemime, file_managed.filesize AS file_managed_filesize, file_managed.status AS file_managed_status, file_managed.created AS file_managed_created, file_managed.changed AS file_managed_changed, SUM(file_usage_file_managed.count) AS file_usage_file_managed_count, MIN(file_managed.fid) AS fid_1
	FROM 
	file_managed file_managed
	LEFT JOIN file_usage file_usage_file_managed ON file_managed.fid = file_usage_file_managed.fid
	GROUP BY fid, file_managed_filename, file_managed_uri, file_managed_filemime, file_managed_filesize, file_managed_status, file_managed_created, file_managed_changed
	ORDER BY file_managed_changed DESC
	LIMIT 51 OFFSET 0

I think this is it. If it used the full table alias instead of the base table exception alias, then I think that would solve the issue.

Edit: yes, kind of.

Back in 7.x-3.x initial commit, (2009-05-17) @merlinofchaos added a comment regarding that the base table base field should always just be the field alias and not the table alias and the field alias. I'm not sure why this is, but that's the bug because field_managed and field_usage have a common column name.

If I let views add the field alias normally, then it works, but I don't know what that would break.

  public function addField($table, $field, $alias = '', $params = array()) {                                                                                    
    // We check for this specifically because it gets a special alias.
    if ($table == $this->view->storage->get('base_table') && $field == $this->view->storage->get('base_field') && empty($alias)) {
      $alias = $this->view->storage->get('base_field');
    }  

Another test would be see if this is reproducible in postgresql on Drupal 7 views 3.

Ok, I think this is the right way to do it if base field alias is important as @merlinofchaos mentioned.

Added comments to explain code. I guess this may not be necessary.

Migrate test failed on the first run, but not the second. No code changes in the patch (comments added only).

Updated issue summary with explanation about patch that doesn't fit as code comments.

Yay, I'm getting over my terrible cold.

The query method depends on a couple of Drupal container methods so I think this will probably have to be a functional test or mocking the container.

The query method on views query plugin returns a database query object. The group property is protected so I don't think we can assert that the keys have table.field format. Doing a __toString() is bad as we're trying to work on another issue to always quote fields/aliases, which means that __toString() will never be the same.

So we can't go that deep. Hopefully we can instantiate the views query plugin sql object with the file overview view and some display plugin. And then after calling query method, assert that $this->groupby has keys in table.field format. The groupby variable is public on that query object.

+++ b/core/modules/views/src/Tests/QueryGroupByTest.php
@@ -189,4 +189,20 @@ public function testGroupByCountOnlyFilters() {
+    $view->displayHandlers->get('default')->options['fields']['name']['group_type'] = 'min';
+    unset($view->displayHandlers->get('default')->options['fields']['id']['group_type']);

This bit confused me until I had to look at the test view "test_group_by_count". There are roughly 20 occurrences of unset() being used like this throughout Views tests.

Works for me. Thanks for the patch.

I attached an interdiff with command: interdiff postgres-2192877-add-table-alias-8.patch ambiguous-column-2192877-21.patch > interdiff-8-21.txt

Flip back to RTBC.

Testing infrastructure memory issue broke HEAD. Confirmed passing tests.