#7 ✓resolved
Steffen Bartsch

Joins in ObligationScope

Reported by Steffen Bartsch | January 30th, 2009 @ 04:57 PM

A test case that currently doesn't work with ObigationScope. See patch.

test_named_scope_multiple_deep_ored_belongs_to has two conditions ORed. The first works fine alone while the second one, if enabled, causes a fail. This is because of our join handling that causes Rails to use INNER JOINs. Thus, we have an implicit AND through the joins.

Apart from that problem, this test case shows a difference in our table aliasing logic. For these case, Rails should let us define custom aliases for each join, really. Could it help to work with condition hashes instead?

Comments and changes to this ticket

  • Brian Langenfeld

    Brian Langenfeld January 30th, 2009 @ 11:29 PM

    Sure, let me take a look. Want to add me to the project and assign it to me?

  • Brian Langenfeld

    Brian Langenfeld January 31st, 2009 @ 08:21 AM

    It looks like the table aliasing code had a bug that was causing the non-numeric part of deep aliases to be truncated a bit (e.g. test_attrs_test_mode_2, instead of the correct test_attrs_test_models_2). Fixing the bug didn't fix the ticketed problem, but I thought I'd make note of it here.

    As for the ticketed problem: putting the joins into @proxy_options[:include] instead of @proxy_options[:joins] causes all of the tests to pass. It works because I think ANSI standard conformant SQL engines evaluate where conditions after performing all of the ANSI-style joins. (I'm not an expert on the standard, so I'm just speculating here... but this does bear out on SQLite3, and I believe it works the same on PostgreSQL, SQL Server, and Oracle).

    Demonstrating the concept:

    -- Returns rows. select A.* from users A left outer join users B on A.id = B.id and B.id = 2 where A.id = 1;

    -- Returns no rows, because the WHERE is evaluated after the LEFT OUTER JOIN. select A.* from users A left outer join users B on A.id = B.id where A.id = 1 and B.id = 2;

    The only annoying thing about using :include instead of join is that the resulting eager loads generate a ton of extra select columns, even if you try to override this behavior with a :select scope option (ActiveRecord just ignores it). Probably not a huge deal -- and I think there's a plugin or gem out there for changing this -- but I don't know that we want to introduce dependencies on other plugins/gems.

    Patch attached.

  • Steffen Bartsch

    Steffen Bartsch January 31st, 2009 @ 06:39 PM

    Brian, I considered adding you as a member, but I seem to need an email address to invite you. If you send me an email, I will invite you.

  • Steffen Bartsch

    Steffen Bartsch January 31st, 2009 @ 06:55 PM

    Thanks for the fix of the aliasing issue.

    LEFT JOINs help as they don't require the joins to be successful. Using :include feels like a hack, though. I'm not sure about the impact, either, but with lots of joins we will definitely have an impact on memory usage.

    Some approaches:

    • Maybe we could change Rails' behavior to use LEFT JOINs in those specific cases.
    • Always use :include for joins.
    • We fall back to using strings for joins in case of OR'ed conditions.
    • We fall back to using :include for joins in case of OR'ed conditions.

    In the two latter cases, we might introduce too much magic, breaking apps in case of rule changes.

  • Brian Langenfeld

    Brian Langenfeld February 4th, 2009 @ 05:48 AM

    Options #1 and #4 seem like the same thing... let me know if I'm missing something.

    The only option I'm really not crazy about is #3 (writing out SQL strings). I think doing it right would involve mimicking or replicating a lot of the code in ActiveRecord. A lot more than just the table aliasing rules.

    In a perfect world, I guess we'd be able to execute each condition check individually (using inner joins as before) and union together the results.

    select foo.* from foo ... where some_condition is true union select foo.* from foo ... where some_other_condition is true

    Barring that, though, I think we should resort to option #4 -- default to :join, but use:include in cases where we have multiple possible conditions that could be met. I don't think this introduces any more "magic" than we're already providing.


  • Steffen Bartsch

    Steffen Bartsch February 10th, 2009 @ 11:14 AM

    As to the difference in #1 and #4: the effect is the same, but changing Rails behavior is much more invasive, I think.

    Yes, lets go for #4 for now.

    Maybe we can suggest some improvement to ActiveRecord to allow better control of the joining behavior.

  • Brian Langenfeld

    Brian Langenfeld February 12th, 2009 @ 05:21 AM

    • State changed from “new” to “open”
    • Assigned user set to “Brian Langenfeld”

    Patch attached. It actually covers several things I'm grouping together as "release 0.3." All tests pass, of course. :-) If you dig, please patch it into the trunk and update the ticket status.

    From the CHANGELOG:

    ** RELEASE 0.3 (February 11, 2009) **
    Changed Authorization::Engine#permit! [Brian Langenfeld]
    - We now convert incoming privileges to symbols (e.g. 'read' is made equivalent to :read).  This ensures the privileges will match those defined in the authorization rules file.
    - The method now properly infers context when checking against an association (e.g. user.posts).  We do this by leveraging ActiveRecord builder method 'new' to instantiate a proper object we can work with.
    - When testing rules for positive results (via Authorization::Attribute#validate?), we now rescue NilAttributeValueError exceptions, simply causing the rule to return a negative result (instead of barfing).
    Changed Authorization::Attribute#validate? [Brian Langenfeld]
    - Encountering a nil value when evaluating an attribute now raises a NilAttributeValueError, instead of an AuthorizationError.  We leave it to the caller to decide what to do about it.
    Changed Authorization::ObligationScope#map_table_alias_for [Brian Langenfeld]
    - Fixed to prevent bad aliases from being produced.
    Changed Authorization::ObligationScope#rebuild_join_options! [Brian Langenfeld]
    - If we're dealing with multiple obligations we have to check (i.e. ones that result in OR'd conditions), we now use :include instead of :joins for our generated scope.  This does seem like a kludge, but until ActiveRecord scopes support unions (for checking obligations individually and consolidating the results), we don't have much choice.  Something to revisit later, for sure.
  • Steffen Bartsch

    Steffen Bartsch February 12th, 2009 @ 09:31 AM

    Looks good. I think, we should solve #8 before the 0.3 release, though. Lets postpone the release for a few days. I will take these patches into the master branch soon, of cause.

    Would you mind creating a fork at github? Then, we could work with pull requests, making merging etc less complex. In addition, we would have nice self-contained patches for each issue.

  • Brian Langenfeld

    Brian Langenfeld February 12th, 2009 @ 10:22 AM

    If you plan to patch this stuff into 0.3, please either pull from my fork or use this patch instead. I had a statement in the wrong place in authorization.rb.

  • Steffen Bartsch

    Steffen Bartsch February 12th, 2009 @ 08:10 PM

    • State changed from “open” to “resolved”

    It's in master now. Apart from resource_controller. Thanks for the work, Brian.

    Maybe we should add tests for permit! now accepting Builder objects.

    Also, for future changes, it would help if the commits are atomic for each issue.

Please Sign in or create a free account to add a new ticket.

With your very own profile, you can contribute to projects, track your activity, watch tickets, receive and update tickets through your email and much more.

New-ticket Create new ticket

Create your profile

Help contribute to this project by taking a few moments to create your personal profile. Create your profile ยป

By now, decl_auth is using the GitHub issue tracker as well. Please use the one over there: http://github.com/stffn/declarative_authorization/issues

People watching this ticket

Referenced by