Scope authoring resource lookups to the action weblog - #162
Conversation
Authoring actions resolve the resource named by a request parameter by id alone, independently of the weblog the action is operating on. Add weblog-scoped lookups beside the existing unscoped ones, modelled on getTemplateByLink(Weblog, String), and point the authoring actions at them: WeblogManager getTemplate(Weblog, String) WeblogEntryManager getWeblogEntry / getWeblogCategory / getComment BookmarkManager getBookmark, getFolderById MediaFileManager getMediaFile, getMediaFileDirectory Each is backed by a named query filtering on the owning weblog, and returns null when the id does not belong to that weblog, the same as for an id that does not exist. The bookmark folder lookup is named getFolderById because getFolder(Weblog, String) already exists as the by-name lookup and the two would otherwise share an erasure. Call sites updated across templates, entries, categories, comments, bookmarks, folders and media files. The two comment loops in Comments already compared the weblog after loading; they now use the scoped lookup instead, which also removes an NPE on ids that do not exist. Left unchanged, having been verified to resolve correctly by other means: EntryBean, StylesheetEdit, CommentDataServlet, and the XML-RPC and Atom handlers, which derive the weblog from the entity itself. Tests: WeblogScopedLookupTest (manager layer), TemplateEditScopingTest and AuthoringActionScopingTest (action layer). Full suite 182 run, 0 failures.
There was a problem hiding this comment.
Reviewed with the multi-agent find-and-verify pass. The scoping is doing its job: ids from other weblogs no longer resolve, and the manager overloads follow the existing getTemplateByLink shape nicely. What the pass turned up is the second half of that change: the actions were written for lookups that always returned something, so the new nulls become NPEs (uncaught 500s, since most catches only handle WebloggerException) on crafted or stale ids. Each site is inline. WeblogConfig is the one to fix first, since it persists a null category that then breaks the Settings page for that weblog on every save; Comments is the odd one, where null silently widens the listing to the whole weblog instead of failing.
EntryBean.java line 308 still uses the unscoped getWeblogCategory(id) with a hand-written ownership check on entry save; the scoped overload this PR adds would let that go away too. Holding approval for the null handling; happy to approve once that's in, since the underlying fix is the right one.
| if(getBean().getBloggerCategoryId() != null && | ||
| !weblog.getBloggerCategory().getId().equals(getBean().getBloggerCategoryId())) { | ||
| weblog.setBloggerCategory(wmgr.getWeblogCategory(getBean().getBloggerCategoryId())); | ||
| weblog.setBloggerCategory(wmgr.getWeblogCategory(getActionWeblog(), getBean().getBloggerCategoryId())); |
There was a problem hiding this comment.
This one persists bad data: when bloggerCategoryId doesn't resolve inside the action weblog, the scoped lookup returns null and setBloggerCategory(null) is committed (bloggercatid is nullable). From then on line 138 dereferences weblog.getBloggerCategory() on every save, inside a catch that only handles WebloggerException, so the Settings page for that weblog 500s until someone repairs the row. A null check that adds a validation error and leaves the existing category alone closes it.
| MediaFileManager manager = WebloggerFactory.getWeblogger() | ||
| .getMediaFileManager(); | ||
| MediaFile mediaFile = manager.getMediaFile(this.mediaFileId); | ||
| MediaFile mediaFile = manager.getMediaFile(getActionWeblog(), this.mediaFileId); |
There was a problem hiding this comment.
doDeleteMediaFile here, doIncludeMediaFileInGallery (line 87), and doMoveSelected (line 150, targetDirectory) all pass the now-nullable scoped result straight through: removeMediaFile(weblog, null) ends in an IllegalArgumentException from em.remove(null), the others NPE, and the catch only covers WebloggerException. A foreign or stale id now yields a 500 page instead of the mediaFile.delete.error / includeInGallery.error / move.errors messages the code intends.
| for (int j = 0; j < bookmarks.length; j++) { | ||
| WeblogBookmark bd = bmgr.getBookmark(bookmarks[j]); | ||
| WeblogBookmark bd = bmgr.getBookmark(getActionWeblog(), bookmarks[j]); | ||
| newFolder.addBookmark(bd); |
There was a problem hiding this comment.
move() never null-checks newFolder (line 228) or bd (line 233), although delete() at line 138 does. A targetFolderId from another weblog gives newFolder == null, addBookmark NPEs past the WebloggerException catch, and the user gets a 500 rather than bookmarksForm.error.move. An unresolved selectedBookmarks entry also adds a null to the folder's in-memory list before bd.setFolder blows up.
| setDirectory(mgr.getMediaFileDirectory(getActionWeblog(), bean.getDirectoryId())); | ||
| } | ||
| } catch (WebloggerException ex) { | ||
| log.error("Error looking up media file directory", ex); |
There was a problem hiding this comment.
The directory set here can now be null for a directoryId outside the action weblog, and myValidate (line 82) calls getDirectory().getMediaFile(...) on it. save() runs myValidate before entering its try, so a foreign or deleted directory id is an unhandled NPE instead of INPUT with a validation error.
| setEntry(wmgr.getWeblogEntry(getActionWeblog(), getBean().getId())); | ||
| } catch (WebloggerException ex) { | ||
| log.error( | ||
| "Error looking up entry by id - " + getBean().getId(), |
There was a problem hiding this comment.
myPrepare now leaves entry null for any id outside the action weblog (the new test asserts exactly that), but execute() (line 126) hands it to EntryBean.copyFrom, which calls entry.getId() immediately. A co-author deleting an entry you have open, or a crafted bean.id, is now a stack-trace page rather than a not-found message. TemplateEdit already handles the same null with 'Unable to locate specified template'; CategoryEdit.execute, FolderEdit.myValidate, and BookmarkEdit.myValidate (line 123) have the same gap.
| // lookup weblog entry if necessary | ||
| if (!StringUtils.isEmpty(getBean().getEntryId())) { | ||
| setQueryEntry(wmgr.getWeblogEntry(getBean().getEntryId())); | ||
| setQueryEntry(wmgr.getWeblogEntry(getActionWeblog(), getBean().getEntryId())); |
There was a problem hiding this comment.
Different failure shape here: a foreign or stale entryId now makes queryEntry null, and null silently means 'all comments in the weblog', so the listing widens from one entry to the whole blog without any indication. Probably worth treating an unresolved entryId as an empty result or an error rather than as no filter.
| MediaFileManager mgr = WebloggerFactory.getWeblogger().getMediaFileManager(); | ||
| MediaFile mediaFile = mgr.getMediaFile(getMediaFileId()); | ||
| MediaFile mediaFile = mgr.getMediaFile(getActionWeblog(), getMediaFileId()); | ||
| bean.copyFrom(mediaFile); |
There was a problem hiding this comment.
Same family: the scoped lookup can return null and bean.copyFrom(mediaFile) dereferences it inside a catch that only handles WebloggerException.
| MediaFile mediaFile = manager.getMediaFile(getActionWeblog(), image); | ||
| String link; | ||
|
|
||
| if (mediaFile.isImageFile()) { |
There was a problem hiding this comment.
A selected image outside the action weblog now resolves to null and the loop dereferences it; the generic catch swallows the NPE, so the new-entry form silently loses the media links rather than telling the user what happened.
| if (getTargetCategoryId() != null) { | ||
| WeblogCategory target = wmgr.getWeblogCategory(getTargetCategoryId()); | ||
| WeblogCategory target = wmgr.getWeblogCategory(getActionWeblog(), getTargetCategoryId()); | ||
| wmgr.moveWeblogCategoryContents(getCategory(), target); |
There was a problem hiding this comment.
targetCategoryId outside the action weblog gives a null target here, which goes straight into moveWeblogCategoryContents.
3 participants
Summary
Roller's manager APIs expose two styles of lookup: by id, and by a weblog plus
some other key —
getTemplateByLink(Weblog, String),getTemplateByName(Weblog, String)and friends. The authoring UI operates inthe context of a single weblog throughout, but has only the id-based form
available for several resource types, so the two styles are mixed within the
same action.
This adds weblog-scoped overloads beside the existing id-based lookups and
points the authoring actions at them, so the lookups an action performs are
consistently expressed in terms of the weblog it is working on. The new
methods follow the shape of the existing
getTemplateByLink(Weblog, String)rather than introducing a different convention.
Manager methods added
WeblogManagergetTemplate(Weblog, String)WeblogTemplate.getByWeblog&IdWeblogEntryManagergetWeblogEntry(Weblog, String)WeblogEntry.getByWebsite&IdWeblogEntryManagergetWeblogCategory(Weblog, String)WeblogCategory.getByWeblog&IdWeblogEntryManagergetComment(Weblog, String)WeblogEntryComment.getByWebsite&IdBookmarkManagergetBookmark(Weblog, String)WeblogBookmark.getByWebsite&IdBookmarkManagergetFolderById(Weblog, String)WeblogBookmarkFolder.getByWebsite&IdMediaFileManagergetMediaFile(Weblog, String)MediaFile.getByWeblogAndIdMediaFileManagergetMediaFileDirectory(Weblog, String)MediaFileDirectory.getByWeblogAndIdEach is backed by a named query filtering on the owning weblog, and returns
null when there is no match.
The folder lookup is named
getFolderByIdrather than being an overload:getFolder(Weblog, String)already exists as the by-name lookup, and the twowould otherwise share an erasure.
Call sites
Updated across templates, entries, categories, comments, bookmarks, folders
and media files in
ui/struts2/editor/. The action weblog is resolved by theinterceptor stack (
params->UIActionInterceptor->UISecurityInterceptor->
UIActionPrepareInterceptor) beforemyPrepare()runs, sogetActionWeblog()is available at each of these call sites withoutreordering anything.
The two comment loops in
Commentspreviously compared the weblog afterloading; they now use the scoped lookup instead, which also removes an NPE on
ids with no match.
Not changed
RollerResourceLoaderandGlobalCommentManagementhave no single weblog incontext by design and keep the id-based lookups.
EntryBean,StylesheetEdit,CommentDataServletand the XML-RPC and Atom handlers reachtheir weblog by other means and were left alone.
Tests
business/WeblogScopedLookupTest— manager layer, covering each new overloadui/struts2/editor/TemplateEditScopingTest— action layerui/struts2/editor/AuthoringActionScopingTest— action layer, coveringEntryRemove,EntryEdit,CategoryRemoveandBookmarkEditFull suite on JDK 11: 182 run, 0 failures, 0 errors, 1 skipped.