search

Bungie-net / api

Resources for the Bungie.net API
Other
1.21k stars 92 forks source link

Request to change Stats paging to throw an error instead of return an empty page @ end of results #656

Open nickwebcouk opened 5 years ago

nickwebcouk commented 5 years ago

Just want to make sure I’m not doing this wrong. Long story boring I’m stepping through all previous activity for a one time data import for a number of users. Using the endpoint

Destiny2/1/Account/4611686018455087480/Character/2305843009263578530/Stats/Activities/

and setting the count variable to 250 nets me the first (250) results. Tacking on th page variable and increasing the page count every loop yields the correct results, but when I got a blank page I’m still getting the error status of 1.

Looking at the API - should this be returning 46 for invalid page number?

Three quick API’s for testing:

https://www.bungie.net/Platform/Destiny2/1/Account/4611686018455087480/Character/2305843009263578530/Stats/Activities/?count=250&page=1 (works, status 1)

https://www.bungie.net/Platform/Destiny2/1/Account/4611686018455087480/Character/2305843009263578530/Stats/Activities/?count=250&page=2 (works, status 1)

https://www.bungie.net/Platform/Destiny2/1/Account/4611686018455087480/Character/2305843009263578530/Stats/Activities/?count=250&page=3 (fails silently, status 1)

Is it my requests or am I messing something up?

Thanks!

vthornheart-bng commented 5 years ago

Hmm, it's an interesting point. Indeed, right now we're not throwing that invalid page number - and I can definitely see throwing that being a much more obvious failure scenario.

Anyone else want to chime in - others who are using this endpoint, do you have any preferences on how you'd like to see this be returned? I ask to the general audience because changing it to return that error code would be what might be considered a breaking change, if there are clients out there written with the assumption that the end of the content is equivalent to an empty page rather than an error message.

ArkahnX commented 5 years ago

Is it possible to have the empty result returned, and have it returned as something other than status 1? that would be non breaking for those that check for errors, and for those that don't

vthornheart-bng commented 5 years ago

Potentially - except that some clients may be looking for non-success messages as an indication that there was an error in the process rather than that it was a potentially expected condition. I don't know if I'd want to deviate from our current approach of "anything other than Success doesn't return a Result body" as that's very different from how the rest of the API works, but I'll give it some more thought.

floatingatoll commented 5 years ago

Does this now return 404?

vthornheart-bng commented 5 years ago

Not yet - I mentioned it in the status codes issue, but we had to roll back our aggressive push to return non-200 error codes with all requests right away. Only the PGCR requests do so for now, and we'll be rolling out the change for everything else ideally sometime in December (to give our mobile apps time to get the non-200 error code-handling version of the app more widely distributed).