Support strict with_role queries. Issue #362 #543
Conversation
Why: * There is an outstanding issues #362 * strict mode should be enforced on role queries and resource queries How: * Pass a strict flag to the scope function * call where or where_strict depending on strict flag in active_record adapter * Add tests for strict mode resource queries Note: * This PR does not address strict mode for mongoid since I am unfamiliar with that ORM
Why: * The tests will fail until mongo works with strict resource querying This change addresses the need by: * Copy the where_strict implementation from the active record adapter
|
OK, I've updated the PR to cover mongo. The tests are passing although it would be good for someone with deeper mongo experience than I to double check the changes there make sense. |
|
Amazing work, hoping to see this get merged and released! |
| return relation.where(conditions) if args[:resource].blank? | ||
|
|
||
| if args[:resource].is_a?(Class) | ||
| processed_args[:resource_type] = args[:resource].to_s |
There was a problem hiding this comment.
And apparently I didn't actually submit my comment... Would refactor this code to avoid mutating processed_args w/reliance on it being referenced within the conditions hash. It's a bit tough because of the wrapping logic, not sure I have a magic proposal at the moment
There was a problem hiding this comment.
I tried a few approaches and the mutating was probably the simplest but I understand why it might be objectionable. I'll take another look in the coming days to see if I can reason something better.
There was a problem hiding this comment.
Yeah - my only suggestion without moving too much of this code around could be doing the wrapping at the return sites? I think it would be more legible at the cost of being a little more repetitive.
There was a problem hiding this comment.
OK I've updated things to avoid the nested hash mutating. There's still a tiny mutation in there but I think it's a bit cleaner. What do you think?
|
Found a bug with the changes from this PR. Fix is here: #575 Would love to have a quick review and merge/release. Thanks @thomas-mcdonald |
Why:
How:
Note: