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.

Sign up for GitHub

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

Jump to bottom

BB-672: Rewrite entity folder using async/await #980

Closed
wants to merge 10 commits into from
Closed

BB-672: Rewrite entity folder using async/await #980

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
bb500aa
BB-672: Rewrite entity folder using async/await
allgandalf Mar 28, 2023
1855318
Update publisher.js
allgandalf Mar 28, 2023
6f4630a
Update publisher.js
allgandalf Mar 28, 2023
ab3d97a
Update publisher.js
allgandalf Mar 28, 2023
9364f9f
Update work.js
allgandalf Mar 28, 2023
69f6468
Indentation Fixed
allgandalf Mar 28, 2023
66ea258
Merge branch 'master' into asyncentity
allgandalf May 18, 2023
f2af265
fix: Solve ESLint errors
allgandalf May 18, 2023
14084b0
fix: Solve ESLint errors
allgandalf May 18, 2023
fc5f27a
fix: Solve ESLint errors
allgandalf May 18, 2023
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
20 changes: 7 additions & 13 deletions src/server/routes/entity/publisher.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -190,7 +190,7 @@ function _setPublisherTitle(res) {
}


router.get('/:bbid', middleware.loadEntityRelationships, middleware.loadWikipediaExtract, (req, res, next) => {
router.get('/:bbid', middleware.loadEntityRelationships, middleware.loadWikipediaExtract, async (req, res, next) => {
// Fetch editions
const {Publisher} = req.app.locals.orm;
const editionRelationsToFetch = [
Expand All @@ -201,18 +201,12 @@ router.get('/:bbid', middleware.loadEntityRelationships, middleware.loadWikipedi
'editionFormat',
'authorCredit.names'
];
const editionsPromise =
Publisher.forge({bbid: res.locals.entity.bbid})
.editions({withRelated: editionRelationsToFetch});

return editionsPromise
.then((editions) => {
res.locals.entity.editions = editions.toJSON();
_setPublisherTitle(res);
res.locals.entity.editions.sort(entityRoutes.compareEntitiesByDate);
return entityRoutes.displayEntity(req, res);
})
.catch(next);
const editions = await Publisher.forge({bbid: res.locals.entity.bbid})
.editions({withRelated: editionRelationsToFetch});
res.locals.entity.editions = editions.toJSON();
_setPublisherTitle(res);
res.locals.entity.editions.sort(entityRoutes.compareEntitiesByDate);
await entityRoutes.displayEntity(req, res);
Comment on lines +204 to +209
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be wrapped in a try/catch block in case one of these promises fails (for example the entity does not exist, etc.).

In the catch block, you can simply call the next function with the error (which is what we were doing before with .catch(next)).

});

router.get('/:bbid/delete', auth.isAuthenticated, (req, res, next) => {
Expand Down
75 changes: 43 additions & 32 deletions src/server/routes/entity/work.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,37 +92,45 @@ router.get(
'/create', auth.isAuthenticated, middleware.loadIdentifierTypes,
middleware.loadLanguages, middleware.loadWorkTypes,
middleware.loadRelationshipTypes,
(req, res, next) => {
async (req, res, next) => {
const {Author, Edition} = req.app.locals.orm;
let relationshipTypeId;
let initialRelationshipIndex = 0;
const propsPromise = generateEntityProps(
const propsPromise = await generateEntityProps(
'work', req, res, {}
);

if (req.query.author) {
propsPromise.author =
Author.forge({bbid: req.query.author})
.fetch({require: false, withRelated: 'defaultAlias'})
.then((data) => data && utils.entityToOption(data.toJSON()));
try {
const data = await Author.forge({bbid: req.query.author})
.fetch({require: false, withRelated: 'defaultAlias'});
propsPromise.author = data && utils.entityToOption(data.toJSON());
}
catch (err) {
log.debug(err);
Copy link
Contributor

Choose a reason for hiding this comment

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

I will suggest that we call next in all these catch blocks, like I described above, so that we catch errors a bit better.

Currently we don't get an error, but the user won't know why the entity hasn't been added to the initial relationships:
For example, with a non existing BBID: https://bookbrainz.org/work/create?author=f1906497-5614-4793-9f51-a3870f3edcad
Page loads, but no relationship (because no author entity with that BBID); I think it would be better for the user to get an error message instead.

Should be as simple as

Suggested change
log.debug(err);
log.debug(err);
return next(err);

}
}

if (req.query.edition) {
propsPromise.edition =
Edition.forge({bbid: req.query.edition})
.fetch({require: false, withRelated: 'defaultAlias'})
.then((data) => data && utils.entityToOption(data.toJSON()));
try {
const data = await Edition.forge({bbid: req.query.edition})
.fetch({require: false, withRelated: 'defaultAlias'});
propsPromise.edition = data && utils.entityToOption(data.toJSON());
}
catch (err) {
log.debug(err);
}
}

async function render(props) {
if (props.author) {
// add initial ralationship with relationshipTypeId = 8 (<Work> is written by <Author>)
// add initial relationship with relationshipTypeId = 8 (<Work> is written by <Author>)
relationshipTypeId = RelationshipTypes.AuthorWroteWork;
addInitialRelationship(props, relationshipTypeId, initialRelationshipIndex++, props.author);
}

if (props.edition) {
// add initial ralationship with relationshipTypeId = 10 (<Work> is contained in <Edition>)
// add initial relationship with relationshipTypeId = 10 (<Work> is contained in <Edition>)
relationshipTypeId = RelationshipTypes.EditionContainsWork;
addInitialRelationship(props, relationshipTypeId, initialRelationshipIndex++, props.edition);
}
Expand Down Expand Up @@ -155,44 +163,47 @@ router.get(
title: props.heading
}));
}
makePromiseFromObject(propsPromise)
.then(render)
.catch(next);
try {
const props = await makePromiseFromObject(propsPromise);
await render(props);
}
catch (err) {
log.debug(err);
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here, we want tot catch the error and pass it along as we did before:

Suggested change
log.debug(err);
log.debug(err);
next(err);

}
}
);


Copy link
Contributor

Choose a reason for hiding this comment

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

The file src/server/routes/entity/edition.ts also has the same refactor you can apply for the '/create' routes at least:
https://github.com/metabrainz/bookbrainz-site/blob/master/src/server/routes/entity/edition.ts#L278-L295

router.post(
'/create', entityRoutes.displayPreview, auth.isAuthenticatedForHandler, middleware.loadIdentifierTypes,
middleware.loadLanguages, middleware.loadWorkTypes,
middleware.loadRelationshipTypes,
async (req, res, next) => {
const {WorkType} = req.app.locals.orm;
const entity = await utils.parseInitialState(req, 'work');
if (entity.workSection?.type) {
entity.workSection.type = await utils.getIdByField(WorkType, 'label', entity.workSection.type);
}
if (entity.workSection) {
entity.workSection = await utils.parseLanguages(entity.workSection, req.app.locals.orm);
}
const propsPromise = generateEntityProps(
'work', req, res, {}, () => entity
);

function render(props) {
try {
const entity = await utils.parseInitialState(req, 'work');
if (entity.workSection?.type) {
entity.workSection.type = await utils.getIdByField(WorkType, 'label', entity.workSection.type);
}
if (entity.workSection) {
entity.workSection = await utils.parseLanguages(entity.workSection, req.app.locals.orm);
}
const props = await generateEntityProps(
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know why this was set up that way previously with makePromiseFromObject, but generateEntityProps does not return a promise, so there's no need to await it.

'work', req, res, {}, () => entity
);
const editorMarkup = entityEditorMarkup(props);
const {markup} = editorMarkup;
const updatedProps = editorMarkup.props;

return res.send(target({
res.send(target({
markup,
props: escapeProps(updatedProps),
script: '/js/entity-editor.js',
title: props.heading
}));
}
makePromiseFromObject(propsPromise)
.then(render)
.catch(next);
catch (err) {
log.debug(err);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Same deal here, we need to froward the error once caught with next(err);

}
);

Expand Down