Velocity Tools
  1. Velocity Tools
  2. VELTOOLS-104

MessageFormat.format() not invoked in ResourceTool.render() if there are no arguments

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.4, 2.0, 2.x
    • Fix Version/s: 2.0, 2.x
    • Component/s: GenericTools
    • Labels:
      None

      Description

      render() currently looks like this:

      public String render(Object resource, Object[] args)
      {
      String value = String.valueOf(resource);
      if (args == null)

      { return value; }

      return MessageFormat.format(value, args);
      }

      Unfortunately, if you're using MessageFormat-ready messages then you should be format()ting even strings with no arguments, even if this seems like an inefficiency.

      We use many strings that are used on their own, or as part of other messages, sometimes they have arguments and sometimes they don't. This creates a sticky situation if you have to use MessageFormat escaping for some messages and not others. For example:

      message1=ResourceTool's problem
      message2=$(message1) is

      {0}

      where "$(message1)" is a substitution for message1 (which our ResourceBundle does for us). ResourceTool renders the first message fine but the second message with an argument doesn't render properly due to the unescaped ' passed to MessageFormat. Even without the special message substitution render() causes problems because the escaping rules are different depending on the message context.

      The messages should actually be:

      message1=ResourceTool''s problem
      message2=$(message1) is {0}

      to be consistent with MessageFormat's rules across all messages. Unfortunately ResourceTool can't handle this because it doesn't pass a no-argument resource to MessageFormat.

      I'm using a subclass of ResourceTool that doesn't have the shortcut:

      @Override
      public String render(Object resource, Object[] args)

      { return MessageFormat.format(String.valueOf(resource), args); }

      So all my messages have the same escaping rules.

        Activity

        Hide
        Nathan Bubna added a comment -

        Hmm. I suppose you're right.

        Show
        Nathan Bubna added a comment - Hmm. I suppose you're right.
        Hide
        Rod added a comment -

        Thanks Nathan,

        javadoc should probably be updated too as it describes the old behaviour, there is potential for it to catch people who are using unescaped no-arg messages:

        /**

        • Renders the specified resource value and arguments as a String.
        • If there are no arguments, then the String value of the resource
        • is returned directly. If there are arguments, then the resource
        • is treated as a {@link MessageFormat} pattern and used to format
          * the specified argument values.
          */

          Maybe:

          /**
          * Renders the specified resource value and arguments as a String.
          * The resource is treated as a {@link MessageFormat}

          pattern which

        • is used for formatting along with any specified argument values.
          */
        Show
        Rod added a comment - Thanks Nathan, javadoc should probably be updated too as it describes the old behaviour, there is potential for it to catch people who are using unescaped no-arg messages: /** Renders the specified resource value and arguments as a String. If there are no arguments, then the String value of the resource is returned directly. If there are arguments, then the resource is treated as a {@link MessageFormat} pattern and used to format * the specified argument values. */ Maybe: /** * Renders the specified resource value and arguments as a String. * The resource is treated as a {@link MessageFormat} pattern which is used for formatting along with any specified argument values. */
        Hide
        Nathan Bubna added a comment -

        Thanks, Rod. Your change looks great.

        Show
        Nathan Bubna added a comment - Thanks, Rod. Your change looks great.
        Hide
        Christopher Schultz added a comment -

        If this change requires a change to the javadocs, it's basically changing previously-expected behavior.

        Should we maybe reprecate the "render" method and implement another one alongside it with these new semantics? I can't really see how the existing semantics make any sense in the first place, but this is supposed to be a stable API.

        Show
        Christopher Schultz added a comment - If this change requires a change to the javadocs, it's basically changing previously-expected behavior. Should we maybe reprecate the "render" method and implement another one alongside it with these new semantics? I can't really see how the existing semantics make any sense in the first place, but this is supposed to be a stable API.
        Hide
        Nathan Bubna added a comment -

        No, the render method is central to most of the ResourceTool methods. The only way to offer both semantics is to make the behavior configurable. I considered that and am still open to it, but i was feeling lazy. I don't think that many people would have relied on MessageFormat not processing messages without args. You're right though, we are trying not only to keep the API as stable as possible now that 2.0 is in beta, and more importantly, we are trying to provide backwards compatibility with 1.3+.

        I suppose the best way to go would be to make ResourceTool watch for deprecationSupportMode. It could then do the old thing for those using older toolboxes or the old ResourceTool class, or anyone who set that property.

        Also, we could mention somewhere that $text.foo.raw (adding '.raw') does the equivalent of the old behavior and returns the resource without letting MessageFormat get its hands on it.

        Show
        Nathan Bubna added a comment - No, the render method is central to most of the ResourceTool methods. The only way to offer both semantics is to make the behavior configurable. I considered that and am still open to it, but i was feeling lazy. I don't think that many people would have relied on MessageFormat not processing messages without args. You're right though, we are trying not only to keep the API as stable as possible now that 2.0 is in beta, and more importantly, we are trying to provide backwards compatibility with 1.3+. I suppose the best way to go would be to make ResourceTool watch for deprecationSupportMode. It could then do the old thing for those using older toolboxes or the old ResourceTool class, or anyone who set that property. Also, we could mention somewhere that $text.foo.raw (adding '.raw') does the equivalent of the old behavior and returns the resource without letting MessageFormat get its hands on it.

          People

          • Assignee:
            Unassigned
            Reporter:
            Rod
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development