-
-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Add WebAPI for fetching torrent metadata #21015
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only preliminary comments after brief review.
@Piccirello In any case, this part of the API should be thought out, implying the subsequent addition of torrents. Otherwise, we may end up with something little useful in practice. |
I'm not convinced that's the approach we'll eventually take. I can imagine sending the .torrent file once, returning the metadata to the client, and then allowing the torrent to be added without needing to re-send the file (likely by transmitting the info hash).
To me the session seems like an appropriate place to store this. I don't think the client should be responsible for parsing this data. It would also mean each client (official and unofficial) would need to implement it.
I agree with this. I'll try to sketch out what the next steps would look like and how this API would be used. |
cec83e2
to
f857170
Compare
I think there is no need to embed the Bencode decoder directly into the client API. This is quite easy to do directly in the “graphical” part. More interesting is the need to provide the ability to assign priority to files in the |
1490741
to
f736e62
Compare
FYI, there certainly exists bencode encoder/decoder library in JS however I'm not aware that they are compatible with bittorrent v2. In the past, I had to mod one to suit my need. |
"bencode" format is independent from BitTorrent so generic "bencode" decoder should not depend on it too. Or do you refer to torrent file specific parsers? |
They had deficiencies in their implementations. Not fully conform with the spec. |
It seems to be the same problem as with bencode editors. I couldn't find BitTorrent independent editor for Linux. |
I ended up exploring how retrieved metadata would tie into the |
I suppose you're talking about percent-encoding, right? |
It would look much simpler and more convenient if "metadata" endpoint accepted only single source. |
This was my initial approach but I changed it because of how it will integrate with the Add Torrent dialogs in the WebUI. With proper documentation and status codes, I think this API will be easy for consumers/clients to understand. |
Then why do you suggest using it non-encoded?
Do "Add Torrent dialogs in the WebUI" really require API endpoint to allow accepting multiple sources at once?
If "metadata" accepts only single source then 202 status would definitely mean that the response does not contain metadata, and 200 - on the contrary, that the metadata is available in the response. |
My question is - is this JSON structure a standard or just some arbitrary decision? If it's arbitrary, why is one structure better than the other? AFAIK there is no standard for representing Bencode encoding in JSON. |
It makes no sense to compare the data structure (e.g. tree) and the way it is encoded (e.g. Bencode, JSON, XML). |
Yes, this is an arbitrary decision. But this decision is based on the analysis of existing torrent files. I assume that qBittorrent already uses this model because otherwise it uses custom decoding, which is generally wrong. |
Well, actually we don't want to fully reflect the structure of the .torrent file, as it is too low-level for our purposes. But it seems to me a good idea to separate the "info" section content from another fields, since this is a key aspect of the torrent metadata structure. (Just my opinion. I don't insist. This PR has more important problems.) |
@NikcN22 |
Yes. In general, it is not necessary to repeat the entire skeleton of the file. Some fields are completely useless for us. For example, the private field, I do not know what it can be used for. The private field is part of info.13 нояб. 2024 г., в 19:09, Vladimir Golovnev ***@***.***> написал(а):
@NikcN22
FYI, "private" is part of "info" section?
—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: ***@***.***>
|
I just mean, that it is part of "info" unlike your example where it is outside of it. |
The API response I'm now proposing is the following. It's based on the Torrent File spec, which Bencode Online seems to be based off of (cc @Chocobo1). Some notable deviations from the Torrent File spec, which I'm happy to revisit:
|
Just to clarify. That app doesn't care about "Torrent File" spec. You get what you feed. The app only tries its best to convert bencoded data to/from JSON. And for the normal user, the most common bencoded data is .torrent file. |
IMO, it is redundant. I would just use correctly ordered "files" list in order to have "natural" indexes. |
I will switch to this approach. |
fcb94a1
to
773b344
Compare
I've updated the API response to the following format:
|
@glassez this PR hasn't made progress in a while, and I would like to get it merged. What objections remain about its implementation? I'm hoping to focus primarily on the API's interface. As you noted earlier:
|
Signed-off-by: Thomas Piccirello <thomas@piccirello.com>
Signed-off-by: Thomas Piccirello <thomas@piccirello.com>
Signed-off-by: Thomas Piccirello <thomas@piccirello.com>
Signed-off-by: Thomas Piccirello <thomas@piccirello.com>
773b344
to
e544b32
Compare
@glassez I have some other PRs that bring massive performance improvements to the WebUI, but they're currently blocked on this PR. I would love to get this PR moving along. |
It is doubtful that "performance improvements to the WebUI" could be blocked on this PR. |
AFAIK there are really only two active devs with merge rights. I personally don't care who approves this, I just want it approved after six months of review. |
The objections are still the same. These are your "cache" classes. I can't move past them unless I close my eyes and pretend they don't exist at all. They look too obviously confusing to me. Here are some examples: metadataCache.add(infoHash, torrentDescr); // can actually replace existing item which is unexpected for method named as `add`.
metadataCache.get(infoHash); // can return `nullopt` which is obviously confusing since I just inserted it.
metadataCache.contains(infoHash); // can return `false` which is obviously confusing since I just inserted it. metadataCache.update(infoHash, torrentInfo);
// What is the status of this operation? Has the item been updated?
// We can't even check in advance if the item we're going to update exists
// because of the confusing behavior of `contains()`. |
torrentmetadatacache.h | ||
torrentsourcecache.h |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would move them to "api" subfolder.
#include "base/bittorrent/infohash.h" | ||
#include "base/bittorrent/torrentdescriptor.h" | ||
|
||
class TorrentMetadataCache : public QObject |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see any good reason for this class (and its satellite) to inherit from QObject.
public: | ||
using QObject::QObject; | ||
|
||
std::optional<BitTorrent::TorrentDescriptor> get(const BitTorrent::InfoHash &infoHash); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is unlikely that it can change the state of the class. So it should be constant:
std::optional<BitTorrent::TorrentDescriptor> get(const BitTorrent::InfoHash &infoHash); | |
std::optional<BitTorrent::TorrentDescriptor> get(const BitTorrent::InfoHash &infoHash) const; |
public: | ||
using QObject::QObject; | ||
|
||
std::optional<BitTorrent::InfoHash> get(const QString &source); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
std::optional<BitTorrent::InfoHash> get(const QString &source); | |
std::optional<BitTorrent::InfoHash> get(const QString &source) const; |
else if (const auto cachedInfoHash = m_torrentSourceCache.get(source)) | ||
infoHash = cachedInfoHash.value(); | ||
|
||
if (infoHash != BitTorrent::InfoHash {}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (infoHash != BitTorrent::InfoHash {}) | |
if (infoHash.isValid()) |
Everywhere.
I'll still review the rest of the code to see how these flaws affect it. |
This PR implements two new APIs for fetching a torrent's metadata. The APIs accept a magnet URI, torrent hash, .torrent URL, or uploaded .torrent file, and return the torrent's associated metadata. This PR also modifies the
/torrents/add
API to support downloading a torrent whose metadata has been previously fetched. The ultimate goal is for the WebUI to provide an Add Torrent experience equivalent to that of the GUI, where content can be reprioritized/unchecked before the torrent is added./fetchMetadata
APIHTTP request
To request metadata for a torrent, specify the torrent in the
source
query parameter of a GET request to/api/v2/torrents/fetchMetadata
. Torrents are supported in the following formats:magnet:?xt=urn:btih:a8eeefc8a0dc402b24686ddfd775a409fe4b00e0&dn=example
)a8eeefc8a0dc402b24686ddfd775a409fe4b00e0
)https://example.com/example.torrent
)HTTP response
Given the asynchronous nature of retrieving metadata, there are two successful HTTP status codes used.
When metadata is requested for a torrent that requires asynchronous background work (i.e. connecting to DHT/peers), the client will receive a 202. A 202 indicates that the request was successful, but additional background work must be completed before a meaningful response can be provided. The response will contain the info hashes, if available, or an empty object.
When metadata is available for the torrent, either because the torrent exists in the transfer list or because the metadata has been retrieved from a prior request, the client will receive a 200.
Retrieved metadata will be cached in the current web session. Subsequent requests performed within the same web session will return the metadata immediately, while other web sessions will be required to reretrieve the torrent's metadata from peers. Once a torrent is added using the cached metadata, the metadata is removed from the cache.
/parseMetadata
APIHTTP request
To request metadata for one to several .torrent file(s), you may upload the files to the
/parseMetadata
API. To do so, submit the file(s) as multipart MIME data. You may use any key for the uploaded value.To test file upload using curl, specify the
-F
flag (e.g.curl https://127.0.0.1:8080/api/v2/torrents/parseMetadata --get -F file=@"/root/example.torrent"
).HTTP response
This API always responds to successful requests (i.e. valid torrent file(s)) with a 200. The response will contain the full metadata for the uploaded torrent(s). The response object is keyed off of the uploaded file's name.
/add
APIThe existing
/add
API now supports using the metadata cache that's populated by the new metadata APIs. When specifying a url and/or torrent file to download, the metadata cache is first checked for the torrent. If found, the metadata is used directly from the cache, rather than needing to re-retrieve it. Note that when specifying the name of a .torrent file uploaded via theparseMetadata
API, you must prependfile:
to the file name. For example, if you uploadedexample.torrent
to theparseMetadata
API, you can add this torrent via the/add
API by specifying a url offile:example.torrent
.When metadata is retrieved directly from the cache, you may also specify a new
filePriorities
parameter. This parameter allows for specifying the file priority of each file in the torrent. This parameter may only be specified when adding a single torrent.Alternatives:
I explored having the metadata API leave the request open until the metadata was available. Once the metadata was fetched, it would be returned directly in the response of the original request. One downside of this approach is that metadata retrieval can take an arbitrary long amount of time. This could result in torrents whose metadata could never be retrieved via this API (e.g. due to the retrieval taking longer than the client's/reverse proxy's request timeout). This approach would also require some further modification of qBittorrent's web application layer to suppress the default behavior of returning a blank response.
Future work:
/fetchMetadata
API. This will likely mean splitting the current Add/Upload Torrent dialog into two dialogs. The first dialog will support specifying the URL(s)/.torrent file(s) to submit, while the second dialog will display the torrent's metadata and allow for modification of file priorities.Closes #20966.