Update BeaconError documentation (issue #262)

sdelatorrep commented 5 years ago

Well, I think that for the top level error (those which prevent the Beacon from giving a response) the HTTP status code provided in the response should be the same as in the BeaconError. But in the case of a partial error, I think the Beacon should answer with everything it can. I don't think that raising a global error for any partial error is the best solution. Please, @jrambla, join the discussion.

sdelatorrep commented 5 years ago

I've just talked to @silverdaz and got a new porposal. Returning a 200 when some non-blocking-error has happened is not good BUT there are other 2xx codes (or even 4xx) which we can use to express the message "hey, I have a response for your query though there was an error in some dataset(s). Take this into consideration when processing the response".

For example, we could use 207 (Multi-status) in both the top-level error and the HTTP status of the response:

[
  {
    "beaconId": "some-beacon",
    "apiVersion": "v1.0.1",
    "exists": true,
    "error": {
          "errorCode": "207",
          "errorMessage": "Some dataset has an error, check the individual responses"
        }
    },
    "alleleRequest": {
      "referenceName": "1",
      "start": 1111,
      "end": null,
      "startMin": null,
      "startMax": null,
      "endMin": null,
      "endMax": null,
      "referenceBases": "A",
      "alternateBases": "C",
      "variantType": null,
      "assemblyId": "GRCh38",
      "datasetIds": [
        "1","2"
      ],
      "includeDatasetResponses": "ALL"
    },
    "datasetAlleleResponses": [
      {
        "datasetId": "1",
        "exists": true,
        "error": {
          "errorCode": "200"
        },
        "frequency": 0.002222,
        "variantCount": 1,
        "callCount": 4,
        "sampleCount": 1,
        "note": null,
        "externalUrl": null,
        "info": null
      },
      {
        "datasetId": "2",
        "exists": null,
        "error": {
          "errorCode": "400",
          "errorMessage": "User provided assemblyId (GRCh38) does not match with dataset assembly (GRCh37)"
        },
        "frequency": null,
        "variantCount": null,
        "callCount": null,
        "sampleCount": null,
        "note": null,
        "externalUrl": null,
        "info": null
      }
    ]
]

and we could rename BeaconError to BeaconStatusand its fields to:

    "status": {
          "code": "207",
          "message": "Some dataset has an error, check the individual responses"
        }

WDYT @mcupak , @teemukataja, @blankdots, @jrambla?

blankdots commented 5 years ago

@sdelatorrep I was thinking of a similar thing in https://github.com/ga4gh-beacon/specification/issues/254#issuecomment-455613983 (also 206, but that is for something else) ...

Just as a question should an implementation of beacon specification support default MIME types and should the multistatus root element should be reflected in the JSON response as well, as per specification (https://tools.ietf.org/html/rfc4918#section-13) ?

I can see how 207 could be applied for any combination of 200, 401 (unauthorized) and 403 (forbidden), but including 404 or even 400 (as in the example) in the mix, not sure. Maybe404 would go better reflecting it is a MISS, still I yet to think of a use case for 400.

blankdots commented 5 years ago

Regarding examples and this comment: https://github.com/ga4gh-beacon/specification/issues/254#issuecomment-462708095 ... see line https://github.com/ga4gh-beacon/specification/pull/267/files#diff-3e4ace27644d8292430d48f9d398492eR131 and https://github.com/ga4gh-beacon/specification/pull/267/files#diff-3e4ace27644d8292430d48f9d398492eR79 those should be integer.

Also I still think custom error codes as here: https://github.com/ga4gh-beacon/specification/issues/254#issuecomment-459703726 might be better for BeaconDatasetAlleleResponse

ga4gh-beacon / specification

Update BeaconError documentation (issue #262) #267