Layer Routes With Layer, Page, and Line Class Refactor#469
Conversation
cubap
left a comment
cubap
left a comment
There was a problem hiding this comment.
Generally acceptable, but I am worried only about the change to the constructor.
| * @seeAlso {@link Layer.build} | ||
| */ | ||
| constructor(projectId, { id, label, pages, creator = null }) { | ||
| constructor(projectId, { id, label, pages, creator = null, total, first, last }) { |
There was a problem hiding this comment.
I think for a constructor, this is information that we don't need, since it is generated from the pages array.
| this.#setRerumId() | ||
| await this.#saveCollectionToRerum() | ||
| } | ||
| this.total = this.pages.length |
There was a problem hiding this comment.
This is generated on .update() so it doesn't matter what it was constructed to. This will break things on accident.
| id: this.id, | ||
| type: 'AnnotationCollection', | ||
| label: { "none": [this.label] }, | ||
| total: this.total, |
There was a problem hiding this comment.
makes sense to generate the first/last/total here instead of requiring it.
| total: this.pages.length, | ||
| first: this.pages.at(0).id, | ||
| last: this.pages.at(-1).id | ||
| first: this.pages.at(0)?.id, |
There was a problem hiding this comment.
This is also not respecting the constructed first/last
| return res.status(200).json(layerAsCollection) | ||
| const layer = await findLayerById(layerId, projectId) | ||
| const layerJson = await layer.asJSON(true) | ||
| return res.status(200).json(layerJson) |
| if (layer.creator) layerAsCollection.creator = layer.creator | ||
| return res.status(200).json(layerAsCollection) | ||
| const layer = await findLayerById(layerId, projectId) | ||
| const layerJson = await layer.asJSON(true) |
There was a problem hiding this comment.
This is JSON-LD by default. Is that what we want here?
| return respondWithError(res, 404, `Line with ID '${req.params.lineId}' not found in page '${req.params.pageId}'`) | ||
| } | ||
| if (!(oldLine.id && oldLine.target && oldLine.body)) oldLine = await fetch(oldLine.id).then(res => res.json()) | ||
| if (!(oldLine.id && oldLine.target && oldLine.body)) oldLine = await fetch(oldLine.id).then(resp => resp.json()) |
| } | ||
| await withOptimisticLocking( | ||
| () => updatePageAndProject(page, project, user._id, true), | ||
| () => updatePageAndProject(page, project, user._id), |
There was a problem hiding this comment.
Looking at the blame... I'm not sure what that true was there for before.
Closes #460
Discovered #468, #470, and #471. Tracked separately, out of scope.
Summary
Refactors the
Layerclass, layer/page/line route handlers, and shared utilities to consolidate AnnotationCollection formatting logic into theLayerclass, improve error handling, and eliminate redundant project fetching patterns.Changes
classes/Layer/Layer.js#hydratedflag to track whether the Layer has been fully loaded from RERUMasJSON(isLD)method — returns the Layer as a W3C AnnotationCollection (JSON-LD whenisLD=true, simple object otherwise). This centralizes the formatting that was previously inline in route handlers#loadAnnotationCollectionDataFromRerum()— fetches and syncs Layer properties from RERUM, called lazily byasJSON()when the Layer hasn't been hydrated yettotal,first,lastconstructor params with defaults derived frompages@contextfromhttp://www.w3.org/ns/anno.jsonldtohttp://iiif.io/api/presentation/3/context.json(IIIF Presentation 3.0)#saveCollectionToRerum()with proper RERUM fetch error wrapping (status 502)pages.at(0)?.idandpages.at(-1)?.idto prevent crashes on empty page arrayslayer/index.jsfindLayerById()+layer.asJSON(true)handleVersionConflictimport and explicit 409 handling in the catch block; addedres.headersSentguards to prevent double-response crashes; response now useslayer.asJSON(true)for consistent formattingProject.getById()call (data is loaded bycheckUserAccess).all()error message to mention both GET and PUTline/index.jsgetProjectById()calls withProject.getById()or relying oncheckUserAccessto load project dataifNewContentparameter fromupdatePageAndProject()callsif (res.headersSent) returnguards afterwithOptimisticLockingand in catch blocksresvariable torespin fetch callbacksif (!project?.data)guards with normalized 404 responses in all route handlerspage/index.jsProject.getById()calls —checkUserAccessalready loads project data via#load()projectObj→projectvariable naming!project/!project?._idto!project?.datautilities/shared.jsgetProjectById()— callers now useProject.getById()directly or rely oncheckUserAccessupdateLayerAndProject()— addedlayerIndex < 0guard (404); wrapped update logic in try-catch with proper error wrapping; refactored page overwrites from.forEach(async ...)+.push()anti-pattern to.filter().map()withPromise.all; added separatepartOfupdate for all pages with optional chaining safetyfindLayerById()— removedrerumparameter (RERUM fetching is now handled byLayer.asJSON()via#loadAnnotationCollectionDataFromRerum()); removed short numeric ID index-based lookup; now passescreatorto the Layer constructorCross-cutting: Error message normalization
`Project ${projectId} was not found`