Page MenuHomePhabricator

REST: Add 'from' and 'to' parameters to historycount endpoint.
Closed, ResolvedPublic2 Story Points

Description

Per user story T234940: Curator gets number of intermediate revisions between two revisions, we want the ability to retrieve the count of intermediate revisions between two revisions. This is primarily for use with the revision comparison endpoint, to know how many edits were involved in the differences being reviewed.

This can make use of RevisionStore::countRevisionsBetween()

T234940: Curator gets number of intermediate revisions between two revisions contains the necessary specifications and details.

Requirements:

  • functionality added
  • Add integration test covering behaviour
  • Review and, if necessary, make corrections to the endpoint reference docs

Event Timeline

BPirkle created this task.Oct 16 2019, 3:45 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptOct 16 2019, 3:45 PM
WDoranWMF triaged this task as Medium priority.Oct 16 2019, 6:25 PM
WDoranWMF set the point value for this task to 2.Oct 16 2019, 7:33 PM
Pchelolo claimed this task.Oct 16 2019, 7:59 PM
Pchelolo moved this task from Ready to Doing on the Core Platform Team Workboards (Green) board.

Change 543689 had a related patch set uploaded (by Ppchelko; owner: Ppchelko):
[mediawiki/core@master] Move countRevisionsBetween from Title to RevisionLookup

https://gerrit.wikimedia.org/r/543689

WDoranWMF updated the task description. (Show Details)Oct 16 2019, 9:41 PM
WDoranWMF updated the task description. (Show Details)

Change 543718 had a related patch set uploaded (by Ppchelko; owner: Ppchelko):
[mediawiki/core@master] REST: Implement /revision/{from}/intermediatecount/{to} endpoint.

https://gerrit.wikimedia.org/r/543718

apaskulin updated the task description. (Show Details)Oct 16 2019, 10:33 PM

Back in 2014 the following patch has added a limit to the Title::countRevisionsBetween method that we intend to use in the implementation here with a rationale that a query was causing DB timeouts. The query we're going to use is the same, thus I think it has the potential to keep timing out.

@eprodromou should we try to implement it without the limit (I don't think we should expect different result trying the same thing), or should we be prepared to specify that if the number of revisions between is more the 1000 (1000 is used by web UI, quite an arbitrary number), we will return something special? If so - what?

Change 543689 merged by jenkins-bot:
[mediawiki/core@master] Move countRevisionsBetween from Title to RevisionStore

https://gerrit.wikimedia.org/r/543689

Anomie added a subscriber: Anomie.Oct 18 2019, 2:43 PM

Back in 2014 the following patch has added a limit to the Title::countRevisionsBetween method that we intend to use in the implementation here with a rationale that a query was causing DB timeouts. The query we're going to use is the same, thus I think it has the potential to keep timing out.

We have SSDs now and faster machines, but that just raises the threshold for "too many" rather than eliminating it. I think a limit would still be a good thing.

we will return something special? If so - what?

If we don't care that it would mean 'count' is sometimes an integer and sometimes a string, I'd do something like "1000+" or ">1000". But we probably do care, in which case I don't have a good idea. We could make it an error case (but what code? 403?), or return a response without a 'count' at all, or just document that "1001" means "some number more than 1000".

BPirkle renamed this task from Implement GET /revision/{from}/intermediatecount/{to} to REST: Add 'from' and 'to' parameters to historycount endpoint..Oct 22 2019, 10:55 PM
BPirkle updated the task description. (Show Details)

@eprodromou should we try to implement it without the limit (I don't think we should expect different result trying the same thing), or should we be prepared to specify that if the number of revisions between is more the 1000 (1000 is used by web UI, quite an arbitrary number), we will return something special? If so - what?

A 500 error should be fine. I think it's pretty hard to make this happen with most UIs.

Change 545923 had a related patch set uploaded (by Will Doran; owner: Ppchelko):
[mediawiki/core@master] REST: Implement from and to for editors count handler.

https://gerrit.wikimedia.org/r/545923

Not sure if I should review https://gerrit.wikimedia.org/r/#/c/mediawiki/core/+/543718/

Are we adding a max? If so, are we doing the 500 error, or the "limit" in the response per https://phabricator.wikimedia.org/T235572#5621346 ?

yeah, @BPirkle I have decided to pause here too in anticipation of the decision what to do. I think we should apply a limit to all the count types and the limit should be the same and we should follow your design there.

@Pchelolo I don't think we can add a limit to all the *count endpoints before the train goes out on Tuesday. If you think we can, let's do it, and I'll modify the use cases. If not, let's capture it as separate tasks that we'll do this sprint or later.

@Pchelolo I don't think we can add a limit to all the *count endpoints before the train goes out on Tuesday. If you think we can, let's do it, and I'll modify the use cases. If not, let's capture it as separate tasks that we'll do this sprint or later.

We can add it to the rest of endpoints separately. What I care about is the eventual consistency.

Change 547682 had a related patch set uploaded (by Ppchelko; owner: Ppchelko):
[mediawiki/core@master] REST History Counts: Limit the number of edits to count

https://gerrit.wikimedia.org/r/547682

@Pchelolo I don't think we can add a limit to all the *count endpoints before the train goes out on Tuesday. If you think we can, let's do it, and I'll modify the use cases. If not, let's capture it as separate tasks that we'll do this sprint or later.

We can add it to the rest of endpoints separately. What I care about is the eventual consistency.

I think that's a great point. I'm going to add this as separate tasks for each of the "count" endpoints, so we can schedule them for later tasks.

for each of the "count" endpoints

PLEASE DON'T! :) Please make one task, this will be one code change, treating each of the counts separately will create the same mess as we've had for deprecation tickets. Too granular.

OK! I added a user story to the extended history epic, T237115.

Change 543718 merged by jenkins-bot:
[mediawiki/core@master] REST: Add 'from' and 'to' parameters to historycount endpoint.

https://gerrit.wikimedia.org/r/543718

eprodromou closed this task as Resolved.Nov 5 2019, 3:48 PM

Change 545923 merged by jenkins-bot:
[mediawiki/core@master] REST: Implement from and to for editors count handler.

https://gerrit.wikimedia.org/r/545923

Pchelolo reopened this task as Open.Nov 5 2019, 4:40 PM
Pchelolo removed Pchelolo as the assignee of this task.
Pchelolo added a subscriber: Pchelolo.

No. It's not done yet.

  • Add integration test covering behaviour
  • Review and, if necessary, make corrections to the endpoint reference docs
Pchelolo updated the task description. (Show Details)Nov 5 2019, 4:40 PM
apaskulin updated the task description. (Show Details)Nov 5 2019, 6:45 PM

Change 552321 had a related patch set uploaded (by Ppchelko; owner: Ppchelko):
[mediawiki/tools/api-testing@master] REST: Add tests for from/to parameters and cache challenges

https://gerrit.wikimedia.org/r/552321

Change 552321 merged by jenkins-bot:
[mediawiki/tools/api-testing@master] REST: Add tests for from/to parameters and cache challenges

https://gerrit.wikimedia.org/r/552321

Pchelolo updated the task description. (Show Details)
Pchelolo removed apaskulin as the assignee of this task.Thu, Nov 21, 9:44 PM
Pchelolo updated the task description. (Show Details)
Pchelolo moved this task from Ready to Done on the Core Platform Team Workboards (Green) board.
Pchelolo added a subscriber: apaskulin.
eprodromou closed this task as Resolved.Mon, Dec 2, 9:10 PM
eprodromou claimed this task.