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

Allow complex queries with table joins #62

Merged
merged 5 commits into from
Sep 25, 2023

Conversation

xjunior
Copy link
Contributor

@xjunior xjunior commented Sep 12, 2023

Allow using external columns for complex joins. In the bellow example we're able to query the user membership within a group:

  class User < ApplicationRecord
    has_and_belongs_to_many :groups

    def self.scim_queryable_attributes
      return {
        'groups'       => { column: Group.arel_table[:id] },
        'groups.value' => { column: Group.arel_table[:id] },
      }
    end
  end

Then:

  class UsersController < Scimitar::ActiveRecordBackedResourcesController
    def storage_class
      ::User
    end

    def storage_scope
      ::User.left_joins(:groups)
    end
  end

@pond
Copy link
Member

pond commented Sep 16, 2023

Sorry to get to this so late.

The code looks good and I'm very enthusiastic about your test coverage. The only missing thing is documentation - sorry, can be tedious, but it's super-important. New features need clear and comprehensive documentation in README.md, otherwise nobody knows the feature exists and future maintainers risk stumbling across code with no apparent purpose.

It looks like a subsection addition under ### Controllers at (v2.4.3) line 224. The important part is ideally not to just give the example in the PR description, but to explain what's really happening. We know, because we know how the under-hood implementation works. Regular gem users won't.

  • You have a User join in storage_scope
  • You have a scim_queryable_attributes with Group.arel_table[:id]
  • Please make sure the reader understands in particular why there's the magic arel_table[:id] there.

The reader wants to understand what this feature is for and how it would work for them with their own data model, so that's why it's not good to force the reader to rely on assumptions and guesswork (e.g. be very clear that you have a run-of-the-mill ApplicationRecord-based User model with a belongs_to - or is it HABTM? - relationship to another run-of-the-mill ApplicationRecord-based Group model).

Hope that's OK.

* Update outdated scim_queryable_attributes format
* Add documentation for relation queries
README.md Outdated Show resolved Hide resolved
Copy link
Member

@pond pond left a comment

Choose a reason for hiding this comment

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

Again is all good except docs - AREL use still not explained - see review comment for a suggestion about putting that into the RDoc comments of the mixin.

(Edited to note - I'm definitely really keen to get this merged, I just wanna make sure people know how to use it).

README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
@xjunior
Copy link
Contributor Author

xjunior commented Sep 22, 2023

@pond let me know how you feel about the changes. I expanded the options documentation a bit, allowing us to introduce more queryable attribute options in the future (I can see a case_sensitive option, for instance).

Copy link
Member

@pond pond left a comment

Choose a reason for hiding this comment

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

The comment additions are enough. The options docs aren't really RDoc, but I'll clean that up before releasing the bumped gem version. This is good to go!

@pond pond merged commit 1870ae2 into RIPAGlobal:main Sep 25, 2023
4 checks passed
@pond pond mentioned this pull request Sep 25, 2023
@pond
Copy link
Member

pond commented Sep 25, 2023

Formatting & RDoc markup tweaks can be seen in the diff at https://github.com/RIPAGlobal/scimitar/pull/67/files - hope you're OK with those changes; let me know if not (e.g. comment on that PR, even if it's merged).

Edited to add - screenshot of RDoc rendering:

scimitar-rdoc

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.

None yet

3 participants