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

Fixed Transmission (and prob others) authentication. #9598

Merged
merged 1 commit into from
Jun 1, 2021

Conversation

p0psicles
Copy link
Contributor

@p0psicles p0psicles commented Jun 1, 2021

  • self.get_auth() was called just after self.url = None was set.
  • PR is based on the DEVELOP branch
  • Don't send big changes all at once. Split up big PRs into multiple smaller PRs that are easier to manage and review
  • Read the contribution guide

* self.get_auth() was called just after self.url = None was set.
@p0psicles p0psicles merged commit 6f67d98 into develop Jun 1, 2021
@p0psicles p0psicles deleted the feature/fix-transmission-auth branch June 1, 2021 07:09
@BenjV
Copy link
Contributor

BenjV commented Jun 4, 2021

@p0psicles
The self.url is filled in the init of the various torrent clients.
So if self.get_auth() is called the init of the clients will populate the self.url.

@p0psicles
Copy link
Contributor Author

Yeah, but the issue was the order.. get_auth() needs to url for some clients. And get_auth was called in the GenericClient init. And at that point URL wasn't initialized yet

@BenjV
Copy link
Contributor

BenjV commented Jun 4, 2021

If you call self.get_auth() in the _init of the generic.py it will call the get_auth() of the client module.
And then first of all the init of the client module is executed before the get_auth().
In that init the self.url is populated and so the get_auth() function gets a correctly populated self.url.

I tested it with Download Station and Transmission and both worked fine.

@p0psicles
Copy link
Contributor Author

It didn't work well with qbittorrent.

And then first of all the init of the client module is executed before the get_auth().

There is no implicit call to the client module init. And thats a good thing. Else you would get into a loop. As the client module init will again call the super() which will call the get_auth().. Etc.

Only thing I could do is move the get_auth back to generic. And put the super call behind the url = ...

@p0psicles
Copy link
Contributor Author

I think it also was an issue with rtorrent. As I use that myself. And the test_authentication was failing before this pr was merged.

@BenjV
Copy link
Contributor

BenjV commented Jun 4, 2021

The init of a class is always called before any of the functions within the class is called.
Because the class is called directly in medusa instead of creating an object with the class as template, every time a methode of that class is called the init of that class is first executed.
To prevent a loop I added the if statement, so if the self.url is filled it will not call the self_get_auth() again.

I looked at rtorrent and I saw the the self.url is not used nor filled at all in self._get_auth() so that can be problematic and create a loop.

So to prevent a loop for those clients you can add the self._get_auth() call to the send_torrent function of the generic.py to be sure the session is authorized.
Or make sure that in all clients the self.url is filled in the init.
Or you could add the self._get_auth() to the init of DownloadStationAPI in downloadstation.py

Because GenericClient() is not used to create an object with but called directly, non of the self variables nor the session header nor cookies are kept alive so before every call to a client the self._get_auth() must be called to create those fresh.

@p0psicles
Copy link
Contributor Author

I really don't know why you think we don't create objects. I've explained in this comment. #9482 (comment)

@BenjV
Copy link
Contributor

BenjV commented Jun 4, 2021

Because I tested it.
The class GenericClient() is used directly and not to create an object with.

Otherwise where is the object created?
There should be somewhere a statement like this:

GenericTorrentClientObject = GenericClient()

And a call to add a torrent should look like
GenericTorrentClientObject.send_torrent(result) and not like GenericClient.send_torrent(result)

Then at that moment the GenericTorrentClientObject is created from the class template and the init code is executed only once.
After that all the self variables from GenericTorrentClientObject are kept alive as part of the object.

But is you call the GenericClient() directly it only function as a set of methodes without keeping the self data alive.

If you don't believe me, just put a debug statement in the init of GenericClient and in the init of rtorrent and you will notice that when adding a torrent both init are execute every time and normal object behavior would be that the init is only executes at creation time of that object( so just once).

@medariox
Copy link
Contributor

medariox commented Jun 4, 2021

@BenjV
You can see that client = torrent.get_client_class(app.TORRENT_METHOD)() is indeed an object instance because of the ending ().

@BenjV
Copy link
Contributor

BenjV commented Jun 4, 2021

@medariox
Yes but it is created new every time a torrent is added, so it does not act as an object.

If you want to use it as an object somewhere in the application it must be create once and only once and not every time new because then all the object data is gone and you only have an object in name not in functionality.

For medusa it would be best to create the client object the moment the user chooses which the torrent client to use.
Then in the init of the object the authorization for that torrent client can be done and things like session headers, cookies and session data can be retained as wel as object data.
And if torrent clients needs a refresh on the session this can be done via the object data.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants