New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] Fix aliased attributes sql literals #318

Open
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@waiting-for-dev
Copy link

waiting-for-dev commented Jan 16, 2019

References #316

This is still a WIP to fix referenced bug: tests are missing and some
previous have been broken. A more comprehensive solution needs to be
found, so I ask for any feedback or help.

When sequel creates a sql expression from an object, it tries to call
one the two following methods in this order:

  • sql_literal_append
  • sql_literal

rom-sql is implementing sql_literal, which only takes as argument the
sequel datastore. However, sql_literal_append takes two arguments: the
datastore and the sql string built so far.

The partial solution implemented in this commit matches the sql string
at the moment of invocation in order to detect whether we are to the
left of the FROM table clause or to the right of it. If we are to the
left we are referencing a column from the SELECT clause, so we keep
appending name AS alias. However, if we are to the right, what we need
is the canonical sql literal representation (i.e. WHERE name = 1).

This is obviously very hacky. I haven't found any way to inspect the
dataset or any other component in order to have a solid knowledge about
which sql clause is being constructed. So I'm afraid it could be the
only possible fix.

In case something in the same vein of this solution is to be
implemented, we should think the most comprehensive way to match the sql
query built so far. For example, the current solution would not work for
an UPDATE table SET name as alias = 1... statement. We should also
study whether it could break edge corner cases, like a column name
containing FROM table in its name.

[WIP] Fix aliased attributes sql literals
References #316

This is still a WIP to fix referenced bug: tests are missing and some
previous have been broken. A more comprehensive solution needs to be
found, so I ask for any feedback or help.

When sequel creates a sql expression from an object, it tries to call
one the two following methods in this order:

- `sql_literal_append`
- `sql_literal`

rom-sql is implementing `sql_literal`, which only takes as argument the
sequel datastore. However, `sql_literal_append` takes two arguments: the
datastore and the sql string built so far.

The partial solution implemented in this commit matches the sql string
at the moment of invocation in order to detect whether we are to the
left of the `FROM table` clause or to the right of it. If we are to the
left we are referencing a column from the `SELECT` clause, so we keep
appending `name AS alias`. However, if we are to the right, what we need
is the canonical sql literal representation (i.e. `WHERE name = 1`).

This is obviously very hacky. I haven't found any way to inspect the
dataset or any other component in order to have a solid knowledge about
which sql clause is being constructed. So I'm afraid it could be the
only possible fix.

In case something in the same vein of this solution is to be
implemented, we should think the most comprehensive way to match the sql
query built so far. For example, the current solution would not work for
an `UPDATE table SET name as alias = 1...` statement. We should also
study whether it could break edge corner cases, like a column name
containing `FROM table` in its name.
@solnic

This comment has been minimized.

Copy link
Member

solnic commented Jan 17, 2019

What about handling this at relation/schema level? This would be more explicit and less risky. We can convert attributes depending on the context in which they are used (this is how it works already in the relation API).

@waiting-for-dev

This comment has been minimized.

Copy link
Author

waiting-for-dev commented Jan 17, 2019

Thanks for your feedback @solnic . I didn't realize it was possible at that level. Cool, I'll dig into it and try to find something better.

@waiting-for-dev

This comment has been minimized.

Copy link
Author

waiting-for-dev commented Jan 17, 2019

Not sure if it is the same issue but, for a :foo aliased attribute the following doesn't work:

relation.where { relation[:foo].like("something") }

The above fails with #like not being a method. Instead, one must do:

relation.where { relation[:foo].canonical.like("something") }

For now, I just leave a note here so I don't forget.

@waiting-for-dev

This comment has been minimized.

Copy link
Author

waiting-for-dev commented Jan 26, 2019

What do you think of something like last commit, @solnic ? If it's ok, then we have to do something similar to order and any other clause where we need it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment