Allow the construction of a CTE from a string (#42561)

[dark-panda]

Summary

Constructing CTE WITH statements from strings broke some time between Rails 6.0 and 6.1.

See #42561 for details.

It might be a good idea to merge this in as a bug fix to 6-1-stable, since this behaviour was working in 6.0.

[rafaelfranca]

I think better than changing this to deal with strings we should build a SQLLiteral node when calling with in the SelectManager like we do with from, group and order.

[dark-panda]

Perhaps that would be better. I can take a look at that tomorrow how about.

[vlado]

@dark-panda I think something like this should work https://github.com/rails/rails/pull/37944/files#diff-79b53b2602bf702bdd8ce677e096be6a6923a54236e17237c16068a510078683R1485

[dark-panda]

@rafaelfranca @vlado I took a look at using an SqlLiteral node and the problem we have at that point is that by the time the code reaches where I've patched, we're expecting a node that has a name and relation of some sort, so that we can effectively build #{name} AS #{query}. In the case of an using an SqlLiteral in the SelectManager to wrap a string, you're already presumably in the form of #{name} AS #{query}, so unless you go and parse the contents of the string to separate out the name and the query you're kind of right back to where you started. I don't think you can avoid modifying #collect_ctes without handling at least part of this case.

Another option might be to have a different node type, kind of like the case for joins, where you can also pass things like strings or symbols or whatever, where you pass a StringJoin node in the case of strings or SqlLiterals. You could potentially have StringWith in that case, although it might be necessary to have one for StringWith and StringWith Recursive.

[rafaelfranca]

My suggestion is to use SQLLiteral but keep the change in this method, but instead of checking for strings we would check for a SQLLiteral. The reason why I think this change would be better is to avoid the build an AST where the nodes are not always a subclass of Node.

[dark-panda]

@rafaelfranca ah okay, that's easy enough. I've pushed up another commit with something along these lines then, let's see if that's closer to what we're looking for.

[dark-panda]

@rafaelfranca any further changes requested here? The aforementioned SQLLiteral changes were made and pushed.

[rails-bot]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Thank you for your contributions.