-
-
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
WebAPI: Do not wrap result if offset is invalid #22174
base: master
Are you sure you want to change the base?
WebAPI: Do not wrap result if offset is invalid #22174
Conversation
@@ -387,7 +387,7 @@ void TorrentsController::infoAction() | |||
if (offset < 0) | |||
offset = size + offset; | |||
if ((offset >= size) || (offset < 0)) | |||
offset = 0; | |||
throw APIError(APIErrorType::Conflict, tr("Offset is out of range")); |
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.
Maybe just remove (offset >= size)
conditional and let it become this:
if (offset < 0)
offset = 0;
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.
Sooo ... leave it unhandled ? Because I see that it was handled in the SearchController
And it also handles both conditions, just split into two different if
-s
offset >= size (maybe I should replace >=
with >
, 🤔 )
offset < 0
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.
offset >= size (maybe I should replace
>=
with>
, 🤔 )
offset == size
is out-of-bound.
I wonder why SearchController
allows such a case. Maybe it's for the caller to be able to uniformly identify the case of the end of the available elements, regardless of whether size is a multiple of the offset or not.
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.
offset < 0
SearchController
allows you to specify negative offset which is treated as offset from the end.
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 behaves as you say if offset
's positive value is less than size, as in this example.
// normalize values
if (offset < 0)
offset = size + offset; // size = 10; offset = -5
if (offset < 0) // offset is now 5
throw APIError(APIErrorType::Conflict, tr("Offset is out of range"));
If offset
's positive/absolute value is larger, it throws out of bound, as in this example.
// normalize values
if (offset < 0)
offset = size + offset; // size = 5, offset = -10
if (offset < 0) // offset is now -5
throw APIError(APIErrorType::Conflict, tr("Offset is out of range"));
So it does handle offset < 0
after applying it from the end, in order not to loop.
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.
So it does handle offset \u003C 0 after applying it from the end, in order not to loop.
Sure. I did not claim otherwise. I just wanted to point out such a widely used practice (negative offset as offset from the end).
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.
But if this endpoint don't support negative offset then I would use BadParams error since client know it in beforehand (unlike in case when offset is greater than size which is valid Conflict error).
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.
unlike in case when offset is greater than size which is valid Conflict error
It is? The final torrent list size is unlikely to be known when there are filters applied. It doesn't seem reasonable to make it an error in this case. If it is over-bound then returning an empty list would be enough IMO (as suggested in the linked issue #22158).
Sooo ... leave it unhandled ?
It is not entirely unhandled. It will still affect the result of torrentList.mid(offset, limit);
call.
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? The final torrent list size is unlikely to be known when there are filters applied. It doesn't seem reasonable to make it an error in this case.
My main thought was that offset < 0
and offset > size
are different (error) cases. One is predictable while the second one not.
Closes #22158.
Description
On requests to
/api/v2/torrents/info
endpoint with anoffset
larger than the number of torrents, theoffset
defaults to0
and returns results when its expected to return an error informing that this offset is out of range.