Skip to content

Commit 8e5e4ba

Browse files
Copilotthehabes
Copilot
and
thehabes
authored
Eliminate redundant project DB fetches across route handlers (#478)
* Initial plan * initial plan: eliminate redundant project data queries Co-authored-by: thehabes <3287006+thehabes@users.noreply.github.com> * eliminate redundant project data queries across route handlers - Add optional `project` param to `findPageById()` and `findLayerById()` in shared.js so callers with a pre-loaded project can skip the redundant DB fetch - Pass pre-loaded `project` to `findLayerById()` and `findPageById()` in layer/index.js - Pass pre-loaded `project` to `findPageById()` in page/index.js PUT /:pageId - Pass pre-loaded `project` to `findPageById()` in line/index.js (POST, PUT, PATCH×2) - Use `project.data` directly in customMetadataRouter.js GET/POST/PUT /:id/custom instead of calling `database.findOne()` again after `checkUserAccess()` already loaded it - Restructure projectReadRouter.js GET /:id to call `loadAsUser()` once and derive the access check from its result via new `userHasAccess()` helper (saves project+group load) - Remove redundant `loadAsUser()` call in projectReadRouter.js GET /:id/manifest; use `project.data` from `checkUserAccess()` for the existence check instead - Add optional `preloadedProjectData` param to `ProjectFactory.exportManifest()` so callers with already-loaded data skip the internal `loadAsUser()` fetch Co-authored-by: thehabes <3287006+thehabes@users.noreply.github.com> * address code review: add Array.isArray guard in userHasAccess, add comment on loadAsUser error pattern Co-authored-by: thehabes <3287006+thehabes@users.noreply.github.com> * no package lock changes * changes during review * changes during review * Changes during review * Changes during review --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: thehabes <3287006+thehabes@users.noreply.github.com> Co-authored-by: Bryan Haberberger <bryan.j.haberberger@slu.edu>
1 parent 9240ab3 commit 8e5e4ba

7 files changed

Lines changed: 65 additions & 28 deletions

File tree

‎classes/Project/ProjectFactory.js‎

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -729,19 +729,23 @@ export default class ProjectFactory {
729729
* manifest data is assembled, and the final JSON is saved to the filesystem.
730730
*
731731
* @param {string} projectId - Project ID for a specific project.
732+
* @param {Object|null} preloadedProjectData - Pre-loaded project data to avoid a redundant DB query. Falls back to loadAsUser() if null.
732733
* @returns {Object} - Returns the assembled IIIF manifest object.
733734
*
734735
* The manifest follows the IIIF Presentation API 3.0 specification and includes:
735736
* - Context, ID, Type, Label, Metadata, Items and Annotations
736737
* - A dynamically fetched list of manifest items, including canvases and their annotations.
737738
* - All elements are embedded in the manifest object.
738739
*/
739-
static async exportManifest(projectId) {
740+
static async exportManifest(projectId, preloadedProjectData = null) {
740741
if (!projectId) {
741742
throw { status: 400, message: "No project ID provided" }
742743
}
743744

744-
const project = await ProjectFactory.loadAsUser(projectId, null)
745+
const project = preloadedProjectData ?? await ProjectFactory.loadAsUser(projectId, null)
746+
if (!project || project instanceof Error) {
747+
throw { status: project?.status || 404, message: project?.message || `No project found with ID '${projectId}'` }
748+
}
745749
const manifestJson = await this.fetchJson(project.manifest[0])
746750

747751
const manifest = {
@@ -1003,7 +1007,7 @@ export default class ProjectFactory {
10031007
},
10041008
{
10051009
$set: {
1006-
roles: { $mergeObjects: [{ $ifNull: ['$thisGroup.customRoles', {}] }, Group.defaultRoles] },
1010+
roles: { $mergeObjects: [Group.defaultRoles, { $ifNull: ['$thisGroup.customRoles', {}] }] },
10071011
}
10081012
},
10091013
{

‎layer/index.js‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,7 @@ router.route('/:layerId')
4242
return respondWithError(res, 403, 'You do not have permission to update this layer')
4343
}
4444
if (!project?.data) return respondWithError(res, 404, `Project ${projectId} was not found`)
45-
const layer = await findLayerById(layerId, projectId)
45+
const layer = await findLayerById(layerId, projectId, project)
4646
// Only update top-level properties that are present in the request
4747
Object.keys(update ?? {}).forEach(key => {
4848
layer[key] = update[key]
@@ -56,7 +56,7 @@ router.route('/:layerId')
5656
})
5757
let pages = []
5858
if (providedPages && Array.isArray(providedPages) && providedPages.length > 0) {
59-
pages = await Promise.all(providedPages.map(p => findPageById(p.split("/").pop(), projectId) ))
59+
pages = await Promise.all(providedPages.map(p => findPageById(p.split("/").pop(), projectId, project) ))
6060
layer.pages = pages
6161
}
6262
await updateLayerAndProject(layer, project, user._id)

‎line/index.js‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -56,7 +56,7 @@ router.post('/', auth0Middleware(), async (req, res) => {
5656
return respondWithError(res, 403, 'You do not have permission to create lines in this project')
5757
}
5858
if (!project?.data) return respondWithError(res, 404, `Project ${req.params.projectId} was not found`)
59-
const page = await findPageById(req.params.pageId, req.params.projectId)
59+
const page = await findPageById(req.params.pageId, req.params.projectId, project)
6060

6161
if (!req.body || (Array.isArray(req.body) && req.body.length === 0)) {
6262
return respondWithError(res, 400, "Request body with line data is required")
@@ -120,7 +120,7 @@ router.put('/:lineId', auth0Middleware(), screenContentMiddleware(), async (req,
120120
return respondWithError(res, 403, 'You do not have permission to update lines in this project')
121121
}
122122
if (!project?.data) return respondWithError(res, 404, `Project ${req.params.projectId} was not found`)
123-
const page = await findPageById(req.params.pageId, req.params.projectId)
123+
const page = await findPageById(req.params.pageId, req.params.projectId, project)
124124
let oldLine = page.items?.find(l => l.id.split('/').pop() === req.params.lineId?.split('/').pop())
125125
if (!oldLine) {
126126
return respondWithError(res, 404, `Line with ID '${req.params.lineId}' not found in page '${req.params.pageId}'`)
@@ -202,7 +202,7 @@ router.patch('/:lineId/text', auth0Middleware(), screenContentMiddleware(), asyn
202202
if (typeof req.body !== 'string') {
203203
return respondWithError(res, 400, 'Invalid request body. Expected a string.')
204204
}
205-
const page = await findPageById(req.params.pageId, req.params.projectId)
205+
const page = await findPageById(req.params.pageId, req.params.projectId, project)
206206
const oldLine = page.items?.find(l => l.id.split('/').pop() === req.params.lineId?.split('/').pop())
207207
if (!oldLine) {
208208
return respondWithError(res, 404, `Line with ID '${req.params.lineId}' not found in page '${req.params.pageId}'`)
@@ -282,7 +282,7 @@ router.patch('/:lineId/bounds', auth0Middleware(), async (req, res) => {
282282
return respondWithError(res, 400, 'Invalid request body. Expected an object with x, y, w, and h as non-negative integers.')
283283
}
284284
const bounds = { x: parseInt(req.body.x, 10), y: parseInt(req.body.y, 10), w: parseInt(req.body.w, 10), h: parseInt(req.body.h, 10) }
285-
const page = await findPageById(req.params.pageId, req.params.projectId)
285+
const page = await findPageById(req.params.pageId, req.params.projectId, project)
286286
const findOldLine = page.items?.find(l => l.id.split('/').pop() === req.params.lineId?.split('/').pop())
287287
if (!findOldLine) {
288288
return respondWithError(res, 404, `Line with ID '${req.params.lineId}' not found in page '${req.params.pageId}'`)

‎page/index.js‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -91,7 +91,7 @@ router.route('/:pageId')
9191
try {
9292
if (hasSuspiciousPageData(req.body)) return respondWithError(res, 400, "Suspicious page data will not be processed.")
9393
// Find the page object
94-
const page = await findPageById(pageId, projectId)
94+
const page = await findPageById(pageId, projectId, project)
9595
page.creator = user.agent.split('/').pop()
9696
page.partOf = layerId
9797

‎project/customMetadataRouter.js‎

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -90,8 +90,7 @@ router.route("/:id/custom").get(auth0Middleware(), async (req, res) => {
9090
return respondWithError(res, 403, "You do not have permission to read this project's metadata")
9191
}
9292

93-
// Fetch the project from database
94-
const projectData = await database.findOne({ _id: id }, "projects")
93+
const projectData = project.data
9594

9695
if (!projectData) {
9796
return respondWithError(res, 404, `No TPEN3 project with ID '${id}' found`)
@@ -139,8 +138,8 @@ router.route("/:id/custom").post(auth0Middleware(), async (req, res) => {
139138
// Get namespace from request origin
140139
const namespace = getNamespaceFromOrigin(req)
141140

142-
// Fetch the full project data
143-
const projectData = await database.findOne({ _id: id }, "projects")
141+
// Use the already-loaded project data
142+
const projectData = project.data
144143

145144
if (!projectData) {
146145
return respondWithError(res, 404, `No TPEN3 project with ID '${id}' found`)
@@ -195,8 +194,8 @@ router.route("/:id/custom").put(auth0Middleware(), async (req, res) => {
195194
// Get namespace from request origin
196195
const namespace = getNamespaceFromOrigin(req)
197196

198-
// Fetch the full project data
199-
const projectData = await database.findOne({ _id: id }, "projects")
197+
// Use the already-loaded project data
198+
const projectData = project.data
200199

201200
if (!projectData) {
202201
return respondWithError(res, 404, `No TPEN3 project with ID '${id}' found`)

‎project/projectReadRouter.js‎

Lines changed: 42 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -92,6 +92,37 @@ function filterProjectInterfaces(project, namespaces) {
9292
}
9393
}
9494

95+
/**
96+
* Check whether a user has the required access on a project loaded via ProjectFactory.loadAsUser().
97+
* This avoids a second database round-trip when the project data (with collaborators/roles) is
98+
* already available from the loadAsUser aggregation result.
99+
*
100+
* @param {Object} projectData - The project object returned by ProjectFactory.loadAsUser()
101+
* @param {string} userId - The user ID to check
102+
* @param {string} action - Required action (e.g. ACTIONS.READ)
103+
* @param {string} scope - Required scope (e.g. SCOPES.ALL)
104+
* @param {string} entity - Required entity (e.g. ENTITIES.PROJECT)
105+
* @returns {boolean}
106+
*/
107+
function userHasAccess(projectData, userId, action, scope, entity) {
108+
const userRoleNames = projectData?.collaborators?.[userId]?.roles
109+
if (!userRoleNames || !Array.isArray(userRoleNames)) return false
110+
const rolePermissions = projectData.roles ?? {}
111+
return userRoleNames.some(role => {
112+
let perms = rolePermissions[role]
113+
if (!perms) return false
114+
// Custom roles may store permissions as a space-delimited string
115+
if (typeof perms === 'string') perms = perms.split(' ')
116+
if (!Array.isArray(perms)) return false
117+
return perms.some(perm => {
118+
const [permAction, permScope, permEntity] = perm.split("_")
119+
return (permAction === action || permAction === "*") &&
120+
(permScope === scope || permScope === "*") &&
121+
(permEntity === entity || permEntity === "*")
122+
})
123+
})
124+
}
125+
95126

96127
router.route("/:id/manifest").get(auth0Middleware(), async (req, res) => {
97128
const { id } = req.params
@@ -100,14 +131,14 @@ router.route("/:id/manifest").get(auth0Middleware(), async (req, res) => {
100131
if (!id) return respondWithError(res, 400, "No TPEN3 ID provided")
101132
if (!validateID(id)) return respondWithError(res, 400, "The TPEN3 project ID provided is invalid")
102133
try {
103-
if (!await new Project(id).checkUserAccess(user._id, ACTIONS.UPDATE, SCOPES.ALL, ENTITIES.PROJECT)) {
134+
const project = new Project(id)
135+
if (!await project.checkUserAccess(user._id, ACTIONS.UPDATE, SCOPES.ALL, ENTITIES.PROJECT)) {
104136
return respondWithError(res, 403, "You do not have permission to export this project")
105137
}
106-
const project = await ProjectFactory.loadAsUser(id, null)
107-
if (!project) {
138+
if (!project.data) {
108139
return respondWithError(res, 404, `No TPEN3 project with ID '${id}' found`)
109140
}
110-
const manifest = await ProjectFactory.exportManifest(id)
141+
const manifest = await ProjectFactory.exportManifest(id, project.data)
111142
await ProjectFactory.uploadFileToGitHub(manifest, `${id}`)
112143
res.status(200).json(manifest)
113144
} catch (error) {
@@ -158,14 +189,17 @@ router.route("/:id").get(auth0Middleware(), async (req, res) => {
158189
if (!id) return respondWithError(res, 400, "No TPEN3 ID provided")
159190
if (!validateID(id)) return respondWithError(res, 400, "The TPEN3 project ID provided is invalid")
160191
try {
161-
const project = new Project(id)
162-
if (!(await project.checkUserAccess(user._id, ACTIONS.READ, SCOPES.ALL, ENTITIES.PROJECT))) {
163-
return respondWithError(res, 403, "You do not have permission to view this project")
164-
}
192+
// loadAsUser() returns an Error object (not throws) on DB failure
165193
const projectData = await ProjectFactory.loadAsUser(id, user._id)
194+
if (projectData instanceof Error) {
195+
return respondWithError(res, projectData.status || 500, projectData.message ?? "An error occurred while fetching the project data.")
196+
}
166197
if (!projectData) {
167198
return respondWithError(res, 404, `No TPEN3 project with ID '${id}' found`)
168199
}
200+
if (!userHasAccess(projectData, user._id, ACTIONS.READ, SCOPES.ALL, ENTITIES.PROJECT)) {
201+
return respondWithError(res, 403, "You do not have permission to view this project")
202+
}
169203

170204
// Filter interfaces based on origin and query parameters
171205
const namespacesToInclude = getNamespacesToInclude(req)

‎utilities/shared.js‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -252,8 +252,8 @@ export const getLayerContainingPage = (project, pageId) => {
252252
}
253253

254254
// Find a page by ID (moved from page/index.js)
255-
export async function findPageById(pageId, projectId) {
256-
const projectData = (await Project.getById(projectId))?.data
255+
export async function findPageById(pageId, projectId, project = null) {
256+
const projectData = project?.data ?? (await Project.getById(projectId))?.data
257257
if (!projectData) {
258258
const error = new Error(`Project ${projectId} was not found`)
259259
error.status = 404
@@ -279,8 +279,8 @@ export async function findPageById(pageId, projectId) {
279279
return new Page(layerContainingPage.id, page)
280280
}
281281

282-
export async function findLayerById(layerId, projectId) {
283-
const p = await Project.getById(projectId)
282+
export async function findLayerById(layerId, projectId, project = null) {
283+
const p = project?.data ? project : await Project.getById(projectId)
284284
if (!p?.data) {
285285
const error = new Error(`Project ${projectId} was not found`)
286286
error.status = 404

0 commit comments

Comments
 (0)