Skip to content
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

Feature: Add External Source (and External Event) Attribute Validation #116

Open
wants to merge 31 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 15 commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
69f2ec2
Add routes for CreateExternalSource, CreateExternalSourceType, and Cr…
Nov 8, 2024
b9839d3
require source type to specify associated event types
Nov 13, 2024
f77f9e0
check event types in external source
Nov 13, 2024
78c94a8
ensure external event attributes actually checked
Nov 14, 2024
ca07a17
add external source validation to gateway
Nov 15, 2024
6995e66
add validation tests
Nov 15, 2024
578fbac
improve return
Nov 15, 2024
05efd84
additional error handling
Nov 18, 2024
897e88c
remove allowed event type functionality from source type creation and…
Nov 18, 2024
eb1ac18
final edits for error handling
Nov 18, 2024
3fbecf9
remove console.logs
Nov 18, 2024
48cb21c
Update error responses from external source/event end-points
Nov 19, 2024
accf345
Update logging messages & use JSON for all responses
Nov 20, 2024
6b68977
Improve error handling w. AJV.errors
Nov 20, 2024
0821432
Formatting fix
Nov 20, 2024
de0f009
Rework external source & event end-points to use multipart/form-data
Nov 21, 2024
30967c3
Rework uploadExternalSource to use form data rather than file
Nov 21, 2024
3ccd817
Rework Hasura responses
Nov 21, 2024
c283394
Formatting fixes
Nov 22, 2024
c62023e
attempt merge i
Nov 22, 2024
cf1dfe9
attempt merge ii
Nov 22, 2024
06b0415
update gateway tests
Nov 22, 2024
e32f95a
Partial refactor of new schema compilation code
Nov 22, 2024
27422a4
Add description for request objects
Nov 25, 2024
574a0cd
update for e2e tests
Nov 25, 2024
6edace6
minor fix post rebase
Nov 25, 2024
092671e
external_events => events
Nov 25, 2024
c8cb137
formatting
Nov 25, 2024
1a0ee20
formatting
Nov 26, 2024
9f90170
Formatting
Dec 3, 2024
d25e67a
Formatting, cleanup
Dec 3, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
683 changes: 369 additions & 314 deletions package-lock.json

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,6 @@
"nodemon": "^3.1.4",
"prettier": "^3.0.0",
"typescript": "^5.1.6",
"vitest": "^1.2.2"
"vitest": "^1.4.0"
}
}
4 changes: 4 additions & 0 deletions src/main.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ import initHasuraRoutes from './packages/hasura/hasura-events.js';
import initHealthRoutes from './packages/health/health.js';
import initPlanRoutes from './packages/plan/plan.js';
import initSwaggerRoutes from './packages/swagger/swagger.js';
import initExternalSourceRoutes from './packages/external-source/external-source.js';
import initExternalEventRoutes from './packages/external-event/external-event.js';
import cookieParser from 'cookie-parser';
import { AuthAdapter } from './types/auth.js';
import { NoAuthAdapter } from './packages/auth/adapters/NoAuthAdapter.js';
Expand Down Expand Up @@ -48,6 +50,8 @@ async function main(): Promise<void> {
initHealthRoutes(app);
initHasuraRoutes(app);
initPlanRoutes(app);
initExternalSourceRoutes(app);
initExternalEventRoutes(app);
initSwaggerRoutes(app);

app.listen(PORT, () => {
Expand Down
136 changes: 136 additions & 0 deletions src/packages/external-event/external-event.ts
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Organization question: why are these separate external-event and external-source packages instead of one united package?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't the feature bundled up under the name external-events in the UI? If so, that works. Otherwise external-source works.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ended up moving all new routes to a singular package as external-source!

Original file line number Diff line number Diff line change
@@ -0,0 +1,136 @@
import type { Express, Request, Response } from 'express';
import type { ExternalEventTypeInsertInput } from '../../types/external-event.js';
import Ajv from 'ajv';
import { getEnv } from '../../env.js';
import getLogger from '../../logger.js';
import gql from './gql.js';
import { HasuraError } from '../../types/hasura.js';

const logger = getLogger('packages/external-event/external-event');
const { HASURA_API_URL } = getEnv();
const GQL_API_URL = `${HASURA_API_URL}/v1/graphql`;

const ajv = new Ajv();

async function uploadExternalEventType(req: Request, res: Response) {
const authorizationHeader = req.get('authorization');

const {
headers: { 'x-hasura-role': roleHeader, 'x-hasura-user-id': userHeader },
} = req;

const { body } = req;
const { external_event_type_name, attribute_schema } = body;
logger.info(`POST /uploadExternalEventType: Uploading External Event Type: ${external_event_type_name}`);

const headers: HeadersInit = {
Authorization: authorizationHeader ?? '',
'Content-Type': 'application/json',
'x-hasura-role': roleHeader ? `${roleHeader}` : '',
'x-hasura-user-id': userHeader ? `${userHeader}` : '',
};

// Validate schema is valid JSON Schema
const schemaIsValid: boolean = ajv.validateSchema(attribute_schema);
if (!schemaIsValid) {
logger.error(
`POST /uploadExternalEventType: Schema validation failed for External Event Type ${external_event_type_name}`,
);
ajv.errors?.forEach(ajvError => logger.error(ajvError));
res.status(500).send({ message: ajv.errors });
return;
}

// Make sure name in schema (title) and provided name match
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;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this a try-catch instead of an if-return?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cleaned up, no longer using try/catch


logger.info(`POST /uploadExternalEventType: Attribute schema was VALID`);
Copy link
Contributor

@Mythicaeda Mythicaeda Nov 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use present tense in logs, unless the log is explicitly referring to a "prior state":

Suggested change
logger.info(`POST /uploadExternalEventType: Attribute schema was VALID`);
logger.info(`POST /uploadExternalEventType: Attribute schema is VALID`);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All logger and response messages now use present tense!


// Run the Hasura migration for creating an external event
const externalEventTypeInsertInput: ExternalEventTypeInsertInput = {
attribute_schema: attribute_schema,
name: external_event_type_name,
};

const response = await fetch(GQL_API_URL, {
body: JSON.stringify({
query: gql.CREATE_EXTERNAL_EVENT_TYPE,
variables: { eventType: externalEventTypeInsertInput },
}),
headers,
method: 'POST',
});

type CreateExternalEventTypeResponse = {
Copy link
Contributor

@Mythicaeda Mythicaeda Nov 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved all type definitions out to types/external-source.ts

data: { createExternalEventType: { attribute_schema: object; name: string } | null };
};
const jsonResponse = await response.json();
const createExternalEventTypeResponse = jsonResponse as CreateExternalEventTypeResponse | HasuraError;

res.json(createExternalEventTypeResponse);
}

export default (app: Express) => {
/**
* @swagger
* /uploadExternalEventType:
* post:
* security:
* - bearerAuth: []
* consumes:
* - application/json
* produces:
* - application/json
* parameters:
* - in: header
* name: x-hasura-role
* schema:
* type: string
* required: false
* requestBody:
* content:
* application/json:
* schema:
* type: object
* properties:
* attribute_schema:
* type: object
* external_event_type_name:
* type: string
* required:
* - external_event_type_name
* attribute_schema
* responses:
* 200:
* description: Created External Event Type
* content:
* application/json:
* schema:
* properties:
* attribute_schema:
* description: JSON Schema for the created External Event Type's attributes
* type: object
* name:
* description: Name of the created External Event Type
* type: string
* 403:
* description: Unauthorized error
* 401:
* description: Unauthenticated error
* summary: Uploads an External Event Type definition (containing name & attributes schema) to Hasura.
* tags:
* - Hasura
*/
app.post('/uploadExternalEventType', uploadExternalEventType);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-worked the routes so they properly use auth with plan.ts as a reference

};
11 changes: 11 additions & 0 deletions src/packages/external-event/gql.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
export default {
CREATE_EXTERNAL_EVENT_TYPE: `#graphql
mutation CreateExternalEventType($eventType: external_event_type_insert_input!)
{
createExternalEventType: insert_external_event_type_one(object: $eventType) {
attribute_schema
name
}
}
`,
};
Loading