Minor: Handle special error format for create_upload endpoint. #22

[ylecuyer]

Fix for #22

It adds a fallback for special error format in create_upload endpoint.

[dblock]

This feels incorrect. The API has predictable response and we're trying to be too smart about handling it.

According to Strava documentation this endpoint returns a Fault which returns only errors and message. Looks like you're getting something else.

• Do you have a VCR / HTTP request/response from Strava for this?
• Does strava return something else in this API for other failures (not a duplicate activity)?

I suggest opening a ticket with Strava and clarifying what the API is supposed to do before trying to hack our way around it. If this is a special case, then let's specialize the error and treat this API differently than others (inherit another class ala Fault, do custom stuff there).

[ylecuyer]

Here is an example with a missing field which is using the standard Fault format:

ylecuyer@inwin:~/Projects/OnMove200$ http --form POST "https://www.strava.com/api/v3/uploads" file@/home/ylecuyer/Desktop/DATA/strava.gpx name='test for #22' external_id='#22' "Authorization: Bearer REDACTED"
HTTP/1.1 400 Bad Request
Cache-Control: no-cache
Connection: keep-alive
Content-Type: application/json; charset=utf-8
Date: Fri, 20 Dec 2019 22:46:05 GMT
Referrer-Policy: strict-origin-when-cross-origin
Status: 400 Bad Request
Transfer-Encoding: chunked
Vary: Origin
Via: 1.1 linkerd
X-Content-Type-Options: nosniff
X-Download-Options: noopen
X-Frame-Options: SAMEORIGIN,DENY
X-Permitted-Cross-Domain-Policies: none
X-RateLimit-Limit: 600,30000
X-RateLimit-Usage: 2,3
X-Request-Id: edbe7eab-9170-4a1f-8f9e-b096bc3286ad
X-XSS-Protection: 1; mode=block
content-encoding: gzip

{
    "errors": [
        {
            "code": "required",
            "field": "data_type",
            "resource": "Upload"
        }
    ],
    "message": "Bad Request"
}

Here is the duplicate activity error which doesn't follow the fault format

ylecuyer@inwin:~/Projects/OnMove200$ http --form POST "https://www.strava.com/api/v3/uploads" file@/home/ylecuyer/Desktop/DATA/strava.gpx name='test for #22' data_type='gpx' external_id='#22' "Authorization: Bearer REDACTED"
HTTP/1.1 400 Bad Request
Cache-Control: no-cache
Connection: keep-alive
Content-Type: application/json; charset=utf-8
Date: Fri, 20 Dec 2019 22:45:29 GMT
Referrer-Policy: strict-origin-when-cross-origin
Status: 400 Bad Request
Transfer-Encoding: chunked
Vary: Origin
Via: 1.1 linkerd
X-Content-Type-Options: nosniff
X-Download-Options: noopen
X-Frame-Options: SAMEORIGIN,DENY
X-Permitted-Cross-Domain-Policies: none
X-RateLimit-Limit: 600,30000
X-RateLimit-Usage: 1,2
X-Request-Id: e48ccecd-5dae-4b9a-8409-1967187d38a7
X-XSS-Protection: 1; mode=block
content-encoding: gzip

{
    "activity_id": null,
    "error": "#22.gpx duplicate of activity 2941971481",
    "external_id": "#22.gpx",
    "id": 3136495967,
    "id_str": "3136495967",
    "status": "There was an error processing your activity."
}

I wont take the time to deal with strava support, feel free to contact them if you want to be sure this is the intended behavior on their end.

[dblock]

I've opened a ticket with Strava, which told me to email developers@strava.com, which opened a ticket #2799.

[dblock]

[dblock]

Given what Strava said we should fix this here, but not by modifying the base class. It's a special case for upload, so we can handle it there.

I tried to write a spec for this, adding to create_upload_spec.rb:

  let(:file) { 'spec/fixtures/strava/files/17611540601.tcx' }
  it 'handles duplicate activities', vcr: { cassette_name: 'client/create_upload_dup' } do
    upload = client.create_upload(
      file: Faraday::UploadIO.new(file, 'application/tcx+xml'),
      name: 'Uploaded Activity',
      description: 'Uploaded by strava-ruby-client.',
      data_type: 'tcx',
      external_id: 'strava-ruby-client-upload-1'
    )
    ...
end

You can get a STRAVA_API_TOKEN by setting STRAVA_API_CLIENT and STRAVA_API_SECRET and running ./bin/strava-oauth-token.

I have pre-uploaded spec/fixtures/strava/files/17611540601.tcx, but I cannot get a duplicate error back. I'm getting a 201 with the original activity.

Did they change this behavior? Are you still able to reproduce this? Can you turn that into a spec?

[ylecuyer]

Did they change this behavior? Are you still able to reproduce this? Can you turn that into a spec?

They didn't change the behavior and can reproduce this error every time.

I tried to do the change with a new error only for the /uploads endpoint, I'm not fan of the end result but if the idea is ok for you I'll add some tests and ruboclean it.

[dblock]

I like this OK, so would merge something like this. The URL being a structure, I'd want to make sure the API verb is POST and the path is /uploads, rather than just checking .to_s which seems brittle.

[dblock]

Better. It needs a spec and a rubocop fix, see failed build.

[dblock]

Let's also add a comment in this code linking to the issue/conversation/something.

[ylecuyer]

Better. It needs a spec and a rubocop fix, see failed build.

I have added the spec and fixed the build :) I disabled some rubocop rules locally which are irrelevants for spec files.

Let's also add a comment in this code linking to the issue/conversation/something.

IMO this is better handled by git blame and we will have the issue number in the changelog and commit name

[simonneutert]

@dblock could you move this pr in a separate branch, so I can try and fix your last requests, before this is ready to merge?

[dblock]

@dblock could you move this pr in a separate branch, so I can try and fix your last requests, before this is ready to merge?

You don't need me for this. Make a branch in your fork, git pull these changes, make any updates and open a new PR.

[dblock]

@simonneutert Want to finish this one too?

[simonneutert]

turns out Strava seemed to have changed to async "background jobs", so they accept the file if nothing is totally wrong. handing out an upload ID for further inquiries.

• when reuploading a file, then querying via the new ID, an error stating "... duplicate ..." is part of the body

I need to reread what I came up with, then I prepare a PR for us to decide on how to finish this.

[simonneutert]

@dblock take a glimpse in my draft, please https://github.com/dblock/strava-ruby-client/pull/67

As a final polish, I want to have the behavior of UploadFailedError much closer to Fault we are using already, but I think moving the error raising in the model is the way to go here.

[dblock]

LGTM, just needs a rebase

[dblock]

Closed via https://github.com/dblock/strava-ruby-client/pull/67