From 9fa02ad2973584404e505ce05603e0a34eeae9e8 Mon Sep 17 00:00:00 2001 From: George Waters Date: Wed, 4 Jan 2023 23:46:40 -0500 Subject: [PATCH 1/5] Make sure repos is always an array --- lib/jekyll-github-metadata/repository.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/jekyll-github-metadata/repository.rb b/lib/jekyll-github-metadata/repository.rb index c8716f2..c2bc53b 100644 --- a/lib/jekyll-github-metadata/repository.rb +++ b/lib/jekyll-github-metadata/repository.rb @@ -118,7 +118,8 @@ def owner_public_repositories :accept => "application/vnd.github.mercy-preview+json", } memoize_value :@owner_public_repositories, Value.new("owner_public_repositories", proc do |c| - c.list_repos(owner, options).map do |r| + repos = c.list_repos(owner, options) || [] + repos.map do |r| r[:releases] = Value.new("owner_public_repositories_releases", proc { c.releases(r[:full_name]) }) r[:contributors] = Value.new("owner_public_repositories_contributors", proc { c.contributors(r[:full_name]) }) r From 2c265a56b21849cfbe7b748a91d618e1789b897f Mon Sep 17 00:00:00 2001 From: George Waters Date: Fri, 6 Jan 2023 20:16:14 -0500 Subject: [PATCH 2/5] Prevent loading all GH data on reset The way this was setup, when running `build` or `serve` with `--watch`, `inspect` was called and was forcing everything to load. This could cause a very large number of API calls to GitHub. We now uninject the MetadataDrop after rendering is complete. With this change `GitHubMetadata` will no longer trigger all of those calls on reset. The other scenario where a bunch of calls were being forced to be made was when putting the `github` key with any sub-keys in `_config.yml`. This went through every key in `GitHubMetadata` and loaded it, once again potentially making a very large number of calls. These changes take care of this scenario as well. --- lib/jekyll-github-metadata/site_github_munger.rb | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/lib/jekyll-github-metadata/site_github_munger.rb b/lib/jekyll-github-metadata/site_github_munger.rb index 0fb1a3d..c723cc0 100644 --- a/lib/jekyll-github-metadata/site_github_munger.rb +++ b/lib/jekyll-github-metadata/site_github_munger.rb @@ -15,6 +15,7 @@ class << self def initialize(site) Jekyll::GitHubMetadata.site = site + @original_config = site.config["github"] end def munge! @@ -28,16 +29,20 @@ def inject_metadata!(payload) payload.site["github"] = github_namespace end + def uninject_metadata!(payload) + payload.site["github"] = @original_config + end + private def github_namespace - case site.config["github"] + case @original_config when nil drop when Hash - Jekyll::Utils.deep_merge_hashes(drop, site.config["github"]) + drop.merge(@original_config) else - site.config["github"] + @original_config end end @@ -94,5 +99,9 @@ def should_warn_about_site_name? Jekyll::Hooks.register :site, :pre_render do |_site, payload| SiteGitHubMunger.global_munger.inject_metadata!(payload) end + + Jekyll::Hooks.register :site, :post_render do |_site, payload| + SiteGitHubMunger.global_munger.uninject_metadata!(payload) + end end end From eced31d33d74a4ac9a6e686a3db3418ae7f913ab Mon Sep 17 00:00:00 2001 From: George Waters Date: Tue, 14 Feb 2023 20:26:27 -0500 Subject: [PATCH 3/5] Add test for preventing load data on reset This commit adds a new fixture site that doesn't load all of `site.github`. It just loads a few items. We then check that those items are loaded once after the first and second times the site is processed. We also use a new spec helper function `not_expect_api_call` to make sure other API calls are not made. This test checks a few enpoints that get loaded when the entire `site.github` is loaded. Before adding the uninject code, these API endpoints would have gotten called, despite not being used by the fixture site. With the uninject changes, they are no longer called. --- spec/site_github_munger_spec.rb | 23 +++++++++++++++++++++++ spec/spec_helpers/web_mock_helper.rb | 5 +++++ spec/test-site-uninject/_config.yml | 9 +++++++++ spec/test-site-uninject/index.html | 16 ++++++++++++++++ spec/test-site-uninject/subvalues.txt | 4 ++++ 5 files changed, 57 insertions(+) create mode 100644 spec/test-site-uninject/_config.yml create mode 100644 spec/test-site-uninject/index.html create mode 100644 spec/test-site-uninject/subvalues.txt diff --git a/spec/site_github_munger_spec.rb b/spec/site_github_munger_spec.rb index 3bfa897..7eb36d7 100644 --- a/spec/site_github_munger_spec.rb +++ b/spec/site_github_munger_spec.rb @@ -231,4 +231,27 @@ end.to raise_error(Jekyll::GitHubMetadata::Client::BadCredentialsError) end end + + context "render the 'uninject' fixture test site" do + let(:source) { File.expand_path("test-site-uninject", __dir__) } + let(:dest) { File.expand_path("../tmp/test-site-uninject-build", __dir__) } + + it "process site twice (simulate reset), check API calls" do + config = Jekyll::Configuration.from({"source" => source, "destination" => dest}) + site = Jekyll::Site.new(config) + site.process + expect_api_call "/repos/jekyll/github-metadata" + expect_api_call "/repos/jekyll/github-metadata/releases/latest" + expect_api_call "/orgs/jekyll" + site.process + expect_api_call "/repos/jekyll/github-metadata" + expect_api_call "/repos/jekyll/github-metadata/releases/latest" + expect_api_call "/orgs/jekyll" + + not_expect_api_call "/repos/jekyll/github-metadata/pages" + not_expect_api_call "/repos/jekyll/github-metadata/contributors?per_page=100" + not_expect_api_call "/orgs/jekyll/public_members?per_page=100" + not_expect_api_call "/users/jekyll/repos?per_page=100&type=public" + end + end end diff --git a/spec/spec_helpers/web_mock_helper.rb b/spec/spec_helpers/web_mock_helper.rb index ad465d1..41f42b7 100644 --- a/spec/spec_helpers/web_mock_helper.rb +++ b/spec/spec_helpers/web_mock_helper.rb @@ -41,6 +41,11 @@ def expect_api_call(path) .with(:headers => request_headers).once end + def not_expect_api_call(path) + expect(WebMock).to have_requested(:get, url(path)) + .with(:headers => request_headers).times(0) + end + def request_headers REQUEST_HEADERS.merge( "Authorization" => "token #{ENV.fetch("JEKYLL_GITHUB_TOKEN", "1234abc")}" diff --git a/spec/test-site-uninject/_config.yml b/spec/test-site-uninject/_config.yml new file mode 100644 index 0000000..c49dc83 --- /dev/null +++ b/spec/test-site-uninject/_config.yml @@ -0,0 +1,9 @@ +repository: jekyll/github-metadata +github: + in_your_config: + setting_your: keyz +plugins: + - jekyll-github-metadata +# remove this once we drop support for Jekyll v3.4 and below +gems: + - jekyll-github-metadata diff --git a/spec/test-site-uninject/index.html b/spec/test-site-uninject/index.html new file mode 100644 index 0000000..8689396 --- /dev/null +++ b/spec/test-site-uninject/index.html @@ -0,0 +1,16 @@ +--- +--- + + + + + + Page Title + + +

{{ site.github.project_title }}

+

{{ site.github.project_tagline }}

+

Owner: {{ site.github.owner_name }}

+

Latest Release: {{ site.github.latest_release }}

+ + diff --git a/spec/test-site-uninject/subvalues.txt b/spec/test-site-uninject/subvalues.txt new file mode 100644 index 0000000..5350e64 --- /dev/null +++ b/spec/test-site-uninject/subvalues.txt @@ -0,0 +1,4 @@ +--- +--- + +owner: {{ site.github.owner }} From a910c3cd05ac75678ec35aef0042568a4c0d5102 Mon Sep 17 00:00:00 2001 From: George Waters Date: Thu, 16 Feb 2023 15:43:36 -0500 Subject: [PATCH 4/5] Check rendered site, add uninject test comments This adds a check to make sure the files in the rendered site match fixture files that are known to have the correct `site.github` data, after running `site.process` twice. This also adds code comments explaining the rationale and purpose behind the different parts of the `uninject` test. --- spec/site_github_munger_spec.rb | 20 +++++++++++++++++-- spec/test-site-uninject-rendered/index.html | 13 ++++++++++++ .../test-site-uninject-rendered/subvalues.txt | 1 + 3 files changed, 32 insertions(+), 2 deletions(-) create mode 100644 spec/test-site-uninject-rendered/index.html create mode 100644 spec/test-site-uninject-rendered/subvalues.txt diff --git a/spec/site_github_munger_spec.rb b/spec/site_github_munger_spec.rb index 7eb36d7..a11049b 100644 --- a/spec/site_github_munger_spec.rb +++ b/spec/site_github_munger_spec.rb @@ -235,23 +235,39 @@ context "render the 'uninject' fixture test site" do let(:source) { File.expand_path("test-site-uninject", __dir__) } let(:dest) { File.expand_path("../tmp/test-site-uninject-build", __dir__) } + let(:config) { Jekyll::Configuration.from({"source" => source, "destination" => dest}) } + let(:fixture_rendered) { File.expand_path("test-site-uninject-rendered", __dir__) } - it "process site twice (simulate reset), check API calls" do - config = Jekyll::Configuration.from({"source" => source, "destination" => dest}) + it "process site twice (simulate reset), check API calls & rendered site" do site = Jekyll::Site.new(config) site.process + # These API calls are expected because we use the attributes in the + # fixture site. expect_api_call "/repos/jekyll/github-metadata" expect_api_call "/repos/jekyll/github-metadata/releases/latest" expect_api_call "/orgs/jekyll" site.process + # After processing the site again, we expect that these API calls were + # still only made once. We cache the results so we shouldn't be making the + # same API call more than once. expect_api_call "/repos/jekyll/github-metadata" expect_api_call "/repos/jekyll/github-metadata/releases/latest" expect_api_call "/orgs/jekyll" + # We do not expect these API calls to have been made since we do not use + # these attributes in the fixture site. not_expect_api_call "/repos/jekyll/github-metadata/pages" not_expect_api_call "/repos/jekyll/github-metadata/contributors?per_page=100" not_expect_api_call "/orgs/jekyll/public_members?per_page=100" not_expect_api_call "/users/jekyll/repos?per_page=100&type=public" + + # Check to make sure the fixture site is rendered with the correct + # site.github values. + Dir.children(dest).each { |file| + rendered_file = File.join(dest, file) + fixture_file = File.join(fixture_rendered, file) + expect(File.read(rendered_file)).to eql(File.read(fixture_file)) + } end end end diff --git a/spec/test-site-uninject-rendered/index.html b/spec/test-site-uninject-rendered/index.html new file mode 100644 index 0000000..9d780c5 --- /dev/null +++ b/spec/test-site-uninject-rendered/index.html @@ -0,0 +1,13 @@ + + + + + Page Title + + +

github-metadata

+

:octocat: `site.github`

+

Owner: jekyll

+

Latest Release: {"url"=>"https://api.github.com/repos/jekyll/github-metadata/releases/5198319", "assets_url"=>"https://api.github.com/repos/jekyll/github-metadata/releases/5198319/assets", "upload_url"=>"https://uploads.github.com/repos/jekyll/github-metadata/releases/5198319/assets{?name,label}", "html_url"=>"https://github.com/jekyll/github-metadata/releases/tag/v2.3.1", "id"=>5198319, "tag_name"=>"v2.3.1", "target_commitish"=>"master", "name"=>"v2.3.1", "draft"=>false, "author"=>{"login"=>"jekyllbot", "id"=>6166343, "avatar_url"=>"https://avatars.githubusercontent.com/u/6166343?v=3", "gravatar_id"=>"", "url"=>"https://api.github.com/users/jekyllbot", "html_url"=>"https://github.com/jekyllbot", "followers_url"=>"https://api.github.com/users/jekyllbot/followers", "following_url"=>"https://api.github.com/users/jekyllbot/following{/other_user}", "gists_url"=>"https://api.github.com/users/jekyllbot/gists{/gist_id}", "starred_url"=>"https://api.github.com/users/jekyllbot/starred{/owner}{/repo}", "subscriptions_url"=>"https://api.github.com/users/jekyllbot/subscriptions", "organizations_url"=>"https://api.github.com/users/jekyllbot/orgs", "repos_url"=>"https://api.github.com/users/jekyllbot/repos", "events_url"=>"https://api.github.com/users/jekyllbot/events{/privacy}", "received_events_url"=>"https://api.github.com/users/jekyllbot/received_events", "type"=>"User", "site_admin"=>false}, "prerelease"=>false, "created_at"=>2017-01-18 20:10:29 UTC, "published_at"=>2017-01-18 20:10:33 UTC, "assets"=>[], "tarball_url"=>"https://api.github.com/repos/jekyll/github-metadata/tarball/v2.3.1", "zipball_url"=>"https://api.github.com/repos/jekyll/github-metadata/zipball/v2.3.1", "body"=>"- Remove log on Octokit::NotFound (#86)\n"}

+ + diff --git a/spec/test-site-uninject-rendered/subvalues.txt b/spec/test-site-uninject-rendered/subvalues.txt new file mode 100644 index 0000000..1daabb5 --- /dev/null +++ b/spec/test-site-uninject-rendered/subvalues.txt @@ -0,0 +1 @@ +owner: {"avatar_url"=>"https://avatars0.githubusercontent.com/u/3083652?v=4", "bio"=>nil, "blog"=>"https://jekyllrb.com", "collaborators"=>nil, "company"=>nil, "created_at"=>2012-12-19 19:37:35 UTC, "description"=>"Jekyll is a blog-aware, static site generator in Ruby.", "email"=>"", "followers"=>0, "following"=>0, "has_organization_projects"=>true, "has_repository_projects"=>true, "hireable"=>nil, "html_url"=>"https://github.com/jekyll", "id"=>3083652, "is_verified"=>true, "location"=>nil, "login"=>"jekyll", "name"=>"Jekyll", "node_id"=>"MDEyOk9yZ2FuaXphdGlvbjMwODM2NTI=", "public_gists"=>0, "public_repos"=>50, "type"=>"Organization", "updated_at"=>2019-01-27 15:27:32 UTC} From 39e44c30ef071619c450b297cebf2bd984a73252 Mon Sep 17 00:00:00 2001 From: George Waters Date: Thu, 16 Feb 2023 17:02:27 -0500 Subject: [PATCH 5/5] Fix for Rubocop --- spec/site_github_munger_spec.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/spec/site_github_munger_spec.rb b/spec/site_github_munger_spec.rb index a11049b..3870bdc 100644 --- a/spec/site_github_munger_spec.rb +++ b/spec/site_github_munger_spec.rb @@ -235,7 +235,7 @@ context "render the 'uninject' fixture test site" do let(:source) { File.expand_path("test-site-uninject", __dir__) } let(:dest) { File.expand_path("../tmp/test-site-uninject-build", __dir__) } - let(:config) { Jekyll::Configuration.from({"source" => source, "destination" => dest}) } + let(:config) { Jekyll::Configuration.from({ "source" => source, "destination" => dest }) } let(:fixture_rendered) { File.expand_path("test-site-uninject-rendered", __dir__) } it "process site twice (simulate reset), check API calls & rendered site" do @@ -263,11 +263,11 @@ # Check to make sure the fixture site is rendered with the correct # site.github values. - Dir.children(dest).each { |file| + Dir.children(dest).each do |file| rendered_file = File.join(dest, file) fixture_file = File.join(fixture_rendered, file) expect(File.read(rendered_file)).to eql(File.read(fixture_file)) - } + end end end end