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

Fix typerror when offline #27

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

yuuki0xff
Copy link
Contributor

Simplenote.authenticate() and get_token() usually return a str. When offline, these methods return None. Some methods do not consider for this behavior. Therefore, None will be set to http header, and TypeError will occur when sending a request.

Related issue: cpbotha/nvpy#191

BROKEN CHANGE:
authenticate() and get_token() no longer catch the IOError, BadStatusLine and part of HTTPError errors.

Simplenote.authenticate() and get_token() usually return a str.  When
offline, these methods return None.  The get_note(), update_note(), etc.
do not consider for this behavior.  Therefore, None will be set to http
header, and TypeError will occur when sending a request.

Related: cpbotha/nvpy#191

BROKEN CHANGE:

authenticate() and get_token() no longer catch the IOError,
BadStatusLine and part of HTTPError errors.
@atomicules
Copy link
Collaborator

Hi,

thanks. This is required in addition to #24 then and the fix that went in there?

I will try to look soon.

To be honest I don't think I've ever thought or cared about the offline situation. Until now...

@atomicules atomicules self-assigned this Oct 4, 2019
@yuuki0xff
Copy link
Contributor Author

#24 and #27 fixes different bugs. #24 fixed a bug that occurred in the following situation:

  1. Get token when online.
  2. Get/update/delete note when offline. It will be raise HTTPException before v2.1.4.

#27 (this pull request) fixes bug that occurred in the following situation:

  1. Get/update/delete note when offline and it have not token. It will be raise TypeError.

The error handling style of this library is golang style instead of try-catch style (standard of the python). However, it is not good to throw an exception under certain circumstances.

@atomicules
Copy link
Collaborator

Hey... sorry for the delay. Just to let you know I am looking at this and will get merged in soon.

@atomicules
Copy link
Collaborator

Ok, just one question...

we are now importing HTTPException, but not actually using it anywhere. Are these meant to be HTTPExceptions?

        try:
            token = self.get_token()
        except Exception as e:
            return e, -1

@atomicules
Copy link
Collaborator

Been trying to test this, but it's actually pretty fiddly for me since I mostly work remotely... so can't really disable the network there 😄

Testing locally (also a pain as on wired desktop) with this branch OR master I get:

>>> import simplenote
>>> sn = simplenote.Simplenote("USER", "PASS")
>>> stuff = sn.get_note_list()
>>> stuff
(URLError(gaierror(8, 'hostname nor servname provided, or not known')), -1)
>>> stuff = sn.get_note("11554a046a65651984a2e262f730ab61")
>>> stuff
(URLError(gaierror(8, 'hostname nor servname provided, or not known')), -1)

I.e. I'm not really seeing any difference there. When performing get_token then:

master, offline

>>> thing = sn.get_token()
>>> thing
>>> thing is None
True

this pr, offline

>>> thing = sn.get_token()
Traceback (most recent call last):
  File "/usr/pkg/lib/python3.7/urllib/request.py", line 1317, in do_open
    encode_chunked=req.has_header('Transfer-encoding'))
  File "/usr/pkg/lib/python3.7/http/client.py", line 1229, in request
    self._send_request(method, url, body, headers, encode_chunked)
  File "/usr/pkg/lib/python3.7/http/client.py", line 1275, in _send_request
    self.endheaders(body, encode_chunked=encode_chunked)
  File "/usr/pkg/lib/python3.7/http/client.py", line 1224, in endheaders
    self._send_output(message_body, encode_chunked=encode_chunked)
  File "/usr/pkg/lib/python3.7/http/client.py", line 1016, in _send_output
    self.send(msg)
  File "/usr/pkg/lib/python3.7/http/client.py", line 956, in send
    self.connect()
  File "/usr/pkg/lib/python3.7/http/client.py", line 1384, in connect
    super().connect()
  File "/usr/pkg/lib/python3.7/http/client.py", line 928, in connect
    (self.host,self.port), self.timeout, self.source_address)
  File "/usr/pkg/lib/python3.7/socket.py", line 707, in create_connection
    for res in getaddrinfo(host, port, 0, SOCK_STREAM):
  File "/usr/pkg/lib/python3.7/socket.py", line 748, in getaddrinfo
    for res in _socket.getaddrinfo(host, port, family, type, proto, flags):
socket.gaierror: [Errno 8] hostname nor servname provided, or not known

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/atomicules/Code/github/simplenote-vim/simplenote.py/simplenote/simplenote.py", line 106, in get_token
    self.token = self.authenticate(self.username, self.password)
  File "/home/atomicules/Code/github/simplenote-vim/simplenote.py/simplenote/simplenote.py", line 83, in authenticate
    res = urllib2.urlopen(request).read()
  File "/usr/pkg/lib/python3.7/urllib/request.py", line 222, in urlopen
    return opener.open(url, data, timeout)
  File "/usr/pkg/lib/python3.7/urllib/request.py", line 525, in open
    response = self._open(req, data)
  File "/usr/pkg/lib/python3.7/urllib/request.py", line 543, in _open
    '_open', req)
  File "/usr/pkg/lib/python3.7/urllib/request.py", line 503, in _call_chain
    result = func(*args)
  File "/usr/pkg/lib/python3.7/urllib/request.py", line 1360, in https_open
    context=self._context, check_hostname=self._check_hostname)
  File "/usr/pkg/lib/python3.7/urllib/request.py", line 1319, in do_open
    raise URLError(err)
urllib.error.URLError: <urlopen error [Errno 8] hostname nor servname provided, or not known>

I am not really understanding the benefit of the PR.

However...

I am going to continue looking to see how to better handle errors when offline so we can get something we are both happy with.

@yuuki0xff
Copy link
Contributor Author

I'm sorry for the late reply.

we are now importing HTTPException, but not actually using it anywhere. Are these meant to be HTTPExceptions?

  try:
       token = self.get_token()
   except Exception as e:
       return e, -1

No. get_token() will raise some exceptions as written in the documentation. I used Exception to handle those exceptions.

The purpose of editing authenticate() and get_token() was to tell application the root cause of the failure. Considering the impact to other apps that use this library, I should consider other approaches. Please give me a little time.

Been trying to test this, but it's actually pretty fiddly for me since I mostly work remotely... so can't really disable the network there

If you can use docker, you can easily create offline environment as follows. :-)

$ docker run --rm -it --net none -v /path/to/git/repository/of/simplenote.py/:/work python:3 bash
# cd /work
# pip install -e .
# python
>>> import simplenote
>>> sn = simplenote.Simplenote(user, pw)
>>> sn.get_note(key)

@yuuki0xff
Copy link
Contributor Author

How about this?

yuuki0xff@aead4f5

@atomicules
Copy link
Collaborator

I'm sorry for the late reply.

No problem.

If you can use docker

I'm on NetBSD so unfortunately not an option. I can test on my desktop (also NetBSD) by taking interface up and down, it's just then I lose my remote session and other things, etc. It's just finding a convenient time, etc.

How about this?

Will take a look over the weekend. Thank you.

@atomicules
Copy link
Collaborator

Will take a look over the weekend

Didn't happen, sorry. I got busy again. Still meaning to look at this again. Hopefully soon.

@atomicules
Copy link
Collaborator

This is still on my todo list... it just moves to the next day everyday 😞 . Worst comes to the worst it's Christmas soon and I have a couple of weeks off and some so time free.

atomicules added a commit that referenced this pull request Mar 2, 2020
I can't recall the status of this, etc.

See: #27
@atomicules
Copy link
Collaborator

After much delay I'm going to have to put this as a "won't fix/merge". That's based on it's current form and what I know so far.

That doesn't mean I won't consider another PR or an update to this PR , etc, but someone else is going to have to put in the work for testing and then convincing me it won't break anything else.

I've been trying to get to this for months and testing something that requires being offline is just too impractical for me:

  • My main development machine is a remote instance
  • I do have a local desktop (obviously), but when I'm sat in front of it it generally means I'm working or doing something else that requires a network connection
  • I just cannot find a good time to test, etc something that requires being offline

I've pushed up the changes I was thinking about in case it's of use to anyone; but it's been awhile since I was looking at this and I can't recall the state of them, etc; Doesn't look like I did much at all unfortunately.

@atomicules atomicules removed their assignment Mar 2, 2020
@atomicules atomicules added enhancement harder up for grabs Maintainer has no plans to work themselves. Feel free to tackle. on hold labels Mar 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement harder on hold up for grabs Maintainer has no plans to work themselves. Feel free to tackle.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants