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

Begin alignment with new API #471

Merged
merged 11 commits into from
Dec 15, 2023
Merged

Begin alignment with new API #471

merged 11 commits into from
Dec 15, 2023

Conversation

TShapinsky
Copy link
Member

@TShapinsky TShapinsky commented Dec 4, 2023

API:

  • New API endpoint structure
  • Internal API functions now use Run and Point objects as parameters instead of the reference Id
  • All mentions of site in the api have changed to run
  • Aliases can replace runIds in the url (you no longer have to get the alias, you can just use it instead of the runId)
  • Added error handler. Now API errors are passed through the API instead of being silenced.
  • Run and Point objects are fetched by preprocessors to make handling consistent.
  • Writing point values now validates points before committing writes.
  • Response format is now consistent
  • sendRunMessage api function will now reject a promise if it times out

Worker:

  • New OS measure to extract building name. Now works for E+ models as well.

Model:

  • Added metadata endpoint for single models
  • Added download endpoint for single models

Run:

  • Now includes a name which defaults to the uploaded model.
  • Deprecated site object

TODO

  • Add API tests outside the functionality of the alfalfa-client. There are a lot of endpoints that it doesn't use.
  • Integrate new api documentation with old api documentation

<IconButton onClick={onClose}>
<Close />
</IconButton>
</Grid>
</DialogTitle>
<DialogContent>
<pre style={{ whiteSpace: "pre-wrap" }}>{site.errorLog}</pre>
<pre style={{ whiteSpace: "pre-wrap" }}>{run.errorLog}</pre>
</DialogContent>
Copy link
Member Author

Choose a reason for hiding this comment

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

Changing from site to run

@@ -90,7 +100,7 @@ export const PointDialog = ({ onClose, site }) => {
<Dialog fullWidth={true} maxWidth="lg" open={true} onClose={onClose}>
<DialogTitle>
<Grid container justifyContent="space-between" alignItems="center">
<span>{`${site.name} Points`}</span>
<span>{`${run.name} Points`}</span>
<IconButton onClick={onClose}>
<Close />
</IconButton>
Copy link
Member Author

Choose a reason for hiding this comment

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

changing from site to run

start: ["READY"],
stop: ["PREPROCESSING", "STARTING", "STARTED", "RUNNING", "STOPPING"],
remove: ["READY", "COMPLETE", "ERROR"],
download: ["READY", "COMPLETE", "ERROR"]
Copy link
Member Author

Choose a reason for hiding this comment

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

I got rid of the code which was lowercasing all of the statuses when they went through the api

sx={{ m: 1 }}>
Download Site
<Button variant="contained" disabled={isDownloadButtonDisabled()} onClick={handleDownloadRun} sx={{ m: 1 }}>
Download Run
</Button>
Copy link
Member Author

Choose a reason for hiding this comment

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

Rest of file is migrating from sites to runs

res.status(500);
res.json({ error: JSON.parse(JSON.stringify(err, Object.getOwnPropertyNames(err))) });
};

Copy link
Member Author

Choose a reason for hiding this comment

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

This allows all api endpoints to call next(err) when they get an error and it will be properly and consistently handled

});

router.param("runId", (req, res, next, id) => {
const error = validate(
Copy link
Member Author

Choose a reason for hiding this comment

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

Whenever runId is a param in the URL this function will parse the id and add the run object to the request object. This allows all downstream function to not have to worry about validating and processing the id. This also allows me to check if an alias exists in cases where the id doesn't correspond to a run.

}
});

router.param("pointId", (req, res, next, id) => {
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the same but just with pointId instead of runId

.catch(next);
});

router.param("modelId", (req, res, next, id) => {
Copy link
Member Author

Choose a reason for hiding this comment

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

same again but for modelId

return res.json({ data });
} else {
return res.status(404).json({
error: `Site ID '${siteId}' does not exist`
Copy link
Member Author

Choose a reason for hiding this comment

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

This 404 has been removed as now that is handled by the pre-processor. You'll see this a lot in this file

})
.catch(() => res.sendStatus(500));
.catch(next);
Copy link
Member Author

Choose a reason for hiding this comment

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

This catch will funnel all errors to the single error handler. You'll see this a lot in this file

error: `Site ID '${siteId}' does not exist`
});
}
.formatRun(req.run)
Copy link
Member Author

Choose a reason for hiding this comment

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

Requests to the internal api are now done with the run object instead of the id. This helps make everything more consistent and also allows us to change the id in the future without having to rewrite everything.

api
.getPointsByRun(req.run)
.then((points) => {
return points.map(api.formatPoint);
Copy link
Member Author

Choose a reason for hiding this comment

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

All db objects now have an accompanying format function which is used to present them to the user.

});
})
.catch((err) => {
errors.push(JSON.parse(JSON.stringify(err, Object.getOwnPropertyNames(err))));
Copy link
Member Author

Choose a reason for hiding this comment

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

validate point writes and collate errors if there are ones.


router.all("/aliases/:alias", (req, res, next) => {
Copy link
Member Author

Choose a reason for hiding this comment

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

This does a similar thing as the other pre-processors. The reason this is an all instead of param is because that would fail on the PUT endpoint. Because when you PUT an alias it doesn't have to exist yet. So if you ever tried to create an alias you'd get a 404 because it didn't exist yet.

@@ -38,64 +37,53 @@ class AlfalfaAPI {
});
}
Copy link
Member Author

Choose a reason for hiding this comment

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

Main thing in this file is making the api more consistent and also having functions operate on the db objects instead of their ids.

return this.runs.findOne({ _id: runObjectId });
};

formatRun = async (run) => {
Copy link
Member Author

Choose a reason for hiding this comment

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

This was previously in two places, now it's a function.

alfalfa_web/server/api.js Outdated Show resolved Hide resolved
modified
}));
} catch (e) {
console.error(e);
Copy link
Member Author

Choose a reason for hiding this comment

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

This kind of error handling was removed in favor of top level error handling to make the api more communicative about what went wrong.

console.error(e);
return Promise.reject();
}
setAlias = async (alias, run) => {
Copy link
Member Author

Choose a reason for hiding this comment

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

Aliases now use the ObjectID of a run instead of it's ref_id this is intended to make the look ups more performant as ref_id may not be indexed.

@@ -46,6 +46,13 @@ def exec(self):
# run workflow
check_output(['openstudio', '--gem_path', '/var/lib/gems/2.7.0', 'run', '-m', '-w', str(submitted_osw_path)])

self.logger.info('Loading Building Metadata')
Copy link
Member Author

Choose a reason for hiding this comment

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

Runs now contain the name of the run. This is extracted by a measure which creates a report including this and possibly other data in the future.

@anyaelena
Copy link
Member

Closes #472

@anyaelena anyaelena self-requested a review December 15, 2023 13:25
anyaelena
anyaelena previously approved these changes Dec 15, 2023
Copy link
Member

@anyaelena anyaelena left a comment

Choose a reason for hiding this comment

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

Thank you for turning this around so quickly!

@anyaelena anyaelena self-requested a review December 15, 2023 21:36
@anyaelena anyaelena merged commit 8796ba7 into develop Dec 15, 2023
6 checks passed
@anyaelena anyaelena deleted the boptest_aligment branch December 15, 2023 21:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants