diff --git a/Gemfile.lock b/Gemfile.lock index b527a36..b20fa84 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -1,7 +1,7 @@ PATH remote: . specs: - entitlements-github-plugin (1.2.3) + entitlements-github-plugin (1.2.4) contracts (~> 0.17.0) faraday (~> 2.0) faraday-retry (~> 2.0) diff --git a/lib/entitlements/backend/github_org/service.rb b/lib/entitlements/backend/github_org/service.rb index 17062e0..ddd61bd 100644 --- a/lib/entitlements/backend/github_org/service.rb +++ b/lib/entitlements/backend/github_org/service.rb @@ -66,6 +66,7 @@ def add_user_to_organization(user, role) return true elsif new_membership[:state] == "active" org_members[user] = role + add_org_member_to_normalized_lookup(user) return true end end @@ -92,6 +93,7 @@ def remove_user_from_organization(user) # operations in this organization will ignore this user. if result org_members.delete(user) + remove_org_member_from_normalized_lookup(user) pending_members.delete(user) end diff --git a/lib/entitlements/backend/github_team/controller.rb b/lib/entitlements/backend/github_team/controller.rb index c8402e4..97751cf 100644 --- a/lib/entitlements/backend/github_team/controller.rb +++ b/lib/entitlements/backend/github_team/controller.rb @@ -26,10 +26,8 @@ def initialize(group_name, config = nil) def prefetch teams = Entitlements::Data::Groups::Calculated.read_all(group_name, config) - teams.each do |team_slug| - entitlement_group = Entitlements::Data::Groups::Calculated.read(team_slug) - provider.read(entitlement_group) - end + entitlement_groups = teams.map { |team_slug| Entitlements::Data::Groups::Calculated.read(team_slug) } + provider.prefetch(entitlement_groups) end # Calculation routines. diff --git a/lib/entitlements/backend/github_team/provider.rb b/lib/entitlements/backend/github_team/provider.rb index f21841c..e270c1c 100644 --- a/lib/entitlements/backend/github_team/provider.rb +++ b/lib/entitlements/backend/github_team/provider.rb @@ -30,6 +30,20 @@ def initialize(config:) @github_team_cache = {} end + # Populate the provider cache for a collection of desired teams. + # + # entitlement_groups - Array of Entitlements::Models::Group objects. + # + # Returns nothing. + Contract C::ArrayOf[Entitlements::Models::Group] => nil + def prefetch(entitlement_groups) + github.read_teams(entitlement_groups).each do |team_name, github_team| + log_loaded_team(github_team) if github_team + @github_team_cache[team_name] = github_team + end + nil + end + # Read in a specific GitHub.com Team and enumerate its members. Results are cached # for future runs. # @@ -39,15 +53,12 @@ def initialize(config:) Contract Entitlements::Models::Group => C::Maybe[Entitlements::Models::Group] def read(entitlement_group) slug = Entitlements::Util::Util.any_to_cn(entitlement_group.cn.downcase) - return @github_team_cache[slug] if @github_team_cache[slug] + return @github_team_cache[slug] if @github_team_cache.key?(slug) github_team = github.read_team(entitlement_group) - # We should not cache a team which does not exist - return nil if github_team.nil? - - Entitlements.logger.debug "Loaded #{github_team.team_dn} (id=#{github_team.team_id}) with #{github_team.member_strings.count} member(s)" - @github_team_cache[github_team.team_name] = github_team + log_loaded_team(github_team) if github_team + @github_team_cache[slug] = github_team end # Dry run of committing changes. Returns a list of users added or removed. @@ -144,13 +155,20 @@ def commit(entitlement_group) # Returns a set of strings with usernames meeting the criteria. Contract Entitlements::Models::Group => C::SetOf[String] def auto_generate_ignored_users(entitlement_group) - org_members = github.org_members.keys.map(&:downcase) - group_members = entitlement_group.member_strings.map(&:downcase) - Set.new(group_members - org_members) + entitlement_group.member_strings.each_with_object(Set.new) do |username, ignored_users| + normalized_username = username.downcase + ignored_users.add(normalized_username) unless github.org_member?(normalized_username) + end end private + Contract Entitlements::Backend::GitHubTeam::Models::Team => nil + def log_loaded_team(github_team) + Entitlements.logger.debug "Loaded #{github_team.team_dn} (id=#{github_team.team_id}) with #{github_team.member_strings.count} member(s)" + nil + end + # Construct an Entitlements::Models::Group for a new group and team # # group - An Entitlements::Models::Group object representing the defined group diff --git a/lib/entitlements/backend/github_team/service.rb b/lib/entitlements/backend/github_team/service.rb index dba731d..4329ecd 100644 --- a/lib/entitlements/backend/github_team/service.rb +++ b/lib/entitlements/backend/github_team/service.rb @@ -4,6 +4,7 @@ require_relative "../../service/github" require "base64" +require "json" require "set" module Entitlements @@ -16,6 +17,9 @@ class Service < Entitlements::Service::GitHub class TeamNotFound < RuntimeError; end + GRAPHQL_TEAM_BATCH_SIZE = 10 + MAX_GRAPHQL_TEAM_PAGES = 100 + # Constructor. # # addr - Base URL a GitHub Enterprise API (leave undefined to use dotcom) @@ -47,86 +51,62 @@ def initialize(org:, token:, ou:, addr: nil, ignore_not_found: false) # Returns a Entitlements::Backend::GitHubTeam::Models::Team or nil if the team does not exist Contract Entitlements::Models::Group => C::Maybe[Entitlements::Backend::GitHubTeam::Models::Team] def read_team(entitlement_group) - team_identifier = entitlement_group.cn.downcase - @team_cache[team_identifier] ||= begin - dn = "cn=#{team_identifier},#{ou}" - begin - entitlement_metadata = entitlement_group.metadata - rescue Entitlements::Models::Group::NoMetadata - entitlement_metadata = nil - end - - if (cached_members = Entitlements::Data::Groups::Cached.members(dn)) - Entitlements.logger.debug "Loading GitHub team #{identifier}:#{org}/#{team_identifier} from cache" - - cached_metadata = Entitlements::Data::Groups::Cached.metadata(dn) - # If both the cached and entitlement metadata are nil, our team metadata is nil - # If one of the cached or entitlement metadata is nil, we use the other populated metadata hash as-is - # If both cached and entitlement metadata exist, we combine the two hashes with the cached metadata taking precedence - # - # The reason we do this is because an entitlement file should be 1:1 to a GitHub Team. However, - # entitlements files allow for metadata tags and the GitHub.com Team does not have a place to store those. - # Therefore, we must combine any existing entitlement metadata entries into the Team metadata hash - if cached_metadata.nil? - team_metadata = entitlement_metadata - elsif entitlement_metadata.nil? - team_metadata = cached_metadata - else - # Always merge the current state metadata (cached or API call) into the entitlement metadata, so that the current state takes precedent - team_metadata = entitlement_metadata.merge(cached_metadata) - end + read_teams([entitlement_group]).fetch(entitlement_group.cn.downcase) + end - team = Entitlements::Backend::GitHubTeam::Models::Team.new( - team_id: -1, - team_name: team_identifier, - members: cached_members, - ou:, - metadata: team_metadata - ) + # Read multiple teams, using predictive state where valid and bounded GraphQL + # batches for the teams that require authoritative state. + # + # entitlement_groups - Array of desired Entitlements::Models::Group objects. + # + # Returns a Hash keyed by lower-case team slug. + Contract C::ArrayOf[Entitlements::Models::Group] => C::HashOf[String => C::Maybe[Entitlements::Backend::GitHubTeam::Models::Team]] + def read_teams(entitlement_groups) + result = {} + authoritative_groups = [] + + entitlement_groups.each do |entitlement_group| + team_identifier = entitlement_group.cn.downcase + if @team_cache.key?(team_identifier) + result[team_identifier] = @team_cache.fetch(team_identifier)[:value] + next + end - { cache: true, value: team } + predictive_team = team_from_predictive_cache(entitlement_group) + if predictive_team + @team_cache[team_identifier] = { cache: true, value: predictive_team } + result[team_identifier] = predictive_team else Entitlements.logger.debug "Loading GitHub team #{identifier}:#{org}/#{team_identifier}" + authoritative_groups << entitlement_group + end + end - begin - teamdata = graphql_team_data(team_identifier) - # The entitlement metadata may have GitHub.com Team metadata which it wants to set, so we must - # overwrite that metadata with what we get from the API - if teamdata[:parent_team_name].nil? - team_metadata = entitlement_metadata - else - parent_team_metadata = { - "parent_team_name" => teamdata[:parent_team_name] - } - if entitlement_metadata.nil? - team_metadata = parent_team_metadata - else - # Always merge the current state metadata (cached or API call) into the entitlement metadata, so that the current state takes precedent - team_metadata = entitlement_metadata.merge(parent_team_metadata) - end - end - - maintainers = teamdata[:members].select { |u| teamdata[:roles][u] == "maintainer" } - team_metadata ||= {} - team_metadata = team_metadata.merge({ "team_maintainers" => maintainers.any? ? maintainers.join(",") : nil }) - - team = Entitlements::Backend::GitHubTeam::Models::Team.new( - team_id: teamdata[:team_id], - team_name: team_identifier, - members: Set.new(teamdata[:members]), - ou:, - metadata: team_metadata - ) - rescue TeamNotFound + unless authoritative_groups.empty? + team_data = if authoritative_groups.one? + team_identifier = authoritative_groups.first.cn.downcase + begin + { team_identifier => graphql_team_data(team_identifier) } + rescue TeamNotFound + { team_identifier => nil } + end + else + graphql_team_data_batch(authoritative_groups.map { |group| group.cn.downcase }) + end + + authoritative_groups.each do |entitlement_group| + team_identifier = entitlement_group.cn.downcase + data = team_data.fetch(team_identifier) + team = data && team_from_graphql_data(entitlement_group, data) + unless team Entitlements.logger.warn "Team #{team_identifier} does not exist in this GitHub.com organization. If applied, the team will be created." - return nil end - - { cache: false, value: team } + @team_cache[team_identifier] = { cache: false, value: team } + result[team_identifier] = team end end - @team_cache[team_identifier][:value] + result end # Determine whether the most recent entry came from the predictive cache or an actual @@ -361,65 +341,187 @@ def team_by_name(org_name:, team_name:) Contract String => { members: C::ArrayOf[String], team_id: Integer, parent_team_name: C::Or[String, nil], roles: C::HashOf[String => String] } def graphql_team_data(team_slug) - cursor = nil - team_id = nil - result = [] - roles = {} - sanity_counter = 0 - - while sanity_counter < 100 - sanity_counter += 1 - first_str = cursor.nil? ? "first: #{max_graphql_results}" : "first: #{max_graphql_results}, after: \"#{cursor}\"" - query = "{ - organization(login: \"#{org}\") { - team(slug: \"#{team_slug}\") { - databaseId - parentTeam { - slug - } - members(#{first_str}, membership: IMMEDIATE) { - edges { - node { - login - } - role - cursor - } - } - } - } - }".gsub(/\n\s+/, "\n") + result = graphql_team_data_batch([team_slug]).fetch(team_slug) + raise TeamNotFound, "Requested team #{team_slug} does not exist in #{org}!" if result.nil? - response = graphql_http_post(query) - unless response[:code] == 200 - Entitlements.logger.fatal "Abort due to GraphQL failure on #{query.inspect}" - raise "GraphQL query failure" - end + result + end + + def graphql_team_data_batch(team_slugs) + states = team_slugs.to_h do |team_slug| + [team_slug, { members: [], roles: {}, team_id: nil, parent_team_name: nil, cursor: nil, pages: 0 }] + end + pending_team_slugs = team_slugs + + until pending_team_slugs.empty? + next_pending_team_slugs = [] + + pending_team_slugs.each_slice(graphql_team_batch_size) do |batch| + alias_to_team = batch.each_with_index.to_h { |team_slug, index| ["team#{index}", team_slug] } + query = graphql_team_batch_query(alias_to_team, states) + response = graphql_http_post(query) + unless response[:code] == 200 + Entitlements.logger.fatal "Abort due to GraphQL failure on #{query.inspect}" + raise "GraphQL query failure" + end + + response_data = response[:data].fetch("data") + organization = response_data.fetch("organization") + raise "GraphQL response missing organization #{org}" if organization.nil? + + log_graphql_rate_limit(response_data["rateLimit"]) - team = response[:data].fetch("data").fetch("organization").fetch("team") - raise TeamNotFound, "Requested team #{team_slug} does not exist in #{org}!" if team.nil? + alias_to_team.each do |team_alias, team_slug| + state = states.fetch(team_slug) + team = organization.fetch(team_alias) + if team.nil? + raise "GitHub team #{team_slug} disappeared during pagination" if state[:pages].positive? - team_id = team.fetch("databaseId") - parent_team_name = team.dig("parentTeam", "slug") + states[team_slug] = nil + next + end + + state[:pages] += 1 + team_id = team.fetch("databaseId") + if state[:team_id] && state[:team_id] != team_id + raise "GitHub team #{team_slug} changed database ID during pagination" + end + state[:team_id] = team_id + state[:parent_team_name] = team.dig("parentTeam", "slug") + + edges = team.fetch("members").fetch("edges") + edges.each do |edge| + username = edge.fetch("node").fetch("login").downcase + state[:members] << username + state[:roles][username] = edge.fetch("role").downcase + end - edges = team.fetch("members").fetch("edges") - break unless edges.any? + next unless edges.size == max_graphql_results - buffer = edges.map { |e| e.fetch("node").fetch("login").downcase } - result.concat buffer + cursor = edges.last.fetch("cursor") + raise "GitHub team #{team_slug} returned a full page without a cursor" if cursor.nil? + if state[:pages] >= MAX_GRAPHQL_TEAM_PAGES + raise "GitHub team #{team_slug} exceeded the #{MAX_GRAPHQL_TEAM_PAGES}-page GraphQL limit" + end - edges.each do |e| - role = e.fetch("role").downcase - roles[e.fetch("node").fetch("login").downcase] = role + state[:cursor] = cursor + next_pending_team_slugs << team_slug + end end - cursor = edges.last.fetch("cursor") - next if cursor && buffer.size == max_graphql_results + pending_team_slugs = next_pending_team_slugs + end + + states.transform_values do |state| + next if state.nil? - break + state.slice(:members, :roles, :team_id, :parent_team_name) end + end + + def graphql_team_batch_query(alias_to_team, states) + team_fields = alias_to_team.map do |team_alias, team_slug| + cursor = states.fetch(team_slug)[:cursor] + pagination = "first: #{max_graphql_results}" + pagination += ", after: #{graphql_string_literal(cursor)}" if cursor + "#{team_alias}: team(slug: #{graphql_string_literal(team_slug)}) { + databaseId + parentTeam { + slug + } + members(#{pagination}, membership: IMMEDIATE) { + edges { + node { + login + } + role + cursor + } + } + }" + end.join("\n") + + "query { + rateLimit { + cost + remaining + resetAt + } + organization(login: #{graphql_string_literal(org)}) { + #{team_fields} + } + }".gsub(/\n\s+/, "\n") + end - { members: result, team_id:, parent_team_name:, roles: } + Contract String => String + def graphql_string_literal(value) + JSON.generate(value) + end + + def graphql_team_batch_size + GRAPHQL_TEAM_BATCH_SIZE + end + + def log_graphql_rate_limit(rate_limit) + return if rate_limit.nil? + + Entitlements.logger.debug( + "GitHub GraphQL team batch cost=#{rate_limit['cost']} remaining=#{rate_limit['remaining']} reset_at=#{rate_limit['resetAt']}" + ) + end + + def team_from_predictive_cache(entitlement_group) + team_identifier = entitlement_group.cn.downcase + dn = "cn=#{team_identifier},#{ou}" + cached_members = Entitlements::Data::Groups::Cached.members(dn) + return if cached_members.nil? + + Entitlements.logger.debug "Loading GitHub team #{identifier}:#{org}/#{team_identifier} from cache" + cached_metadata = Entitlements::Data::Groups::Cached.metadata(dn) + entitlement_metadata = metadata_from_entitlement(entitlement_group) + team_metadata = if cached_metadata.nil? + entitlement_metadata + elsif entitlement_metadata.nil? + cached_metadata + else + entitlement_metadata.merge(cached_metadata) + end + + Entitlements::Backend::GitHubTeam::Models::Team.new( + team_id: -1, + team_name: team_identifier, + members: cached_members, + ou:, + metadata: team_metadata + ) + end + + def team_from_graphql_data(entitlement_group, teamdata) + team_identifier = entitlement_group.cn.downcase + entitlement_metadata = metadata_from_entitlement(entitlement_group) + parent_team_name = teamdata[:parent_team_name] + team_metadata = if parent_team_name.nil? + entitlement_metadata + else + (entitlement_metadata || {}).merge("parent_team_name" => parent_team_name) + end + + maintainers = teamdata[:members].select { |username| teamdata[:roles][username] == "maintainer" } + team_metadata = (team_metadata || {}).merge("team_maintainers" => maintainers.any? ? maintainers.join(",") : nil) + + Entitlements::Backend::GitHubTeam::Models::Team.new( + team_id: teamdata[:team_id], + team_name: team_identifier, + members: Set.new(teamdata[:members]), + ou:, + metadata: team_metadata + ) + end + + def metadata_from_entitlement(entitlement_group) + entitlement_group.metadata + rescue Entitlements::Models::Group::NoMetadata + nil end # Ensure that the given team ID actually matches up to the team slug on GitHub. This is in place diff --git a/lib/entitlements/service/github.rb b/lib/entitlements/service/github.rb index 0a977a9..e280c45 100644 --- a/lib/entitlements/service/github.rb +++ b/lib/entitlements/service/github.rb @@ -4,6 +4,7 @@ require "net/http" require "octokit" +require "set" require "uri" module Entitlements @@ -96,6 +97,20 @@ def org_members Entitlements.cache[:github_org_members][org_signature][:value] end + # Determine whether a username is an active member of the organization. + # + # username - GitHub username to look up. + # + # Returns true if the username is an active organization member. + Contract String => C::Bool + def org_member?(username) + org_members + entry = Entitlements.cache[:github_org_members].fetch(org_signature) + entry[:normalized_members] ||= Set.new(entry[:value].keys) + normalized_username = /[A-Z]/.match?(username) ? username.downcase : username + entry[:normalized_members].include?(normalized_username) + end + # Returns true if the github instance is an enterprise server instance Contract C::None => C::Bool def enterprise? @@ -160,6 +175,24 @@ def invalidate_org_members_predictive_cache private + # Keep an already-materialized membership lookup synchronized after an addition or + # role change without forcing it to be built. + Contract String => nil + def add_org_member_to_normalized_lookup(username) + entry = Entitlements.cache[:github_org_members][org_signature] + entry[:normalized_members].add(username.downcase) if entry&.key?(:normalized_members) + nil + end + + # Keep an already-materialized membership lookup synchronized after a removal without + # forcing it to be built. + Contract String => nil + def remove_org_member_from_normalized_lookup(username) + entry = Entitlements.cache[:github_org_members][org_signature] + entry[:normalized_members].delete(username.downcase) if entry&.key?(:normalized_members) + nil + end + # The octokit object is initialized the first time it's called. # # Takes no arguments. @@ -369,9 +402,7 @@ def graphql_result_retryable?(result) # Returns { code: , data: } Contract String => { code: Integer, data: C::Or[nil, Hash] } def graphql_http_post_real(query) - uri = URI.parse(File.join(octokit.api_endpoint, "graphql")) - http = Net::HTTP.new(uri.host, uri.port) - http.use_ssl = uri.scheme == "https" + uri = graphql_uri request = Net::HTTP::Post.new(uri) request.add_field("Authorization", "bearer #{token}") @@ -379,7 +410,7 @@ def graphql_http_post_real(query) request.body = JSON.generate("query" => query) begin - response = http.request(request) + response = graphql_http.request(request) if response.code != "200" # The retry wrapper retries on 5xx, so log those at WARN to avoid misleading @@ -411,11 +442,41 @@ def graphql_http_post_real(query) # Catch-all for any unexpected exception (network blip OR local code bug). # We retry below via the synthesized 500, but log at ERROR because this # branch can mask programming errors that operators must see. + reset_graphql_http Entitlements.logger.error "Caught #{e.class} POSTing to #{uri}: #{e.message}" { code: 500, data: nil } end end + # Return the URI used for GraphQL requests. + Contract C::None => URI::Generic + def graphql_uri + @graphql_uri ||= URI.parse(File.join(octokit.api_endpoint, "graphql")) + end + + # Return a started HTTP connection that can be reused across GraphQL requests. + Contract C::None => Net::HTTP + def graphql_http + @graphql_http ||= begin + uri = graphql_uri + http = Net::HTTP.new(uri.host, uri.port) + http.use_ssl = uri.scheme == "https" + http.start + http + end + end + + # Discard a failed GraphQL connection so the retry wrapper creates a new one. + Contract C::None => nil + def reset_graphql_http + http, @graphql_http = @graphql_http, nil + http.finish if http&.started? + nil + rescue IOError, SystemCallError => e + Entitlements.logger.warn "Failed to close GraphQL connection: #{e.message}" + nil + end + # Create a unique signature for this GitHub instance to identify it in a global cache. # # Takes no arguments. diff --git a/lib/version.rb b/lib/version.rb index cd6c58b..6908551 100644 --- a/lib/version.rb +++ b/lib/version.rb @@ -2,6 +2,6 @@ module Entitlements module Version - VERSION = "1.2.3" + VERSION = "1.2.4" end end diff --git a/spec/unit/entitlements/backend/github_org/service_spec.rb b/spec/unit/entitlements/backend/github_org/service_spec.rb index 05c8a99..b8340aa 100644 --- a/spec/unit/entitlements/backend/github_org/service_spec.rb +++ b/spec/unit/entitlements/backend/github_org/service_spec.rb @@ -129,6 +129,39 @@ expect(subject.pending_members).to eq(Set.new) expect(subject.org_members).to eq("bob" => "admin") end + + it "adds active members and role changes to a materialized normalized lookup" do + allow(subject).to receive(:members_and_roles_from_rest).and_return("bob" => "MEMBER") + expect(subject.org_member?("bob")).to eq(true) + + stub_request(:put, "https://github.fake/api/v3/orgs/kittensinc/memberships/alice").to_return( + status: 200, + headers: { + "Content-type" => "application/json" + }, + body: JSON.generate( + "url" => "https://github.fake/api/v3/orgs/kittensinc/memberships/alice", + "state" => "active", + "role" => "admin" + ) + ) + stub_request(:put, "https://github.fake/api/v3/orgs/kittensinc/memberships/bob").to_return( + status: 200, + headers: { + "Content-type" => "application/json" + }, + body: JSON.generate( + "url" => "https://github.fake/api/v3/orgs/kittensinc/memberships/bob", + "state" => "active", + "role" => "admin" + ) + ) + + expect(subject.send(:add_user_to_organization, "alice", "admin")).to eq(true) + expect(subject.org_member?("alice")).to eq(true) + expect(subject.send(:add_user_to_organization, "bob", "admin")).to eq(true) + expect(subject.org_member?("bob")).to eq(true) + end end context "sad path" do @@ -244,5 +277,17 @@ expect(subject.pending_members).to eq(Set.new) expect(subject.org_members).to eq({}) end + + it "removes members from a materialized normalized lookup" do + allow(subject).to receive(:members_and_roles_from_rest).and_return("bob" => "ADMIN") + allow(subject).to receive(:enterprise?).and_return(false) + allow(subject).to receive(:pending_members_from_graphql).and_return(Set.new) + expect(subject.org_member?("bob")).to eq(true) + + stub_request(:delete, "https://github.fake/api/v3/orgs/kittensinc/memberships/bob").to_return(status: 204) + + expect(subject.send(:remove_user_from_organization, "bob")).to eq(true) + expect(subject.org_member?("bob")).to eq(false) + end end end diff --git a/spec/unit/entitlements/backend/github_team/controller_spec.rb b/spec/unit/entitlements/backend/github_team/controller_spec.rb index 44a4869..fad0513 100644 --- a/spec/unit/entitlements/backend/github_team/controller_spec.rb +++ b/spec/unit/entitlements/backend/github_team/controller_spec.rb @@ -27,6 +27,7 @@ "RUSSIANBLue" => "member" } end + let(:org_member_set) { Set.new(org_member_hash.keys.map(&:downcase)) } describe "#calculate" do let(:russian_blue_team) do @@ -121,9 +122,11 @@ expect(Entitlements::Backend::GitHubTeam::Service).to receive(:new).with(hash_including(addr: nil)).and_return(dotcom_obj) allow(dotcom_obj).to receive(:identifier).and_return("github.com") allow(dotcom_obj).to receive(:org).and_return("kittensinc") - allow(dotcom_obj).to receive(:read_team).with(russian_blue_group).and_return(russian_blue_team) - allow(dotcom_obj).to receive(:read_team).with(snowshoe_group).and_return(snowshoe_team) - allow(dotcom_obj).to receive(:org_members).and_return(org_member_hash) + allow(dotcom_obj).to receive(:read_teams) + .with([snowshoe_group, russian_blue_group]) + .and_return("snowshoes" => snowshoe_team, "russian-blues" => russian_blue_team) + expect(dotcom_obj).not_to receive(:org_members) + allow(dotcom_obj).to receive(:org_member?) { |username| org_member_set.include?(username) } allow(dotcom_obj).to receive(:from_predictive_cache?).and_return(false) expect(logger).to receive(:debug).with("Loaded cn=russian-blues,ou=kittensinc,ou=GitHub,dc=github,dc=com (id=1001) with 2 member(s)") @@ -168,8 +171,11 @@ expect(Entitlements::Backend::GitHubTeam::Service).to receive(:new).with(hash_including(addr: nil)).and_return(dotcom_obj) allow(dotcom_obj).to receive(:identifier).and_return("github.com") allow(dotcom_obj).to receive(:org).and_return("kittensinc") - allow(dotcom_obj).to receive(:read_team).with(russian_blue_group).and_return(russian_blue_team) - allow(dotcom_obj).to receive(:org_members).and_return(org_member_hash) + allow(dotcom_obj).to receive(:read_teams) + .with([russian_blue_group]) + .and_return("russian-blues" => russian_blue_team) + expect(dotcom_obj).not_to receive(:org_members) + allow(dotcom_obj).to receive(:org_member?) { |username| org_member_set.include?(username) } allow(dotcom_obj).to receive(:from_predictive_cache?).and_return(false) expect(logger).to receive(:debug).with("Loaded cn=russian-blues,ou=kittensinc,ou=GitHub,dc=github,dc=com (id=1001) with 2 member(s)") @@ -216,9 +222,11 @@ allow(dotcom_obj).to receive(:identifier).and_return("github.com") allow(dotcom_obj).to receive(:org).and_return("kittensinc") allow(dotcom_obj).to receive(:ou).and_return("GitHub") - allow(dotcom_obj).to receive(:read_team).with(russian_blue_group).and_return(nil) - allow(dotcom_obj).to receive(:read_team).with(snowshoe_group).and_return(snowshoe_team) - allow(dotcom_obj).to receive(:org_members).and_return(org_member_hash) + allow(dotcom_obj).to receive(:read_teams) + .with([snowshoe_group, russian_blue_group]) + .and_return("snowshoes" => snowshoe_team, "russian-blues" => nil) + expect(dotcom_obj).not_to receive(:org_members) + allow(dotcom_obj).to receive(:org_member?) { |username| org_member_set.include?(username) } allow(dotcom_obj).to receive(:from_predictive_cache?).and_return(false) expect(logger).to receive(:debug).with("Loaded cn=snowshoes,ou=kittensinc,ou=GitHub,dc=github,dc=com (id=1002) with 2 member(s)") diff --git a/spec/unit/entitlements/backend/github_team/provider_spec.rb b/spec/unit/entitlements/backend/github_team/provider_spec.rb index d3df3dc..2875f03 100644 --- a/spec/unit/entitlements/backend/github_team/provider_spec.rb +++ b/spec/unit/entitlements/backend/github_team/provider_spec.rb @@ -98,6 +98,28 @@ end end + describe "#prefetch" do + let(:missing_group) do + Entitlements::Models::Group.new( + dn: "cn=missing-cats,ou=Github,dc=github,dc=fake", + members: Set.new + ) + end + + it "populates the existing provider cache for present and missing teams" do + allow(subject).to receive(:github).and_return(github) + expect(github).to receive(:read_teams).with([group, missing_group]) + .and_return("cats" => team, "missing-cats" => nil) + expect(logger).to receive(:debug).with("Loaded cn=cats,ou=kittensinc,ou=GitHub,dc=github,dc=fake (id=1001) with 2 member(s)") + + subject.prefetch([group, missing_group]) + + expect(github).not_to receive(:read_team) + expect(subject.read(group)).to eq(team) + expect(subject.read(missing_group)).to be_nil + end + end + describe "#diff" do let(:team_identifier) { "grumpy-cats" } let(:team_dn) { "cn=#{team_identifier},ou=kittensinc,ou=GitHub,dc=github,dc=fake" } @@ -454,6 +476,30 @@ end end + describe "#auto_generate_ignored_users" do + it "returns lowercase non-members and does not enumerate organization membership" do + entitlement_group = instance_double( + Entitlements::Models::Group, + member_strings: Set.new(%w[SnowShoe RUSSIAN_BLUE PendingCat]) + ) + allow(subject).to receive(:github).and_return(github) + expect(github).not_to receive(:org_members) + expect(github).to receive(:org_member?).with("snowshoe").and_return(true) + expect(github).to receive(:org_member?).with("russian_blue").and_return(false) + expect(github).to receive(:org_member?).with("pendingcat").and_return(false) + + expect(subject.auto_generate_ignored_users(entitlement_group)).to eq(Set.new(%w[russian_blue pendingcat])) + end + + it "returns an empty set for an empty team" do + entitlement_group = instance_double(Entitlements::Models::Group, member_strings: Set.new) + allow(subject).to receive(:github).and_return(github) + expect(github).not_to receive(:org_member?) + + expect(subject.auto_generate_ignored_users(entitlement_group)).to eq(Set.new) + end + end + describe "#create_github_team_group" do it "returns a new empty team" do entitlement_group = Entitlements::Models::Group.new( diff --git a/spec/unit/entitlements/backend/github_team/service_spec.rb b/spec/unit/entitlements/backend/github_team/service_spec.rb index bc430af..83e67fd 100644 --- a/spec/unit/entitlements/backend/github_team/service_spec.rb +++ b/spec/unit/entitlements/backend/github_team/service_spec.rb @@ -79,9 +79,7 @@ it "returns nil when the team does not exist" do graphql_response = '{"data":{"organization":{"team":null}}}' stub_request(:post, "https://github.fake/api/v3/graphql") - .with( - body: "{\"query\":\"{\\norganization(login: \\\"kittensinc\\\") {\\nteam(slug: \\\"team-does-not-exist\\\") {\\ndatabaseId\\nparentTeam {\\nslug\\n}\\nmembers(first: 100, membership: IMMEDIATE) {\\nedges {\\nnode {\\nlogin\\n}\\nrole\\ncursor\\n}\\n}\\n}\\n}\\n}\"}" - ).to_return(status: 200, body: graphql_response) + .to_return(status: 200, body: graphql_response.sub('"team"', '"team0"')) expect(logger).to receive(:debug).with("Setting up GitHub API connection to https://github.fake/api/v3/") expect(logger).to receive(:debug).with("Loading GitHub team github.fake:kittensinc/team-does-not-exist") @@ -92,9 +90,7 @@ it "returns a Entitlements::Backend::GitHubTeam::Models::Team object when the team exists" do stub_request(:post, "https://github.fake/api/v3/graphql") - .with( - body: "{\"query\":\"{\\norganization(login: \\\"kittensinc\\\") {\\nteam(slug: \\\"cuddly-kittens\\\") {\\ndatabaseId\\nparentTeam {\\nslug\\n}\\nmembers(first: 100, membership: IMMEDIATE) {\\nedges {\\nnode {\\nlogin\\n}\\nrole\\ncursor\\n}\\n}\\n}\\n}\\n}\"}" - ).to_return(status: 200, body: graphql_response(cuddly_kittens, 0, 100)) + .to_return(status: 200, body: graphql_response(cuddly_kittens, 0, 100)) expect(logger).to receive(:debug).with("Setting up GitHub API connection to https://github.fake/api/v3/") expect(logger).to receive(:debug).with("Loading GitHub team github.fake:kittensinc/cuddly-kittens") @@ -109,9 +105,7 @@ it "returns a Entitlements::Backend::GitHubTeam::Models::Team object with parent team when the team exists" do stub_request(:post, "https://github.fake/api/v3/graphql") - .with( - body: "{\"query\":\"{\\norganization(login: \\\"kittensinc\\\") {\\nteam(slug: \\\"cuddly-kittens\\\") {\\ndatabaseId\\nparentTeam {\\nslug\\n}\\nmembers(first: 100, membership: IMMEDIATE) {\\nedges {\\nnode {\\nlogin\\n}\\nrole\\ncursor\\n}\\n}\\n}\\n}\\n}\"}" - ).to_return(status: 200, body: graphql_response(cuddly_kittens, 0, 100, parent_team: "parent-cats")) + .to_return(status: 200, body: graphql_response(cuddly_kittens, 0, 100, parent_team: "parent-cats")) expect(logger).to receive(:debug).with("Setting up GitHub API connection to https://github.fake/api/v3/") expect(logger).to receive(:debug).with("Loading GitHub team github.fake:kittensinc/cuddly-kittens") @@ -128,9 +122,7 @@ it "returns a Entitlements::Backend::GitHubTeam::Models::Team object with parent team when the team exists but has empty entitlement metadata" do stub_request(:post, "https://github.fake/api/v3/graphql") - .with( - body: "{\"query\":\"{\\norganization(login: \\\"kittensinc\\\") {\\nteam(slug: \\\"cuddly-kittens\\\") {\\ndatabaseId\\nparentTeam {\\nslug\\n}\\nmembers(first: 100, membership: IMMEDIATE) {\\nedges {\\nnode {\\nlogin\\n}\\nrole\\ncursor\\n}\\n}\\n}\\n}\\n}\"}" - ).to_return(status: 200, body: graphql_response(cuddly_kittens_no_metadata, 0, 100, parent_team: "parent-cats")) + .to_return(status: 200, body: graphql_response(cuddly_kittens_no_metadata, 0, 100, parent_team: "parent-cats")) expect(logger).to receive(:debug).with("Setting up GitHub API connection to https://github.fake/api/v3/") expect(logger).to receive(:debug).with("Loading GitHub team github.fake:kittensinc/cuddly-kittens") @@ -225,6 +217,226 @@ end end + describe "#read_teams" do + let(:alpha_group) do + Entitlements::Models::Group.new( + dn: "cn=alpha-cats,ou=kittensinc,ou=GitHub,dc=github,dc=fake", + members: Set.new, + metadata: { "application_owner" => "snowshoe" } + ) + end + + let(:beta_group) do + Entitlements::Models::Group.new( + dn: "cn=beta-cats,ou=kittensinc,ou=GitHub,dc=github,dc=fake", + members: Set.new, + metadata: nil + ) + end + + let(:missing_group) do + Entitlements::Models::Group.new( + dn: "cn=missing-cats,ou=kittensinc,ou=GitHub,dc=github,dc=fake", + members: Set.new, + metadata: {} + ) + end + + it "batches aliases, maps reordered responses, and paginates only unfinished teams" do + allow(subject).to receive(:max_graphql_results).and_return(2) + queries = [] + responses = [ + { + "data" => { + "rateLimit" => { "cost" => 5, "remaining" => 4_995, "resetAt" => "later" }, + "organization" => { + "team2" => nil, + "team1" => { + "databaseId" => 202, + "parentTeam" => nil, + "members" => { "edges" => [] } + }, + "team0" => { + "databaseId" => 101, + "parentTeam" => { "slug" => "parent-cats" }, + "members" => { + "edges" => [ + { "node" => { "login" => "ALPHA" }, "role" => "MAINTAINER", "cursor" => "cursor-1" }, + { "node" => { "login" => "Beta" }, "role" => "MEMBER", "cursor" => "cursor-2" } + ] + } + } + } + } + }, + { + "data" => { + "organization" => { + "team0" => { + "databaseId" => 101, + "parentTeam" => { "slug" => "parent-cats" }, + "members" => { + "edges" => [ + { "node" => { "login" => "GAMMA" }, "role" => "MEMBER", "cursor" => "cursor-3" } + ] + } + } + } + } + } + ] + allow(subject).to receive(:graphql_http_post) do |query| + queries << query + { code: 200, data: responses.shift } + end + + expect(logger).to receive(:debug).with("Loading GitHub team github.fake:kittensinc/alpha-cats") + expect(logger).to receive(:debug).with("Loading GitHub team github.fake:kittensinc/beta-cats") + expect(logger).to receive(:debug).with("Loading GitHub team github.fake:kittensinc/missing-cats") + expect(logger).to receive(:debug).with("GitHub GraphQL team batch cost=5 remaining=4995 reset_at=later") + expect(logger).to receive(:warn).with("Team missing-cats does not exist in this GitHub.com organization. If applied, the team will be created.") + + result = subject.read_teams([alpha_group, beta_group, missing_group]) + + expect(queries.size).to eq(2) + expect(queries.first).to include('team0: team(slug: "alpha-cats")') + expect(queries.first).to include('team1: team(slug: "beta-cats")') + expect(queries.first).to include('team2: team(slug: "missing-cats")') + expect(queries.last).to include('team0: team(slug: "alpha-cats")') + expect(queries.last).to include('after: "cursor-2"') + expect(queries.last).not_to include("beta-cats") + expect(queries.last).not_to include("missing-cats") + + expect(result.fetch("alpha-cats").member_strings).to eq(Set.new(%w[alpha beta gamma])) + expect(result.fetch("alpha-cats").team_id).to eq(101) + expect(result.fetch("alpha-cats").metadata).to eq( + "application_owner" => "snowshoe", + "parent_team_name" => "parent-cats", + "team_maintainers" => "alpha" + ) + expect(result.fetch("beta-cats").member_strings).to eq(Set.new) + expect(result.fetch("missing-cats")).to be_nil + end + + it "excludes predictive-cache hits from authoritative batches" do + alpha_dn = "cn=alpha-cats,ou=kittensinc,ou=GitHub,dc=github,dc=fake" + cache[:predictive_state] = { + by_dn: { alpha_dn => { members: Set.new(%w[CachedCat]), metadata: nil } }, + invalid: Set.new + } + authoritative_data = { + members: ["api-cat"], + roles: { "api-cat" => "member" }, + team_id: 202, + parent_team_name: nil + } + + expect(subject).to receive(:graphql_team_data).with("beta-cats").and_return(authoritative_data) + result = subject.read_teams([alpha_group, beta_group]) + + expect(result.fetch("alpha-cats").team_id).to eq(-1) + expect(result.fetch("alpha-cats").member_strings).to eq(Set.new(%w[cachedcat])) + expect(result.fetch("beta-cats").team_id).to eq(202) + expect(subject.from_predictive_cache?(alpha_group)).to eq(true) + expect(subject.from_predictive_cache?(beta_group)).to eq(false) + end + + it "respects the configured batch boundary" do + allow(subject).to receive(:graphql_team_batch_size).and_return(2) + groups = %w[one two three].map do |slug| + Entitlements::Models::Group.new( + dn: "cn=#{slug},ou=kittensinc,ou=GitHub,dc=github,dc=fake", + members: Set.new, + metadata: nil + ) + end + queries = [] + allow(subject).to receive(:graphql_http_post) do |query| + queries << query + aliases = query.scan(/(team\d+): team\(slug: "([^"]+)"\)/) + organization = aliases.to_h do |team_alias, slug| + [team_alias, { "databaseId" => slug.length, "parentTeam" => nil, "members" => { "edges" => [] } }] + end + { code: 200, data: { "data" => { "organization" => organization } } } + end + + subject.read_teams(groups) + + expect(queries.size).to eq(2) + expect(queries.first.scan(/team\d+: team/).size).to eq(2) + expect(queries.last.scan(/team\d+: team/).size).to eq(1) + end + + it "escapes GraphQL string values" do + expect(subject.send(:graphql_string_literal, "cats\"\\\n")).to eq("\"cats\\\"\\\\\\n\"") + end + + it "fails when the organization is missing from an otherwise successful response" do + allow(subject).to receive(:graphql_http_post) + .and_return(code: 200, data: { "data" => { "organization" => nil } }) + + expect do + subject.read_teams([alpha_group, beta_group]) + end.to raise_error(RuntimeError, "GraphQL response missing organization kittensinc") + end + + it "enforces the per-team pagination sanity limit" do + stub_const("#{described_class}::MAX_GRAPHQL_TEAM_PAGES", 2) + allow(subject).to receive(:max_graphql_results).and_return(1) + allow(subject).to receive(:graphql_http_post).and_return( + code: 200, + data: { + "data" => { + "organization" => { + "team0" => { + "databaseId" => 101, + "parentTeam" => nil, + "members" => { + "edges" => [ + { "node" => { "login" => "cat" }, "role" => "MEMBER", "cursor" => "next" } + ] + } + } + } + } + } + ) + + expect do + subject.read_teams([alpha_group]) + end.to raise_error(RuntimeError, "GitHub team alpha-cats exceeded the 2-page GraphQL limit") + end + + it "fails when a team changes database ID during pagination" do + allow(subject).to receive(:max_graphql_results).and_return(1) + responses = [101, 202].map do |team_id| + { + code: 200, + data: { + "data" => { + "organization" => { + "team0" => { + "databaseId" => team_id, + "parentTeam" => nil, + "members" => { + "edges" => [ + { "node" => { "login" => "cat" }, "role" => "MEMBER", "cursor" => "next" } + ] + } + } + } + } + } + } + end + allow(subject).to receive(:graphql_http_post) { responses.shift } + + expect do + subject.read_teams([alpha_group]) + end.to raise_error(RuntimeError, "GitHub team alpha-cats changed database ID during pagination") + end + end + describe "#from_predictive_cache?" do let(:people) { Set.new(%w[blackmanx ragamuffin russianblue]) } @@ -699,7 +911,7 @@ end it "raises a custom exception when team is not found" do - empty = JSON.generate("data" => { "organization" => { "team" => nil } }) + empty = JSON.generate("data" => { "organization" => { "team0" => nil } }) stub_request(:post, "https://github.fake/api/v3/graphql").to_return(status: 200, body: empty) expect do subject.send(:graphql_team_data, "crying-cat-face") @@ -718,12 +930,11 @@ it "parses team data from a single page of results" do stub_request(:post, "https://github.fake/api/v3/graphql") .with( - body: "{\"query\":\"{\\norganization(login: \\\"kittensinc\\\") {\\nteam(slug: \\\"grumpy-cat\\\") {\\ndatabaseId\\nparentTeam {\\nslug\\n}\\nmembers(first: 100, membership: IMMEDIATE) {\\nedges {\\nnode {\\nlogin\\n}\\nrole\\ncursor\\n}\\n}\\n}\\n}\\n}\"}", headers: { "Authorization" => "bearer GoPackGo", "Content-Type" => "application/json" } - ).to_return(status: 200, body: graphql_dotcom_response) + ).to_return(status: 200, body: graphql_dotcom_response.sub('"team"', '"team0"')) result = subject.send(:graphql_team_data, "grumpy-cat") members = ["highlander", "blackmanx", "toyger", "ocicat", "hubot", "korat", "mainecoon", "russianblue", @@ -761,9 +972,9 @@ it "parses team data from paginated results" do stub_request(:post, "https://github.fake/api/v3/graphql") .to_return( - { status: 200, body: graphql_dotcom_response_1 }, - { status: 200, body: graphql_dotcom_response_2 }, - { status: 200, body: graphql_dotcom_response_3 } + { status: 200, body: graphql_dotcom_response_1.sub('"team"', '"team0"') }, + { status: 200, body: graphql_dotcom_response_2.sub('"team"', '"team0"') }, + { status: 200, body: graphql_dotcom_response_3.sub('"team"', '"team0"') } ) result = subject.send(:graphql_team_data, "grumpy-cat") @@ -802,9 +1013,9 @@ it "parses team data from paginated results" do stub_request(:post, "https://github.fake/api/v3/graphql") .to_return( - { status: 200, body: graphql_dotcom_response_1 }, - { status: 200, body: graphql_dotcom_response_2 }, - { status: 200, body: graphql_dotcom_response_3 } + { status: 200, body: graphql_dotcom_response_1.sub('"team"', '"team0"') }, + { status: 200, body: graphql_dotcom_response_2.sub('"team"', '"team0"') }, + { status: 200, body: graphql_dotcom_response_3.sub('"team"', '"team0"') } ) result = subject.send(:graphql_team_data, "grumpy-cat") diff --git a/spec/unit/entitlements/service/github_spec.rb b/spec/unit/entitlements/service/github_spec.rb index eea708e..5a209f5 100644 --- a/spec/unit/entitlements/service/github_spec.rb +++ b/spec/unit/entitlements/service/github_spec.rb @@ -47,6 +47,71 @@ end end + describe "#org_member?" do + let(:members_and_roles) do + { + "alice" => "MEMBER", + "bob" => "ADMIN" + } + end + + before do + allow(subject).to receive(:members_and_roles_from_rest).and_return(members_and_roles) + end + + it "performs case-insensitive lookups" do + expect(subject.org_member?("ALIce")).to eq(true) + expect(subject.org_member?("charles")).to eq(false) + end + + it "reuses the normalized membership set" do + expect(subject.org_member?("alice")).to eq(true) + normalized_members = Entitlements.cache[:github_org_members]["https://github.fake/api/v3|kittensinc"][:normalized_members] + expect(subject.org_member?("bob")).to eq(true) + expect(Entitlements.cache[:github_org_members]["https://github.fake/api/v3|kittensinc"][:normalized_members]).to equal(normalized_members) + end + + it "shares the normalized membership set between services for the same organization signature" do + other = described_class.new( + addr: "https://github.fake/api/v3", + org: "kittensinc", + token: "DifferentToken", + ou: "ou=kittensinc,ou=GitHub,dc=github,dc=fake", + ignore_not_found: false + ) + + expect(subject.org_member?("alice")).to eq(true) + normalized_members = Entitlements.cache[:github_org_members]["https://github.fake/api/v3|kittensinc"][:normalized_members] + expect(other.org_member?("bob")).to eq(true) + expect(Entitlements.cache[:github_org_members]["https://github.fake/api/v3|kittensinc"][:normalized_members]).to equal(normalized_members) + end + + it "isolates normalized membership sets by organization and GitHub instance" do + other_org = described_class.new( + addr: "https://github.fake/api/v3", + org: "puppiesinc", + token: "GoPackGo", + ou: "ou=puppiesinc,ou=GitHub,dc=github,dc=fake", + ignore_not_found: false + ) + other_instance = described_class.new( + addr: "https://github.example/api/v3", + org: "kittensinc", + token: "GoPackGo", + ou: "ou=kittensinc,ou=GitHub,dc=github,dc=example", + ignore_not_found: false + ) + allow(other_org).to receive(:members_and_roles_from_rest).and_return("charles" => "MEMBER") + allow(other_instance).to receive(:members_and_roles_from_rest).and_return("david" => "MEMBER") + + expect(subject.org_member?("alice")).to eq(true) + expect(other_org.org_member?("alice")).to eq(false) + expect(other_org.org_member?("charles")).to eq(true) + expect(other_instance.org_member?("alice")).to eq(false) + expect(other_instance.org_member?("david")).to eq(true) + end + end + describe "#enterprise?" do it "returns false if an instance is not enterprise" do stub_request(:get, "https://github.fake/api/v3/meta"). @@ -122,6 +187,8 @@ # First load should read from the cache. expect(subject.org_members).to eq(answer.map { |k, v| [k, v.downcase] }.to_h) + expect(subject.org_member?("monalisa")).to eq(true) + normalized_members = Entitlements.cache[:github_org_members]["https://github.fake/api/v3|kittensinc"][:normalized_members] # Invalidating cache should force a re-read. answer_2 = answer.dup @@ -137,6 +204,8 @@ # Check that the re-read has occurred and the correct result is achieved. expect(subject).not_to receive(:members_and_roles_from_graphql) # Should already be in object's cache expect(subject.org_members).to eq(answer_2.map { |k, v| [k, v.downcase] }.to_h) + expect(subject.org_member?("ragamuffin")).to eq(true) + expect(Entitlements.cache[:github_org_members]["https://github.fake/api/v3|kittensinc"][:normalized_members]).not_to equal(normalized_members) end end @@ -312,6 +381,33 @@ let(:answer) { { "foo" => "bar" } } let(:query) { "my query here" } + it "reconnects and retries after the connection times out" do + response = instance_double(Net::HTTPOK, code: "200", body: JSON.generate(answer)) + timed_out_http = Net::HTTP.new("github.fake", 443) + replacement_http = Net::HTTP.new("github.fake", 443) + + expect(Net::HTTP).to receive(:new). + with("github.fake", 443). + twice. + and_return(timed_out_http, replacement_http) + expect(timed_out_http).to receive(:use_ssl=).with(true) + expect(timed_out_http).to receive(:start) + expect(timed_out_http).to receive(:request). + and_raise(Net::ReadTimeout, "execution expired") + expect(timed_out_http).to receive(:started?).and_return(true) + expect(timed_out_http).to receive(:finish) + expect(replacement_http).to receive(:use_ssl=).with(true) + expect(replacement_http).to receive(:start) + expect(replacement_http).to receive(:request).and_return(response) + expect_any_instance_of(Object).to receive(:sleep).with(1).once + expect(logger).to receive(:error). + with("Caught Net::ReadTimeout POSTing to https://github.fake/api/v3/graphql: Net::ReadTimeout with \"execution expired\"") + expect(logger).to receive(:warn). + with("GraphQL failed on try 1 of 3. Will retry.") + + expect(subject.send(:graphql_http_post, query)).to eq(code: 200, data: answer) + end + it "returns immediately for success" do expect(subject).to receive(:graphql_http_post_real).with(query).and_return(code: 200, data: answer) expect_any_instance_of(Object).not_to receive(:sleep) @@ -361,6 +457,52 @@ end describe "#graphql_http_post_real" do + it "reuses a started HTTP connection across requests" do + answer = { "foo" => "bar" } + response = instance_double(Net::HTTPOK, code: "200", body: JSON.generate(answer)) + http = Net::HTTP.new("github.fake", 443) + + expect(Net::HTTP).to receive(:new).with("github.fake", 443).once.and_return(http) + expect(http).to receive(:use_ssl=).with(true).once + expect(http).to receive(:start).once + expect(http).to receive(:request).twice.and_return(response) + + 2.times do + expect(subject.send(:graphql_http_post_real, "nonsense")).to eq(code: 200, data: answer) + end + end + + it "discards a failed connection so the next request reconnects" do + answer = { "foo" => "bar" } + response = instance_double(Net::HTTPOK, code: "200", body: JSON.generate(answer)) + failed_http = Net::HTTP.new("github.fake", 443) + replacement_http = Net::HTTP.new("github.fake", 443) + + expect(Net::HTTP).to receive(:new).with("github.fake", 443).twice.and_return(failed_http, replacement_http) + expect(failed_http).to receive(:use_ssl=).with(true) + expect(failed_http).to receive(:start) + expect(failed_http).to receive(:request).and_raise(EOFError, "closed connection") + expect(failed_http).to receive(:started?).and_return(true) + expect(failed_http).to receive(:finish) + expect(replacement_http).to receive(:use_ssl=).with(true) + expect(replacement_http).to receive(:start) + expect(replacement_http).to receive(:request).and_return(response) + expect(logger).to receive(:error). + with("Caught EOFError POSTing to https://github.fake/api/v3/graphql: closed connection") + + expect(subject.send(:graphql_http_post_real, "nonsense")).to eq(code: 500, data: nil) + expect(subject.send(:graphql_http_post_real, "nonsense")).to eq(code: 200, data: answer) + end + + it "logs a warning if a failed connection cannot be closed" do + http = instance_double(Net::HTTP, started?: true) + subject.instance_variable_set(:@graphql_http, http) + + expect(http).to receive(:finish).and_raise(IOError, "already closed") + expect(logger).to receive(:warn).with("Failed to close GraphQL connection: already closed") + expect(subject.send(:reset_graphql_http)).to be_nil + end + it "returns code=200 and parsed JSON for a successful response" do answer = { "foo" => ["bar", "baz" => "fizz"] } stub_request(:post, "https://github.fake/api/v3/graphql").to_return(status: 200, body: JSON.generate(answer)) diff --git a/spec/unit/spec_helper.rb b/spec/unit/spec_helper.rb index 2a4ee2a..db3d0ea 100644 --- a/spec/unit/spec_helper.rb +++ b/spec/unit/spec_helper.rb @@ -57,15 +57,14 @@ def default_filters end def graphql_response(team, slice_start, slice_length, parent_team: nil) - team_id = rand(1..10000) edges = team.member_strings.sort.to_a.slice(slice_start, slice_length).map do |m| { "node" => { "login" => m }, "role" => "MEMBER", "cursor" => Base64.encode64(m) } end struct = { "data" => { "organization" => { - "team" => { - "databaseId" => team_id, + "team0" => { + "databaseId" => team.team_id, "members" => { "edges" => edges },