1114843 - Pushlog should return an error if the startID push doesn't exist, and ideally be consistent with &fromchange=INVALID-REV

Status: REOPENED

(Developer Services :: Mercurial: Pushlog, defect)

Description

[Ed Morley [:emorley]]

Broken out from bug 1065771, which morphed along the way.

Comment 1

[Ed Morley [:emorley]]

Example:

https://hg.mozilla.org/mozilla-central/json-pushes?version=2&startID=999999999

Expected:
HTTP 404
"unknown push ID '999999999'"

Or (but I suspect this may not be simple):
HTTP 404
{
 "lastpushid": 28005, 
 "pushes": {}
}

Actual:
HTTP 200
{
 "lastpushid": 28005, 
 "pushes": {}
}

Comment 2

[Ed Morley [:emorley]]

Attachment: pushlog: return HTTP 404 if the startID push doesn't exist

Comment 3

[Gregory Szorc [:gps]]

Comment on attachment 8540485 [details] [diff] [review]
pushlog: return HTTP 404 if the startID push doesn't exist

Review of attachment 8540485 [details] [diff] [review]:
-----------------------------------------------------------------

Changing this is backwards incompatible and scares me a little. You speak for for TreeHerder. But I also need to hear back from everyone else consuming pushlog.

It would be much safer if we could change the behavior only for version=2 requests (which I plan to make the default once all consumers not passing version=* disappear).

Furthermore, I'm not a huge fan of a single string as the response. I know this is valid JSON. But we like objects. Let's add an "error" property to the version 2 object and remove or make empty the "pushes" entry.

Is this proposal acceptable?

Comment 4

[Ed Morley [:emorley]]

(In reply to Gregory Szorc [:gps] from comment #3)
> It would be much safer if we could change the behavior only for version=2
> requests (which I plan to make the default once all consumers not passing
> version=* disappear).

wfm :-)
 
> Furthermore, I'm not a huge fan of a single string as the response. I know
> this is valid JSON. But we like objects. Let's add an "error" property to
> the version 2 object and remove or make empty the "pushes" entry.
> 
> Is this proposal acceptable?

Yeah agree this is preferable (I almost added an "error" property to my "Or...:" example in comment 0), I was just less sure how to get a 404 without using ErrorResponse(), so went with the inferior option due to not wanting to spend too much more time on this. Would you mind driving the fix using this preferable approach? :-)

Comment 5

[Gregory Szorc [:gps]]

Yes, I can implement this.

Comment 6

[Gregory Szorc [:gps]]

Attachment: MozReview Request: bz://1114843/gps

Comment 7

[Gregory Szorc [:gps]]

/r/1715 - pushlog: return an error if startID is too large (bug 1114843)

Pull down this commit:

hg pull review -r 4fb4e6f2f3580f433eac1f012900894804041985

Comment 8

[Ed Morley [:emorley]]

https://reviewboard.mozilla.org/r/1713/#review1103

Ah of course, I was overthinking it, the answer is to still use ErrorResponse(). Thanks for taking a look at this :-)

r=me with s/200/404/

::: hgext/pushlog-legacy/pushlog-feed.py
(Diff revision 1)
> +                    raiseHTTPJSONError(200, 'PUSH_ID_GREATER_THAN_AVAILABLE',

s/200/404/ right? :-)

::: hgext/pushlog-legacy/tests/test-hgweb.t
(Diff revision 1)
> +  200

And here

Comment 9

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/1713/#review1105

> s/200/404/ right? :-)

I used to be a HTTP/REST zealot. I once would have strongly argued for 404 here: "the thing I'm requesting doesn't exist, so 404 Not Found is appropriate." However, I no longer hold that view.

`json-pushes` is an RPC-style interface. The base URL/resource here is `json-pushes`. Everything after the `?` are arguments to the remote procedure call. In my mind, 404 means *the resource isn't available.* The resource is `json-pushes` (it's an RPC API). And that resource **is** available. However, the content requested by that resource (a set of pushes) is not available.

HTTP status codes communicate state of the resource. I don't think it is appropriate to return 404 here because the `json-pushes` resource/API is still available - it just doesn't have the data you want. I think internal status codes within the HTTP response body are the proper mechanism here.

FWIW, once you accept that not everything (notably RPC-style APIs) fits inside the REST framework, API designs becomes much easier :)

Comment 10

[Ed Morley [:emorley]]

In that case, can we stop returning the 404 if a non existent &fromchange=REV is specified? I'm happy to agree with your reasoning in comment 9, I'd just like consistency either way if possible :-)

Comment 12

[Gregory Szorc [:gps]]

Attachment: MozReview Request: pushlog: return an error if startID is too large (bug 1114843); r?smacleod

Comment 13

[Ed Morley [:emorley]]

Was this ready to land? :-)

Comment 14

[Gregory Szorc [:gps]]

It was. However I'm hesitant to land it right now since we're cutting a number of releases. Leaving needinfo open to track doing this in a few days.

Comment 15

[Gregory Szorc [:gps]]

url:        https://hg.mozilla.org/hgcustom/version-control-tools/rev/3912cb415898ee50999630f2b8f2231573abb40b
changeset:  3912cb415898ee50999630f2b8f2231573abb40b
user:       Gregory Szorc <[e-mail]>
date:       Mon Aug 10 14:00:36 2015 -0700
description:
pushlog: return an error if startID is too large (bug 1114843); r=edmorley

Clients need a mechanism to identify when a push ID value is out of
bounds. This patch gives them that.

This patch also introduces an error reporting mechanism for version 2
payloads. The new error mechanism may not be used everywhere yet. But it
should be. Failure to utilize the new error mechanism should be
considered a bug.

Comment 16

[Gregory Szorc [:gps]]

This is deploying currently.

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Seems like this broke taskcluster's gecko-decision (bug 1193098). Any chance we can get this undeployed?

Comment 18

[Gregory Szorc [:gps]]

url:        https://hg.mozilla.org/hgcustom/version-control-tools/rev/07bfdf492c6e62b9eeefc3b88e0e139cd7e75d8e
changeset:  07bfdf492c6e62b9eeefc3b88e0e139cd7e75d8e
user:       Gregory Szorc <[e-mail]>
date:       Mon Aug 10 18:46:41 2015 -0700
description:
pushlog: backout 3912cb415898 (bug 1114843) for breaking TaskCluster

I suspect this is due to type coercion between ints and strings.

Comment 19

[Gregory Szorc [:gps]]

Comment on attachment 8618967 [details]
MozReview Request: pushlog: return an error if startID is too large (bug 1114843); r?smacleod

pushlog: return an error if startID is too large (bug 1114843); r?smacleod

Clients need a mechanism to identify when a push ID value is out of
bounds. This commit gives them that.

This commit also introduces an error reporting mechanism for version 2
payloads. The new error mechanism may not be used everywhere yet. But it
should be. Failure to utilize the new error mechanism should be
considered a bug.

Comment 20

[Steven MacLeod [:smacleod]]

Comment on attachment 8618967 [details]
MozReview Request: pushlog: return an error if startID is too large (bug 1114843); r?smacleod

https://reviewboard.mozilla.org/r/1715/#review14655

::: hgext/pushlog-legacy/pushlog-feed.py:304
(Diff revision 2)
> -        query.querystart_value = req.form.get('startID', ['0'])[0]
> +        query.querystart_value = int(req.form.get('startID', ['0'])[0])

Will this cast to int cause a 500 if it fails due to something non int-able being passed in?

::: hgext/pushlog-legacy/tests/test-hgweb.t:127
(Diff revision 2)
> +Querying against a startID that is too high results in an error

Can you also toss in tests for non numeric startID and endID?

Comment 21

[Gregory Szorc [:gps]]

Comment on attachment 8618967 [details]
MozReview Request: pushlog: return an error if startID is too large (bug 1114843); r?smacleod

pushlog: return an error if startID is too large (bug 1114843); r?smacleod

Clients need a mechanism to identify when a push ID value is out of
bounds. This commit gives them that.

This commit also introduces an error reporting mechanism for version 2
payloads. The new error mechanism may not be used everywhere yet. But it
should be. Failure to utilize the new error mechanism should be
considered a bug.

Comment 22

[Gregory Szorc [:gps]]

3 years later and I'm definitely not actively working on this.