github-metadata icon indicating copy to clipboard operation
github-metadata copied to clipboard

Empty owner data

Open AlfredoRamos opened this issue 4 years ago • 9 comments
trafficstars

Hi,

I noticed today that site.github.owner.* was returning null on some properties (like bio or login), and after debugging the object I found that all of its values are null.

The following sample code:

{{ site.github.owner | jsonify }}

Returns this data:

{
    "avatar_url": null,
    "bio": null,
    "blog": null,
    "collaborators": null,
    "company": null,
    "created_at": null,
    "description": null,
    "email": null,
    "followers": null,
    "following": null,
    "has_organization_projects": null,
    "has_repository_projects": null,
    "hireable": null,
    "html_url": null,
    "id": null,
    "is_verified": null,
    "location": null,
    "login": null,
    "name": null,
    "node_id": null,
    "public_gists": null,
    "public_repos": null,
    "type": null,
    "updated_at": null
}

Running jekyll with the --verbose flag does not show any error.

Everything else seems to be fine, just the owner object is having issues.

bundle exec jekyll server --verbose
$ bundle exec jekyll server --verbose
  Logging at level: debug
    Jekyll Version: 4.2.0
Configuration file: /home/alfredo/Public/Git/AlfredoRamos.github.io/_config.yml
  Logging at level: debug
    Jekyll Version: 4.2.0
         Requiring: /home/alfredo/Public/Git/AlfredoRamos.github.io/_plugins/social_network_filter.rb
         Requiring: /home/alfredo/Public/Git/AlfredoRamos.github.io/_plugins/archive_pages_generator.rb
         Requiring: /home/alfredo/Public/Git/AlfredoRamos.github.io/_plugins/read_time_filter.rb
         Requiring: jekyll-github-metadata
         Requiring: jekyll-include-cache
         Requiring: jekyll-paginate-v2
         Requiring: jekyll-sitemap
         Requiring: jekyll-target-blank
   GitHub Metadata: Initializing...
            Source: /home/alfredo/Public/Git/AlfredoRamos.github.io
       Destination: /home/alfredo/Public/Git/AlfredoRamos.github.io/_site
 Incremental build: disabled. Enable with --incremental
      Generating... 
   GitHub Metadata: Generating for AlfredoRamos/AlfredoRamos.github.io
   GitHub Metadata: Calling @client.repository("AlfredoRamos/AlfredoRamos.github.io", {:accept=>"application/vnd.github.drax-preview+json"})
   GitHub Metadata: Calling @client.pages("AlfredoRamos/AlfredoRamos.github.io", {})
   GitHub Metadata: Calling @client.contributors("AlfredoRamos/AlfredoRamos.github.io")
   GitHub Metadata: Calling @client.latest_release("AlfredoRamos/AlfredoRamos.github.io")
   GitHub Metadata: Calling @client.organization("AlfredoRamos")
   GitHub Metadata: Calling @client.organization_public_members("AlfredoRamos")
   GitHub Metadata: Calling @client.list_repos("AlfredoRamos", {:type=>"public", :accept=>"application/vnd.github.mercy-preview+json"})
   GitHub Metadata: Calling @client.releases("AlfredoRamos/AlfredoRamos.github.io")
   GitHub Metadata: Calling @client.organization("AlfredoRamos")

And jekyll doctor says that everything is fine.

bundle exec jekyll doctor --trace
$ bundle exec jekyll doctor --trace
Configuration file: /home/alfredo/Public/Git/AlfredoRamos.github.io/_config.yml
         AutoPages: Disabled/Not configured in site.config.
        Pagination: Complete, processed 1 pagination page(s)
  Your test results are in. Everything looks fine.

My setup:

  • Ruby 2.7.2p137
  • Jekyll 4.2.0
  • jekyll-metadata 2.13.0
  • Personal access token with public_repo and read:user* scopes
  • repository key inside my _config.yml
  • JEKYLL_GITHUB_TOKEN inside a .env file

* I added the read:user scope just to test if it might need it, but it didn't make a difference.

AlfredoRamos avatar Dec 26 '20 02:12 AlfredoRamos

The issue seems to come from the evaluation in the Jekyll::GitHubMetadata::Owner::owner_info() method.

https://github.com/jekyll/github-metadata/blob/c0d0fa8057c02bb5de23af90b4b8c27e529350d7/lib/jekyll-github-metadata/owner.rb#L78-L87

c.organization(owner_login) returns the following object:

{:message=>"Not Found", :documentation_url=>"https://docs.github.com/rest/reference/orgs#get-an-organization"}

So c.user(owner_login) is not evaluated next.

https://talk.jekyllrb.com/t/the-jekyll-github-metadata-returns-empty-owner-data/5402/4?u=alfredoramos

AlfredoRamos avatar Dec 27 '20 23:12 AlfredoRamos

Can I suggest for the org and user lookups, that there is a check for the presence of a value that indicates success (it would be missing on a failure).

Like the name field. I don't know if it is owner_login or name or something but assuming that it is name:

c.organization(owner_login)[:name] || c.user(owner_login)[:name] || {}

For the first two parts, it will get the value for the name key and then return truthy if the key is found (and if the string is not empty). And will return a falsy nil if not found.

As an extra, the line does a lot so could be moved out to a new separate short function. Maybe one that even exists already or this could be reused.

Example of calling it:

get_org_or_user(owner_login).to_h

MichaelCurrin avatar Dec 28 '20 16:12 MichaelCurrin

This issue has been automatically marked as stale because it has not been commented on for at least two months.

The resources of the Jekyll team are limited, and so we are asking for your help.

If this is a bug and you can still reproduce this error on the master branch, please reply with all of the information you have about it in order to keep the issue open.

If this is a feature request, please consider whether it can be accomplished in another way. If it cannot, please elaborate on why it is core to this project and why you feel more than 80% of users would find this beneficial.

This issue will automatically be closed in two months if no further activity occurs. Thank you for all your contributions.

jekyllbot avatar Feb 28 '21 16:02 jekyllbot

It doesn't look like anyone picked this up.

I'm offering to make a PR.

I think login is better as a check rather than name. As login must be set and is what we are looking up against.

If org and username requests both return empty responses (except for error message), we could log a warning message.

MichaelCurrin avatar Mar 01 '21 06:03 MichaelCurrin

I made a draft PR #203 which I must test locally still.

MichaelCurrin avatar Mar 08 '21 14:03 MichaelCurrin

I'm stuck with this.

@AlfredoRamos in your comment above, you said a bad login returns

{:message=>"Not Found", :documentation_url=>"https://docs.github.com/rest/reference/orgs#get-an-organization"}

But when running unit tests as is, I see that (before the to_h bit), either a Sawyer object is returned or false.

According to the unit tests approach, a bad login gives false and then that evaluates to {}. It doesn't use the 404 data you provided.

Code split over two lines and with a log before.

proc do |c|
  puts c.organization(owner_login)
  owner = c.organization(owner_login) || c.user(owner_login) || {}
  owner.to_h
end
  is an ORG
#<Sawyer::Resource:0x00007f86bb29a1f0>
    fetches login
#<Sawyer::Resource:0x00007f86bb291988>
    fetches has_organization_projects
...

  is a USER
false
    fetches followers

I tried doing a check this this. Which works fine for the ORG context test, but throws an error for undefined method in the case of the USER test cases. owner_spec.rb

c.organization(owner_login).login
c.organization(owner_login)[:login]

I ran tests like this:

$ script/test spec/owner_spec.rb

MichaelCurrin avatar Mar 28 '21 07:03 MichaelCurrin

Regarding my suggestion to add a test case to handle owner_info method returning {} for bad username.

I found that there is no test coverage on the owner_info method.

I tried subject.owner_info which doesn't work because it is private method.

It is listed under hash delegator keys of owner.b, but I still got an error on trying this on existing cases.

    EXPECTED_ATTRIBUTES_ORG = {
      :owner_info       => 'some value',
      :login            => "jekyll",
    }

MichaelCurrin avatar Mar 28 '21 08:03 MichaelCurrin

Hi @MichaelCurrin and thanks for the interest.

According to the unit tests approach, a bad login gives false and then that evaluates to {}. It doesn't use the 404 data you provided.

Yes, that's the reason I opened this issues, because I do not believe that the result I got was the expected result.

I got that object by modifying the owner_info method:

p c.organization(owner_login)
#=> {:message=>"Not Found", :documentation_url=>"https://docs.github.com/rest/reference/orgs#get-an-organization"}

I also checked the whole condition:

p (c.organization(owner_login) || c.user(owner_login) || {})
#=> {:message=>"Not Found", :documentation_url=>"https://docs.github.com/rest/reference/orgs#get-an-organization"}

But since c.organization(owner_login) is checked first and its result didn't evaluate to false, I got the same result as above.

However, we migrated all our sites to another static site generator, including my own personal website, so I don't believe I would be able to help further, since we don't use Jekyll anymore.

In fact I was waiting for this to be closed as stale, due to the lack of interests of the gem developers (I'm well aware it's OSS).

AlfredoRamos avatar Mar 28 '21 18:03 AlfredoRamos

What was the owner_login you specified? When I specify an org that doesn't exist or is actually a User account, then I get false rather than {:message=>"Not Found",... like you got.

parkr avatar May 11 '22 03:05 parkr