[Trinidad] Resolving differences between client-side and server-side message formatter?

[Cale Scholl <ca...@oracle.com>]

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, "'");
}