openmobilityfoundation / mobility-data-specification

A data standard to enable right-of-way regulation and two-way communication between mobility companies and local governments.
https://www.openmobilityfoundation.org/about-mds/
Other
676 stars 232 forks source link

Agency error and bulk responses #856

Closed thekaveman closed 1 year ago

thekaveman commented 1 year ago

This is related to #852 but different enough that I wanted to open a new issue.

Agency references the general Error Messages and Bulk Responses - these describe JSON structures for what succeeded and what failed for a given request. I think this originated in #796 around this commit: https://github.com/openmobilityfoundation/mobility-data-specification/commit/1f4d197d1a6a8d9252d58aae79fa682f8e4a187c

It seems clear that a successful registration/update in an Agency endpoint should return a Bulk Response. #852 is asking which code to use between 200 vs 201.

I'm not as clear on which codes and in what situations either a singular JSON Error Message or a larger JSON Bulk Response (with failures) should be returned?

500

The Responses table for 500 says:

In this case, the answer may contain a text/plain body with an error message for troubleshooting.

It seems like the (JSON) Error Message or Bulk Response should not be used here, since the content type is text/plain. Is that correct?

Or should the language around 500 be updated to allow JSON? Would it return Error Message for GET and Bulk Response for POST/PUT?

409

The Responses table for 409 says:

POST operations when an object already exists and an update is not possible.

This seems like a case for using a Bulk Response - but only for POST? MDN says 409 is more common with PUT.

I also see from the linked commit/PR above that 409 was previously the code associated with these error responses in the Agency docs.

406, 404, and 401

These codes don't really make sense to use with Error Messages or Bulk Responses, no body content is returned with these.

400

Maybe? But then 400 is not referenced in any of the Agency docs, only in the General Info Responses table.

schnuerle commented 1 year ago

Hi @marie-x and @avatarneil do you have some thoughts on this you can share today?

marie-x commented 1 year ago

I'll schedule time with @avatarneil to discuss this morning (Tue 2 May)

thekaveman commented 1 year ago

Here's what I came up with for errors needing Bulk Responses, I'm implementing in the OpenAPI as a starting point but we can always adjust if needed:

Sorted by endpoint

Endpoint HTTP method Error key Error description HTTP code
/event POST bad_param A validation error occurred 400
/event POST missing_param A required parameter is missing 400
/event POST unregistered This device_id is unregistered 404
/reports POST bad_param A validation error occurred 400
/stops POST already_registered A stop with stop_id is already registered 409
/stops POST bad_param A validation error occurred 400
/stops POST missing_param A required parameter is missing 400
/stops PUT bad_param A validation error occurred 400
/stops PUT missing_param A required parameter is missing 400
/stops PUT unregistered This stop_id is unregistered 404
/telemetry POST bad_param A validation error occurred 400
/telemetry POST missing_param A required parameter is missing 400
/telemetry POST unregistered This device_id is unregistered 404
/trips POST bad_param A validation error occurred 400
/trips POST missing_param A required parameter is missing 400
/trips POST unregistered This device_id is unregistered 404
/vehicles POST already_registered A vehicle with device_id is already registered 409
/vehicles POST bad_param A validation error occurred 400
/vehicles POST missing_param A required parameter is missing 400
/vehicles PUT bad_param A validation error occurred 400
/vehicles PUT unregistered This device_id is unregistered 404

Sorted by Error key

Endpoint HTTP method Error key Error description HTTP code
/stops POST already_registered A stop with stop_id is already registered 409
/vehicles POST already_registered A vehicle with device_id is already registered 409
/event POST bad_param A validation error occurred 400
/reports POST bad_param A validation error occurred 400
/stops POST bad_param A validation error occurred 400
/stops PUT bad_param A validation error occurred 400
/telemetry POST bad_param A validation error occurred 400
/trips POST bad_param A validation error occurred 400
/vehicles POST bad_param A validation error occurred 400
/vehicles PUT bad_param A validation error occurred 400
/event POST missing_param A required parameter is missing 400
/stops POST missing_param A required parameter is missing 400
/stops PUT missing_param A required parameter is missing 400
/telemetry POST missing_param A required parameter is missing 400
/trips POST missing_param A required parameter is missing 400
/vehicles POST missing_param A required parameter is missing 400
/event POST unregistered This device_id is unregistered 404
/stops PUT unregistered This stop_id is unregistered 404
/telemetry POST unregistered This device_id is unregistered 404
/trips POST unregistered This device_id is unregistered 404
/vehicles PUT unregistered This device_id is unregistered 404
thekaveman commented 1 year ago

I think the final question on this is for 500 -- should that really be a text/plain response, or should that be application/json with the body of the single-error response?

{
  "error": "...",
  "error_description": "...",
  "error_details": ["...", "..."]
}
schnuerle commented 1 year ago

I've made a pass across all APIs to align each endpoint to the responses listed in this comment and in the current OpenAPI and Stoplight docs.

Take a look at the release branch to confirm. Example of how I documented it - see the 'Responses' header.

I did not address the 500 question yet. I could go either way on this so welcome feedback @thekaveman @marie-x @avatarneil If it's a quick easy change to OpenAPI (and we think it's a good idea) let's do it today. For MDS spec it's easy now and just changing one line here.

marie-x commented 1 year ago

application/json I'd say

thekaveman commented 1 year ago

It sounds like would want to update the 500 response across all OpenAPI docs then. I'll work on this this morning and get back to this thread.

thekaveman commented 1 year ago

Fixed the 500 response type/schema across all OpenAPI endpoints in https://github.com/openmobilityfoundation/mds-openapi/pull/37

thekaveman commented 1 year ago

I think this issue can be closed after the one-line update in the spec around 500 response type.

marie-x commented 1 year ago

Nice! Thanks!

schnuerle commented 1 year ago

Alright updated the language here: https://github.com/openmobilityfoundation/mobility-data-specification/pull/837/commits/00189922689039e0fe828734a26ca2dc60982634 (and then fixed "a" to "an").