mraible commented on code in PR #162:
URL: https://github.com/apache/roller/pull/162#discussion_r3891402029
##########
app/src/main/java/org/apache/roller/weblogger/ui/struts2/editor/WeblogConfig.java:
##########
@@ -136,7 +136,7 @@ public String save() {
// if blogger category changed then lookup new cat and set it
if(getBean().getBloggerCategoryId() != null &&
!weblog.getBloggerCategory().getId().equals(getBean().getBloggerCategoryId())) {
-
weblog.setBloggerCategory(wmgr.getWeblogCategory(getBean().getBloggerCategoryId()));
+
weblog.setBloggerCategory(wmgr.getWeblogCategory(getActionWeblog(),
getBean().getBloggerCategoryId()));
Review 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.
##########
app/src/main/java/org/apache/roller/weblogger/ui/struts2/editor/MediaFileBase.java:
##########
@@ -62,7 +62,7 @@ protected void doDeleteMediaFile() {
log.debug("Processing delete of file id - " + this.mediaFileId);
MediaFileManager manager = WebloggerFactory.getWeblogger()
.getMediaFileManager();
- MediaFile mediaFile = manager.getMediaFile(this.mediaFileId);
+ MediaFile mediaFile = manager.getMediaFile(getActionWeblog(),
this.mediaFileId);
Review 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.
##########
app/src/main/java/org/apache/roller/weblogger/ui/struts2/editor/Bookmarks.java:
##########
@@ -225,12 +225,12 @@ public String move() {
}
// Move bookmarks to new parent folder.
- WeblogBookmarkFolder newFolder =
bmgr.getFolder(getTargetFolderId());
+ WeblogBookmarkFolder newFolder =
bmgr.getFolderById(getActionWeblog(), getTargetFolderId());
String bookmarks[] = getSelectedBookmarks();
if (null != bookmarks && bookmarks.length > 0) {
for (int j = 0; j < bookmarks.length; j++) {
- WeblogBookmark bd = bmgr.getBookmark(bookmarks[j]);
+ WeblogBookmark bd = bmgr.getBookmark(getActionWeblog(),
bookmarks[j]);
newFolder.addBookmark(bd);
Review 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.
##########
app/src/main/java/org/apache/roller/weblogger/ui/struts2/editor/MediaFileEdit.java:
##########
@@ -67,7 +67,7 @@ public void myPrepare() {
try {
MediaFileManager mgr =
WebloggerFactory.getWeblogger().getMediaFileManager();
if (!StringUtils.isEmpty(bean.getDirectoryId())) {
- setDirectory(mgr.getMediaFileDirectory(bean.getDirectoryId()));
+ setDirectory(mgr.getMediaFileDirectory(getActionWeblog(),
bean.getDirectoryId()));
}
} catch (WebloggerException ex) {
log.error("Error looking up media file directory", ex);
Review 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.
##########
app/src/main/java/org/apache/roller/weblogger/ui/struts2/editor/EntryEdit.java:
##########
@@ -104,7 +104,7 @@ public void myPrepare() {
// retrieve from DB WeblogEntry based on ID
WeblogEntryManager wmgr = WebloggerFactory.getWeblogger()
.getWeblogEntryManager();
- setEntry(wmgr.getWeblogEntry(getBean().getId()));
+ setEntry(wmgr.getWeblogEntry(getActionWeblog(),
getBean().getId()));
} catch (WebloggerException ex) {
log.error(
"Error looking up entry by id - " + getBean().getId(),
Review 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.
##########
app/src/main/java/org/apache/roller/weblogger/ui/struts2/editor/Comments.java:
##########
@@ -102,7 +102,7 @@ public void loadComments() {
// lookup weblog entry if necessary
if (!StringUtils.isEmpty(getBean().getEntryId())) {
- setQueryEntry(wmgr.getWeblogEntry(getBean().getEntryId()));
+ setQueryEntry(wmgr.getWeblogEntry(getActionWeblog(),
getBean().getEntryId()));
Review 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.
##########
app/src/main/java/org/apache/roller/weblogger/ui/struts2/editor/MediaFileImageDim.java:
##########
@@ -50,7 +50,7 @@ public MediaFileImageDim() {
public String execute() {
try {
MediaFileManager mgr =
WebloggerFactory.getWeblogger().getMediaFileManager();
- MediaFile mediaFile = mgr.getMediaFile(getMediaFileId());
+ MediaFile mediaFile = mgr.getMediaFile(getActionWeblog(),
getMediaFileId());
bean.copyFrom(mediaFile);
Review Comment:
Same family: the scoped lookup can return `null` and
`bean.copyFrom(mediaFile)` dereferences it inside a catch that only handles
`WebloggerException`.
##########
app/src/main/java/org/apache/roller/weblogger/ui/struts2/editor/EntryAddWithMediaFile.java:
##########
@@ -71,7 +71,7 @@ public String execute() {
if (selectedImages != null) {
for (String image : selectedImages) {
- MediaFile mediaFile = manager.getMediaFile(image);
+ MediaFile mediaFile =
manager.getMediaFile(getActionWeblog(), image);
String link;
if (mediaFile.isImageFile()) {
Review 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.
##########
app/src/main/java/org/apache/roller/weblogger/ui/struts2/editor/CategoryRemove.java:
##########
@@ -112,7 +112,7 @@ public String remove() {
WeblogEntryManager wmgr =
WebloggerFactory.getWeblogger().getWeblogEntryManager();
if (getTargetCategoryId() != null) {
- WeblogCategory target =
wmgr.getWeblogCategory(getTargetCategoryId());
+ WeblogCategory target =
wmgr.getWeblogCategory(getActionWeblog(), getTargetCategoryId());
wmgr.moveWeblogCategoryContents(getCategory(), target);
Review Comment:
`targetCategoryId` outside the action weblog gives a `null` target here,
which goes straight into `moveWeblogCategoryContents`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
