WebUI search API. Closes #2495#8584
Conversation
glassez
added
WebUI
Piccirello
force-pushed
the
new-search-api-2
branch
2 times, most recently
from
ffad20c to
ca0248b
Compare
There was a problem hiding this comment.
When the user specifies "Skip checking", it means that qBittorrent will not check the files, but will begin to seed them, assuming they already exist. This comment means "it would be nice to check the files existence before".
Your changes is meaningless since qBittorrent creates save folder when add torrent to session.
There was a problem hiding this comment.
In the WebUI the user can specify whatever path they'd like. There's no guarantee that the path will actually exist- and that it isn't complete gibberish- so that's what this check is for. This has nothing to do with the "Skip checking" option.
There was a problem hiding this comment.
This has nothing to do with the "Skip checking" option.
Why did you delete this comment? It's just about "Skip checking" option.
There's no guarantee that the path will actually exist- and that it isn't complete gibberish- so that's what this check is for.
Then you should try to create it, but not check for its existence. As I said, it's allowed case when path doesn't exist before we add torrent.
There was a problem hiding this comment.
Then you should try to create it, but not check for its existence. As I said, it's allowed case when path doesn't exist before we add torrent.
I think you're describing the case where the last directory doesn't exist. I'm focusing on the issue where the user specifies a path /mnt1/Disk/Torrents/Downloads where /mnt1 doesn't exist. Surely in this case we shouldn't create that entire path, but throw an error.
There was a problem hiding this comment.
I don't want this commit to be the focus of this PR. If it's that contentious a change I'll gladly remove it.
There was a problem hiding this comment.
I don't want this commit to be the focus of this PR.
Me too. I just wanted to clear the space for the main review.
If it's that contentious a change I'll gladly remove it.
It is not as unambiguous as it may seem at first glance. It is better to move it in a separate PR.
I think you're describing the case where the last directory doesn't exist.
No, I'm describing common case. It should create save path even if it multilevel.
There was a problem hiding this comment.
It is not as unambiguous as it may seem at first glance. It is better to move it in a separate PR.
I'll move it to a separate PR.
No, I'm describing common case. It should create save path even if it multilevel.
In the case I described (/mnt1/Disk/Torrents/Downloads) would you expect qBittorrent to create all directories, or would you expect an error for /mnt1 not existing? I personally would expect the latter.
There was a problem hiding this comment.
I didn't say what I expected. I was talking about how it works now.
But, IMO, this behavior is quite expected, otherwise users (especially, remote users) will get a hell of a headache (it breaks some automations).
P.S. Let's continue this discussion in its own topic.
Piccirello
force-pushed
the
new-search-api-2
branch
4 times, most recently
from
d1da8cd to
5574420
Compare
There was a problem hiding this comment.
can you omit these?
If there are any messages should print out, it should go through Logger.
There was a problem hiding this comment.
"Plugin is not supported." without any particular information is quite meaningless message.
There was a problem hiding this comment.
If there are any messages should print out, it should go through Logger.
qDebug and Logger are both used pretty extensively in the codebase. Is the qDebug way of logging deprecated?
There was a problem hiding this comment.
Is the qDebug way of logging deprecated?
IMO yes, we don't need to distribute these code/messages unless we want users to help debug something, even so, we can provide a discrete build for them instead of using qDebug.
There was a problem hiding this comment.
I'll use the logger so that they'll be visible when the Log is added to the webui. I'll also do this for when plugin updates fail - though this is admittedly a poor solution.
There was a problem hiding this comment.
IMO, qDebug is still usefull for dev-eyes-only messages (e.g. to indicate some important execution points etc.). Also you free to use it as you want when you test some new code, but many of them are redundant in result and should be removed.
There was a problem hiding this comment.
const QJsonObject result = QJsonObject::fromVariantMap({ {"status", "Ok."}, {"id", id} });should be possible...
There was a problem hiding this comment.
Why do you need this intermediate object? I mean QVariantMap.
There was a problem hiding this comment.
should be possible...
I didn't know about this inline form. Thanks.
Why do you need this intermediate object? I mean QVariantMap.
Because I don't know of another way. I'm still very new to C++, so when pointing out that there are other ways, suggestions are welcome.
There was a problem hiding this comment.
suggestions are welcome
You should use QJson types directly, unless you already have data in different format.
QJsonObject has similar interface with QHash/QMap.
QJsonArray has similar interface with QList.
You can do either:
const QJsonObject result = {{"status", "Ok."}, {"id", id}};
setResult(result);or (if you don't need intermediate variable):
setResult(QJsonObject {{"status", "Ok."}, {"id", id}});So please don't create QVariantXXX types only to convert it to QJsonXXX types (everywhere in this code).
There was a problem hiding this comment.
I would omit curly braces here and other places.
There was a problem hiding this comment.
Please refactor, use a variable or two here.
There was a problem hiding this comment.
and use {} replacing QList<SearchResult>()
There was a problem hiding this comment.
QSet doesn't maintain order of its elements, which is required for the webui implementation of categories.
There was a problem hiding this comment.
Can you explain your plan? or just remove it.
There was a problem hiding this comment.
I haven't thought of a good way of handling this. Removing.
There was a problem hiding this comment.
Please use range-based for, this applies to all similar instances.
There was a problem hiding this comment.
I think this is wrong (in webapplication.cpp too), the do while() should be replaced by if()
Nevermind.
There was a problem hiding this comment.
searchResultsVariantList += QMap<QString, QVariant> {
{ "fileName", searchResult.fileName }
//...
};
// or alternative
const QMap<QString, QVariant> searchResultMap {
{ "fileName", searchResult.fileName }
//...
};
searchResultsVariantList << searchResultMap;There was a problem hiding this comment.
I get this error when compiling with += and <<
webui/api/searchcontroller.cpp:233:34: error: no match for ‘operator+=’ (operand types are ‘QVariantList {aka QList<QVariant>}’ and ‘<brace-enclosed initializer list>’)
There was a problem hiding this comment.
I get this error when compiling with += and <<
pardon, I updated the code above.
There was a problem hiding this comment.
pardon, I updated the code above.
As I requested before, it should be QJsonObject/QJsonArray here too.
There was a problem hiding this comment.
const QMap<QString, QVariant> resultMap = {
{"status", ((isSearchActive) || (queueSize > 0)) ? "Loading." : "Done."}
// ...
};
There was a problem hiding this comment.
const QList<SearchResult> &searchResults
There was a problem hiding this comment.
There was a problem hiding this comment.
base headers should go first, please move up
There was a problem hiding this comment.
QQueue inherits from QList, if you used QList m_stdoutQueue instead, you can just return m_stdoutQueue; below.
There was a problem hiding this comment.
Or if you can predict some upper bound of the size of m_stdoutQueue, you should use QVector + QVector::reserve()
There was a problem hiding this comment.
QQueue inherits from QList, if you used QList m_stdoutQueue instead, you can just return m_stdoutQueue; below.
I limit the number of results to 500 (arbitrary number) so that the response isn't too large. Though I'm switching to using QList::mid() for this.
There was a problem hiding this comment.
bool WebSearchHandler::isActive() const
|
Preliminary webapi documentation. This isn't finished yet and is missing fields SearchStart searchPOST /api/v2/search/start HTTP/1.1
Host: 127.0.0.1
Cookie: SID=your_sid
Content-Type: application/x-www-form-urlencoded
Content-Length: length
pattern=test&category=all&plugins=enabledStop searchPOST /api/v2/search/stop HTTP/1.1
Host: 127.0.0.1
Cookie: SID=your_sid
Content-Type: application/x-www-form-urlencoded
Content-Length: length
id=123456Get search statusGET /api/v2/search/status?id=123456 HTTP/1.1
Host: 127.0.0.1Get search resultsPOST /api/v2/search/results HTTP/1.1
Host: 127.0.0.1
Cookie: SID=your_sid
Content-Type: application/x-www-form-urlencoded
Content-Length: length
id=123456Delete searchPOST /api/v2/search/delete HTTP/1.1
Host: 127.0.0.1
Cookie: SID=your_sid
Content-Type: application/x-www-form-urlencoded
Content-Length: length
id=123456Get search categoriesGET /api/v2/search/categories?pluginName=legittorrents HTTP/1.1
Host: 127.0.0.1Get search pluginsGET /api/v2/search/plugins HTTP/1.1
Host: 127.0.0.1Install search pluginPOST /api/v2/search/installPlugin HTTP/1.1
Host: 127.0.0.1
Cookie: SID=your_sid
Content-Type: application/x-www-form-urlencoded
Content-Length: length
sources=https://url.to/pluginUninstall search pluginPOST /api/v2/search/uninstallPlugin HTTP/1.1
Host: 127.0.0.1
Cookie: SID=your_sid
Content-Type: application/x-www-form-urlencoded
Content-Length: length
names=legittorrentsEnable search pluginPOST /api/v2/search/enablePlugin HTTP/1.1
Host: 127.0.0.1
Cookie: SID=your_sid
Content-Type: application/x-www-form-urlencoded
Content-Length: length
names=legittorrents&enabled=trueUpdate search pluginsPOST /api/v2/search/updatePlugins HTTP/1.1
Host: 127.0.0.1
Cookie: SID=your_sid
Content-Type: application/x-www-form-urlencoded
Content-Length: length |
|
I never liked the (legacy) format of the Web API documentation. Why do you keep repeating all these HTTP headers etc.? It's so obvious, and therefore it is absolutely unnecessary here. Please consider RSS Web API documentation format. |
Well, blackmail is a powerful weapon! |
|
Thank you all! |
|
@glassez @Piccirello I quite like the new Web API documentation format, but wouldn't it be good to also specify the type of method in the description (GET, POST, ...)? I agree that the other headers are unnecessary. |
|
Currently both methods are allowed. |
|
@glassez So to be clear, the request type can be either GET or POST for all methods of the WebAPI? |
|
Yes. |
|
@glassez Thanks for the clarification. Perhaps that should be mentioned in the Wiki page for the Web API? I could edit that now and perhaps star converting the documentation of older methods into the new format. |
It would be nice! |
|
I’ve avoided documenting this because I actually think it’s something we should be more strict about, and something I’d like to enforce. Any non-idempotent methods should be a POST, and all others GET. |
|
@Piccirello I have started the conversion to the new documentation format. However, there are some things I don't know. For example, in the old documentation format, a lot of methods had examples of successful execution. In the new documentation format, there is an "HTTP status code/scenario" table instead of such examples. The problem is that I often did not know what to put in the "HTTP status code/Scenario" table, since I do not know what all methods do in all scenarios (and the old documentation format did not specify what happened in all scenarios), so I marked such entries as TODO. I preferred this approach rather than add a single entry to all with "200 | Success" since this way it is more obvious that it needs to be better documented. Can you take a look and fill those entries in, since you know the WebAPI much better? Just Ctrl+F TODO. Thanks. |
|
@FranciscoPombal I've filled in all HTTP status codes marked TODO. There's only one remaining TODO, which is JSON response info for I noticed there were several APIs erroneously documented as always returning I'm not sure if there's value in keeping the Thanks again for transitioning this info to the new format. I really think the updated format and status codes will be useful to people leveraging the qBittorrent Web API. |
|
It appears @glassez @Chocobo1 What are your thoughts on the best way forward? Should we just document this as-is? Ideally the three |
IIRC it has discussed before, personally I don't think it is wise to give out version number to everyone for free. So ideally getting version number would require auth when the legacy APIs are deprecated. |
I only copied the content that was there before. I would advise you to actually revise the document thoroughly after it is fully converted to the new format; other things may be wrong/missing. |
|
@FranciscoPombal Oh yeah, I do see that now. This documentation is seriously outdated. A mass cleanup is probably required to fix everything. |
|
@Chocobo1 Could there be benefit to exposing the API version/min without auth? If we ever change the login API, other apps won’t be able to determine which login flow to use without trying + catching failure. |
Other than letting strangers fingerprint you easily, none I am aware of.
I doubt we will easily do backward-incompatible changes to this part... |
|
@Piccirello this PR instatiates the SearchPluginManager at program start. This is a bad idea. |
Depending core Search feature from its UI it was legacy qBittorrent behavior. Now it doesn't fit since SearchPluginManager should be accessible from different places.
We should make it optional at all. I.e. we should allow to disable entire Search subsystem, not only its UI (search tab). |
There is some legacy (bad) code related to Search subsystem in qBittorrent. E.g. SearchPluginManager doesn't handle all the management and some other code parts do python related stuff directly. It should be really improved. |
IIRC, before this PR the subsystem was entirely disabled if the tab wasn't enabled. I am not against disengaging the connection between the subsystem and the UI tab. We could have it as a config option. But still an option. |
|
Opened #9889 to rectify this. Let's discuss the implementation over there. |
7 participants
Continuation of #8152