[mds-agency] Feature: add versioning to agency #302

janedotx · 2020-05-06T01:36:34Z

PR Checklist

simple searchable title - [mds-db] Add PG env var, [config] Fix eslint config
briefly describe the changes in this PR
mark as draft if should not be merged
write tests for all new functionality

Impacts

…on field is in response body.

…ning_to_agency

packages/mds-agency/types.ts

avatarneil

Any changes to error responses are breaking changes, are these in conformance with the agency spec?

mdurling · 2020-05-06T20:32:59Z

Any changes to error responses are breaking changes, are these in conformance with the agency spec?

Any changes to the response envelope at all are breaking.

avatarneil · 2020-05-06T21:03:25Z

packages/mds-agency/request-handlers.ts

-        error_description: 'none of the provided data was valid',
-        result: 'no valid telemetry submitted',
-        failures
+        error: new ValidationError('none of the provided data was valid', { failures })


Ought to be providing something like

{ error: 'invalid_data', error_description: 'none of the provided data was valid', error_details: failures }

See the Agency Telemetry spec

packages/mds-agency/request-handlers.ts

avatarneil · 2020-05-06T21:06:11Z

packages/mds-agency/request-handlers.ts

-      error_description: 'none of the provided data was valid',
-      result: 'no valid telemetry submitted',
-      failures: [`device_id ${data[0].device_id}: not found`]
+      error: new ValidationError('none of the provided data was valid.', {


This should actually be a 500 response, probably with the error being a new ServerError().

I don't want to make it a ServerError object, since the spec says it should be a string.

avatarneil · 2020-05-06T21:07:07Z

packages/mds-agency/request-handlers.ts

  const start = Date.now()

  const { data } = req.body
  const { provider_id } = res.locals
  if (!provider_id) {
    res.status(400).send({
-      error: 'bad_param',
-      error_description: 'bad or missing provider_id'
+      error: new BadParamsError('bad or missing provider_id')


Should have a error_description field as well, considering we had it before

Also error needs to be a string of something like bad_param, see the Agency Telemetry spec

Can we just merge this versioning one and I'll cut another PR to address all the places the agency code diverges from the spec? Some of the 200 responses also diverge from it. I'd rather handle all divergences in one PR.

avatarneil · 2020-05-06T21:12:24Z

packages/mds-agency/request-handlers.ts

@@ -254,18 +283,16 @@ export const submitVehicleEvent = async (req: AgencyApiRequest, res: AgencyApiRe
    if (message.includes('duplicate')) {
      logger.info(name, 'duplicate event', event.event_type)
      res.status(409).send({
-        error: 'duplicate_event',
-        error_description: 'an event with this device_id and timestamp has already been received'
+        error: new ConflictError('an event with this device_id and timestamp has already been received')


The spec does not actually have 409 as a valid response code for the events endpoint. I recommend changing this to a 400 with a bad_param error, see the Agency Events spec

avatarneil · 2020-05-06T21:12:57Z

packages/mds-agency/request-handlers.ts

      })
    } else if (message.includes('not found') || message.includes('unregistered')) {
      logger.info(name, 'event for unregistered', event.device_id, event.event_type)
      res.status(400).send({
-        error: 'unregistered',
-        error_description: 'the specified device_id has not been registered'
+        error: new DependencyMissingError('the specified device_id has not been registered')


Need to retain the format of error and error_description from the Agency Events spec.

avatarneil · 2020-05-06T21:14:24Z

packages/mds-agency/request-handlers.ts

  } catch (err) {
    if (String(err).includes('duplicate')) {
      res.status(409).send({
-        error: 'already_registered',
-        error_description: 'A vehicle with this device_id is already registered'
+        error: new ConflictError('A vehicle with this device_id is already registered')


Need to follow the same response format as the spec, see the Agency Vehicles spec

avatarneil · 2020-05-06T21:14:37Z

packages/mds-agency/request-handlers.ts

+    if (typeof failure !== 'boolean') {
+      return res.status(400).send(failure)
+    }


What's this for?

badDevice either returns true or an error object. The error object is formatted according to the error schema spelled out in the spec. I'll tweak it so it returns null if there are no errors, and the typeof check can go away.

avatarneil · 2020-05-06T21:16:10Z

packages/mds-agency/tests/integration-tests.ts

-  it('verifies non-uuid provider_id is rejected', done => {
-    request
-      .get('/devices')
-      .set('Authorization', AUTH_GARBAGE_PROVIDER)
-      .expect(400)
-      .end((err, result) => {
-        test.value(result).hasHeader('content-type', APP_JSON)
-        test.string(result.body.result).contains('invalid provider_id', 'is not a UUID')
-        done(err)
-      })
-  })
-
-  it('verifies unknown provider_id is rejected', done => {
-    request
-      .get('/devices')
-      .set('Authorization', AUTH_UNKNOWN_UUID_PROVIDER)
-      .expect(400)
-      .end((err, result) => {
-        test.value(result).hasHeader('content-type', APP_JSON)
-        test.string(result.body.result).contains('invalid provider_id', 'is not a known provider')
-        done(err)
-      })
-  })
-


Why are we removing these tests?

These endpoints simply don't exist in the API.

The reason they pass at all is because the auth token is bad.

Right right, cool 👍

avatarneil · 2020-05-06T21:17:05Z

packages/mds-agency/tests/integration-tests.ts

@@ -362,7 +334,7 @@ describe('Tests API', () => {
      .expect(201)
      .end((err, result) => {
        log('err', err, 'body', result.body)
-        test.string(result.body.result).contains('success')


As per the Agency Vehicles spec, we should be returning nothing along with the 201

Nothing but the version.

…ning_to_agency

…' into feature/add_versioning_to_agency

janedotx · 2020-05-09T01:23:21Z

packages/mds-agency/request-handlers.ts

  } catch (err) {
    if (String(err).includes('duplicate')) {
      res.status(409).send({
-        error: 'already_registered',


Changed since already_registered is not an error type in the spec.

already_registered is definitely an error type in the spec

ah you're right

janedotx · 2020-05-09T01:23:51Z

packages/mds-agency/request-handlers.ts


 // eslint-disable-next-line @typescript-eslint/no-floating-promises
 stream.initialize()
+const agencyServerError = { error: 'server_error', error_description: 'Unknown server error' }


It's not in the spec, but nothing's currently specified for 500s and this at least follows the format.

avatarneil · 2020-05-13T16:58:58Z

packages/mds-agency/request-handlers.ts

  } catch (err) {
    if (String(err).includes('duplicate')) {
      res.status(409).send({
-        error: 'already_registered',


already_registered is definitely an error type in the spec

avatarneil · 2020-05-13T18:32:03Z

packages/mds-agency/request-handlers.ts

+      error: 'not_found',
+      error_description: 'Device not found'


It looks like not_found isn't even in the spec, the spec dictates we should just send an empty body for a 404 here ¯_(ツ)_/¯

/shrug indeed

packages/mds-agency/request-handlers.ts

avatarneil · 2020-05-13T18:41:45Z

packages/mds-agency/request-handlers.ts

    }

-    return res.status(500).send(new ServerError())
+    return res.status(500).send(agencyServerError)


Could just use new ServerError here too I think because we don't have the same error structure

avatarneil · 2020-05-13T18:41:57Z

packages/mds-agency/request-handlers.ts

  } catch (err) {
-    res.status(500).send(new ServerError())
+    res.status(500).send(agencyServerError)


Same thing about ServerError

packages/mds-agency/request-handlers.ts

avatarneil · 2020-05-13T18:42:23Z

packages/mds-agency/request-handlers.ts

  try {
    const stops = await db.readStops()

    if (!stops) {
-      return res.status(404).send(new NotFoundError())
+      return res.status(404).send({ error: 'not_found', error_description: 'No stops were found' })


Can use a new NotFoundError here

avatarneil · 2020-05-13T18:42:31Z

packages/mds-agency/request-handlers.ts

  } catch (err) {
-    return res.status(500).send(new ServerError())
+    return res.status(500).send(agencyServerError)


Same thing about ServerError

avatarneil

One outstanding thing I'd definitely like resolved before merging (the special error bit), besides that in good shape

avatarneil · 2020-05-14T22:04:35Z

packages/mds-agency/request-handlers.ts

-    res.status(404).send({
-      error: 'not_found'
-    })
+    res.status(404).send({})


I think we can just do res.status(404).send() without defining an empty object

We could, but then the middleware drops charset utf-8 from the version header, which then makes it slightly different from the other endpoints.

avatarneil · 2020-05-14T22:05:09Z

packages/mds-agency/request-handlers.ts

-    res.status(404).send({
-      error: 'not_found'
-    })
+    res.status(404).send({})


Same note about sending an empty object vs none

avatarneil · 2020-05-14T22:05:39Z

packages/mds-agency/request-handlers.ts

@@ -186,22 +196,23 @@ export const updateVehicle = async (req: AgencyApiRequest, res: AgencyApiRespons
      await updateVehicleFail(req, res, provider_id, device_id, 'not found')
    } else {
      const device = await db.updateDevice(device_id, provider_id, update)
+      // TODO should we warn instead of fail if the cache/stream doesn't work?


That's what we're doing, no?

avatarneil · 2020-05-14T22:07:06Z

packages/mds-agency/request-handlers.ts

-      result: 'no valid telemetry submitted',
-      failures: [`device_id ${data[0].device_id}: not found`]
+    res.status(500).send({
+      error: 'server_error',


The error_details that are special are actually inaccurate for this code path, the original error message we sent was super off, and has actually lead to some diagnostic issues in the past

janedotx added 7 commits April 30, 2020 19:02

API responses typed up and versioning added.

2dad8cf

Updating application headers in tests.

14fa4df

minor fixes

ecdbf63

Type updated.

3086673

Mostly done fixing failing tests. Still need tests for ensuring versi…

19d91fc

…on field is in response body.

minor fixes

3471c36

Merge remote-tracking branch 'origin/develop' into feature/add_versio…

2a6e3bb

…ning_to_agency

janedotx requested review from avatarneil, mdurling, marie-x, macsj200 and wesleyfsmith May 6, 2020 01:39

minor fixes

33008fe

mdurling reviewed May 6, 2020

View reviewed changes

packages/mds-agency/types.ts Outdated Show resolved Hide resolved

macsj200 approved these changes May 6, 2020

View reviewed changes

janedotx added 2 commits May 6, 2020 15:35

Changing version number

e8e8da3

Merge branch 'develop' into feature/add_versioning_to_agency

ea31ae6

avatarneil requested changes May 6, 2020

View reviewed changes

janedotx added 5 commits May 7, 2020 14:02

Merge branch 'develop' into feature/add_versioning_to_agency

f820d7f

Making error responses conform to spec better.

ae3825a

Merge remote-tracking branch 'origin/develop' into feature/add_versio…

0eae7e0

…ning_to_agency

Merge remote-tracking branch 'origin/feature/add_versioning_to_agency…

0ab584a

…' into feature/add_versioning_to_agency

minor fixes

cc00f2d

janedotx commented May 9, 2020

View reviewed changes

janedotx added 2 commits May 8, 2020 21:46

minor fixes

dff72af

conflict resolution

67975af

janedotx requested a review from avatarneil May 11, 2020 22:01

avatarneil reviewed May 13, 2020

View reviewed changes

janedotx added 5 commits May 14, 2020 00:49

updating error handling in agency

f56bdbe

Spec realignment minor fixes.

72547db

conflict resolution

7e0b41f

conflict resolution

ca8936a

Merge branch 'develop' into feature/add_versioning_to_agency

053e7fc

avatarneil approved these changes May 14, 2020

View reviewed changes

Merge branch 'develop' into feature/add_versioning_to_agency

f5f9473

janedotx merged commit 7b01a7f into develop May 14, 2020

marie-x deleted the feature/add_versioning_to_agency branch March 10, 2021 16:57

[mds-agency] Feature: add versioning to agency #302

[mds-agency] Feature: add versioning to agency #302

Conversation

janedotx commented May 6, 2020

PR Checklist

Impacts

avatarneil left a comment

Choose a reason for hiding this comment

mdurling commented May 6, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

janedotx May 6, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

janedotx May 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

avatarneil May 13, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

avatarneil left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

janedotx May 6, 2020 •

edited

Loading

janedotx May 9, 2020 •

edited

Loading

avatarneil May 13, 2020 •

edited

Loading