Refactor app file in sequencing server #715
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| const simulationDatasetId = req.body.input.simulationDatasetId as number; | ||
| const [expansionSet, simulatedActivities] = await Promise.all([ | ||
| context.expansionSetDataLoader.load({ expansionSetId }), | ||
| context.simulatedActivitiesDataLoader.load({ simulationDatasetId }), |
Check failure
Code scanning / CodeQL
Uncontrolled data used in path expression (experimental)
| const result = Result.fromJSON( | ||
| await (piscina.run( | ||
| { | ||
| expansionLogic, |
Check failure
Code scanning / CodeQL
SQL database query built from user-controlled sources (experimental)
Comment on lines
+17
to
+67
| commandExpansionRouter.post('/put-expansion', async (req, res, next) => { | ||
| const context: Context = res.locals['context']; | ||
|
|
||
| const activityTypeName = req.body.input.activityTypeName as string; | ||
| const expansionLogic = req.body.input.expansionLogic as string; | ||
| const authoringCommandDictionaryId = req.body.input.authoringCommandDictionaryId as number | null; | ||
| const authoringMissionModelId = req.body.input.authoringMissionModelId as number | null; | ||
|
|
||
| const { rows } = await db.query( | ||
| ` | ||
| insert into expansion_rule (activity_type, expansion_logic, authoring_command_dict_id, | ||
| authoring_mission_model_id) | ||
| values ($1, $2, $3, $4) | ||
| returning id; | ||
| `, | ||
| [activityTypeName, expansionLogic, authoringCommandDictionaryId, authoringMissionModelId], | ||
| ); | ||
|
|
||
| if (rows.length < 1) { | ||
| throw new Error(`POST /put-expansion: No expansion was updated in the database`); | ||
| } | ||
|
|
||
| const id = rows[0].id; | ||
| logger.info(`POST /put-expansion: Updated expansion in the database: id=${id}`); | ||
|
|
||
| if (authoringMissionModelId == null || authoringCommandDictionaryId == null) { | ||
| res.status(200).json({ id }); | ||
| return next(); | ||
| } | ||
|
|
||
| const commandTypes = await context.commandTypescriptDataLoader.load({ dictionaryId: authoringCommandDictionaryId }); | ||
| const activitySchema = await context.activitySchemaDataLoader.load({ | ||
| missionModelId: authoringMissionModelId, | ||
| activityTypeName, | ||
| }); | ||
| const activityTypescript = generateTypescriptForGraphQLActivitySchema(activitySchema); | ||
|
|
||
| const result = Result.fromJSON( | ||
| await (piscina.run( | ||
| { | ||
| expansionLogic, | ||
| commandTypes: commandTypes, | ||
| activityTypes: activityTypescript, | ||
| }, | ||
| { name: 'typecheckExpansion' }, | ||
| ) as ReturnType<typeof typecheckExpansion>), | ||
| ); | ||
|
|
||
| res.status(200).json({ id, errors: result.isErr() ? result.unwrapErr() : [] }); | ||
| return next(); | ||
| }); |
Check failure
Code scanning / CodeQL
Missing rate limiting
Comment on lines
+69
to
+152
| commandExpansionRouter.post('/put-expansion-set', async (req, res, next) => { | ||
| const context: Context = res.locals['context']; | ||
|
|
||
| const commandDictionaryId = req.body.input.commandDictionaryId as number; | ||
| const missionModelId = req.body.input.missionModelId as number; | ||
| const expansionIds = req.body.input.expansionIds as number[]; | ||
|
|
||
| const [expansions, commandTypes] = await Promise.all([ | ||
| context.expansionDataLoader.loadMany(expansionIds.map(id => ({ expansionId: id }))), | ||
| context.commandTypescriptDataLoader.load({ dictionaryId: commandDictionaryId }), | ||
| ]); | ||
|
|
||
| const typecheckErrorPromises = await Promise.allSettled( | ||
| expansions.map(async (expansion, index) => { | ||
| if (expansion instanceof Error) { | ||
| throw new InheritedError(`Expansion with id: ${expansionIds[index]} could not be loaded`, expansion); | ||
| } | ||
| const activitySchema = await context.activitySchemaDataLoader.load({ | ||
| missionModelId, | ||
| activityTypeName: expansion.activityType, | ||
| }); | ||
| const activityTypescript = generateTypescriptForGraphQLActivitySchema(activitySchema); | ||
| const result = Result.fromJSON( | ||
| await (piscina.run( | ||
| { | ||
| expansionLogic: expansion.expansionLogic, | ||
| commandTypes: commandTypes, | ||
| activityTypes: activityTypescript, | ||
| }, | ||
| { name: 'typecheckExpansion' }, | ||
| ) as ReturnType<typeof typecheckExpansion>), | ||
| ); | ||
|
|
||
| return result; | ||
| }), | ||
| ); | ||
|
|
||
| const errors = unwrapPromiseSettledResults(typecheckErrorPromises).reduce((accum, item) => { | ||
| if (item instanceof Error) { | ||
| accum.push(item); | ||
| } else if (item.isErr()) { | ||
| accum.push(...item.unwrapErr()); | ||
| } | ||
| return accum; | ||
| }, [] as (Error | ReturnType<UserCodeError['toJSON']>)[]); | ||
|
|
||
| if (errors.length > 0) { | ||
| throw new InheritedError( | ||
| `Expansion set could not be type checked`, | ||
| errors.map(e => ({ | ||
| name: 'TypeCheckError', | ||
| stack: e.stack ?? null, | ||
| // @ts-ignore Message is not spread when it comes from an Error object because it's a getter | ||
| message: e.message, | ||
| ...e, | ||
| })), | ||
| ); | ||
| } | ||
|
|
||
| const { rows } = await db.query( | ||
| ` | ||
| with expansion_set_id as ( | ||
| insert into expansion_set (command_dict_id, mission_model_id) | ||
| values ($1, $2) | ||
| returning id), | ||
| rules as (select id, activity_type from expansion_rule where id = any ($3::int[]) order by id) | ||
| insert | ||
| into expansion_set_to_rule (set_id, rule_id, activity_type) | ||
| select a.id, b.id, b.activity_type | ||
| from (select id from expansion_set_id) a, | ||
| (select id, activity_type from rules) b | ||
| returning (select id from expansion_set_id); | ||
| `, | ||
| [commandDictionaryId, missionModelId, expansionIds], | ||
| ); | ||
|
|
||
| if (rows.length < 1) { | ||
| throw new Error(`POST /command-expansion/put-expansion-set: No expansion set was inserted in the database`); | ||
| } | ||
| const id = rows[0].id; | ||
| logger.info(`POST /command-expansion/put-expansion-set: Updated expansion set in the database: id=${id}`); | ||
| res.status(200).json({ id }); | ||
| return next(); | ||
| }); |
Check failure
Code scanning / CodeQL
Missing rate limiting
Comment on lines
+154
to
+313
|
|
||
| const expansionBuildArtifacts = await expansionBuildArtifactsCache.get(expansion.id)!; | ||
|
|
||
| if (expansionBuildArtifacts.isErr()) { | ||
| return { | ||
| activityInstance: simulatedActivity, | ||
| commands: null, | ||
| errors: expansionBuildArtifacts.unwrapErr(), | ||
| }; | ||
| } | ||
|
|
||
| const buildArtifacts = expansionBuildArtifacts.unwrap(); | ||
|
|
||
| const executionResult = Result.fromJSON( | ||
| await (piscina.run( | ||
| { | ||
| serializedActivityInstance: serializeWithTemporal(simulatedActivity), | ||
| buildArtifacts, | ||
| }, | ||
| { name: 'executeExpansionFromBuildArtifacts' }, | ||
| ) as ReturnType<typeof executeExpansionFromBuildArtifacts>), | ||
| ); | ||
|
|
||
| if (executionResult.isErr()) { | ||
| return { | ||
| activityInstance: simulatedActivity, | ||
| commands: null, | ||
| errors: executionResult.unwrapErr(), | ||
| }; | ||
| } | ||
|
|
||
| return { | ||
| activityInstance: simulatedActivity, | ||
| commands: executionResult.unwrap(), | ||
| errors: [], | ||
| }; | ||
| }), | ||
| ); | ||
|
|
||
| const rejectedExpansionResults = settledExpansionResults.filter(isRejected).map(p => p.reason); | ||
|
|
||
| for (const expansionResult of rejectedExpansionResults) { | ||
| logger.error(expansionResult.reason); | ||
| } | ||
| if (rejectedExpansionResults.length > 0) { | ||
| throw new Error(rejectedExpansionResults.map(rejectedExpansionResult => rejectedExpansionResult.reason).join('\n')); | ||
| } | ||
|
|
||
| const expandedActivityInstances = settledExpansionResults.filter(isResolved).map(p => ({ | ||
| id: p.value.activityInstance.id, | ||
| commands: p.value.commands, | ||
| errors: p.value.errors, | ||
| })); | ||
|
|
||
| // Store expansion run and activity instance commands in DB | ||
| const { rows } = await db.query( | ||
| ` | ||
| with expansion_run_id as ( | ||
| insert into expansion_run (simulation_dataset_id, expansion_set_id) | ||
| values ($1, $2) | ||
| returning id) | ||
| insert | ||
| into activity_instance_commands (expansion_run_id, | ||
| activity_instance_id, | ||
| commands, | ||
| errors) | ||
| select * | ||
| from unnest( | ||
| array_fill((select id from expansion_run_id), array [array_length($3::int[], 1)]), | ||
| $3::int[], | ||
| $4::jsonb[], | ||
| $5::jsonb[] | ||
| ) | ||
| returning (select id from expansion_run_id); | ||
| `, | ||
| [ | ||
| simulationDatasetId, | ||
| expansionSetId, | ||
| expandedActivityInstances.map(result => result.id), | ||
| expandedActivityInstances.map(result => (result.commands !== null ? JSON.stringify(result.commands) : null)), | ||
| expandedActivityInstances.map(result => JSON.stringify(result.errors)), | ||
| ], | ||
| ); | ||
|
|
||
| if (rows.length < 1) { | ||
| throw new Error( | ||
| `POST /command-expansion/expand-all-activity-instances: No expansion run was inserted in the database`, | ||
| ); | ||
| } | ||
| const id = rows[0].id; | ||
| logger.info( | ||
| `POST /command-expansion/expand-all-activity-instances: Inserted expansion run to the database: id=${id}`, | ||
| ); | ||
|
|
||
| res.status(200).json({ | ||
| id, | ||
| expandedActivityInstances, | ||
| }); | ||
| return next(); | ||
| }); |
Check failure
Code scanning / CodeQL
Missing rate limiting
Comment on lines
+123
to
+249
| on sequence_to_simulated_activity.simulated_activity_id = | ||
| activity_instance_commands.activity_instance_id | ||
| join expansion_run | ||
| on activity_instance_commands.expansion_run_id = expansion_run.id | ||
| where sequence.seq_id = $2 | ||
| and sequence.simulation_dataset_id = $1), | ||
| max_values as (select activity_instance_id, max(expansion_run_id) as max_expansion_run_id | ||
| from joined_table | ||
| group by activity_instance_id) | ||
| select joined_table.commands, | ||
| joined_table.activity_instance_id, | ||
| joined_table.errors | ||
| from joined_table, | ||
| max_values | ||
| where joined_table.activity_instance_id = max_values.activity_instance_id | ||
| and joined_table.expansion_run_id = max_values.max_expansion_run_id; | ||
| `, | ||
| [simulationDatasetId, seqId], | ||
| ), | ||
| db.query<{ | ||
| metadata: Record<string, any>; | ||
| }>( | ||
| ` | ||
| select metadata | ||
| from sequence | ||
| where sequence.seq_id = $2 | ||
| and sequence.simulation_dataset_id = $1; | ||
| `, | ||
| [simulationDatasetId, seqId], | ||
| ), | ||
| ]); | ||
|
|
||
| const seqMetadata = assertOne( | ||
| seqRows, | ||
| `No sequence found with seq_id: ${seqId} and simulation_dataset_id: ${simulationDatasetId}`, | ||
| ).metadata; | ||
|
|
||
| const simulatedActivities = await context.simulatedActivityInstanceBySimulatedActivityIdDataLoader.loadMany( | ||
| activityInstanceCommandRows.map(row => ({ | ||
| simulationDatasetId, | ||
| simulatedActivityId: row.activity_instance_id, | ||
| })), | ||
| ); | ||
| const simulatedActivitiesLoadErrors = simulatedActivities.filter(ai => ai instanceof Error); | ||
| if (simulatedActivitiesLoadErrors.length > 0) { | ||
| res.status(500).json({ | ||
| message: 'Error loading simulated activities', | ||
| cause: simulatedActivitiesLoadErrors, | ||
| }); | ||
| return next(); | ||
| } | ||
|
|
||
| const sortedActivityInstances = (simulatedActivities as Exclude<(typeof simulatedActivities)[number], Error>[]).sort( | ||
| (a, b) => Temporal.Duration.compare(a.startOffset, b.startOffset), | ||
| ); | ||
|
|
||
| const sortedSimulatedActivitiesWithCommands = sortedActivityInstances.map(ai => { | ||
| const row = activityInstanceCommandRows.find(row => row.activity_instance_id === ai.id); | ||
| // Hasn't ever been expanded | ||
| if (!row) { | ||
| return { | ||
| ...ai, | ||
| commands: null, | ||
| errors: null, | ||
| }; | ||
| } | ||
| return { | ||
| ...ai, | ||
| commands: row.commands?.map(CommandStem.fromSeqJson) ?? null, | ||
| errors: row.errors, | ||
| }; | ||
| }); | ||
|
|
||
| const errors = sortedSimulatedActivitiesWithCommands.flatMap(ai => ai.errors ?? []); | ||
|
|
||
| // This is here to easily enable a future feature of allowing the mission to configure their own sequence | ||
| // building. For now, we just use the 'defaultSeqBuilder' until such a feature request is made. | ||
| const seqBuilder = defaultSeqBuilder; | ||
| const sequenceJson = seqBuilder( | ||
| sortedSimulatedActivitiesWithCommands, | ||
| seqId, | ||
| seqMetadata, | ||
| simulationDatasetId, | ||
| ).toSeqJson(); | ||
|
|
||
| if (errors.length > 0) { | ||
| res.json({ | ||
| status: FallibleStatus.FAILURE, | ||
| seqJson: sequenceJson, | ||
| errors, | ||
| }); | ||
| } else { | ||
| res.json({ | ||
| status: FallibleStatus.SUCCESS, | ||
| seqJson: sequenceJson, | ||
| errors, | ||
| }); | ||
| } | ||
| return next(); | ||
| }); |
Check failure
Code scanning / CodeQL
Missing rate limiting
Comment on lines
+251
to
+422
| ), | ||
| ]); | ||
|
|
||
| // This is here to easily enable a future feature of allowing the mission to configure their own sequence | ||
| // building. For now, we just use the 'defaultSeqBuilder' until such a feature request is made. | ||
| const seqBuilder = defaultSeqBuilder; | ||
|
|
||
| const promises = await Promise.allSettled( | ||
| inputs.map(async ({ seqId, simulationDatasetId }) => { | ||
| const activityInstanceCommandRowsForSeq = activityInstanceCommandRows.filter( | ||
| row => row.seq_id === seqId && row.simulation_dataset_id === simulationDatasetId, | ||
| ); | ||
| const seqRowsForSeq = seqRows.find( | ||
| row => row.seq_id === seqId && row.simulation_dataset_id === simulationDatasetId, | ||
| ); | ||
|
|
||
| const seqMetadata = assertDefined( | ||
| seqRowsForSeq, | ||
| `No sequence found with seq_id: ${seqId} and simulation_dataset_id: ${simulationDatasetId}`, | ||
| ).metadata; | ||
|
|
||
| const simulatedActivitiesForSeqId = | ||
| await context.simulatedActivityInstanceBySimulatedActivityIdDataLoader.loadMany( | ||
| activityInstanceCommandRowsForSeq.map(row => ({ | ||
| simulationDatasetId, | ||
| simulatedActivityId: row.activity_instance_id, | ||
| })), | ||
| ); | ||
|
|
||
| const simulatedActivitiesLoadErrors = simulatedActivitiesForSeqId.filter(ai => ai instanceof Error); | ||
|
|
||
| if (simulatedActivitiesLoadErrors.length > 0) { | ||
| throw new Error( | ||
| `Error loading simulated activities for seqId: ${seqId}, simulationDatasetId: ${simulationDatasetId}`, | ||
| { cause: simulatedActivitiesLoadErrors }, | ||
| ); | ||
| } | ||
|
|
||
| const sortedActivityInstances = ( | ||
| simulatedActivitiesForSeqId as Exclude<(typeof simulatedActivitiesLoadErrors)[number], Error>[] | ||
| ).sort((a, b) => Temporal.Instant.compare(a.startTime, b.startTime)); | ||
|
|
||
| const sortedSimulatedActivitiesWithCommands = sortedActivityInstances.map(ai => { | ||
| const row = activityInstanceCommandRows.find(row => row.activity_instance_id === ai.id); | ||
| // Hasn't ever been expanded | ||
| if (!row) { | ||
| return { | ||
| ...ai, | ||
| commands: null, | ||
| errors: null, | ||
| }; | ||
| } | ||
| return { | ||
| ...ai, | ||
| commands: row.commands?.map(CommandStem.fromSeqJson) ?? null, | ||
| errors: row.errors, | ||
| }; | ||
| }); | ||
|
|
||
| const errors = sortedSimulatedActivitiesWithCommands.flatMap(ai => ai.errors ?? []); | ||
|
|
||
| const sequenceJson = seqBuilder( | ||
| sortedSimulatedActivitiesWithCommands, | ||
| seqId, | ||
| seqMetadata, | ||
| simulationDatasetId, | ||
| ).toSeqJson(); | ||
|
|
||
| if (errors.length > 0) { | ||
| return { | ||
| status: FallibleStatus.FAILURE, | ||
| seqJson: sequenceJson, | ||
| errors, | ||
| }; | ||
| } else { | ||
| return { | ||
| status: FallibleStatus.SUCCESS, | ||
| seqJson: sequenceJson, | ||
| errors: [], | ||
| }; | ||
| } | ||
| }), | ||
| ); | ||
|
|
||
| res.json( | ||
| promises.map(promise => { | ||
| if (isResolved(promise)) { | ||
| return promise.value; | ||
| } else { | ||
| return { | ||
| status: FallibleStatus.FAILURE, | ||
| seqJson: null, | ||
| errors: [promise.reason], | ||
| }; | ||
| } | ||
| }), | ||
| ); | ||
|
|
||
| return next(); | ||
| }); |
Check failure
Code scanning / CodeQL
Missing rate limiting
camargo
added
refactor
goetzrrGit
requested changes
Mar 7, 2023
|
Could you try to rebase again as this PR has @mattdailis and @camargo commits mixed into yours. |
cohansen
force-pushed
the
feature/refactor-app
branch
2 times, most recently
from
dbe53a7 to
ed31a0f
Compare
Yeah I accidentally merged it with |
camargo
approved these changes
Mar 14, 2023
cohansen
force-pushed
the
feature/refactor-app
branch
from
ed31a0f to
7b51207
Compare
goetzrrGit
approved these changes
Mar 14, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Just a small refactor to split out some of the endpoints. See the comment below and we should discuss moving the last few endpoints into another (?) file.
This should be merged after Ryan fixes the issue with #714. There might be some changes needed later that's merged.
Verification
Tested manually and ran unit tests.
Documentation
NA
Future work
Possibly some rename, or moving
put-dictionary,get-activity-typescript, andget-command-typescriptinto another file.