From 9e34ce0a43122bc3ff80a2469de1d465924c6dce Mon Sep 17 00:00:00 2001 From: Mathieu Baudier Date: Mon, 27 Apr 2015 08:23:55 +0000 Subject: [PATCH] - Introduce CMS specific TextInterpreter - Centralise persistence in TextViewer - Validate format *before* storing - Gracefully manage edition errors git-svn-id: https://svn.argeo.org/commons/trunk@8072 4cfe0d0a-d680-48aa-b62c-e0a02a3f76cc --- .../cms/internal/text/AbstractTextViewer.java | 121 ++++++------ .../internal/text/MarkupValidatorCopy.java | 172 ++++++++++++++++++ .../internal/text/TextInterpreterImpl.java | 33 ++++ .../cms/text/IdentityTextInterpreter.java | 15 +- .../argeo/cms/viewers/AbstractPageViewer.java | 37 +++- 5 files changed, 320 insertions(+), 58 deletions(-) create mode 100644 org.argeo.cms/src/org/argeo/cms/internal/text/MarkupValidatorCopy.java create mode 100644 org.argeo.cms/src/org/argeo/cms/internal/text/TextInterpreterImpl.java diff --git a/org.argeo.cms/src/org/argeo/cms/internal/text/AbstractTextViewer.java b/org.argeo.cms/src/org/argeo/cms/internal/text/AbstractTextViewer.java index 4a15701c6..8aabd362a 100644 --- a/org.argeo.cms/src/org/argeo/cms/internal/text/AbstractTextViewer.java +++ b/org.argeo.cms/src/org/argeo/cms/internal/text/AbstractTextViewer.java @@ -24,7 +24,6 @@ import org.argeo.cms.CmsImageManager; import org.argeo.cms.CmsNames; import org.argeo.cms.CmsSession; import org.argeo.cms.CmsTypes; -import org.argeo.cms.text.IdentityTextInterpreter; import org.argeo.cms.text.Img; import org.argeo.cms.text.Paragraph; import org.argeo.cms.text.TextInterpreter; @@ -64,7 +63,7 @@ public abstract class AbstractTextViewer extends AbstractPageViewer implements private final Section mainSection; - private TextInterpreter textInterpreter = new IdentityTextInterpreter(); + private TextInterpreter textInterpreter = new TextInterpreterImpl(); private CmsImageManager imageManager = CmsSession.current.get() .getImageManager(); @@ -285,6 +284,7 @@ public abstract class AbstractTextViewer extends AbstractPageViewer implements currentParagraph = newParagraph; currentParagraphN = newNode; } + persistChanges(sectionNode); } // TODO or rather return the created paragarphs? layout(toLayout.toArray(new Control[toLayout.size()])); @@ -322,8 +322,8 @@ public abstract class AbstractTextViewer extends AbstractPageViewer implements "TAB", "SHIFT+TAB", "ALT+ARROW_LEFT", "ALT+ARROW_RIGHT", "ALT+ARROW_UP", "ALT+ARROW_DOWN", "RETURN", "CTRL+RETURN", "ENTER", "DELETE" }); - text.setData(RWT.CANCEL_KEYS, new String[] { "ALT+ARROW_LEFT", - "ALT+ARROW_RIGHT" }); + text.setData(RWT.CANCEL_KEYS, new String[] { "RETURN", + "ALT+ARROW_LEFT", "ALT+ARROW_RIGHT" }); text.addKeyListener(this); } else if (part instanceof Img) { ((Img) part).setFileUploadListener(fileUploadListener); @@ -335,7 +335,7 @@ public abstract class AbstractTextViewer extends AbstractPageViewer implements try { Node paragraphNode = paragraph.getNode(); paragraphNode.setProperty(CMS_STYLE, style); - paragraphNode.getSession().save(); + persistChanges(paragraphNode); updateContent(paragraph); layout(paragraph); } catch (RepositoryException e1) { @@ -389,8 +389,16 @@ public abstract class AbstractTextViewer extends AbstractPageViewer implements // if we die in between, at least we still have the whole text // in the first node - textInterpreter.write(secondNode, second); - textInterpreter.write(firstNode, first); + try { + textInterpreter.write(secondNode, second); + textInterpreter.write(firstNode, first); + } catch (Exception e) { + // so that no additional nodes are created: + JcrUtils.discardUnderlyingSessionQuietly(firstNode); + throw e; + } + + persistChanges(firstNode); Paragraph secondParagraph = paragraphSplitted(paragraph, secondNode); @@ -410,7 +418,7 @@ public abstract class AbstractTextViewer extends AbstractPageViewer implements sectionNode.getProperty(Property.JCR_TITLE), txt.substring(0, caretPosition)); sectionNode.orderBefore(p(paragraphNode.getIndex()), p(1)); - sectionNode.getSession().save(); + persistChanges(sectionNode); Paragraph paragraph = sectionTitleSplitted(sectionTitle, paragraphNode); @@ -437,7 +445,7 @@ public abstract class AbstractTextViewer extends AbstractPageViewer implements String previousTxt = textInterpreter.read(previousNode); textInterpreter.write(previousNode, previousTxt + txt); paragraphNode.remove(); - sectionNode.getSession().save(); + persistChanges(sectionNode); Paragraph previousParagraph = paragraphMergedWithPrevious( paragraph, previousNode); @@ -469,7 +477,7 @@ public abstract class AbstractTextViewer extends AbstractPageViewer implements .getIdentifier()); nextNode.remove(); - sectionNode.getSession().save(); + persistChanges(sectionNode); paragraphMergedWithNext(paragraph, removed); edit(paragraph, txt.length()); @@ -499,7 +507,7 @@ public abstract class AbstractTextViewer extends AbstractPageViewer implements } // sectionNode.orderBefore(p(partNode.getIndex()), // p(newNode.getIndex())); - sectionNode.getSession().save(); + persistChanges(sectionNode); Img img = newImg((TextSection) section, newNode); edit(img, null); layout(img.getControl()); @@ -570,7 +578,7 @@ public abstract class AbstractTextViewer extends AbstractPageViewer implements refresh(newSection); newSection.getParent().layout(); layout(newSection); - newSectionNode.getSession().save(); + persistChanges(sectionNode); } else if (getEdited() instanceof SectionTitle) { SectionTitle sectionTitle = (SectionTitle) getEdited(); Section section = sectionTitle.getSection(); @@ -593,7 +601,7 @@ public abstract class AbstractTextViewer extends AbstractPageViewer implements TextSection newSection = new TextSection(section, section.getStyle(), sectionN); refresh(newSection); - previousSectionN.getSession().save(); + persistChanges(previousSectionN); } } catch (RepositoryException e) { throw new CmsException("Cannot deepen " + getEdited(), e); @@ -669,7 +677,7 @@ public abstract class AbstractTextViewer extends AbstractPageViewer implements refresh(mergedSection); mergedSection.getParent().layout(); layout(mergedSection); - mergedNode.getSession().save(); + persistChanges(mergedNode); } } catch (RepositoryException e) { throw new CmsException("Cannot undeepen " + getEdited(), e); @@ -746,53 +754,58 @@ public abstract class AbstractTextViewer extends AbstractPageViewer implements // KEY LISTENER @Override - public void keyPressed(KeyEvent e) { + public void keyPressed(KeyEvent ke) { if (log.isTraceEnabled()) - log.trace(e); + log.trace(ke); if (getEdited() == null) return; - boolean altPressed = (e.stateMask & SWT.ALT) != 0; - boolean shiftPressed = (e.stateMask & SWT.SHIFT) != 0; - boolean ctrlPressed = (e.stateMask & SWT.CTRL) != 0; - - // Common - if (e.keyCode == SWT.ESC) { - cancelEdit(); - } else if (e.character == '\r') { - splitEdit(); - } else if (e.character == 'S') { - if (ctrlPressed) - saveEdit(); - } else if (e.character == '\t') { - if (!shiftPressed) { - deepen(); - } else if (shiftPressed) { - undeepen(); - } - } else { - if (getEdited() instanceof Paragraph) { - Paragraph paragraph = (Paragraph) getEdited(); - Section section = paragraph.getSection(); - if (altPressed && e.keyCode == SWT.ARROW_RIGHT) { - edit(section.nextSectionPart(paragraph), 0); - } else if (altPressed && e.keyCode == SWT.ARROW_LEFT) { - edit(section.previousSectionPart(paragraph), 0); - } else if (e.character == SWT.BS) { - Text text = (Text) paragraph.getControl(); - int caretPosition = text.getCaretPosition(); - if (caretPosition == 0) { - mergeWithPrevious(); - } - } else if (e.character == SWT.DEL) { - Text text = (Text) paragraph.getControl(); - int caretPosition = text.getCaretPosition(); - int charcount = text.getCharCount(); - if (caretPosition == charcount) { - mergeWithNext(); + boolean altPressed = (ke.stateMask & SWT.ALT) != 0; + boolean shiftPressed = (ke.stateMask & SWT.SHIFT) != 0; + boolean ctrlPressed = (ke.stateMask & SWT.CTRL) != 0; + + try { + // Common + if (ke.keyCode == SWT.ESC) { + cancelEdit(); + } else if (ke.character == '\r') { + splitEdit(); + } else if (ke.character == 'S') { + if (ctrlPressed) + saveEdit(); + } else if (ke.character == '\t') { + if (!shiftPressed) { + deepen(); + } else if (shiftPressed) { + undeepen(); + } + } else { + if (getEdited() instanceof Paragraph) { + Paragraph paragraph = (Paragraph) getEdited(); + Section section = paragraph.getSection(); + if (altPressed && ke.keyCode == SWT.ARROW_RIGHT) { + edit(section.nextSectionPart(paragraph), 0); + } else if (altPressed && ke.keyCode == SWT.ARROW_LEFT) { + edit(section.previousSectionPart(paragraph), 0); + } else if (ke.character == SWT.BS) { + Text text = (Text) paragraph.getControl(); + int caretPosition = text.getCaretPosition(); + if (caretPosition == 0) { + mergeWithPrevious(); + } + } else if (ke.character == SWT.DEL) { + Text text = (Text) paragraph.getControl(); + int caretPosition = text.getCaretPosition(); + int charcount = text.getCharCount(); + if (caretPosition == charcount) { + mergeWithNext(); + } } } } + } catch (Exception e) { + ke.doit = false; + notifyEditionException(e); } } diff --git a/org.argeo.cms/src/org/argeo/cms/internal/text/MarkupValidatorCopy.java b/org.argeo.cms/src/org/argeo/cms/internal/text/MarkupValidatorCopy.java new file mode 100644 index 000000000..39ce3c374 --- /dev/null +++ b/org.argeo.cms/src/org/argeo/cms/internal/text/MarkupValidatorCopy.java @@ -0,0 +1,172 @@ +package org.argeo.cms.internal.text; + +import java.io.StringReader; +import java.text.MessageFormat; +import java.util.Arrays; +import java.util.HashMap; +import java.util.List; +import java.util.Map; + +import javax.xml.parsers.SAXParser; +import javax.xml.parsers.SAXParserFactory; + +import org.eclipse.rap.rwt.SingletonUtil; +import org.eclipse.swt.widgets.Widget; +import org.xml.sax.Attributes; +import org.xml.sax.InputSource; +import org.xml.sax.helpers.DefaultHandler; + +/** Copy of RAP v2.3 since it is in an internal package. */ +class MarkupValidatorCopy { + + // Used by Eclipse Scout project + public static final String MARKUP_VALIDATION_DISABLED + = "org.eclipse.rap.rwt.markupValidationDisabled"; + + private static final String DTD = createDTD(); + private static final Map SUPPORTED_ELEMENTS = createSupportedElementsMap(); + private final SAXParser saxParser; + + public static MarkupValidatorCopy getInstance() { + return SingletonUtil.getSessionInstance( MarkupValidatorCopy.class ); + } + + public MarkupValidatorCopy() { + saxParser = createSAXParser(); + } + + public void validate( String text ) { + StringBuilder markup = new StringBuilder(); + markup.append( DTD ); + markup.append( "" ); + markup.append( text ); + markup.append( "" ); + InputSource inputSource = new InputSource( new StringReader( markup.toString() ) ); + try { + saxParser.parse( inputSource, new MarkupHandler() ); + } catch( RuntimeException exception ) { + throw exception; + } catch( Exception exception ) { + throw new IllegalArgumentException( "Failed to parse markup text", exception ); + } + } + + public static boolean isValidationDisabledFor( Widget widget ) { + return Boolean.TRUE.equals( widget.getData( MARKUP_VALIDATION_DISABLED ) ); + } + + private static SAXParser createSAXParser() { + SAXParser result = null; + SAXParserFactory parserFactory = SAXParserFactory.newInstance(); + try { + result = parserFactory.newSAXParser(); + } catch( Exception exception ) { + throw new RuntimeException( "Failed to create SAX parser", exception ); + } + return result; + } + + private static String createDTD() { + StringBuilder result = new StringBuilder(); + result.append( "" ); + result.append( "" ); + result.append( "" ); + result.append( "" ); + result.append( "" ); + result.append( "" ); + result.append( "" ); + result.append( "" ); + result.append( "" ); + result.append( "" ); + result.append( "]>" ); + return result.toString(); + } + + private static Map createSupportedElementsMap() { + Map result = new HashMap(); + result.put( "html", new String[ 0 ] ); + result.put( "br", new String[ 0 ] ); + result.put( "b", new String[] { "style" } ); + result.put( "strong", new String[] { "style" } ); + result.put( "i", new String[] { "style" } ); + result.put( "em", new String[] { "style" } ); + result.put( "sub", new String[] { "style" } ); + result.put( "sup", new String[] { "style" } ); + result.put( "big", new String[] { "style" } ); + result.put( "small", new String[] { "style" } ); + result.put( "del", new String[] { "style" } ); + result.put( "ins", new String[] { "style" } ); + result.put( "code", new String[] { "style" } ); + result.put( "samp", new String[] { "style" } ); + result.put( "kbd", new String[] { "style" } ); + result.put( "var", new String[] { "style" } ); + result.put( "cite", new String[] { "style" } ); + result.put( "dfn", new String[] { "style" } ); + result.put( "q", new String[] { "style" } ); + result.put( "abbr", new String[] { "style", "title" } ); + result.put( "span", new String[] { "style" } ); + result.put( "img", new String[] { "style", "src", "width", "height", "title", "alt" } ); + result.put( "a", new String[] { "style", "href", "target", "title" } ); + return result; + } + + private static class MarkupHandler extends DefaultHandler { + + @Override + public void startElement( String uri, String localName, String name, Attributes attributes ) { + checkSupportedElements( name, attributes ); + checkSupportedAttributes( name, attributes ); + checkMandatoryAttributes( name, attributes ); + } + + private static void checkSupportedElements( String elementName, Attributes attributes ) { + if( !SUPPORTED_ELEMENTS.containsKey( elementName ) ) { + throw new IllegalArgumentException( "Unsupported element in markup text: " + elementName ); + } + } + + private static void checkSupportedAttributes( String elementName, Attributes attributes ) { + if( attributes.getLength() > 0 ) { + List supportedAttributes = Arrays.asList( SUPPORTED_ELEMENTS.get( elementName ) ); + int index = 0; + String attributeName = attributes.getQName( index ); + while( attributeName != null ) { + if( !supportedAttributes.contains( attributeName ) ) { + String message = "Unsupported attribute \"{0}\" for element \"{1}\" in markup text"; + message = MessageFormat.format( message, new Object[] { attributeName, elementName } ); + throw new IllegalArgumentException( message ); + } + index++; + attributeName = attributes.getQName( index ); + } + } + } + + private static void checkMandatoryAttributes( String elementName, Attributes attributes ) { + checkIntAttribute( elementName, attributes, "img", "width" ); + checkIntAttribute( elementName, attributes, "img", "height" ); + } + + private static void checkIntAttribute( String elementName, + Attributes attributes, + String checkedElementName, + String checkedAttributeName ) + { + if( checkedElementName.equals( elementName ) ) { + String attribute = attributes.getValue( checkedAttributeName ); + try { + Integer.parseInt( attribute ); + } catch( NumberFormatException exception ) { + String message + = "Mandatory attribute \"{0}\" for element \"{1}\" is missing or not a valid integer"; + Object[] arguments = new Object[] { checkedAttributeName, checkedElementName }; + message = MessageFormat.format( message, arguments ); + throw new IllegalArgumentException( message ); + } + } + } + + } + +} diff --git a/org.argeo.cms/src/org/argeo/cms/internal/text/TextInterpreterImpl.java b/org.argeo.cms/src/org/argeo/cms/internal/text/TextInterpreterImpl.java new file mode 100644 index 000000000..c252efe4f --- /dev/null +++ b/org.argeo.cms/src/org/argeo/cms/internal/text/TextInterpreterImpl.java @@ -0,0 +1,33 @@ +package org.argeo.cms.internal.text; + +import javax.jcr.Item; +import javax.jcr.RepositoryException; + +import org.argeo.cms.text.IdentityTextInterpreter; + +/** + * Text interpreter that sanitise and validates before saving, and support CMS + * specific formatting and integration. + */ +class TextInterpreterImpl extends IdentityTextInterpreter { + private MarkupValidatorCopy markupValidator = MarkupValidatorCopy + .getInstance(); + + @Override + protected void validateBeforeStoring(String raw) { + markupValidator.validate(raw); + } + + @Override + protected String convertToStorage(Item item, String content) + throws RepositoryException { + return super.convertToStorage(item, content); + } + + @Override + protected String convertFromStorage(Item item, String content) + throws RepositoryException { + return super.convertFromStorage(item, content); + } + +} diff --git a/org.argeo.cms/src/org/argeo/cms/text/IdentityTextInterpreter.java b/org.argeo.cms/src/org/argeo/cms/text/IdentityTextInterpreter.java index db90ea0f8..4284dc55b 100644 --- a/org.argeo.cms/src/org/argeo/cms/text/IdentityTextInterpreter.java +++ b/org.argeo.cms/src/org/argeo/cms/text/IdentityTextInterpreter.java @@ -19,6 +19,7 @@ public class IdentityTextInterpreter implements TextInterpreter, CmsNames { Node node = (Node) item; if (node.isNodeType(CmsTypes.CMS_STYLED)) { String raw = convertToStorage(node, content); + validateBeforeStoring(raw); node.setProperty(CMS_CONTENT, raw); } else { throw new CmsException("Don't know how to interpret " @@ -28,7 +29,7 @@ public class IdentityTextInterpreter implements TextInterpreter, CmsNames { Property property = (Property) item; property.setValue(content); } - item.getSession().save(); + // item.getSession().save(); } catch (RepositoryException e) { throw new CmsException("Cannot set content on " + item, e); } @@ -55,7 +56,7 @@ public class IdentityTextInterpreter implements TextInterpreter, CmsNames { node.setProperty(CMS_CONTENT, ""); node.getSession().save(); } - + return node.getProperty(CMS_CONTENT).getString(); } else { throw new CmsException("Don't know how to interpret " @@ -70,12 +71,22 @@ public class IdentityTextInterpreter implements TextInterpreter, CmsNames { } } + // EXTENSIBILITY + /** + * To be overridden, in order to make sure that only valid strings are being + * stored. + */ + protected void validateBeforeStoring(String raw) { + } + + /** To be overridden, in order to support additional formatting. */ protected String convertToStorage(Item item, String content) throws RepositoryException { return content; } + /** To be overridden, in order to support additional formatting. */ protected String convertFromStorage(Item item, String content) throws RepositoryException { return content; diff --git a/org.argeo.cms/src/org/argeo/cms/viewers/AbstractPageViewer.java b/org.argeo.cms/src/org/argeo/cms/viewers/AbstractPageViewer.java index 837cf9929..abdeb0d61 100644 --- a/org.argeo.cms/src/org/argeo/cms/viewers/AbstractPageViewer.java +++ b/org.argeo.cms/src/org/argeo/cms/viewers/AbstractPageViewer.java @@ -5,6 +5,7 @@ import java.util.Observer; import javax.jcr.Node; import javax.jcr.RepositoryException; +import javax.jcr.Session; import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; @@ -20,6 +21,7 @@ import org.eclipse.swt.events.MouseListener; import org.eclipse.swt.widgets.Composite; import org.eclipse.swt.widgets.Control; import org.eclipse.swt.widgets.Widget; +import org.xml.sax.SAXParseException; /** Base class for viewers related to a page */ public abstract class AbstractPageViewer extends ContentViewer implements @@ -165,8 +167,16 @@ public abstract class AbstractPageViewer extends ContentViewer implements if (edited == part) return; - if (edited != null && edited != part) - stopEditing(true); + if (edited != null && edited != part) { + EditablePart previouslyEdited = edited; + try { + stopEditing(true); + } catch (Exception e) { + notifyEditionException(e); + edit(previouslyEdited, caretPosition); + return; + } + } part.startEditing(); updateContent(part); @@ -247,6 +257,29 @@ public abstract class AbstractPageViewer extends ContentViewer implements "Edited should not be null or disposed at this stage"); } + /** Persist all changes. */ + protected void persistChanges(Session session) throws RepositoryException { + session.save(); + // TODO notify that changes have been persisted + } + + /** Convenience method using a Node in order to save the underlying session. */ + protected void persistChanges(Node anyNode) throws RepositoryException { + persistChanges(anyNode.getSession()); + } + + /** Notify edition exception */ + protected void notifyEditionException(Throwable e) { + Throwable eToLog = e; + if (e instanceof IllegalArgumentException) + if (e.getCause() instanceof SAXParseException) + eToLog = e.getCause(); + log.error(eToLog.getMessage()); + if (log.isTraceEnabled()) + log.trace("Full stack of " + eToLog.getMessage(), e); + // TODO Light error notification popup + } + // GETTERS / SETTERS public boolean isReadOnly() { return readOnly; -- 2.30.2