Add script to load Candid data #24

jasonaowen · 2023-05-20T02:07:42Z

This is by no means finished, but I wanted to get something out before I left in case y'all are interested in looking before I get back to this.

jasonaowen · 2023-05-23T18:57:42Z

@slifty this should be far enough along for you to get some sense of what's involved on the Charity Navigator side; I'd also be happy to split the first few commits out into their own PR if there's enough there for you to start building without wanting to wait on this one.

README.md

src/index.ts

package.json

src/pdc-api.ts

jasonaowen · 2023-05-25T20:48:17Z

Okay, I pushed the scripts that I used to test PhilanthropyDataCommons/service#373 . I'm still not done; in particular, I still need to handle rate limits (by sleeping), upload in the bulk case, resolve linter errors, and rebase to address merge conflicts from recent dependabots.

It sounds like both @slifty and @bickelj are against using winston for logging here, so setting up pino with a useful configuration is on my list, too. If either of you want to take a pass at that, I'd welcome the help.

In the meantime, I'm going to go eat!

jasonaowen · 2023-05-26T00:29:44Z

Still have some cleanup to do, but it should now successfully upload Candid data for each proposal in the PDC. Hard to test for sure without fully importing the database locally, which I don't want to do.

I am beginning to suspect that, in the oh-so-grand tradition of this repo, we will run the script and use the results before the PR introducing the script is merged.

jasonaowen · 2023-05-26T00:34:39Z

src/candid.ts

+          args.oidcBaseUrl as string,
+          args.oidcClientId as string,
+          args.oidcClientSecret as string,


I'm not a fan of this, nor of the sequence of type guards alternative, but either the types for yargs or TypeScript itself doesn't allow for statically typing these slightly-dynamic options. If you have any suggestions for how best to tell TypeScript that "hey, this can't be undefined, actually", I'd love to hear them!

I'm not sure I understand the problem space, but does this help? https://github.com/yargs/yargs/blob/main/docs/typescript.md#typescript-usage-examples

jasonaowen · 2023-05-26T00:37:00Z

src/candid.ts

+        const token = await getToken(
+          args.oidcBaseUrl as string,
+          args.oidcClientId as string,
+          args.oidcClientSecret as string,
+        );
+        const eins = await getEinsFromPdc(args.pdcApiBaseUrl as string, token);


I'm also not a fan of this pattern of passing a token + the base URL around. An alternative I considered is to make a PDC client class that initializes an Axios client, keeps it in the instance, and has a bunch of methods that use it; another alternative is to pass that AxiosInstance around directly. I'm kinda leaning towards the latter, but haven't implemented it yet.

src/candid.ts

jasonaowen · 2023-05-29T18:51:17Z

Now that PhilanthropyDataCommons/service#373 has been deployed - thank you, @bickelj - it turns out we have some EINs that Candid does not know about, so I still need to handle 404s.

@slifty, how're you doing with Charity Navigator? Do you want to just add on to this PR? Or what are you thinking? Sorry this has been such a mess :/

slifty · 2023-05-30T00:36:26Z

@jasonaowen I think it'd be cleaner to have Charity Navigator as a separate PR (I have it on another branch from this one / am fine rebasing as needed). I was with family today for the holiday, but should have Charity Navigator data in the API tomorrow.

src/candid.ts

jasonaowen · 2023-05-30T02:16:32Z

@jasonaowen I think it'd be cleaner to have Charity Navigator as a separate PR (I have it on another branch from this one / am fine rebasing as needed).

Okay. I'm deep into the front-end right now, and probably won't have a chance to work on this for a few days. Again, I'm sorry to open this big sprawling PR - particularly with the logging misstep - and then basically have no bandwidth to work on it further!

I was with family today for the holiday, but should have Charity Navigator data in the API tomorrow.

I won't be able to get Charity Navigator wired up in the front end by tomorrow's 10am PT pre-demo, then.

slifty · 2023-05-30T14:49:56Z

 I won't be able to get Charity Navigator wired up in the front end by tomorrow's 10am PT pre-demo, then.

There isn't much data to show anyway! Sorry if I should have spoken up to be clear I wouldn't be working over the weekend.

slifty · 2023-05-30T14:53:01Z

src/candid.ts

+        await postPlatformProviderData(
+          args.pdcApiBaseUrl as string,
+          token,
+          args.ein as string,


I just wanted to call out this pattern since I just got bit by it -- this violates type safety, since the actual value is not a string. This means when doing something like JSON.stringify(args.ein) the result is an encoded number, not a string.

I don't quite know what magic yargs is implicitly doing around arguments (I can't tell where ein is actually defined, for instance), but it would be really nice to have all arguments be explicitly typed and provided to us a tool-typed value instead of shoehorned to pass type safety checks

Oh, that's great feedback! It sounds like I misunderstood how that works.

I can't tell where ein is actually defined

It is defined as an argument to the update command via the angle brackets in the command definition:

data-scripts/src/candid.ts

Line 102 in 9780b99

'update <ein>',

See the docs for positional arguments.

Looks like there's a way to specify the type of positional arguments, which I'd missed - sorry for leaving a trap that bit you, @slifty!

I just wanted to call out this pattern since I just got bit by it -- this violates type safety, since the actual value is not a string. This means when doing something like JSON.stringify(args.ein) the result is an encoded number, not a string.

Hm, I wish I had dug into this more deeply while @slifty was around more. I haven't been able to reproduce this.

Positional arguments seem to default to be parsed as strings; entering numbers on the command line - and, in particular, numeric expressions like EINs - show up as strings for me. Specifying that a positional argument or an option is a 'number' does cause it to be parsed as such.

The one wrinkle is that if you set an option to be a string, and then pass in a --config file that has the value as a JSON number, it will get passed in as a number. The reverse is not true: setting the option type to number, and passing in a --config file with a string-encoded numeric value will be parsed as a number.

I also looked more deeply into the original concern I had about needing to add type casts on use, and I think I figured out a solution! We'll need to declare a type for the expected arguments, a la React, and the handler should infer that its args is of that type:

interface ExampleCommandArgs { aRequiredString: string; anOptionalNumber?: number; } const exampleCommand: CommandModule<{}, ExampleCommandArgs> = { command: 'example', builder: { 'a-required-string': { type: 'string', demandOption: true }, 'an-optional-number': { type: 'number' }, }, handler: (args) => { console.log(`It is safe to use string methods ${args.aRequiredString.substring(0)}`); if (args.anOptionalNumber) { assert(args.anOptionalNumber + 0 !== args.anOptionalNumber + '0'); } }, };

This is better than adding as string or whatever to every use, although it does not rise to the level of runtime type assertions. This addresses my concern, although I don't know if it addresses @slifty's.

jasonaowen · 2023-06-13T03:14:53Z

In addition to rebasing to resolve merge conflicts, I'm also going to split out the infrastructure bits from the Candid-specific bits. That'll be in a separate PR, which this will ladder off.

src/client.ts

bickelj · 2023-06-16T19:15:02Z

src/pdc-api.ts

+  baseUrl: string,
+  path: string,
+  params: Record<string, string>,
+  token: AccessTokenSet,


For the PDC functions I'd prefer the simpler string here rather than forcing AccessTokenSet. Somewhere in the middle (i.e. other functions that call PDC functions and have an AccessTokenSet at hand) would call with accessTokenSet.access_token. A script wouldn't be required to use the client_credentials flow from oidc.ts, then, but the client_credentials flow would still be easy with the code from oidc.ts.

I think where I'm leaning now is to pass around a client (an AxiosInstance, specifically) that includes both the base URL, the authentication header, common logging options, and whatever else. That way, other non-client-credential users can construct the client however they want.

(I mentioned this as a possibility earlier.)

I didn't get to this, I'm afraid.

I'd still prefer not to have the token here be strictly an AccessTokenSet defined in oidc.ts but the token string. If I find myself writing code to call this function later in another script, I might change it.

src/pdc-api.ts

jasonaowen · 2023-06-24T00:33:40Z

I've split off #48 for adding logging. After that, I'll add the yargs framework in a followup PR. Then, I'll rebase this PR - which will only focus on Candid - onto those.

jasonaowen · 2023-06-27T22:03:36Z

I split out #52 for adding yargs.

jasonaowen

This is as done as it will be this month, I'm afraid. I'm not completely happy with where I ran out of time, but it works, it's a mostly-decent example of how to add commands to the unified script, and it passes the linter.

Sorry to drop this at the end of the day at the end of the week at the end of the month!

jasonaowen · 2023-07-01T00:42:57Z

src/candid.ts

+    .option('candid-api-key', {
+      describe: 'Candid Premier API key; get from account management at https://dashboard.candid.org/',
+      demandOption: true,
+      type: 'string',
+    })


I don't love repeating this stanza from lookup.

Meh, no big deal.

jasonaowen · 2023-07-01T00:43:21Z

src/candid.ts

+      args.oidcBaseUrl as string,
+      args.oidcClientId as string,
+      args.oidcClientSecret as string,


I couldn't figure out a way to avoid these as being typed as unknown.

jasonaowen · 2023-07-01T00:43:51Z

src/pdc-api.ts

+  baseUrl: string,
+  path: string,
+  params: Record<string, string>,
+  token: AccessTokenSet,


I didn't get to this, I'm afraid.

bickelj · 2023-07-06T18:51:36Z

src/candid.ts

+  ein: string,
+): Promise<CandidPremierResult> => {
+  logger.debug(`Looking up EIN ${ein} in Candid Premier API`);
+  const { data } = await client.get<CandidPremierResult>(


When this returns a 401 with npm start candid lookup 12-3456789 -- --candid-api-key='testing', I don't see the logger boilerplate. It's not a problem, really, but it differs from the behavior exemplified in npm start error.

Aha, great catch, @bickelj! It turns out error handling behaves differently between synchronously throwing an error and asynchronously returning a Promise that later rejects. I've added a commit to fix that!

src/candid.ts

bickelj · 2023-07-06T18:58:27Z

src/client.ts

+import axios, { InternalAxiosRequestConfig, AxiosResponse } from 'axios';
+import { logger } from './logger';
+
+const clientLogger = logger.child({}, { msgPrefix: '🌐 ' });


Haha, what? I like it. But I wasn't able to make the debug messages happen yet. I guess it's related to when an error happens on the axios.get.

You should be able to see it with LOG_LEVEL=debug: LOG_LEVEL=debug npm start -- candid lookup 12345678.

bickelj

I couldn't actually call Candid (no API key) but it looks good and trust that it works!

slifty · 2023-09-13T16:32:18Z

@jasonaowen probably worth getting this merged since it's been approved.

Start interacting with the Candid API by adding a command that takes an EIN and will either display the results or write them to a file. Build out an Axios client that has some reasonable defaults, including some light request and response logging.

Given an EIN, look up an organization in the Candid Premier API, and upload it to the PDC data platform provider cache. Unfortunately, the type inference around yargs commands is not quite powerful enough. I spent some time fighting the type checker and couldn't get it to do the right thing around reused `Options`; it just isn't able to figure them out, or at least I haven't been able to figure out how to help it do so correctly. It works better when the `builder` is a literal instead of a function, but positional arguments cannot be specified via the literal syntax, and the `check` validator is also only available via the function builder. So, sadly, back to sprinkling `as string` around.

Add the last command for storing Candid data in the PDC data platform provider cache: update-all. Fetch the list of applications from the PDC, find their EINs, call Candid with each EIN, and upload the response from Candid to the PDC. Happily, as this command does not include any positional arguments and so does not need to use the yargs function builder API, the type definitions are much better.

@bickelj

Yargs handles commands that throw synchronous errors differently from commands that return rejected promises. The example code introduced in commit 863379d threw a synchronous error, but the error returned by axios is via a rejected Promise. Configure yargs to not include its standard error handling behavior, so that our error handler will fire, and update the example error command to be async so as to exercise that behavior. Thanks to @bickelj for catching this! https://github.com/yargs/yargs/blob/main/docs/api.md#failfn--boolean

jasonaowen · 2023-09-19T21:39:14Z

I rebased onto main and added a commit to address a concern @bickelj raised. I'll leave this open for a day or two in case anyone wants to review that new change, and then I'll merge.

jasonaowen requested review from slifty and bickelj May 20, 2023 02:07

jasonaowen force-pushed the candid branch from bbe25aa to e3cef06 Compare May 23, 2023 18:55

jasonaowen mentioned this pull request May 23, 2023

Add data platform provider endpoints PhilanthropyDataCommons/service#373

Merged

bickelj reviewed May 25, 2023

View reviewed changes

README.md Outdated Show resolved Hide resolved

bickelj reviewed May 25, 2023

View reviewed changes

src/index.ts Show resolved Hide resolved

slifty reviewed May 25, 2023

View reviewed changes

package.json Outdated Show resolved Hide resolved

bickelj reviewed May 25, 2023

View reviewed changes

src/pdc-api.ts Show resolved Hide resolved

jasonaowen force-pushed the candid branch from fb35f28 to b9ed4a2 Compare May 26, 2023 00:27

jasonaowen commented May 26, 2023

View reviewed changes

jasonaowen commented May 30, 2023

View reviewed changes

src/candid.ts Outdated Show resolved Hide resolved

slifty reviewed May 30, 2023

View reviewed changes

bickelj reviewed Jun 16, 2023

View reviewed changes

src/client.ts Show resolved Hide resolved

bickelj reviewed Jun 16, 2023

View reviewed changes

src/pdc-api.ts Show resolved Hide resolved

This was referenced Jun 23, 2023

Add a script that POSTS proposal data from CSV #40

Merged

Generate type definitions PhilanthropyDataCommons/service#408

Closed

jasonaowen mentioned this pull request Jun 27, 2023

Introduce framework for unified scripts #52

Merged

jasonaowen force-pushed the candid branch 2 times, most recently from 374a1f6 to a69beb3 Compare July 1, 2023 00:41

jasonaowen commented Jul 1, 2023

View reviewed changes

jasonaowen marked this pull request as ready for review July 1, 2023 00:45

bickelj reviewed Jul 6, 2023

View reviewed changes

src/candid.ts Show resolved Hide resolved

bickelj reviewed Jul 6, 2023

View reviewed changes

src/candid.ts Show resolved Hide resolved

bickelj reviewed Jul 6, 2023

View reviewed changes

src/candid.ts Show resolved Hide resolved

bickelj reviewed Jul 6, 2023

View reviewed changes

bickelj approved these changes Jul 6, 2023

View reviewed changes

jasonaowen force-pushed the candid branch 3 times, most recently from 07eedb1 to 12829cc Compare September 19, 2023 21:26

jasonaowen added 4 commits September 19, 2023 17:33

jasonaowen force-pushed the candid branch from 12829cc to 4c663ab Compare September 19, 2023 21:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add script to load Candid data #24

Add script to load Candid data #24

jasonaowen commented May 20, 2023

jasonaowen commented May 23, 2023

jasonaowen commented May 25, 2023

jasonaowen commented May 26, 2023

jasonaowen May 26, 2023 •

edited

slifty May 30, 2023

jasonaowen May 26, 2023

jasonaowen commented May 29, 2023

slifty commented May 30, 2023

jasonaowen commented May 30, 2023

slifty commented May 30, 2023 •

edited

slifty May 30, 2023 •

edited

jasonaowen Jun 13, 2023

jasonaowen Jun 27, 2023

jasonaowen commented Jun 13, 2023

bickelj Jun 16, 2023 •

edited

jasonaowen Jun 22, 2023 •

edited

jasonaowen Jul 1, 2023

bickelj Jul 6, 2023

jasonaowen commented Jun 24, 2023

jasonaowen commented Jun 27, 2023

jasonaowen left a comment

jasonaowen Jul 1, 2023

bickelj Jul 6, 2023 •

edited

jasonaowen Jul 1, 2023

jasonaowen Jul 1, 2023

bickelj Jul 6, 2023

jasonaowen Sep 19, 2023

bickelj Jul 6, 2023 •

edited

jasonaowen Sep 19, 2023

bickelj left a comment

slifty commented Sep 13, 2023

jasonaowen commented Sep 19, 2023

Add script to load Candid data #24

Are you sure you want to change the base?

Add script to load Candid data #24

Conversation

jasonaowen commented May 20, 2023

jasonaowen commented May 23, 2023

jasonaowen commented May 25, 2023

jasonaowen commented May 26, 2023

jasonaowen May 26, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasonaowen commented May 29, 2023

slifty commented May 30, 2023

jasonaowen commented May 30, 2023

slifty commented May 30, 2023 • edited

slifty May 30, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasonaowen commented Jun 13, 2023

bickelj Jun 16, 2023 • edited

Choose a reason for hiding this comment

jasonaowen Jun 22, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasonaowen commented Jun 24, 2023

jasonaowen commented Jun 27, 2023

jasonaowen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bickelj Jul 6, 2023 • edited

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

bickelj Jul 6, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bickelj left a comment

Choose a reason for hiding this comment

slifty commented Sep 13, 2023

jasonaowen commented Sep 19, 2023

jasonaowen May 26, 2023 •

edited

slifty commented May 30, 2023 •

edited

slifty May 30, 2023 •

edited

bickelj Jun 16, 2023 •

edited

jasonaowen Jun 22, 2023 •

edited

bickelj Jul 6, 2023 •

edited

bickelj Jul 6, 2023 •

edited