mraible commented on code in PR #244:
URL: https://github.com/apache/roller/pull/244#discussion_r4559393661
##########
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:
Two things about this method:
- It's a check and then an insert with nothing stopping two requests doing
it at once, and `roller_mediafiledir` has no unique constraint on weblog and
name. On a weblog with no default folder, the media library page and the image
chooser (or two uploads) arriving together could each create a "default". After
that `getMediaFileDirectoryByName` would throw `NonUniqueResultException`,
since `getSingleResult()` only catches `NoResultException`, and the media
library would be broken for that weblog. I haven't reproduced it. Weblogs
upgraded from old versions are exactly where this runs, so a one-time fix in
the upgrade (or a unique constraint) might be safer than creating on demand.
- `roller.flush()` commits whatever else is pending in the session.
`EntryEdit` calls this in the middle of a save, so if a later step fails, the
folder and anything flushed with it stay committed. The callers used to create
the folder without flushing.
##########
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:
The de, ja, ko and zh_CN bundles keep the old explanation text that says the
output is shown "below" ("siehe unten"), but the output is now inside the
collapsed panel, and the sentence about pressing Start Roller only exists in
English since `installer.tryBootstrapping` was removed. Dropping these
translated keys so they fall back to English, or updating them, would keep the
page consistent.
##########
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:
`PageServlet` and `PreviewServlet` used to wrap this lookup in `catch
(Exception)` and answer 404. Now a runtime exception from a factory (a
user-configured one from `rendering.userRendererFactories`, say) escapes
`doGet` as a 500. A try/catch here that logs and moves on to the next factory
would keep the old behaviour.
##########
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:
This drops every Dispatcher message that starts with "Could not find action
or result", not only the ones for unknown URLs. I believe Struts logs a missing
result mapping under the same message (the "No result defined for action X and
result input" case), so a misconfigured action would give a 404 with nothing in
the log to explain it. Could the filter be narrower, or log these at one line
without the stack trace instead of dropping them?
--
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]
