Add more validation to Rite Aid API #547

Mr0grog · 2022-02-02T08:25:31Z

I thought this wouldn’t be too complex, but alas, I was wrong, as usual.

This is a follow-on to the Rite Aid API issues discovered while counting slots last week in usdigitalresponse/appointment-data-insights#3. The main idea was to check the number of slots the API is reporting to make sure it’s reasonable (since it was reporting thousands per store for a couple months). Of course, as I got into it, it expanded:

Check the number of slots to make sure the total is reasonable (this only triggers a warning, since it’s possible our definition of “reasonable” could be bad).
Check the slots to make sure it’s not reporting mismatching values, i.e. where the available slots are more than the total slots. This causes an actual error, since this would seem to indicate things are wrong enough we don’t want to try and read this data.
Clean up the workflow slightly so an error formatting a single store doesn’t break all the other stores, more like how some of the more recent sources work.
Check the schema of the response to make sure it hasn’t changed (spoiler: it has changed).
Clean up phone numbers. I foolishly thought I should check the phone number format in the schema, and discovered that Rite Aid’s API removes leading zeroes from the local part of the phone number!!! For example, this is an actual phone number the API is currently surfacing: “(203) 382-9”
Clean up the error parsing issue in Improve handling of Rite Aid API errors #537 since we’re mucking about in Rite Aid code anyway.

Fixes #537.

- If the total slots is unreasonably high, log a warning, but don't fail. - If the total slots is less than the available slots, throw an error (something is way broken). Along the way, this amends the overall flow to match more recently written sources, where an exception formatting a single location doesn't stop other locations from being formatted and sent.

Also adds parsing for phone numbers, because I was foolish enough to try checking the phone number pattern in the schema, and discovered that leading zeroes on the local part of the phone numbers are missing in Rite Aid's data (!!!).

Mr0grog · 2022-02-02T08:32:38Z

loader/src/sources/riteaid/api.js

+  assertSchema(
+    riteAidWrapperSchema,
+    response.body,
+    "Response did not match schema"
+  );


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

constructor(errors, data, message = null) {

message = message || "Data did not match schema";

const details = errors.map(SchemaError.formatAjvError);

const detailMessage = details.map((detail) => ` ${detail}`).join("\n");

super(`${message} - ${errors.length} errors:\n${detailMessage}`);

Mr0grog · 2022-02-02T08:39:03Z

loader/src/sources/riteaid/api.js

+    // 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);


This is kind of ugly, but withScope() can’t handle async code. Definitely open to changes here if you have good thoughts on better patterns, or nice sugar.

Sentry has built-in support for managing async stuff with domains (for example, the express plugin uses them), but domains are a deprecated feature that are going away at some point in Node.js (I think in favor of Async Hooks). So I’m hesitant to build a fancy abstraction on them.

There are also some Sentry issues tracking better async support:

Support/make use of Async Hooks getsentry/sentry-javascript#3660

Better scope management for async code getsentry/sentry-javascript#3751

Sentry produces incorrect data when async code is involved getsentry/sentry-javascript#4071

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.

I could have sworn I tested the previous approach, but clearly not. :\

astonm · 2022-02-02T18:58:10Z

loader/src/sources/riteaid/api.js

-    booking_phone: provider.contact.booking_phone,
+    booking_phone:
+      provider.contact.booking_phone &&
+      parseUsPhoneNumber(provider.contact.booking_phone),


nit: Any reason not to have parseUsPhoneNumber handle the falsy check as well?

Ah, I was trying to match the behavior of parseUsAddress, which also does not support non-string input. It also seems a little easier to reason about if you can guarantee the return type will be a string.

But for this use case, it would definitely be simpler. Think I should update parseUsAddress, too? (I think the non-support for null/undefined in parseUsAddress is currently supporting some edge cases in NJVSS and Albertsons, so it may require some more changes.)

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, parseUsAddress is complicated enough and used elsewhere enough that I don't know if it's worth messing with. I do appreciate that the two parseUsXYZ functions are consistent in whether or not you need to look before you leap.

In that case, I think I'll keep it as-is for now, and maybe find ways to handle null for both of them later.

astonm · 2022-02-02T19:02:15Z

loader/src/sources/riteaid/api.js

+    // 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);


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.

Mr0grog added 6 commits February 1, 2022 18:06

Separate raw data loading and formatting in Rite Aid API

eb73382

Add schema checking for Rite Aid API

77f5a80

Also adds parsing for phone numbers, because I was foolish enough to try checking the phone number pattern in the schema, and discovered that leading zeroes on the local part of the phone numbers are missing in Rite Aid's data (!!!).

Update fixture file to current API format

1dd14ba

Clean up Rite Aid errors, fixes #537

452e670

Genericize the mocks for logging

768dd42

Mr0grog requested a review from astonm February 2, 2022 08:25

Add helpful comments, remove unhelpful ones

324bf8a

Mr0grog commented Feb 2, 2022

View reviewed changes

Mr0grog added 3 commits February 2, 2022 00:51

Fix lint error

632bdb2

I could have sworn I tested the previous approach, but clearly not. :\

Use pre-node-16-style strict assertions

0f3a199

Incredibly, the XHR API has the phone number problem, too

26eb461

astonm approved these changes Feb 2, 2022

View reviewed changes

Mr0grog merged commit 681ad77 into main Feb 2, 2022

Mr0grog deleted the sometimes-rite-aid-gives-somewhat-unrealistic-numbers-of-slots branch February 2, 2022 21:36

Mr0grog mentioned this pull request Jun 16, 2022

Add schema checking for HyVee #716

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add more validation to Rite Aid API #547

Add more validation to Rite Aid API #547

Mr0grog commented Feb 2, 2022

Mr0grog Feb 2, 2022

Mr0grog Feb 2, 2022

astonm Feb 2, 2022

astonm Feb 2, 2022

Mr0grog Feb 2, 2022

astonm Feb 2, 2022

Mr0grog Feb 2, 2022

astonm Feb 2, 2022

	constructor(errors, data, message = null) {
	message = message \|\| "Data did not match schema";
	const details = errors.map(SchemaError.formatAjvError);
	const detailMessage = details.map((detail) => ` ${detail}`).join("\n");

	super(`${message} - ${errors.length} errors:\n${detailMessage}`);

Add more validation to Rite Aid API #547

Add more validation to Rite Aid API #547

Conversation

Mr0grog commented Feb 2, 2022

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