Page MenuHomePhabricator

Interaction Timeline makes an API request to MediaWiki that returns a warning
Open, Needs TriagePublic

Description

On a query like:
https://tools.wmflabs.org/interaction-timeline/?wiki=testwiki&user=Test-apples&user=Test-bananas&startDate=1509926400&endDate=1510012799
clicking on "Kansas" on the left will make an API request to:
https://test.wikipedia.org/w/api.php?action=compare&fromrev=338497&torelative=prev&formatversion=2&format=json&origin=*&prop=diff|user|ids
that request returns this warning:

Revision 338497 is the earliest revision of Kansas, there is no revision for "torelative=prev" to compare to.

How should this warning be resolved and still provide the diff in the interface?

If this warning can be disregarded, perhaps it should be info instead of a warning?

Event Timeline

dbarratt created this task.Dec 19 2018, 3:36 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptDec 19 2018, 3:36 PM
dbarratt renamed this task from Interaction Timeline makes an API request to MediaWiki that returns an error to Interaction Timeline makes an API request to MediaWiki that returns a warning.Dec 19 2018, 3:36 PM
dbarratt updated the task description. (Show Details)Dec 19 2018, 3:44 PM
Anomie added a subscriber: Anomie.

Note that the normal web UI doesn't provide a diff in this situation either: https://test.wikipedia.org/wiki/Special:Diff/338497/prev produces

If you can't detect the first-revision situation beforehand and avoid making the API call in the first place, you're welcome to ignore this warning. If you set errorformat to anything except bc you can identify it by the "compare-no-prev" code on the warning.

SPoore added a subscriber: SPoore.Dec 19 2018, 4:35 PM

Note that the normal web UI doesn't provide a diff in this situation either

The web UI shows the entire revision, so the diff is not necessary. The InteractionTimeline has a different use-case.

you're welcome to ignore this warning

If warnings are safely ignorable, why are they warnings?

The fact this is an error doesn't make much sense here, since this is a common behavior that is expected. You reach the "edge" of a result set (either the end or the beginning) not as an error, but as a state.

Having this emit an error means we have errors in the console, which makes no sense, because that state is not actually erroneous. This forces us to bend over backwards in the code to double check whether the error is real or not before responding -- which is entirely the point of why this shouldn't be an error to begin with.

I do see the point of letting the requester know that they're in a state where they've reached the end of the set (either no previous or no forward results) but that can happen through an array of different other options that are not resulting in an error thrown, like adding an info parameter to the result, returning a null result on one end, or other options.

Can we please reconsider this error? It has actual effect on the request that doesn't really make much sense *as* an error, and while InteractionTimeline is the first to provide an example, it won't be the last to run into this issue, making users deal with an error when requesting valid data from the API.

Hi, everyone! I want to make sure that we get to the right solution on this problem.

As far as I can tell, calling the API to ask for a diff-to-prev on the first revision, or a diff-to-next on the last revision, is a request that doesn't have a well-defined answer.

Ideally, API clients should test each revision to see if it's the first or last and not ask for diffs that are poorly defined.

If a client is navigating through a list of revisions, this shouldn't be too hard to do.

But if the revision ID is being used without a list of revisions as a context, that might be harder. Is that the case we're seeing here?

But if the revision ID is being used without a list of revisions as a context, that might be harder. Is that the case we're seeing here?

That's correct, we get a list of revisions on pages where both users have edited.
https://tools.wmflabs.org/interaction-timeline/api/testwiki/interaction?user=Test-apples|Test-bananas&start_date=1509926400&end_date=1510012799

The revisions are not sequential and the UI does not discriminate the first one from a middle one as it is always showing the diff of the "previous" one (not knowing what the previous revision is).

Just like in Git, if it's the first revision, it would be empty on the left and show all of the changes on the right (as it does right now).

Since the previous revision is not known, and it is not known if it is the first revision or not, it would be nice to just be able to get the diff back (like we do now). But now the API throws a warning, indicating that something needs to be changed to avoid the warning.

I suppose we could update our endpoint to indicate the parent revision id (and therefor we would know if it's the first revision or not). But that doesn't resolve how we would display an empty diff without a warning (unless you can pass in 0 to the API?)

@Anomie @EvanProdromou - It sounds like the use case here (from a high level) is that we would like to be able to retrieve from the API the substance of an arbitrary edit regardless of whether it is the first edit to an article or not. Can the API support this use case (either now or in the future)?

EvanProdromou added a comment.EditedDec 20 2018, 2:56 PM

@kaldari I think the main thing we want to avoid is getting ourselves into a case where different clients expect different output for the same corner case. Since we store full revisions and not deltas, we typically ask for the different between two revisions. That's usually not a big deal, unless we're in the case we see here.

I'll talk to @Anomie today and see if we can make this case work. I'm also taking it as a task to document the correct [behaviour] on https://www.mediawiki.org/wiki/API:Compare.

@kaldari I think the main thing we want to avoid is getting ourselves into a case where different clients expect different output for the same corner case. Since we store full revisions and not deltas, we typically ask for the different between two revisions. That's usually not a big deal, unless we're in the case we see here.

I'll talk to @Anomie today and see if we can make this case work. I'm also taking it as a task to document the correct [behaviour] on https://www.mediawiki.org/wiki/API:Compare.

That's fair.

It's worth noting, though, that this is a somewhat new behavior in that aspect. I don't know of any other cases in the API where a client expects an error for a state that is technically valid (so, expecting the error to give you information rather than telling you something is actually wrong) -- so, if our goal is to come up with consistency, I'd offer that this is a good time we find a consistent way to get that kind of information out without necessarily having to use a warning, which has secondary effects that usually require the consumer to work around them.

@EvanProdromou for some background, this was working before rMW1ab2f7a56ba5d1e4b613107d3e2991bbc36ba68e. That change caused the InteractionTimeline to throw an error T207658 and that was changed to a warning in T203433.

If the warning is the intended behavior, that's fine, but I think warnings imply that something can be changed in order to resolve the warning. If the warning can be safely ignored then it's just noise.

So, I've been discussing with @Anomie and here's where we are at:

  1. I've documented the current behaviour at https://www.mediawiki.org/wiki/API:Compare#Relative_comparison_at_first_and_last_revision . Please note the caveats, especially around default content.
  2. We're going to remove the warning for the current release.
  3. @Anomie is pretty adamant that it's a developer error to ask for the prev of the first revision or the next of the last revision, so we are going to start a ticket for deprecating this behaviour in this call. If we go forward with that, it will mean a deprecation warning initially, followed by an error in the future. I'll link the ticket here.

I've started a ticket for deprecating this behaviour here https://phabricator.wikimedia.org/T212511