Restored alias to single, non-symbol attribute selects

[kobsy]

This was a tricky bug to track down, but this commit introduces a subtle regression that actually seems related to some odd behavior in Rails' type casting. From what I can gather, Rails' type casting maps results and what type they should be cast to by name; when each select's name is unique (or aliased to something unique), there's no problem. However, when two selects share a name, either by selecting the same-named column in joined tables (as in the linked issue) OR by calling the same SQL function (I ran into it by plucking COALESCE() functions), Rails will cast all of them to the type of the last one. This leads to unexpected results when, say, one COALESCE results in an array and another in a string. What you expected to be an array will instead show up as "{value1,value2,value3}" ... or more confoundingly nil instead of []! 😱

This commit reverts the removal of built-in aliasing to mitigate the issue.

[boblail]

👏 Great job troubleshooting and thanks for the PR!

Instead of a test that reciprocates your solution, would you be willing to write a test that reproduces the original problem? (Maybe one that uses COALESCE twice — even if the scenario is fairly contrived!)

I'd be really interested to see if the failure is the same for all 3 supported databases and across all 5 supported versions of Rails.

Have you used Appraisal before? Did you try it with PluckMap? You can do be appraisal install to bundle all 5 gemfiles and then be appraisal rake test to run your specs against all 5 versions of Rails.

It'd also be useful to pin down the problem being solved so we could explore different solutions. PluckMap is in an ideal position to detect this scenario and maybe we could provide hints to the type_caster or apply patches to Rails as in association_scope.rb

[boblail]

Is this an example of the original problem?

Interestingly, it only happens for Rails' Postgres adapter. MySQL and SQLite are in the clear.

[boblail]

Here's the output from each database in its REPL:

SQLite

sqlite> select COALESCE(NULL, 'four'), COALESCE(NULL, 4);
COALESCE(NULL, 'four')  COALESCE(NULL, 4)
----------------------  -----------------
four                    4

MySQL

mysql> select COALESCE(NULL, 'four'), COALESCE(NULL, 4);
+------------------------+-------------------+
| COALESCE(NULL, 'four') | COALESCE(NULL, 4) |
+------------------------+-------------------+
| four                   |                 4 |
+------------------------+-------------------+
1 row in set (0.00 sec)

PostgreSQL

boblail=# select COALESCE(NULL, 'four'), COALESCE(NULL, 4);
 coalesce | coalesce
----------+----------
 four     |        4
(1 row)

I bet that's what Rails is getting too 🤔

[boblail]

See rails/rails#36186

[kobsy]

Yeah, I thought after I'd signed off Friday that that test was probably not adequate and that I should probably write one that demonstrates the issue in the Rails type-caster. But then you wrote #17, which seems to exactly demonstrate the problem; I'm confident that this solution would make that test pass in Postgres (so interesting that it's a Postgres-only problem!). But then you went a step further and offered a solution to Rails! 😆

In light of all that, what's the best way forward on this PR? I can incorporate the test from #17 into this PR, which would give the solution a meaningful test. But the whole thing becomes moot if/when rails/rails#36186 is merged and released with Rails. Would there be a way to merge it now (since I doubt your Rails fix will make it back to pre-6 versions of Rails) but also make a note somewhere that it can be removed (hopefully) in future versions of Rails?

[boblail]

👍 I see you've incorporated a test like the one in #17. I'll close #17.

Regardless of the Rails fix, let's keep this change in PluckMap.

But I think we can drop the test from attribute_builder_test.rb.

And, in fact, let's move the implementation from Attribute#initialize to Presenter#selects (in the future — in 1.0 — there will be alternate ways of naming values selected from the database that won't expect AS "name" syntax)

[boblail]

There's no need for .tr("_", " ") (I think this might've been a vestigial convenience for Members' version of the to_csv presenter, but it's inconsequential because we're not using alias in PluckMap's to_csv.rb and as: provides a way for clients to specify arbitrary human-readable column names); so when you move this to Presenter#selects, we can just use Attribute#name and don't have to reintroduce #alias.

[boblail]

As of 4d030c7, selects will never contain a String; all raw SQL values will already be wrapped in Arel.sql.

Instead of this

Arel.sql("#{select} AS \"#{name}\"") 

do this:

select.as(name)

Arel::Nodes::SqlLiteral includes Arel::AliasPredication, so we can use as to set one up. That way our code isn't responsible for knowing

1. whether the current database supports AS
2. whether the current database quotes values with "
3. whether we've escaped name (or @alias) appropriately

[kobsy]

Neat! It's much more elegant to be assured of getting an Arel::Nodes::SqlLiteral 👍

[boblail]

What do we do in this scenario?

PluckMap[Example].define do
  field select: [
    Arel.sql("COALESCE(NULL, 'four')"),
    Arel.sql("COALESCE(NULL, 4)")
  ], map: ->(string, number) { "#{string}#{number}" }
end

🤔

Backing up ...

Most of the time AS x is superfluous. We work with the values of the query and know their identities by position. We disregard name.

AS x could serve two purposes:

1. It circumvents a bug in Rails that occurs only with the Postgres database but it doesn't circumvent the problem 100% of the time: it doesn't solve for the scenario in Plucking associated data from an enum column returns nil rails/rails#36042 where two columns in different tables share the same name but not the same type.
2. It would be useful to use AS x in a query like COPY ( ... ) TO STDOUT WITH CSV HEADER where the database constructs the desired document without any subsequent mapping. This would only work when PluckMap doesn't need to do any subsequent mapping which is why the original aliasing only happened in the branch of a conditional where selects.length == 1.

So AS x is only useful:

1. when the DB is Postgres (and Rails isn't patched)
2. to a hypothetical to_csv__optimized method

🤔🤔 Time for me to go to work, but now I'm leaning back toward Can PluckMap backport my fix? or Can PluckMap apply a less-surgical patch that "tricks" Rails' buggy implementation into doing the right thing?

Alternately, if my PR is accepted, my plan is to inquire whether I may create copies of the PR for older versions of Rails.

[kobsy]

I dropped the extraneous test and moved the aliasing into Presenter#selects. I don't love this solution, as the aliases in the query become somewhat meaningless, but it has the advantages of not having to work backward to find the attribute(s) for a select and elegantly handling multiple selects-per-attribute situation. I agree that the ideal would probably be if we could back port your Rails fix or somehow patch it in pluck_map; that would allow the consumer to add their own .as() to selects as needed/desired. But neither am I sure how to best patch/work around ActiveRecord::Result from within this gem. 😅 Thoughts?

[kobsy]

🤔 Hm, it looks like it's just the 4.2 build that's failing. Not sure why it's not properly converting the .as(...) to SQL. Did pluck change how it works between 4.2 and 5.0 (I checked Arel's as, and that seems to be unchanged between 6.x and current)?

[kobsy]

It turns out pluck did change how it worked between 4.2 and 5.0! Prior to Rails 5, pluck called .map!(&:to_s) on its parameters; after, it converts everything to arel attributes. I updated Presenter#selects to optionally call to_sql if needed for versions of Rails prior to 5.

[kobsy]

... but of course the Rails constant isn’t defined in tests... 🤦‍♂️

[boblail]

You can use ActiveRecord.version (e.g.)

[kobsy]

Yeah, that makes sense. This is what I get for working on this too late last night. 😅

[boblail]

👍I'm good with this.

🤔 I think I'll add a lengthy comment to it