Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Credentials to_json method could have clearer documentation and API #494

Open
adelevie opened this issue May 4, 2020 · 4 comments
Open
Labels
type: docs Improvement to the documentation for an API.

Comments

@adelevie
Copy link

adelevie commented May 4, 2020

Is your feature request related to a problem? Please describe.

I need to serialize the Credentials data as JSON, and I just discovered the to_json method. It's very helpful, as the pickle-related techniques in the quickstart (e.g. https://developers.google.com/docs/api/quickstart/python) rely on the file system and binary data, which means the token cannot be persisted in an environment variable (useful for Heroku and other server deployments).

Anyways, the to_json method has the following in the doc string:

        Returns:
            str: A JSON representation of this instance, suitable to pass to
                 from_json().

I looked and looked for from_json() and couldn't find it in this library. If it does exist, I can't find it, and maybe should be easier to find. Assuming it doesn't exist, I found (mostly) what I needed in the class method from_authorized_user_info:

    if os.path.exists('token.json'):
        with open('token.json', 'r') as token:
            token_file = token.read()
            token_json = json.loads(token_file)
            creds = Credentials.from_authorized_user_info(token_json, scopes=SCOPES)
            creds.token = token_json['token']

That last line is used to ensure creds.valid returns True.

Describe the solution you'd like

The documentation and/or API for serializing/de-serializing as JSON should be clearer. If there is a from_json method, it should be mentioned somewhere in the docs. If there isn't, the doc string in to_json should specify the (class) method most suitable to ingest the serialized JSON. If that method happens to be from_authorized_user_info, it should ingest the token key as well, avoiding the need for the sort of hacky value set after instantiation/building.

Describe alternatives you've considered

The alternative I considered is the code sample for my workaround mentioned earlier in this issue.

Additional context

None I can think of.

@busunkim96 busunkim96 added the type: docs Improvement to the documentation for an API. label May 4, 2020
@alvyjudy
Copy link
Contributor

alvyjudy commented May 4, 2020

The from_authorized_user_file seems to provide that functionality. It reads a json file and returns a credential instance by calling from_authorized_user_info:

def from_authorized_user_file(cls, filename, scopes=None):
    """Creates a Credentials instance from an authorized user json file."""
    with io.open(filename, "r", encoding="utf-8") as json_file:
        data = json.load(json_file)
        return cls.from_authorized_user_info(data, scopes)

Is it a good idea to just replace from_json with from_authorized_user_file in the docstring?

@adelevie
Copy link
Author

adelevie commented May 4, 2020

Thanks for the quick note, @alvyjudy.

Is it a good idea to just replace from_json with from_authorized_user_file in the docstring?

I think that would help. Maybe also point to from_authorized_user_info. I haven't checked, but does from_authorized_user_file handle the setting of the creds.token param? On the other hand, if those class methods truly aren't supposed to set the token param, then it might be a bug if Credentials.valid requires the presence of the token to return True. I'm not super familiar with these internals, so I lack a good deal of context as to what everything is supposed to do...

@alvyjudy
Copy link
Contributor

alvyjudy commented May 4, 2020

Hi @adelevie, tbh I'm not an expert either. But token is not necessary in the construction of Credential instance from the json file. It is the refresh token that is necessary. As a result, the freshly constructed instance is invalid and refresh() method must be called to provide the credential.token parameter.

>>> cred = from_authorized_user_file("path/to/file.json")
>>> cred.valid
False
>>> cred.refresh(None)
>>> cred.valid
True

(I didn't test this but it is my understanding. Hope it isn't wrong)

This is a snippet that shows how from_authorized_user_info construct the instance

@classmethod
def from_authorized_user_info(cls, info): #info is the parsed mapping object from a json file
    return cls(
        None, #this is where the token would be but it is intentionally set to None
        refresh_token = info['refresh_token'],
    )

Side note on the refresh method, it provides a hook but is not used. So just set its to None.

gcf-merge-on-green bot pushed a commit that referenced this issue Jun 11, 2020
from #494, 

this PR updated the docstring of ``to_json`` method in the credential so that it points user to ``from_authorized_user_info`` instead of the non-existent ``from_json``
@vimota
Copy link

vimota commented Jul 15, 2020

An additional note on to_json - it doesn't serialize the "type" which the from_authorized_user_info relies on to read user auth (as opposed to service account auth) based credentials.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: docs Improvement to the documentation for an API.
Projects
None yet
Development

No branches or pull requests

4 participants