Feature: Add External Source (and External Event) Attribute Validation #116
Conversation
…eateExternalEventType
pranav-super
requested review from
joswig,
Mythicaeda,
dandelany and
AaronPlave
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| } catch (error) { | ||
| logger.error((error as Error).message); | ||
| res.status(500); | ||
| res.send((error as Error).message); |
There was a problem hiding this comment.
This is not Aerie Gateway standard. Do not just send a text message back as an error message, you need to send a JSON. See L43 of middleware.ts for an example of the correct way to return an error status. This applies to all places where you're returning error statuses.
| async function uploadExternalEventType(req: Request, res: Response) { | ||
| logger.info(`POST /uploadExternalEventType: Entering function...`); | ||
| const authorizationHeader = req.get('authorization'); | ||
| logger.info(`POST /uploadExternalEventType: ${authorizationHeader}`); |
There was a problem hiding this comment.
Remove this, you should never be logging user credentials like this.
There was a problem hiding this comment.
Sorry - missed this from cleaning up some testing debugs, removed now!
| const ajv = new Ajv(); | ||
|
|
||
| async function uploadExternalEventType(req: Request, res: Response) { | ||
| logger.info(`POST /uploadExternalEventType: Entering function...`); |
There was a problem hiding this comment.
This log is unnecessary, please remove it.
| if (!schemaIsValid) { | ||
| throw new Error("Schema was not a valid JSON Schema."); | ||
| } | ||
| } catch (error) { | ||
| logger.error((error as Error).message); | ||
| res.status(500); | ||
| res.send((error as Error).message); |
There was a problem hiding this comment.
Can ajv.validateSchema throw an Error? If not, why is this a try-catch? If it can, then this comment can be resolved.
There was a problem hiding this comment.
Good catch - no it looks like all errors are just directly accessible through ajv.errors. I updated the logger and res.send statements to use ajv.errors instead of relying on a try / catch.
| try { | ||
| if (attribute_schema['title'] === undefined || attribute_schema.title !== external_event_type_name) { | ||
| throw new Error('Schema title does not match provided external event type name.'); | ||
| } | ||
| } catch (error) { | ||
| logger.error( | ||
| `POST /uploadExternalEventType: Error occurred during External Event Type ${external_event_type_name} upload`, | ||
| ); | ||
| logger.error((error as Error).message); | ||
| res.status(500).send({ message: (error as Error).message }); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Why is this a try-catch instead of an if-return?
| return; | ||
| } | ||
|
|
||
| logger.info(`POST /uploadExternalEventType: Attribute schema was VALID`); |
There was a problem hiding this comment.
Please use present tense in logs, unless the log is explicitly referring to a "prior state":
| logger.info(`POST /uploadExternalEventType: Attribute schema was VALID`); | |
| logger.info(`POST /uploadExternalEventType: Attribute schema is VALID`); |
| method: 'POST', | ||
| }); | ||
|
|
||
| type CreateExternalEventTypeResponse = { |
There was a problem hiding this comment.
Types should be declared in a file in the types directory, not inline. Additionally, this type should not start at the data wrapper object but just be the middle { attribute_schema: object; name: string }
Please use types/plans.ts for a reference of what I mean.
| * tags: | ||
| * - Hasura | ||
| */ | ||
| app.post('/uploadExternalEventType', uploadExternalEventType); |
There was a problem hiding this comment.
This endpoint is currently unauthenticated. Example of an authenticated endpoint from L716 of packages/plan/plan.ts. Please also speak with @duranb regarding the upload.single part.
app.post('/uploadDataset', upload.single('external_dataset'), refreshLimiter, auth, uploadDataset);
There was a problem hiding this comment.
Organization question: why are these separate external-event and external-source packages instead of one united package?
There was a problem hiding this comment.
I started with them separate since they'd been separated at times when doing the UI work in the original pull-request, but I do think it makes sense to condense to one package - I can't think of a name though, maybe just naming it external-source or external-source-events?
There was a problem hiding this comment.
Isn't the feature bundled up under the name external-events in the UI? If so, that works. Otherwise external-source works.
Parsing is done on the UI side, and parsed data is sent as a multipart/form-data request
Still a few areas to refactor/cleanup, WIP
This pull request adds external source/event attribute validation to Aerie.
This means that when an external source is uploaded to AERIE, via endpoints like the CLI or the UI, it is forwarded here, has its formatting checked against a schema, and additionally has the attributes of the source and each of its events checked for validity, before submitting it to the database.
Submission of event/source type schemas for the purpose of later validation is also handled here - the gateway simply checks that they are valid JSON Schema.
A more thorough explanation of the feature can be found in the AERIE PR, here.