Skip to content

Commit

Permalink
Allow client to request only current role appointments
Browse files Browse the repository at this point in the history
The prime minister page does not need the former role appointments to
render, so there is no need to retrieve them.

By adding an argument to the `role_appointments` link field, we can
reduce the number of documents whose links need to be further expanded.

An index for editions with a current value in the details was added in
e1b1955.
  • Loading branch information
brucebolt committed Feb 5, 2025
1 parent 61eb70f commit 4b4628c
Show file tree
Hide file tree
Showing 4 changed files with 115 additions and 44 deletions.
28 changes: 22 additions & 6 deletions app/graphql/sources/reverse_linked_to_editions_source.rb
Original file line number Diff line number Diff line change
@@ -1,18 +1,34 @@
module Sources
class ReverseLinkedToEditionsSource < GraphQL::Dataloader::Source
# rubocop:disable Lint/MissingSuper
def initialize(parent_object:)
def initialize(parent_object:, only_current: false)
@object = parent_object
@content_store = parent_object.content_store.to_sym
@only_current = only_current
end
# rubocop:enable Lint/MissingSuper

def fetch(link_types)
all_links = @object
.document
.reverse_links
.includes(source_documents: @content_store)
.where(link_type: link_types)
all_links = if @only_current
query = <<~SQL
SELECT links.*
FROM links
LEFT JOIN link_sets ON links.link_set_id = link_sets.id
LEFT JOIN documents AS link_set_link_documents ON link_sets.content_id = link_set_link_documents.content_id
LEFT JOIN editions AS link_set_link_editions ON link_set_link_documents.id = link_set_link_editions.document_id
LEFT JOIN editions AS edition_link_editions ON links.edition_id = edition_link_editions.id
WHERE links.target_content_id = '#{@object.content_id}'
AND links.link_type IN (#{link_types.map { |type| "'#{type}'" }.join(',')})
AND (link_set_link_editions.content_store = '#{@content_store}' OR edition_link_editions.content_store = '#{@content_store}')
AND (link_set_link_editions.details ->> 'current' = 'true' OR edition_link_editions.details ->> 'current' = 'true')
SQL

Link.from("(#{query}) AS links")

Check warning

Code scanning / Brakeman

Possible SQL injection. Warning

Possible SQL injection.
else
Link
.where(target_content_id: @object.content_id, link_type: link_types)
.includes(source_documents: @content_store)
end

link_types_map = link_types.index_with { [] }

Expand Down
2 changes: 1 addition & 1 deletion app/graphql/types/base_object.rb
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ def self.reverse_links_field(field_name_and_link_type, belongs_to, graphql_field
field(field_name_and_link_type.to_sym, graphql_field_type)

define_method(field_name_and_link_type.to_sym) do
dataloader.with(Sources::ReverseLinkedToEditionsSource, parent_object: object)
dataloader.with(Sources::ReverseLinkedToEditionsSource, parent_object: object, only_current: false)
.load(belongs_to.to_s)
end
end
Expand Down
10 changes: 6 additions & 4 deletions app/graphql/types/edition_type.rb
Original file line number Diff line number Diff line change
Expand Up @@ -99,14 +99,16 @@ class EditionLinks < Types::BaseObject
reverse_links_field :related_to_step_navs, :pages_related_to_step_nav, [EditionType]
reverse_links_field :secondary_to_step_navs, :secondary_to_step_navs, [EditionType]

field :role_appointments, [EditionType]
field :role_appointments, [EditionType] do
argument :only_current, Boolean, required: false, default_value: false
end

def role_appointments
def role_appointments(only_current:)
if %w[role ministerial_role].include?(object.document_type)
dataloader.with(Sources::ReverseLinkedToEditionsSource, parent_object: object)
dataloader.with(Sources::ReverseLinkedToEditionsSource, parent_object: object, only_current:)
.load("role")
else
dataloader.with(Sources::ReverseLinkedToEditionsSource, parent_object: object)
dataloader.with(Sources::ReverseLinkedToEditionsSource, parent_object: object, only_current:)
.load("person")
end
end
Expand Down
119 changes: 86 additions & 33 deletions spec/graphql/sources/reverse_linked_to_editions_source_spec.rb
Original file line number Diff line number Diff line change
@@ -1,45 +1,98 @@
RSpec.describe Sources::ReverseLinkedToEditionsSource do
it "returns the specified reverse link set links" do
target_edition = create(:edition)
source_edition_1 = create(:edition)
source_edition_2 = create(:edition)
source_edition_3 = create(:edition)
link_set_1 = create(:link_set, content_id: source_edition_1.content_id)
link_set_2 = create(:link_set, content_id: source_edition_2.content_id)
link_set_3 = create(:link_set, content_id: source_edition_3.content_id)
create(:link, link_set: link_set_1, target_content_id: target_edition.content_id, link_type: "test_link")
create(:link, link_set: link_set_2, target_content_id: target_edition.content_id, link_type: "another_link_type")
create(:link, link_set: link_set_3, target_content_id: target_edition.content_id, link_type: "test_link")

GraphQL::Dataloader.with_dataloading do |dataloader|
request = dataloader.with(described_class, parent_object: target_edition).request("test_link")

expect(request.load).to eq([source_edition_1, source_edition_3])
context "when only_current is not set" do
it "returns the specified reverse link set links" do
target_edition = create(:edition)
source_edition_1 = create(:edition)
source_edition_2 = create(:edition)
source_edition_3 = create(:edition)
link_set_1 = create(:link_set, content_id: source_edition_1.content_id)
link_set_2 = create(:link_set, content_id: source_edition_2.content_id)
link_set_3 = create(:link_set, content_id: source_edition_3.content_id)
create(:link, link_set: link_set_1, target_content_id: target_edition.content_id, link_type: "test_link")
create(:link, link_set: link_set_2, target_content_id: target_edition.content_id, link_type: "another_link_type")
create(:link, link_set: link_set_3, target_content_id: target_edition.content_id, link_type: "test_link")

GraphQL::Dataloader.with_dataloading do |dataloader|
request = dataloader.with(described_class, parent_object: target_edition).request("test_link")

expect(request.load).to eq([source_edition_1, source_edition_3])
end
end

it "returns the specified reverse edition links" do
target_edition = create(:edition)

source_edition_1 = create(:edition,
links_hash: {
"test_link" => [target_edition.content_id],
})

source_edition_2 = create(:edition,
links_hash: {
"test_link" => [target_edition.content_id],
})

create(:edition,
links_hash: {
"another_link_type" => [target_edition.content_id],
})

GraphQL::Dataloader.with_dataloading do |dataloader|
request = dataloader.with(described_class, parent_object: target_edition).request("test_link")

expect(request.load).to eq([source_edition_1, source_edition_2])
end
end
end

it "returns the specified reverse edition links" do
target_edition = create(:edition)
context "when only_current is set" do
it "returns the specified reverse link set links for current role appointments only" do
target_edition = create(:edition)
source_edition_1 = create(:edition, details: { current: true })
source_edition_2 = create(:edition, details: { current: true })
source_edition_3 = create(:edition, details: { current: false })
link_set_1 = create(:link_set, content_id: source_edition_1.content_id)
link_set_2 = create(:link_set, content_id: source_edition_2.content_id)
link_set_3 = create(:link_set, content_id: source_edition_3.content_id)
create(:link, link_set: link_set_1, target_content_id: target_edition.content_id, link_type: "test_link")
create(:link, link_set: link_set_2, target_content_id: target_edition.content_id, link_type: "another_link_type")
create(:link, link_set: link_set_3, target_content_id: target_edition.content_id, link_type: "test_link")

source_edition_1 = create(:edition,
links_hash: {
"test_link" => [target_edition.content_id],
})
GraphQL::Dataloader.with_dataloading do |dataloader|
request = dataloader.with(described_class, parent_object: target_edition, only_current: true).request("test_link")

expect(request.load).to eq([source_edition_1])
end
end

source_edition_2 = create(:edition,
links_hash: {
"test_link" => [target_edition.content_id],
})
it "returns the specified reverse edition links for current role appointments only" do
target_edition = create(:edition)

create(:edition,
links_hash: {
"another_link_type" => [target_edition.content_id],
})
source_edition_1 = create(:edition,
details: { current: true },
links_hash: {
"test_link" => [target_edition.content_id],
})

GraphQL::Dataloader.with_dataloading do |dataloader|
request = dataloader.with(described_class, parent_object: target_edition).request("test_link")
create(:edition,
details: { current: false },
links_hash: {
"test_link" => [target_edition.content_id],
})

expect(request.load).to eq([source_edition_1, source_edition_2])
create(:edition,
details: { current: true },
links_hash: {
"another_link_type" => [target_edition.content_id],
})

GraphQL::Dataloader.with_dataloading do |dataloader|
request = dataloader.with(described_class, parent_object: target_edition, only_current: true).request("test_link")

expect(request.load).to eq([source_edition_1])
end
end

## TODO: add a test for retrieving from the right content store
end
end

0 comments on commit 4b4628c

Please sign in to comment.