Skip to content

Commit

Permalink
Remove pre-fetching for grant associations
Browse files Browse the repository at this point in the history
When finding grants, we were loading associations for some general
convenience. However, we do not inspect the grants in detail for our
current use cases -- we supply very specific join/where conditions and
check for their existence as the implementation of the authorization
rules.

This preloading would be useful for building an administrative
interface, for example, but is unncessary now. The reason to remove it
is to avoid complexity and a performance problem introduced by fetching
the institution memberships: we were loading all of them for any matched
institution and doing an in-memory join on the object graph. In
production, that can be a giant list, consuming excessive time and
memory.

This problem can be solved by "adjusting the combine", applying
appropriate pieces of the filter conditions the association nodes, but
it is not worth doing now. We would be doing extra work and extra
queries to throw the data away.
  • Loading branch information
botimer committed Jun 28, 2024
1 parent ed794b8 commit 4d3f00d
Show file tree
Hide file tree
Showing 2 changed files with 2 additions and 13 deletions.
6 changes: 2 additions & 4 deletions lauth/app/repositories/grant_repo.rb
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,7 @@ def for_collection_class(username:, client_ip:, collection_class:)
.join(collections.name.dataset, uniqueIdentifier: grants[:coll], dlpsDeleted: "f")
.where(collections[:dlpsClass] => collection_class)

rel = grants.class.new(ds)
rel.to_a
grants.class.new(ds).to_a
end

def for(username:, collection:, client_ip: nil)
Expand All @@ -25,8 +24,7 @@ def for(username:, collection:, client_ip: nil)
ds = base_grants_for(username: username, network: smallest_network)
.where(grants[:coll] => collection.uniqueIdentifier)

rel = grants.class.new(ds)
rel.combine(:user, institutions: {institution_memberships: :users}).to_a
grants.class.new(ds).to_a
end

private
Expand Down
9 changes: 0 additions & 9 deletions lauth/spec/repositories/grant_repo_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -59,15 +59,6 @@

expect(grants).to eq []
end

# TODO: extract this
describe "grant association loading" do
subject(:found_grant) { repo.for(username: "lauth-allowed", collection: collection).first }

it "loads user" do
expect(found_grant.user.userid).to eq grant.user.userid
end
end
end

context "with a member of an authorized institution" do
Expand Down

0 comments on commit 4d3f00d

Please sign in to comment.