snoopdave commented on code in PR #244:
URL: https://github.com/apache/roller/pull/244#discussion_r4569212455
##########
app/src/main/java/org/apache/roller/weblogger/business/jpa/JPAMediaFileManagerImpl.java:
##########
@@ -524,6 +524,20 @@ public MediaFileDirectory
getDefaultMediaFileDirectory(Weblog weblog)
return getMediaFileDirectoryByName(weblog, "default");
}
+ /**
+ * {@inheritDoc}
+ */
+ @Override
+ public MediaFileDirectory getOrCreateDefaultMediaFileDirectory(Weblog
weblog)
+ throws WebloggerException {
+ MediaFileDirectory directory = getDefaultMediaFileDirectory(weblog);
+ if (directory == null) {
Review Comment:
🐞Claude Issue: **Important:** get-then-create is not atomic. Two first
requests for a weblog with no default folder (for example the media view and
the image chooser loading together) can both see null and both create a folder
named "default". Later lookups by name then find two rows. Make the
check-and-create safe, for example synchronize on the weblog id or re-query
after the flush and keep the first row, and add a test for the second caller.
##########
app/src/main/java/org/apache/roller/weblogger/ui/rendering/servlets/NoRenderer.java:
##########
@@ -0,0 +1,61 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. The ASF licenses this file to You
+ * under the Apache License, Version 2.0 (the "License"); you may not
+ * use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License. For additional information regarding
+ * copyright in this work, please see the NOTICE file in the top level
+ * directory of this distribution.
+ */
+
+package org.apache.roller.weblogger.ui.rendering.servlets;
+
+import java.io.IOException;
+import jakarta.servlet.http.HttpServletResponse;
+import org.apache.commons.logging.Log;
+import org.apache.roller.weblogger.WebloggerException;
+import org.apache.roller.weblogger.pojos.Template;
+import org.apache.roller.weblogger.pojos.TemplateRendition.RenditionType;
+import org.apache.roller.weblogger.pojos.Weblog;
+
+/**
+ * The answer to a page or preview request for a template that no renderer can
+ * render: log it once and send a 404.
+ */
+final class NoRenderer {
+
+ private NoRenderer() {
+ }
+
+ static void sendNotFound(HttpServletResponse response, Template page,
Weblog weblog, Log log)
+ throws IOException {
+ if (hasNoContent(page)) {
+ // nothing else logs this case
+ log.warn("No renderer for template '" + page.getName() + "' (" +
page.getId()
+ + ") of weblog '" + weblog.getHandle() + "': it has no
content.");
+ } else {
+ // the renderer factory already logged why it could not create a
renderer
+ log.debug("No renderer for template " + page.getId());
+ }
+ if (!response.isCommitted()) {
+ response.reset();
+ }
+ response.sendError(HttpServletResponse.SC_NOT_FOUND);
+ }
+
+ private static boolean hasNoContent(Template page) {
+ try {
+ return page.getTemplateRendition(RenditionType.STANDARD) == null;
+ } catch (WebloggerException e) {
Review Comment:
🐞Claude Issue: **Important:** the catch swallows the WebloggerException
without logging it. The caller then logs only at debug, so a database error
while reading the rendition leaves no trace. Log the exception at warn here, or
return true only when the lookup succeeded and found nothing.
##########
app/src/main/java/org/apache/roller/weblogger/webservices/atomprotocol/MediaCollection.java:
##########
@@ -126,7 +126,7 @@ public AtomEntry postMedia(AtomRequest areq, AtomEntry
entry) throws AtomExcepti
}
MediaFileDirectory mdir = justPath.isEmpty()
- ? fileMgr.getDefaultMediaFileDirectory(website)
+ ? fileMgr.getOrCreateDefaultMediaFileDirectory(website)
Review Comment:
🐞Claude Issue: **Important:** this is a read path (a GET of the media
collection). It now inserts a folder row for a weblog that has none, so a
read-only AtomPub client changes data. Prefer the plain getter here and return
the "cannot find media directory" 404 that already follows, or document why a
read should create the folder. The same applies at line 304.
--
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]
