I think this is OK. let me give the patch tomorrow a shot, in case someone has an issue with this.
-M On Sat, Nov 1, 2008 at 11:28 AM, Cale Scholl <[EMAIL PROTECTED]> wrote: > Hello, > > I'm sending out this message again this time in html as well so that > hopefully my comments are easier to read. This time I also attached a patch > with my suggested changes (I did a clean install and everything built > successfully). The patch is also available here: > https://issues.apache.org/jira/browse/TRINIDAD-1231 > > Thank you, > Cale > > -------- Original Message -------- > Subject: [Trinidad] Resolving differences between client-side and > server-side message formatter? > Date: Thu, 30 Oct 2008 10:50:16 -0700 > From: Cale Scholl <[EMAIL PROTECTED]> > Reply-To: MyFaces Development <[email protected]> > To: [email protected] > > Hello, > > I'm pretty new to the Trinidad community, so let me apologize in advance > if there's something I've misunderstood. > > What I'd like to talk about are the differences between the client-side > message formatter: > > trinidad-impl\src\main\javascript\META-INF\adf\jsLibs\Core.js -> > function _formatErrorString(errorFormat, tokens) > > and the server-side message formatter: > > trinidad-api\src\main\java\org\apache\myfaces\trinidad\util\FastMessageFormat.java > -> public String format(Object[] source) > > The problem occurs when the application developer provides a custom > error message, such as messageDetailXYZ="Isn't {0} cool?". When this is > formatted by the client formatter, everything works fine. But let's say, > for whatever reason, client validation is now disabled, and therefore > this error message is formatted by the server formatter. When it hits > the apostrophe, the rest of the message is interpreted literally, so the > resulting formatted error message is "Isnt {0} cool?". This is bound to > confuse the app developer. If the app developer instead provides > messageDetailXYZ="Isn''t {0} cool?", then both the client and server > formatter return "Isn't token0 cool?" (that is, it's formatted > correctly), but I think requiring special escaping complicates things as > well. Ultimately, I think messageDetailXYZ="Isn't {0} cool?" should be > formatted correctly by both the client and server formatter. > > I realize that since the server formatter is part of the public api, it > may be difficult to change now. However, if others think it might be > possible to resolve the client and server formatter differences, then > please see the code pasted below, where I have inserted several > questions and comments of the form: > > /// > // my comment here > /// > > Thank you, > Cale > > --- > > Let's start with the server formatter. Personally, I prefer the server > formatter's method of replacing patterns with tokens. However, if a > pattern doesn't have an associated token, I'd prefer the server > formatter just copy the pattern to the output string (in other words, > interpret it as literal text), instead of throwing an exception. This > way, there's no need to use single quotes for special escaping behavior. > > trinidad-api\src\main\java\org\apache\myfaces\trinidad\util\FastMessageFormat.java: > > /** > * Formats the given array of strings based on the initial > * pattern. It is legal for this array to be shorter > * than that indicated by the pattern, or to have null > * entries - these will simply be ignored. > * <p> > * @param source an array of strings > */ > public String format(Object[] source) > { > int formatLength = _formatText.length; > int length = 0; > int sourceCount = source.length; > for (int i = 0; i < sourceCount; i++) > { > Object sourceString = source[i]; > if (sourceString != null) > { > length += sourceString.toString().length(); > } > } > > StringBuffer buffer = new StringBuffer(length + formatLength); > > /// > // The above buffer size assumes that each pattern is only replaced once, > i.e. > // if {0} occurs two or more times, only one instance is replaced. If we are > // making this assumption, then it should be enforced when replacing > patterns below. > // If it's perfectly legal for {0} to be replaced in several spots, then we > should comment > // that the above buffer size is just a rough initial estimate. > // Furthermore, if this function is only accessed by a single thread, we > should > // instead use StringBuilder for performance purposes. > /// > > int lastStart = 0; > boolean inQuote = false; > for (int i = 0; i < formatLength; i++) > { > char ch = _formatText[i]; > if (inQuote) > { > if (ch == '\'') > { > buffer.append(_formatText, lastStart, i - lastStart); > i++; > lastStart = i; > inQuote = false; > } > } > else > { > if (ch == '\'') > { > buffer.append(_formatText, lastStart, i - lastStart); > i++; > lastStart = i; > > // Check for doubled-up quotes > if ((i < formatLength) && (_formatText[i] == '\'')) > { > // Do nothing; we'll add the doubled-up quote later > ; > } > else > { > inQuote = true; > } > } > > /// > // I think using single quotes to escape text is unnecessary. We should only > try to > // replace patters of the type "{number}" for which 'number' is a valid > token index. > /// > > else if (ch == '{') > { > buffer.append(_formatText, lastStart, i - lastStart); > > int sourceIndex = 0; > int j = i + 1; > for (; j < formatLength; j++) > { > char patternChar = _formatText[j]; > if (patternChar == '}') > { > break; > } > else > { > if ((patternChar < '0') || (patternChar > '9')) > throw new > IllegalArgumentException(_LOG.getLogger().getResourceBundle().getString("FASTMESSAGEFORMAT_ONLY_SUPPORT_NUMERIC_ARGUMENTS")); > sourceIndex = (sourceIndex * 10) + (patternChar - '0'); > } > } > > /// > // This seems overly complex. Can't we put a hard-coded limit on the pattern > number? > // For example, are we ever going to have more than 10 tokens? It would be > much simpler > // if we only had to look for a single digit number followed by a '}'. > /// > > if (j == formatLength) > throw new > IllegalArgumentException(_LOG.getLogger().getResourceBundle().getString("END_OF_PATTERN_NOT_FOUND")); > if (j == i + 1) > throw new > IllegalArgumentException(_LOG.getLogger().getResourceBundle().getString("FASTMESSAGEFORMAT_FIND_EMPTY_ARGUMENT")); > > /// > // I think for the above cases we should just interpret the text literally > instead > // of throwing exceptions. > /// > > if (sourceIndex < sourceCount) > { > Object sourceString = source[sourceIndex]; > if (sourceString != null) > buffer.append(sourceString.toString()); > } > > /// > // If the pattern "{*}" contains anything besides a > // non-negative number, we throw an exception, but if it contains a number >>= the > // number of tokens, the whole "{*}" is replaced by the empty string (I'm > surprised > // that for consistency purposes that an exception isn't thrown instead). > However, > // I think that in both of these cases the pattern should just be > interpreted as literal text, > // and that the pattern should only be replaced with the empty string when > it has an > // associated token and the token is null or empty. > /// > > i = j; > lastStart = i + 1; > } > else > { > // Do nothing. The character will be added in later > ; > } > } > } > > buffer.append(_formatText, lastStart, formatLength - lastStart); > > return new String(buffer); > } > > --- > > Now let's talk about the client formatter. Using regular expressions to > match patterns is elegant, but personally I think it's overkill for what > we need here. The server formatter's method of replacing patterns with > tokens is faster, plus it completely avoids the issue of infinite > pattern replacement, should the token itself contain "{0}" or something > like that. > > trinidad-impl\src\main\javascript\META-INF\adf\jsLibs\Core.js: > > /** > * Performs token replacement on the the error format, replacing each > * token found in the token Object with the value for that token. > */ > function _formatErrorString( > errorFormat, // error format string with embedded tokens to be replaced > tokens // tokens Object containin token names and values to be > replaced > ) > { > var currString = errorFormat; > > /// > // We have a fundamental difference in behavior between this formatter and > the > // server formatter. This formatter only tries to replace patterns for which > we > // actually have a token; the server formatter throws exceptions. > /// > > // loop through all of the tokens, replacing them if they are present > for (var currToken in tokens) > { > var currValue = tokens[currToken]; > > // if the token has no value > if (!currValue) > { > currValue = ""; > } > > // TRINIDAD-829: > // we replace '{' and '}' to ensure, that tokens containing values > // like {3} aren't parsed more than once... > // Only do this if it is typeof string (see TRINIDAD-873) > if (typeof currValue == "string") > { > currValue = currValue.replace("{","{'"); > currValue = currValue.replace("}","'}"); > } > > /// > // 1) - footnote for above text > // This is a good way to avoid the infinite pattern replacement problem; > however, > // even better would be to use the server formatter's replacement method :) > /// > > // the tokens are delimited by '{' before and '}' after the token > var currRegExp = "{" + currToken + "}"; > > // support tokens of the form %token% as well as {token} > currString = currString.replace(new RegExp('%' + currToken + '%', 'g'), > currRegExp); > > /// > // Do we still need to support tokens of the type %number%? The server > formatter > // only supports tokens of the type {number}. If one supports both types, so > should > // the other. > /// > > // Replace the token. Don't use String.replace, as the value may > // include dollar signs, which leads Netscape astray (bug 2242675) > var indexOf = currString.indexOf(currRegExp); > > /// > // Doesn't footnote 1) make the following check unnecessary? > /// > > if (currValue.indexOf && currValue.indexOf(currRegExp) >= 0) > { > var b1 = ''; > for (i=0; i<currValue.length; i++) > { > b1 = b1 + 'placeHolderString'; > } > > while (indexOf >= 0) > { > currString=(currString.substring(0,indexOf) > + b1 > + currString.substring(indexOf+currRegExp.length)); > indexOf = currString.indexOf(currRegExp); > } > > indexOf = currString.indexOf(b1); > > while (indexOf >= 0) > { > currString =(currString.substring(0,indexOf) > + currValue > + currString.substring(indexOf+b1.length)); > indexOf = currString.indexOf(b1); > } > } > else > while (indexOf >= 0) > { > currString = (currString.substring(0, indexOf) > + currValue > + currString.substring(indexOf + currRegExp.length)); > indexOf = currString.indexOf(currRegExp); > } > } > > // TRINIDAD-829: > // we finally re-replace the '{' and '}'... > while(currString.indexOf("{'")!=-1) > { > currString= currString.replace("{'","{"); > currString= currString.replace("'}","}"); > } > > /// > // Having the above behavior means already existing "{'" will be replaced as > well; > // if we must keep this behavior, it should be well documented. > /// > > // And now take any doubled-up single quotes down to one, > // to handle escaping > var twoSingleQuotes = /''/g; > > /// > // Having the above behavior means already existing "''" will be replaced as > well; > // if we must keep this behavior, it should be well documented. > /// > > return currString.replace(twoSingleQuotes, "'"); > } > > Index: > trinidad-api/src/main/java/org/apache/myfaces/trinidad/util/FastMessageFormat.java > =================================================================== > --- > trinidad-api/src/main/java/org/apache/myfaces/trinidad/util/FastMessageFormat.java > (revision 708412) > +++ > trinidad-api/src/main/java/org/apache/myfaces/trinidad/util/FastMessageFormat.java > (working copy) > @@ -6,9 +6,9 @@ > * 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 > @@ -24,7 +24,7 @@ > * The FastMessageFormat class is a greatly reduced version > * of the java.text.MessageFormat class. It's also much faster > * and much less expensive to create, which is especially > - * valuable when it is created and thrown away many times - > + * valuable when it is created and thrown away many times - > * a common use case in web applications. > * <p> > * The only syntax supported by this class is simple index-based > @@ -65,7 +65,7 @@ > * <p> > * @param source an array of strings > */ > - public String format(Object[] source) > + /*public String formatOld(Object[] source) > { > int formatLength = _formatText.length; > int length = 0; > @@ -130,33 +130,29 @@ > } > else > { > - if ((patternChar < '0') || > - (patternChar > '9')) > - throw new > IllegalArgumentException(_LOG.getLogger().getResourceBundle().getString( > - "FASTMESSAGEFORMAT_ONLY_SUPPORT_NUMERIC_ARGUMENTS")); > + if ((patternChar < '0') || (patternChar > '9')) > + throw new > IllegalArgumentException(_LOG.getLogger().getResourceBundle().getString("FASTMESSAGEFORMAT_ONLY_SUPPORT_NUMERIC_ARGUMENTS")); > sourceIndex = (sourceIndex * 10) + (patternChar - '0'); > } > } > > if (j == formatLength) > - throw new > IllegalArgumentException(_LOG.getLogger().getResourceBundle().getString( > - "END_OF_PATTERN_NOT_FOUND")); > + throw new > IllegalArgumentException(_LOG.getLogger().getResourceBundle().getString("END_OF_PATTERN_NOT_FOUND")); > if (j == i + 1) > - throw new > IllegalArgumentException(_LOG.getLogger().getResourceBundle().getString( > - "FASTMESSAGEFORMAT_FIND_EMPTY_ARGUMENT")); > + throw new > IllegalArgumentException(_LOG.getLogger().getResourceBundle().getString("FASTMESSAGEFORMAT_FIND_EMPTY_ARGUMENT")); > if (sourceIndex < sourceCount) > { > Object sourceString = source[sourceIndex]; > if (sourceString != null) > buffer.append(sourceString.toString()); > } > - > + > i = j; > lastStart = i + 1; > } > else > { > - // Do nothing. The character will be added in later > + // Do nothing. The character will be added in later > ; > } > } > @@ -165,13 +161,67 @@ > buffer.append(_formatText, lastStart, formatLength - lastStart); > > return new String(buffer); > - } > + }*/ > > - private final char[] _formatText; > - private static final TrinidadLogger _LOG = > TrinidadLogger.createTrinidadLogger( > - FastMessageFormat.class); > -} > + /** > + * This formatter will only replace patterns of the type "{[0-9]}" > + * for which there is an associated token. > + * Any other use of '{}' will be interpreted as literal text. > + * This aims to have the same behavior as TrFastMessageFormatUtils.format > + * on the client. > + * <p> > + * @param source an array of strings (tokens) > + */ > + public String format(Object[] source) > + { > + int formatLength = _formatText.length; > + int length = 0; > + int tokenCount = source.length; > + for (int i = 0; i < tokenCount; i++) > + { > + Object sourceString = source[i]; > + if (sourceString != null) > + { > + length += sourceString.toString().length(); > + } > + } > + > + // The following buffer size is just an initial estimate. It is legal > for > + // any given pattern, such as {0}, to occur more than once, in which > case > + // the buffer size will expand automatically if need be. > + StringBuilder buffer = new StringBuilder(length + formatLength); > > + int lastStart = 0; > + for (int i = 0; i < formatLength; i++) > + { > + char ch = _formatText[i]; > + if (ch == '{') > + { > + // Only check for single digit patterns that have an associated > token. > + if (i + 2 < formatLength && _formatText[i+2] == '}') > + { > + int tokenIndex = _formatText[i+1] - '0'; > + if (tokenIndex >= 0 && tokenIndex < tokenCount) > + { > + buffer.append(_formatText, lastStart, i - lastStart); > + Object sourceString = source[tokenIndex]; > + if (sourceString != null) > + buffer.append(sourceString.toString()); > + > + i += 2; > + lastStart = i + 1; > + } > + } > + } > + // ELSE: Do nothing. The character will be added in later. > + } > > + buffer.append(_formatText, lastStart, formatLength - lastStart); > > + return new String(buffer); > + } > > + private final char[] _formatText; > + private static final TrinidadLogger _LOG = > + TrinidadLogger.createTrinidadLogger(FastMessageFormat.class); > +} > Index: > trinidad-api/src/main/java/org/apache/myfaces/trinidad/util/MessageFactory.java > =================================================================== > --- > trinidad-api/src/main/java/org/apache/myfaces/trinidad/util/MessageFactory.java > (revision 708412) > +++ > trinidad-api/src/main/java/org/apache/myfaces/trinidad/util/MessageFactory.java > (working copy) > @@ -430,7 +430,7 @@ > { > if (parameters == null) > return pattern; > - > + > FastMessageFormat formatter = new FastMessageFormat(pattern); > String fmtedMsgStr = formatter.format(parameters); > return fmtedMsgStr; > @@ -610,7 +610,7 @@ > _customDetailErrorMessage = customDetailErrorMessage; > _hasBoundParameters = hasBoundParameters; > } > - > + > // Currently only detail message can be customized. So we override the > // detail message. If summary is to be overridden we have to do the > // same to it also. > @@ -619,6 +619,7 @@ > { > FacesContext context = FacesContext.getCurrentInstance(); > String detailMsgPattern = > (String)_customDetailErrorMessage.getValue(context.getELContext()); > + > if(detailMsgPattern == null) > { > // Set a default message that might get used by FacesException > @@ -631,7 +632,10 @@ > > // Since that string will get parsed by FastMessageFormat, the { } > // of the EL must be escaped > - detailMsgPattern = '\'' + detailMsgPattern + '\''; > + //detailMsgPattern = '\'' + detailMsgPattern + '\''; > + > + // No need to format this string, just return it here. > + return detailMsgPattern; > } > > Object[] params = super.getParameters(); > Index: > trinidad-api/src/test/java/org/apache/myfaces/trinidad/util/FastMessageFormatTest.java > =================================================================== > --- > trinidad-api/src/test/java/org/apache/myfaces/trinidad/util/FastMessageFormatTest.java > (revision 0) > +++ > trinidad-api/src/test/java/org/apache/myfaces/trinidad/util/FastMessageFormatTest.java > (revision 0) > @@ -0,0 +1,59 @@ > +/* > + * Licensed to the Apache Software Foundation (ASF) under one > + * or more contributor license agreements. See the NOTICE file > + * distributed with this work for additional information > + * regarding copyright ownership. 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. > + */ > +package org.apache.myfaces.trinidad.util; > + > +import junit.framework.Test; > +import junit.framework.TestCase; > +import junit.framework.TestSuite; > + > +import org.apache.myfaces.trinidad.util.FastMessageFormat; > + > +public class FastMessageFormatTest > + extends TestCase > +{ > + public static final Test suite() > + { > + return new TestSuite(FastMessageFormatTest.class); > + } > + > + public static void main(String[] args) > + throws Throwable > + { > + junit.textui.TestRunner.run(suite()); > + } > + > + public FastMessageFormatTest(String testName) > + { > + super(testName); > + } > + > + public void testGet() > + { > + // {0} and {1} should be replaced. > + // Param for {2} is null, so remove {2}. > + // The rest is interpreted literally. > + // Expected result: "beef {{3} isn't {} {a} {12a}kosher {" > + FastMessageFormat fmf = > + new FastMessageFormat("{0} {{3} isn't {} {a} {12a}{2}{1} {"); > + String[] params = { "beef", "kosher", null }; > + String result = fmf.format(params); > + assertEquals(result, "beef {{3} isn't {} {a} {12a}kosher {"); > + } > +} > + > Index: trinidad-impl/src/main/javascript/META-INF/adf/jsLibs/Locale.js > =================================================================== > --- trinidad-impl/src/main/javascript/META-INF/adf/jsLibs/Locale.js > (revision 708412) > +++ trinidad-impl/src/main/javascript/META-INF/adf/jsLibs/Locale.js > (working copy) > @@ -830,7 +830,7 @@ > * @param {any...:undefined} Varargs objects to substitute for positional > parameters. > * Each parameter will be converted to a String and substituted into the > format. > */ > -TrFastMessageFormatUtils.format = function( > +/*TrFastMessageFormatUtils.formatOld = function( > formatString, // error format string with embedded indexes to be replaced > parameters // {any...:undefined} Varargs objects to substitute for > positional parameters. > ) > @@ -846,6 +846,67 @@ > // TODO - move the code of the function below into here after > // simplifying it > return _formatErrorString(formatString, tempArray); > +}*/ > + > + /** > + * This formatter will only replace patterns of the type "{[0-9]}" > + * for which there is an associated token. > + * Any other use of '{}' will be interpreted as literal text. > + * This aims to have the same behavior as FastMessageFormat.format > + * on the server. > + * @param {String} String to format > + * @param {any...:undefined} Varargs objects to substitute for positional > parameters. > + * Each parameter will be converted to a String and substituted into the > format. > + */ > +TrFastMessageFormatUtils.format = function( > + formatString, // error format string with embedded indexes to be replaced > + parameters // {any...:undefined} Varargs objects to substitute for > positional parameters. > + ) > +{ > + // There are arguments.length - 1 tokens: > + // arguments[1], ..., arguments[arguments.length-1] > + var formatLength = formatString.length; > + var tokenCount = arguments.length - 1; > + > + // Use the javascript StringBuffer technique. > + var buffer = []; > + > + var lastStart = 0; > + for (var i = 0; i < formatLength; i++) > + { > + var ch = formatString[i]; > + if (ch == '{') > + { > + // Only check for single digit patterns that have an associated > token. > + if (i + 2 < formatLength && formatString[i+2] == '}') > + { > + var tokenIndex = formatString[i+1] - '0'; > + if (tokenIndex >= 0 && tokenIndex < tokenCount) > + { > + // Use the javascript StringBuffer technique for append(string) > + var substr = formatString.substring(lastStart, i); > + buffer.push(substr); > + > + var token = arguments[tokenIndex+1]; > + if (token != null) > + buffer.push(token); > + > + i += 2; > + lastStart = i + 1; > + } > + } > + } > + // ELSE: Do nothing. The character will be added in later. > + } > + > + if (lastStart < formatLength) > + { > + var substr = formatString.substring(lastStart); > + buffer.push(substr); > + } > + > + // Use the javascript StringBuffer technique for toString() > + return buffer.join(""); > } > > var TrMessageFactory = new Object(); > > -- Matthias Wessendorf blog: http://matthiaswessendorf.wordpress.com/ sessions: http://www.slideshare.net/mwessendorf twitter: http://twitter.com/mwessendorf
