snoopdave commented on code in PR #244:
URL: https://github.com/apache/roller/pull/244#discussion_r4569386549
##########
app/src/main/resources/log4j2.xml:
##########
@@ -74,6 +74,11 @@ limitations under the License.
<Logger name="org.apache.velocity" level="info"
additivity="false"> <AppenderRef ref="asyncRoller"/> </Logger>
<Logger name="org.springframework" level="info"
additivity="false"> <AppenderRef ref="asyncRoller"/> </Logger>
<Logger name="org.apache.struts2" level="info"
additivity="false"> <AppenderRef ref="asyncRoller"/> </Logger>
+ <!-- Struts logs a full stack trace for every request to an unknown
.rol URL; the user gets a 404 -->
+ <Logger name="org.apache.struts2.dispatcher.Dispatcher" level="info"
additivity="false">
+ <RegexFilter regex="Could not find action or result: .*"
onMatch="DENY" onMismatch="NEUTRAL"/>
Review Comment:
🤖Claude: not fixed: Log4j2 can't tell an unknown URL from a missing result
mapping, because both log the same message. The filter hides both, as you say.
I'd rather remove it (and its CHANGES.md line) than hide a misconfigured
action. Dave, please decide.
##########
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) {
+ directory = createDefaultMediaFileDirectory(weblog);
+ roller.flush();
+ }
Review Comment:
🤖Claude: partly fixed in ed1013895: `getMediaFileDirectoryByName` now uses
`getResultList()`. It logs a warning and uses the first row if two "default"
folders exist, so the library no longer throws `NonUniqueResultException`. I
did not add a unique constraint or a one-time migration, so two requests can
still create a duplicate row. A test covers the duplicate case.
##########
app/src/main/java/org/apache/roller/weblogger/ui/rendering/RendererManager.java:
##########
@@ -81,17 +81,28 @@ private RendererManager() {
public static Renderer getRenderer(Template template,
MobileDeviceRepository.DeviceType deviceType)
throws RenderingException {
- Renderer renderer;
+ Renderer renderer = findRenderer(template, deviceType);
+ if (renderer == null) {
+ throw new RenderingException("No renderer found for template " +
template.getId() + "!");
+ }
+ return renderer;
+ }
+
+ /**
+ * Like {@link #getRenderer}, but returns null when no factory can render
+ * the template, for callers that answer that case with a 404. A factory
+ * that fails while creating a renderer has already logged why.
+ */
+ public static Renderer findRenderer(Template template,
MobileDeviceRepository.DeviceType deviceType) {
// iterate over our renderer factories and see if one of them
// wants to handle this content
for (RendererFactory rendererFactory : rendererFactories) {
- renderer = rendererFactory.getRenderer(template, deviceType);
+ Renderer renderer = rendererFactory.getRenderer(template,
deviceType);
Review Comment:
🤖Claude: fixed in ed1013895: `RendererManager.findRenderer` catches a
`RuntimeException` from a factory, logs it at ERROR and tries the next factory,
so a bad factory gives a 404 and not a 500.
##########
app/src/main/resources/ApplicationResources_de.properties:
##########
@@ -397,7 +397,6 @@ installer.tablesCreated=Tabellen erfolgreich erstellt
installer.tablesCreatedExplanation=Datenbanktabellen wurden erfolgreich
erstellt (siehe unten).
Review Comment:
🤖Claude: fixed in ed1013895: removed the stale `installer.*Explanation`
texts for creating and upgrading tables from the de, ja, ko and zh_CN bundles.
They fall back to English.
##########
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: partly fixed in ed1013895: see the reply on the thread at line 537.
The lookup tolerates duplicates. Preventing them needs a unique constraint or a
migration, which I have not added.
##########
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: fixed in ed1013895: `NoRenderer` now logs the exception at WARN
with the template id before it returns false.
##########
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: fixed in ed1013895: the root-collection GET now uses
`getDefaultMediaFileDirectory`, so a read does not create a folder and the
existing 404 applies. The POST (upload) at line 129 still uses get-or-create,
because it writes.
--
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]
