Spring Framework
  1. Spring Framework
  2. SPR-7209

SpEL: Elvis operator throwing NPE in case of an empty base expression

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Complete
    • Affects Version/s: 3.0.2
    • Fix Version/s: 3.0.3
    • Component/s: Core
    • Labels:
      None
    • Last commented by a User:
      false

      Description

      Embedded SpEL expressions throw an NPE if looked-up property-placeholder variable is null.

      Using the Elvis operator which is designed to deal with null:
      @Value("#{$

      {property1}?:'default'}") private String property2;

      This works fine if ${property1}

      evaluates to a non-null but provides the following if it is null.

      Caused by: java.lang.NullPointerException
      at org.springframework.expression.spel.ast.SpelNodeImpl.<init>(SpelNodeImpl.java:50)
      at org.springframework.expression.spel.ast.Elvis.<init>(Elvis.java:33)
      at org.springframework.expression.spel.standard.InternalSpelExpressionParser.eatExpression(InternalSpelExpressionParser.java:143)
      at org.springframework.expression.spel.standard.InternalSpelExpressionParser.doParseExpression(InternalSpelExpressionParser.java:114)
      at org.springframework.expression.spel.standard.SpelExpressionParser.doParseExpression(SpelExpressionParser.java:56)
      at org.springframework.expression.spel.standard.SpelExpressionParser.doParseExpression(SpelExpressionParser.java:1)
      at org.springframework.expression.common.TemplateAwareExpressionParser.parseExpressions(TemplateAwareExpressionParser.java:128)
      at org.springframework.expression.common.TemplateAwareExpressionParser.parseTemplate(TemplateAwareExpressionParser.java:74)
      at org.springframework.expression.common.TemplateAwareExpressionParser.parseExpression(TemplateAwareExpressionParser.java:64)
      at org.springframework.context.expression.StandardBeanExpressionResolver.evaluate(StandardBeanExpressionResolver.java:119)

      The same thing happens if you use a .property file with the following
      property1=
      property2=#{$

      {property1}

      ?:'default'}

      Notes:

        Issue Links

          Activity

          Hide
          drekbour added a comment -

          Before anyone mentions other ways of setting defaults there are three issues with $

          {x:y}

          syntax here:
          (1) @Value("$

          {property1:default}") resolves to the empty string in this example as "property1=" is not null but equivalent to property1=""
          (2) Knowing (1), we can set defaults using this (deep breath) @Value( "#{'${property1}

          '.length()==0?'default':'$

          {property1:}

          '}" )
          (3) This is a general issue relating to parsing #{} blocks

          Show
          drekbour added a comment - Before anyone mentions other ways of setting defaults there are three issues with $ {x:y} syntax here: (1) @Value("$ {property1:default}") resolves to the empty string in this example as "property1=" is not null but equivalent to property1="" (2) Knowing (1), we can set defaults using this (deep breath) @Value( "#{'${property1} '.length()==0?'default':'$ {property1:} '}" ) (3) This is a general issue relating to parsing #{} blocks
          Hide
          Juergen Hoeller added a comment -

          Actually, the NPE does seem to be an issue with the Elvis operator specifically: After placeholder substitution, the above looks like "#{?:'default'}" which reproducibly throws that NPE in SpelNodeImpl because the base expression object (before the operator) is null. Andy, could you please have a look at this? We need to catch that case.

          As a secondary issue, placeholder defaulting as well as the Elvis operator only fall back to the specified default value if the primary value resolves to null. An empty String, as in your case with that "property1=" entry in the file, is not specifically detected. Maybe we should consider treating empty Strings like null there...

          Juergen

          Show
          Juergen Hoeller added a comment - Actually, the NPE does seem to be an issue with the Elvis operator specifically: After placeholder substitution, the above looks like "#{?:'default'}" which reproducibly throws that NPE in SpelNodeImpl because the base expression object (before the operator) is null. Andy, could you please have a look at this? We need to catch that case. As a secondary issue, placeholder defaulting as well as the Elvis operator only fall back to the specified default value if the primary value resolves to null . An empty String, as in your case with that "property1=" entry in the file, is not specifically detected. Maybe we should consider treating empty Strings like null there... Juergen
          Hide
          Andy Clement added a comment -

          Ok - yes the parser is failing if nonsense is passed in. I've addressed the error. Normally I would throw a parser error for something like "?:'default'" as it is not well formed, but that seems unhelpful here so I now tolerate it (and on evaluation it will return 'default'). I also tolerate the simple "?:" with no argument and no default value... this will return null. Juergen - do you think this is OK or would you prefer a real parser error for a bad expression like that? It would mean the caller would have to ensure the expression is well formed.

          At the same time I also addressed the same problem for ternary - parsing will not fail (although you will get an error on evaluation that the condition is not a boolean)

          > Maybe we should consider treating empty Strings like null there...

          I am ok with that, done it. Now an empty string is treated like null for elvis. This is consistent with elvis meaning in groovy (how it interprets empty strings). Depending on negative feedback here, we could make it a configurable property of the parser/evaluation context - in case the old behaviour is required in some situations.

          Show
          Andy Clement added a comment - Ok - yes the parser is failing if nonsense is passed in. I've addressed the error. Normally I would throw a parser error for something like "?:'default'" as it is not well formed, but that seems unhelpful here so I now tolerate it (and on evaluation it will return 'default'). I also tolerate the simple "?:" with no argument and no default value... this will return null. Juergen - do you think this is OK or would you prefer a real parser error for a bad expression like that? It would mean the caller would have to ensure the expression is well formed. At the same time I also addressed the same problem for ternary - parsing will not fail (although you will get an error on evaluation that the condition is not a boolean) > Maybe we should consider treating empty Strings like null there... I am ok with that, done it. Now an empty string is treated like null for elvis. This is consistent with elvis meaning in groovy (how it interprets empty strings). Depending on negative feedback here, we could make it a configurable property of the parser/evaluation context - in case the old behaviour is required in some situations.
          Hide
          Juergen Hoeller added a comment -

          Wow, that was quick - thanks, Andy! Being lenient when interpreting that case is fine with me, and hopefully also with this issue's reporter Since this is quite intuitive, I don't think we need a configuration option here. Let's see whether anybody comes up with a specific use case for the old behavior.

          Juergen

          Show
          Juergen Hoeller added a comment - Wow, that was quick - thanks, Andy! Being lenient when interpreting that case is fine with me, and hopefully also with this issue's reporter Since this is quite intuitive, I don't think we need a configuration option here. Let's see whether anybody comes up with a specific use case for the old behavior. Juergen

            People

            • Assignee:
              Andy Clement
              Reporter:
              drekbour
              Last updater:
              Trevor Marshall
            • Votes:
              1 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:
                Days since last comment:
                3 years, 49 weeks, 1 day ago