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 table alias generation mismatch in ConditionsExtractor #786

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

daveallie
Copy link

Previously in the provided test case, the aliases would have been generated as:

  • mentions_users
  • mentions_users_2
  • mentions_users_2_3

This was causing issues where the join would be aliases as mentions_users_3, however the where statement would be querying a column off mentions_users_2_3 when using accessible_by

After this change, the aliases are generated as:

  • mentions_users
  • mentions_users_2
  • mentions_users_3

This allows table names to correctly match up and the query to succeed.


Happy to provide a deeper example if needed, however I would need to anonymise the table names

Previously in the provided test case, the aliases would have been
generated as:
- mentions_users
- mentions_users_2
- mentions_users_2_3

This was causing issues with mismatched table aliases in the
joins statements when using `accessible_by`

After this change, the aliases are generated as:
- mentions_users
- mentions_users_2
- mentions_users_3
@daveallie
Copy link
Author

daveallie commented May 19, 2022

This may address #696

@coorasse coorasse self-assigned this Jun 23, 2022
@daveallie
Copy link
Author

@coorasse is there anything you'd like to discuss or need me to action here?

@daveallie
Copy link
Author

daveallie commented Nov 13, 2022

@coorasse is there someone better to help get this merged, as far as I can tell, there shouldn't be anything blocking this

@daveallie
Copy link
Author

@coorasse it has been over a year since I opened this PR, based on the activity on the repo, you're still engaging with it. Would you mind reviewing this? I'd love to come back onto mainline and off my fork, but can't until this is resolved.

@dramalho
Copy link

I just got bitten by this myself , super cool that it's fixed - simple enough PR - but unsure if it raises any subtle questions elsewhere that's stopping the merge?

Are you guys monkey patching in the meantime?

@daveallie
Copy link
Author

I've just required the gem directly from my fork:

# Awaiting merge of https://github.com/CanCanCommunity/cancancan/pull/786 and subsequent release
gem 'cancancan', git: 'https://github.com/daveallie/cancancan.git',
                 ref: 'c7dbdb9bb7dae4bc290397c34d09f5b871679d5e'

@dramalho
Copy link

yeah, or that :) -- I don't want to annoy Alessandro but hoping it gets official :) -- thank you for the patch Dave

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.

3 participants