breaks with older github enterprise versions #794

electrofelix · 2018-03-15T15:45:12Z

Typically Github Enterprise is lagging keywords in the responses that are seen from github.com. Currently seeing the following error on trying to use this library with a local GitHub Enterprise instance.

IncompleteResponse [The library was expecting more data in the response (KeyError(u'license',)). Either GitHub modified it's response body, or your token is not properly scoped to retrieve this information.]

Thrown by a call to (github3.enterprise_login()).repository()

Patching the library to deal with this issue only results in seeing another issue:

IncompleteResponse: None The library was expecting more data in the response (KeyError(u'plan',)). Either GitHub modified it's response body, or your token is not properly scoped to retrieve this information.

Thrown by a call to (github3.enterprise_login()).me()

In both cases my token had full access, just that the public Github is returning more than the private versions.

One solution would be to update

github3.py/github3/models.py

Lines 160 to 163 in bf44c16

    
           try: 
        
               ret = response.json() 
        
           except ValueError: 
        
               raise exceptions.UnexpectedResponse(response)

to use something like the following:

        ret = collections.defaultdict(lambda: None)
        try:
            ret.update(response.json())
        except ValueError:
            raise exceptions.UnexpectedResponse(response)

But that would also require updating locations such as

github3.py/github3/users.py

Line 757 in bf44c16

self.plan = Plan(user['plan'], self)

to check if the value was not None before trying to use as an argument to create an object:

        if user['plan'] is not None:
            self.plan = Plan(user['plan'], self)
        else:
            self.plan = None

That would mean looking through each definition of _update_attributes() and modifying to test for None where the result of dictionary lookup was used to create an object.

This would certainly allow the library to work across a wide range of Github Enterprise releases, as well as simply returning None for certain data items when users scope tokens to have less permissions.

If this seems reasonable I can put together a PR to do the needful.

The text was updated successfully, but these errors were encountered:

sigmavirus24 · 2018-03-16T03:00:47Z

I was worried this would be the case. The problem we have, though, is that we're trying to provide a consistent interface for people. Previously an object would have None but None could have meant that the attribute wasn't present or that it was set to null and there was no way to distinguish between either. We tried experimenting with an Empty singleton but that proved incredibly confusing for users.

The worst part of this is that, as I understand it, there's nothing that would force a GitHubEnterprise to ever upgrade so we could potentially be supporting ancient versions for ever and that's very unappealing given the lack of funding by the corporate Enterprise users.

It's entirely plausible that supporting GitHubEnterprise will have to end.

An alternate would be to have a separate set of "Enterprise" objects that don't attempt to use attributes we know aren't there in the latest version. Sadly, however, there's 0 documentation for this.

electrofelix · 2018-03-16T09:14:06Z

I should point out that the Github Enterprise instance I'm working with is 2.12 and was last updated this year. The problem is that new features added to the public one will take a few months to appear in even the most uptodate on premise installation.

However I think this goes beyond just the problem of dealing with older Github Enterprise instances, the current behaviour appears to prevent use of limited scope tokens because the error returned complains about not having sufficient scope. So any time a token does not have scope for information that results in a response will less information than expected, the user gets an exception.

When you talk about a singleton, you mean something like github3.Github.NotSet = object(), where any case where something was 'null' in the json response it is set to github3.Github.NotSet.

I guess the underlying question is, whether to support limited scope tokens or not? Because I believe that any solution for that, will also solve the issue for anyone using enterprise instances.

guykisel · 2018-03-16T22:34:27Z

If the github API fundamentally changes in a backwards-incompatible way, I think it's reasonable to stop supporting the old version of an API. However, if the github API just adds some new fields (such as license data) then I think the API wrapper should handle missing fields gracefully instead of just erroring.

As @electrofelix noted, GitHub Enterprise usually lags behind public GitHub releases by at least several months, so this will always be an issue.

In the meantime I just pinned my tool internally to github3.py==0.9.6 so at least there's an easy workaround :)

electrofelix · 2018-03-17T10:12:23Z

Had some time to think about this and I think the solution might be to add a strict parameter defaulted to True to the GitHubCore object at

github3.py/github3/models.py

Lines 28 to 33 in bf44c16

    
               def __init__(self, json, session): 
        
                   """Initialize our basic object. 
        
                   Pretty much every object will pass in decoded JSON and a Session. 
        
                   """

This would be used to control at

github3.py/github3/models.py

Lines 160 to 163 in bf44c16

    
           try: 
        
               ret = response.json() 
        
           except ValueError: 
        
               raise exceptions.UnexpectedResponse(response)

whether the decoded json is returned as a normal dictionary or one where missing settings return a github3.github.Empty object.

This would allow most users to continue without needing to know about github3.github.Empty, and those that are coding for Github Enterprise or looking to handle restricted scoped tokens can enable the strict=False behaviour. In such cases they would then need to understand the differences between github3.github.Empty and None.

This would still mean in a few places the code in this library would need to test for github3.github.Empty before trying to create an object such as

github3.py/github3/users.py

Line 757 in bf44c16

self.plan = Plan(user['plan'], self)

Might need to think a little on how to ensure strict it can be set on the initial connection, and then retained for any subsequent calls instead of needing to be set each time.

Does that strike the right balance between avoiding confusion for majority of users by not exposing such behaviour while ensuring those that may want to handle the more complex scenarios are also catered for?

omgjlk · 2018-03-19T22:57:03Z

Some thoughts from a library user that needs to support both GHE and public GitHub. I think, given the nature of GitHub APIs and the fact that there can be N installs of Y versions of GitHub Enterprise out there, that we shouldn't be TOO strict on having keys populated with real data. I think it's reasonable that we can add the keys when Public API adds them, but allow the keys to be populated with None when the API we're talking to (like GHE installs) does not return a value. It DOES add some ambiguity to the response, but I think that's just a price that will be paid for supporting GHE at all.

It's unfortunate that Python doesn't really have a differentiation between "This value is not set" and "The value is empty/None", but I don't think it's worth inventing a new object to stuff in there to manufacture a difference.

I also think that introducing a strict option is overkill here too. I really think we just implement populating that data with None when we don't get a response and adding commentary around it in the source code to clarify what's happening, as well as some documentation that calls out GitHub Enterprise and the lagging API changes.

sigmavirus24 · 2018-03-19T23:23:19Z

@omgjlk I think that's the most reasonable route forward. I'd like us to make only a select few attributes optional, however, to avoid ambiguity. There must be some subset of attributes that we can reliably present to the user. Either way this mess seems unavoidable, sadly.

Without a whole lot of experimentation and time that I simply do not have, this won't get done. As people can find and pick off areas where we need to be less stringent, I'll happily merge and release PRs. @omgjlk can release things too at this point, so hopefully that should help with the cadence here.

omgjlk · 2018-03-20T00:13:30Z

Thanks @sigmavirus24 . I'll see if I can get access to some GHE installs to test against, but in the mean time I think we've got some data to move forward. I'll work on some patches in the next day or two.

electrofelix · 2018-03-20T19:36:46Z

@sigmavirus24 Seems you are favouring being explicit as to which attributes are optional, so I'm wondering if altering the code at https://github.com/sigmavirus24/github3.py/blob/bf44c16510be84db7ec21cd7b2ae791626dc488c/github3/repos/repo.py#L2450-L245 to look like the following:

        self.original_license = repo.get('license', None)
        if self.original_license is not None:
            self.original_license = licenses.ShortLicense(
                self.original_license, self
)

Is reasonable?

Seems to align with the approach taken with the following blocks:

github3.py/github3/repos/repo.py

Lines 2458 to 2460 in bf44c16

    
           self.parent = repo.get('parent', None) 
        
           if self.parent is not None: 
        
               self.parent = ShortRepository(self.parent, self)

and

github3.py/github3/repos/repo.py

Lines 2463 to 2465 in bf44c16

    
           self.source = repo.get('source', None) 
        
           if self.source is not None: 
        
               self.source = ShortRepository(self.source, self)

castillr · 2018-03-20T19:50:29Z

Just another note.

github3.exceptions.IncompleteResponse: None The library was expecting more data in the response (KeyError(u'archived',)). Either GitHub modified it's response body, or your token is not properly scoped to retrieve this information.

We are running 2.11 so the archiving feature is missing. Does anyone know where the expected responses from GitHub Enterprise v3 are documented? I haven't been able to find it.

Back to v1.0.0a4 for me!

omgjlk · 2018-03-20T21:26:31Z

The documentation would be at https://developer.github.com/enterprise/2.11/v3/

sigmavirus24 · 2018-03-20T23:40:09Z

Just to clarify, I expect any attribute that is being made optional to have that reflected in the documentation. So for @electrofelix's example, I expect a corresponding change to the documentation that looks like:

.. attribute:: original_license

    This is the :class:`~github3.license.ShortLicense` returned as part of
    the repository. To retrieve the most recent license, see the
    :meth:`~github3.repos.repo.Repository.license` method.

Perhaps something like:

.. attribute:: original_license

    .. note::

        Under circumstances X, Y, and Z, this attribute will not be returned.
        To handle these situations sensitively, the attribute will be set to ``None``.
        Repositories may have a license associated with them in these cases.

    This is the :class:`~github3.license.ShortLicense` returned as part of
    the repository. To retrieve the most recent license, see the
    :meth:`~github3.repos.repo.Repository.license` method.

castillr · 2018-03-20T23:48:14Z

Interesting, https://developer.github.com/enterprise/2.11/v3/repos/#get states that "archived" is an expected key in the response. Don't know why our deployment isn't including it.

Github Enterprise instances <= 2.12.7 do not include the license attribute in responses for repository data. Allow this to be not set and include a note in the documentation that the corresponding attr may be set to None. Partially fixes sigmavirus24#794

Github Enterprise instances <= 2.12.7 do not include the license attribute in responses for repository data. Allow this to be not set and include a note in the documentation that the corresponding attr may be set to None. Partially B494 fixes sigmavirus24#794

Github Enterprise instances <= 2.12.7 do not include the license attribute in responses for repository data. Allow this to be not set and include a note in the documentation that the corresponding attr may be set to None. Partially fixes sigmavirus24#794

Github Enterprise instances <= 2.12.7 do not include the `plan` attribute in responses for authenticated user data. Allow this to be not set and include a note in the documentation that the corresponding attr may be set to `None`. Partially fixes sigmavirus24#794

guykisel mentioned this issue Mar 16, 2018

initializing a repository object fails on github enterprise #796

Closed

electrofelix mentioned this issue Mar 21, 2018

Make license attribute in response optional #804

Merged

sigmavirus24 closed this as completed in #804 Mar 24, 2018

electrofelix mentioned this issue Mar 26, 2018

Make plan attribute in authenticated user response optional #813

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

breaks with older github enterprise versions #794

breaks with older github enterprise versions #794

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

breaks with older github enterprise versions #794

breaks with older github enterprise versions #794

Comments

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!