-
Notifications
You must be signed in to change notification settings - Fork 2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add more validation to Rite Aid API #547
Changes from all commits
eb73382
acab328
77f5a80
1dd14ba
452e670
768dd42
324bf8a
632bdb2
0f3a199
26eb461
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,27 @@ | ||
"use strict"; | ||
const originalModule = jest.requireActual("../utils"); | ||
|
||
const warningLoggers = {}; | ||
const warnings = []; | ||
|
||
const mock = { | ||
...originalModule, | ||
|
||
createWarningLogger(prefix) { | ||
warningLoggers[prefix] = jest.fn((message) => warnings.push(message)); | ||
return warningLoggers[prefix]; | ||
}, | ||
|
||
// Only for use in tests: get mock function for a logger. | ||
__getWarningLogger(prefix) { | ||
prefix = prefix || Object.keys(warningLoggers).pop(); | ||
return warningLoggers[prefix]; | ||
}, | ||
|
||
// Only for use in tests: get a list of all warnings that were logged. | ||
__getWarnings() { | ||
return warnings; | ||
}, | ||
}; | ||
|
||
module.exports = mock; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,11 +1,25 @@ | ||
const { DateTime } = require("luxon"); | ||
const Sentry = require("@sentry/node"); | ||
const geocoding = require("../../geocoding"); | ||
const { ParseError } = require("../../exceptions"); | ||
const { Available, LocationType } = require("../../model"); | ||
const { createWarningLogger, httpClient, RateLimit } = require("../../utils"); | ||
const { | ||
createWarningLogger, | ||
httpClient, | ||
parseUsPhoneNumber, | ||
RateLimit, | ||
} = require("../../utils"); | ||
const { | ||
assertSchema, | ||
requireAllProperties, | ||
} = require("../../schema-validation"); | ||
const { RiteAidApiError } = require("./common"); | ||
|
||
const warn = createWarningLogger("Rite Aid API"); | ||
|
||
// Log a warning if a location has more than this many slots in a given day. | ||
const MAXIMUM_SLOT_COUNT = 500; | ||
|
||
// States in which Rite Aid has stores. | ||
const riteAidStates = new Set([ | ||
"CA", | ||
|
@@ -28,6 +42,78 @@ const riteAidStates = new Set([ | |
"WA", | ||
]); | ||
|
||
const riteAidWrapperSchema = requireAllProperties({ | ||
type: "object", | ||
properties: { | ||
Status: { type: "string" }, | ||
ErrCde: {}, | ||
ErrMsg: {}, | ||
ErrMsgDtl: {}, | ||
Data: requireAllProperties({ | ||
type: "object", | ||
properties: { | ||
providerDetails: { type: "array" }, | ||
}, | ||
additionalProperties: false, | ||
}), | ||
}, | ||
}); | ||
|
||
const riteAidLocationSchema = requireAllProperties({ | ||
type: "object", | ||
properties: { | ||
id: { type: "integer", minimum: 1 }, | ||
// parseUpdateTime() does fancy checking, so no need to check format here. | ||
last_updated: { type: "string" }, | ||
name: { type: "string", pattern: "Rite Aid" }, | ||
location: requireAllProperties({ | ||
type: "object", | ||
properties: { | ||
resourceType: { type: "null" }, | ||
id: { type: "null" }, | ||
identifier: { type: "null" }, | ||
name: { type: "null" }, | ||
telecom: { type: "null" }, | ||
address: { type: "null" }, | ||
position: { type: "null" }, | ||
meta: { type: "null" }, | ||
description: { type: "null" }, | ||
street: { type: "string" }, | ||
street_line_2: { type: "string", nullable: true }, | ||
city: { type: "string" }, | ||
state: { type: "string", pattern: "[A-Z]{2}" }, | ||
zipcode: { type: "string", pattern: "\\d{1,5}(-\\d{4})?" }, | ||
county: { type: "null" }, | ||
identifiers: { type: "null" }, | ||
}, | ||
additionalProperties: false, | ||
}), | ||
contact: requireAllProperties({ | ||
type: "object", | ||
properties: { | ||
booking_phone: { type: "string", nullable: true }, | ||
booking_url: { type: "string", format: "uri" }, | ||
info_phone: { type: "string", nullable: true }, | ||
info_url: { type: "string", format: "uri" }, | ||
}, | ||
additionalProperties: false, | ||
}), | ||
availability: { | ||
type: "array", | ||
items: requireAllProperties({ | ||
type: "object", | ||
properties: { | ||
date: { type: "string", format: "date" }, | ||
total_slots: { type: "integer", minimum: 0 }, | ||
available_slots: { type: "integer", minimum: 0 }, | ||
}, | ||
additionalProperties: false, | ||
}), | ||
}, | ||
}, | ||
additionalProperties: false, | ||
}); | ||
|
||
async function queryState(state, rateLimit = null) { | ||
const RITE_AID_URL = process.env["RITE_AID_URL"]; | ||
const RITE_AID_KEY = process.env["RITE_AID_KEY"]; | ||
|
@@ -40,22 +126,24 @@ async function queryState(state, rateLimit = null) { | |
|
||
if (rateLimit) await rateLimit.ready(); | ||
|
||
const body = await httpClient({ | ||
const response = await httpClient({ | ||
url: RITE_AID_URL, | ||
headers: { "Proxy-Authorization": "ldap " + RITE_AID_KEY }, | ||
searchParams: { stateCode: state }, | ||
}).json(); | ||
|
||
if (body.Status !== "SUCCESS") { | ||
console.error(body.Status); | ||
console.error(body.ErrCde); | ||
console.error(body.ErrMsg); | ||
console.error(body.ErrMsgDtl); | ||
responseType: "json", | ||
}); | ||
|
||
throw new Error("RiteAid API request failed"); | ||
if (response.body.Status !== "SUCCESS") { | ||
throw new RiteAidApiError(response); | ||
} | ||
|
||
return body.Data.providerDetails.map(formatStore); | ||
assertSchema( | ||
riteAidWrapperSchema, | ||
response.body, | ||
"Response did not match schema" | ||
); | ||
|
||
return response.body.Data.providerDetails; | ||
} | ||
|
||
/** | ||
|
@@ -90,6 +178,12 @@ function parseUpdateTime(text) { | |
} | ||
|
||
function formatStore(provider) { | ||
assertSchema( | ||
riteAidLocationSchema, | ||
provider, | ||
"API location did not match schema" | ||
); | ||
|
||
const address = formatAddress(provider.location); | ||
|
||
let county = provider.location.county; | ||
|
@@ -121,9 +215,13 @@ function formatStore(provider) { | |
state: provider.location.state, | ||
postal_code: provider.location.zipcode, | ||
county, | ||
booking_phone: provider.contact.booking_phone, | ||
booking_phone: | ||
provider.contact.booking_phone && | ||
parseUsPhoneNumber(provider.contact.booking_phone), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: Any reason not to have There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, I was trying to match the behavior of But for this use case, it would definitely be simpler. Think I should update There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. One approach that can solve for that is to allow the caller to provide a default fallback value, or else the function will throw an error given bad input (including wrong data type). That said, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In that case, I think I'll keep it as-is for now, and maybe find ways to handle |
||
booking_url: provider.contact.booking_url, | ||
info_phone: provider.contact.info_phone, | ||
info_phone: | ||
provider.contact.info_phone && | ||
parseUsPhoneNumber(provider.contact.info_phone), | ||
info_url: provider.contact.info_url, | ||
|
||
availability: { | ||
|
@@ -144,12 +242,30 @@ function formatAvailable(provider) { | |
} | ||
|
||
function formatCapacity(provider) { | ||
return provider.availability.map((apiData) => ({ | ||
date: apiData.date, | ||
available: apiData.available_slots > 0 ? Available.yes : Available.no, | ||
available_count: apiData.available_slots, | ||
unavailable_count: apiData.total_slots - apiData.available_slots, | ||
})); | ||
let maxDailySlots = 0; | ||
const result = provider.availability.map((apiData) => { | ||
maxDailySlots = Math.max(maxDailySlots, apiData.total_slots); | ||
if (apiData.available_slots > apiData.total_slots) { | ||
throw new Error("More available slots than total slots at a Rite Aid"); | ||
} | ||
|
||
return { | ||
date: apiData.date, | ||
available: apiData.available_slots > 0 ? Available.yes : Available.no, | ||
available_count: apiData.available_slots, | ||
unavailable_count: apiData.total_slots - apiData.available_slots, | ||
}; | ||
}); | ||
|
||
if (maxDailySlots > MAXIMUM_SLOT_COUNT) { | ||
warn( | ||
"Unrealistic slot count at a Rite Aid", | ||
{ slots: maxDailySlots }, | ||
true | ||
); | ||
} | ||
|
||
return result; | ||
} | ||
|
||
function formatAddress(location) { | ||
|
@@ -174,7 +290,7 @@ async function checkAvailability(handler, options) { | |
|
||
if (!states.length) { | ||
const statesText = Array.from(riteAidStates).join(", "); | ||
console.warn(`No states set for riteAidApi (supported: ${statesText})`); | ||
warn(`No states set for riteAidApi (supported: ${statesText})`); | ||
} | ||
|
||
if (options.rateLimit != null && isNaN(options.rateLimit)) { | ||
|
@@ -185,11 +301,26 @@ async function checkAvailability(handler, options) { | |
|
||
let results = []; | ||
for (const state of states) { | ||
let stores; | ||
// Sentry's withScope() doesn't work for async code, so we have to manually | ||
// track the context data we want to add. :( | ||
const errorContext = { state, source: "Rite Aid API" }; | ||
|
||
const stores = []; | ||
try { | ||
stores = await queryState(state, rateLimit); | ||
const rawData = await queryState(state, rateLimit); | ||
for (const rawLocation of rawData) { | ||
Sentry.withScope((scope) => { | ||
scope.setContext("context", errorContext); | ||
scope.setContext("location", { id: rawLocation.id }); | ||
try { | ||
stores.push(formatStore(rawLocation)); | ||
} catch (error) { | ||
warn(error); | ||
} | ||
}); | ||
} | ||
} catch (error) { | ||
warn(error, { state, source: "Rite Aid API" }, true); | ||
warn(error, errorContext, true); | ||
Comment on lines
+304
to
+323
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is kind of ugly, but Sentry has built-in support for managing async stuff with domains (for example, the There are also some Sentry issues tracking better async support: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, I'm not sure it's worthwhile trying to build a generalized solve here, or at least not yet. The code as written reads fine to me and doesn't seem overly messy. |
||
continue; | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
const assert = require("assert").strict; | ||
const { HttpApiError } = require("../../exceptions"); | ||
|
||
class RiteAidApiError extends HttpApiError { | ||
parse(response) { | ||
assert.equal(typeof response.body, "object"); | ||
this.details = response.body; | ||
this.message = `${this.details.Status} ${this.details.ErrCde}: ${this.details.ErrMsg}`; | ||
} | ||
} | ||
|
||
module.exports = { RiteAidApiError }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made separate schemas for the API response wrapper and the actual objects because:
It’s possible only some locations could get out of spec, so this lets us avoid breaking everything in that case. (OTOH, maybe we should break everything in that case? Also, not sure this scenario is at all likely to occur.)
From a practical standpoint, the error message lists every error, and if there is a systemic change, the error message would be incredibly long because it repeats the same things hundreds or thousands of times (one for every location object) if there was just one schema instead of a separate one for the locations that we check when formatting on each location.
Of course, that could be mitigated by limiting how many errors we include in the actual error message here:
univaf/loader/src/schema-validation.js
Lines 10 to 15 in 19f0f92