Honor association views when wrapping #335
Conversation
|
Can i get a review on this? |
| expect(users[:id].as(:uid).as(:uuid).sql_literal(ds)). | ||
| to eql(%("users"."id" AS "uuid")) | ||
| end | ||
| end |
There was a problem hiding this comment.
It would be good to extract this into a separate PR. I think it's a good fix that can be shipped separately.
lib/rom/sql/attribute.rb
Outdated
| @@ -36,7 +36,7 @@ def self.[](*args) | |||
| # @api public | |||
| def aliased(alias_name) | |||
| super.with(name: name || alias_name).meta( | |||
| sql_expr: sql_expr.as(alias_name) | |||
| sql_expr: alias_sql_expr(sql_expr, alias_name) | |||
There was a problem hiding this comment.
it looks like we should have our own composable wrapper for aliased expressions, that case analysis in alias_sql_expr is leaking
There was a problem hiding this comment.
I think this will be challenging to do in this PR, i'm not sure exactly how to approach it since from what i can see, sql_expr is an actual Sequel::* class everywhere so it may have unexpected breakage. Can we postpone this refactoring?
|
@flash-gordon I noticed a weird error when wrapping an aliased function. essentially, the attribute name has an extra prefix that is never unwrapped, so for the failing spec 27f4604, the wrapped key is |
|
Fixed the issue with aliasing functions. But now I've discovered a new regression in 5.0, aliasing attributes in wrapped relations does not work. So working on that now. |
|
Alright, so I believe this PR is ready. It might not be the prettiest, but at least the behavior is correct now. With this patch, our (large) production app can now pass all specs on rom 5! What this patch addresses
Excited to get this in so we do not have to run off a fork, and contribute upstream to bugfixes, features, etc.
|
|
PR comments have been addressed :) |
|
@flash-gordon could you take a look as well, please? |
There was a problem hiding this comment.
It's good except for one place I pointed. The whole thing about aliased attributes is a solid candidate for refactoring in the next version. It's too error-prone and leaky now. Specifically, I don't like Sequel being mentioned. Instead of using case analysis via reflection we should always rely on duck typing.
lib/rom/sql/mapper_compiler.rb
Outdated
|
|
||
| def extract_wrapped_name(node) | ||
| _, _, meta_options = node | ||
| unwrapped_name = meta_options[:alias].to_s |
There was a problem hiding this comment.
in ruby 2.7, Symbol#to_s start returning frozen strings, this code will break
|
@flash-gordon @solnic fixed the last issue. code climate seems to be failing for an unrelated reason. merge? 😄 |
|
@ianks nice, please squash the commits, I'll merge it shortly after |
|
@flash-gordon All squashed. |
Previously, when wrapping a relation, the view for the association would not be honored. This would make the output of
.wrapdifferent from combine, and cause unexpected behavior.This PR addresses that by first applying the view to the association target if there is one.
Also, since
.wrapwith prefix attributes, any previously aliased attributes would fail loudly sinceSequel::SQL::AliasedExpressiondoes not respond to.as. To fix this, we now will handle aliasSequel::SQL::AliasedExpressionby constructing a new instance with the new alias name.