Skip to content
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

Fix ActiveRecord::StatementInvalid error when using UUID as primary key #894

Closed
wants to merge 1 commit into from

Conversation

calvertyang
Copy link
Contributor

@calvertyang calvertyang commented May 4, 2018

If using UUID as primary key, the relationships feature raises ActiveRecord::StatementInvalid error when you try something like:

User.first.find_related_skills
ActiveRecord::StatementInvalid: PG::SyntaxError: ERROR:  syntax error at or near "a56f2e"
LINE 1: ...w FROM users, tags, taggings WHERE (users.id != 96a56f2e-393...
                                                             ^

@calvertyang calvertyang closed this May 4, 2018
@calvertyang calvertyang reopened this May 4, 2018
@calvertyang calvertyang closed this May 4, 2018
@calvertyang calvertyang reopened this May 4, 2018
@@ -49,7 +49,7 @@ def related_tags_for(context, klass, options = {})
private

def exclude_self(klass, id)
"#{klass.table_name}.#{klass.primary_key} != #{id} AND" if [self.class.base_class, self.class].include? klass
"#{klass.table_name}.#{klass.primary_key} != '#{id}' AND" if [self.class.base_class, self.class].include? klass
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fixed uuid PK but broke integer/bigint

Copy link
Contributor Author

@calvertyang calvertyang Jun 1, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@seuros Thank you for pointing out my mistake.

How do you think if I use klass.columns_hash['id'].type to check the PK type first, and modify the method like this:

def exclude_self(klass, id)
  new_id = klass.columns_hash[klass.primary_key].type == :uuid ? "'#{id}'" : id if klass.primary_key

  "#{klass.table_name}.#{klass.primary_key} != #{new_id} AND" if [self.class.base_class, self.class].include? klass
end

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should use Arel in this part of code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@seuros Thanks for your suggestion.
I'm not familiar with Arel, would you please confirm the following code is correct.

def exclude_self(klass, id)
  "#{klass.arel_table[klass.primary_key].not_eq(id).to_sql} AND" if [self.class.base_class, self.class].include? klass
end

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, i think it should work.
Can you rebase your PR ?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also please include a test case.

@seuros seuros self-assigned this Jun 1, 2018
@calvertyang calvertyang force-pushed the bugfix-uuid-related branch 2 times, most recently from 442b429 to 1c6ab08 Compare June 2, 2018 08:24
@calvertyang calvertyang force-pushed the bugfix-uuid-related branch from 1c6ab08 to a605829 Compare June 2, 2018 10:00
@calvertyang
Copy link
Contributor Author

@seuros Due to the definition of taggable_id in spec/internal/db/schema.rb is integer, the test case I wrote always failed.

And if I change the definition of taggable_id to uuid will make other test case broken, do you have any idea that how to modify my test case to make the CI build passed.

@seuros
Copy link
Collaborator

seuros commented Jun 2, 2018

Thank you
We could generate uuid in postgresql and leave integer for mysql and sqlite3

@calvertyang
Copy link
Contributor Author

Oh, I see. But I have no idea about how to modify the spec/internal/db/schema.rb that only generate uuid in postgresql and leave integer for mysql and sqlite3.

@calvertyang calvertyang force-pushed the bugfix-uuid-related branch 2 times, most recently from d7b4519 to a605829 Compare June 4, 2018 08:19
@seuros
Copy link
Collaborator

seuros commented Jun 16, 2018

cherry-picked on #898 .

@seuros seuros closed this Jun 16, 2018
@calvertyang calvertyang deleted the bugfix-uuid-related branch June 19, 2018 01:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants