Struts 2
  1. Struts 2
  2. WW-4066

Submitting form with parameters using brackets while devMode=true yields StringIndexOutOfBoundsException

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.3.14
    • Fix Version/s: 2.3.16
    • Component/s: Core Actions
    • Labels:
      None

      Description

      Our BaseAction which extends ActionSupport overrides the addActionMessage() with the following:

      @Override
      public void addActionMessage(String message) {
        super.addActionMessage(getText(message));
      }
      

      With the above method in place during devMode=true, the following error stack trace occurs:

      java.lang.StringIndexOutOfBoundsException: String index out of range: -1
        at java.lang.String.substring(String.java:1871)
        at com.opensymphony.xwork2.util.LocalizedTextUtil.findText(LocalizedTextUtil.java:426)
        at com.opensymphony.xwork2.util.LocalizedTextUtil.findText(LocalizedTextUtil.java:362)
        at com.opensymphony.xwork2.TextProviderSupport.getText(TextProviderSupport.java:208)
        at com.opensymphony.xwork2.TextProviderSupport.getText(TextProviderSupport.java:123)
        at com.opensymphony.xwork2.ActionSupport.getText(ActionSupport.java:103)
        at com.setech.dw.common.web.BaseAction.addActionMessage(BaseAction.java:209)
        at com.opensymphony.xwork2.interceptor.ParametersInterceptor.setParameters(ParametersInterceptor.java:337)
        at com.opensymphony.xwork2.interceptor.ParametersInterceptor.doIntercept(ParametersInterceptor.java:241)
      
      1. ParametersInterceptor.java
        6 kB
        Johno Crawford
      2. testcase.zip
        6.72 MB
        Chris Cranford

        Activity

        Chris Cranford created issue -
        Lukasz Lenart made changes -
        Field Original Value New Value
        Description Our BaseAction which extends ActionSupport overrides the addActionMessage() with the following:

        @Override
        public void addActionMessage(String message) {
          super.addActionMessage(getText(message));
        }

        With the above method in place during devMode=true, the following error stack trace occurs:

        java.lang.StringIndexOutOfBoundsException: String index out of range: -1
          at java.lang.String.substring(String.java:1871)
          at com.opensymphony.xwork2.util.LocalizedTextUtil.findText(LocalizedTextUtil.java:426)
          at com.opensymphony.xwork2.util.LocalizedTextUtil.findText(LocalizedTextUtil.java:362)
          at com.opensymphony.xwork2.TextProviderSupport.getText(TextProviderSupport.java:208)
          at com.opensymphony.xwork2.TextProviderSupport.getText(TextProviderSupport.java:123)
          at com.opensymphony.xwork2.ActionSupport.getText(ActionSupport.java:103)
          at com.setech.dw.common.web.BaseAction.addActionMessage(BaseAction.java:209)
          at com.opensymphony.xwork2.interceptor.ParametersInterceptor.setParameters(ParametersInterceptor.java:337)
          at com.opensymphony.xwork2.interceptor.ParametersInterceptor.doIntercept(ParametersInterceptor.java:241)
        Our BaseAction which extends ActionSupport overrides the addActionMessage() with the following:

        {code:java}
        @Override
        public void addActionMessage(String message) {
          super.addActionMessage(getText(message));
        }
        {code}

        With the above method in place during devMode=true, the following error stack trace occurs:

        {noformat}
        java.lang.StringIndexOutOfBoundsException: String index out of range: -1
          at java.lang.String.substring(String.java:1871)
          at com.opensymphony.xwork2.util.LocalizedTextUtil.findText(LocalizedTextUtil.java:426)
          at com.opensymphony.xwork2.util.LocalizedTextUtil.findText(LocalizedTextUtil.java:362)
          at com.opensymphony.xwork2.TextProviderSupport.getText(TextProviderSupport.java:208)
          at com.opensymphony.xwork2.TextProviderSupport.getText(TextProviderSupport.java:123)
          at com.opensymphony.xwork2.ActionSupport.getText(ActionSupport.java:103)
          at com.setech.dw.common.web.BaseAction.addActionMessage(BaseAction.java:209)
          at com.opensymphony.xwork2.interceptor.ParametersInterceptor.setParameters(ParametersInterceptor.java:337)
          at com.opensymphony.xwork2.interceptor.ParametersInterceptor.doIntercept(ParametersInterceptor.java:241)
        {noformat}
        Lukasz Lenart made changes -
        Fix Version/s 2.3.15 [ 12324267 ]
        Hide
        Lukasz Lenart added a comment -

        Could you prepare a small demo app, maven based?

        Show
        Lukasz Lenart added a comment - Could you prepare a small demo app, maven based?
        Hide
        Lukasz Lenart added a comment -

        I need that example or more detailed informations how to reproduce the problem. I have been trying just to add an indexed field and override the method as you, but no luck

        Show
        Lukasz Lenart added a comment - I need that example or more detailed informations how to reproduce the problem. I have been trying just to add an indexed field and override the method as you, but no luck
        Hide
        Chris Cranford added a comment -

        I will get a demo app together for you later today or first thing in the morning.

        Show
        Chris Cranford added a comment - I will get a demo app together for you later today or first thing in the morning.
        Hide
        Chris Cranford added a comment -

        Maven test case project, including built WAR using JDK1.7

        Show
        Chris Cranford added a comment - Maven test case project, including built WAR using JDK1.7
        Chris Cranford made changes -
        Attachment testcase.zip [ 12583665 ]
        Lukasz Lenart made changes -
        Assignee Lukasz Lenart [ lukaszlenart ]
        Hide
        Lukasz Lenart added a comment - - edited

        I have switched to 2.1.8.1 and the problem still exists and I have tested all versions till 2.3.15-SNAPSHOT :\

        Something is really weird

        Show
        Lukasz Lenart added a comment - - edited I have switched to 2.1.8.1 and the problem still exists and I have tested all versions till 2.3.15-SNAPSHOT :\ Something is really weird
        Hide
        Lukasz Lenart added a comment -

        I have tested with the latest 2.3.15-SNAPSHOT version and the only problem I have spotted is related to search button and the presented warning is a desired behaviour in devMode - to allow developers simply find typos in setters / names of parameters eg. search and setSeaarch(String value).

        Show
        Lukasz Lenart added a comment - I have tested with the latest 2.3.15-SNAPSHOT version and the only problem I have spotted is related to search button and the presented warning is a desired behaviour in devMode - to allow developers simply find typos in setters / names of parameters eg. search and setSeaarch(String value).
        Lukasz Lenart made changes -
        Status Open [ 1 ] Resolved [ 5 ]
        Resolution Not A Problem [ 8 ]
        Show
        Lukasz Lenart added a comment - I have added a note to the docs https://cwiki.apache.org/confluence/display/WW/Parameters+Interceptor#ParametersInterceptor-Warningonmissingparameters
        Hide
        Lukasz Lenart added a comment -

        To clarify - I removed <interceptors/> from your struts.xml - maybe we should double / move modelDriven interceptor before the params interceptor in paramsPrepareParamsStack

        Show
        Lukasz Lenart added a comment - To clarify - I removed <interceptors/> from your struts.xml - maybe we should double / move modelDriven interceptor before the params interceptor in paramsPrepareParamsStack
        Lukasz Lenart made changes -
        Resolution Not A Problem [ 8 ]
        Status Resolved [ 5 ] Reopened [ 4 ]
        Hide
        Lukasz Lenart added a comment -

        The problems lays in paramsPrepareParamsStack - it has to be updated to match changes in defaultStack

        Show
        Lukasz Lenart added a comment - The problems lays in paramsPrepareParamsStack - it has to be updated to match changes in defaultStack
        Hide
        Chris Cranford added a comment -

        I have tested with the latest 2.3.15-SNAPSHOT version and the only problem I have spotted is related to search button and the presented warning is a desired behaviour in devMode - to allow developers simply find typos in setters / names of parameters eg. search and setSeaarch(String value).

        In the case of search, we generally treat the key="" portion of that struts2 tag like key="button.search". I haven't looked at the code prior to making this post, but if key="button.search" translates to various values depending on the localization being used, wouldn't that mean I would have different setters per language?

        Show
        Chris Cranford added a comment - I have tested with the latest 2.3.15-SNAPSHOT version and the only problem I have spotted is related to search button and the presented warning is a desired behaviour in devMode - to allow developers simply find typos in setters / names of parameters eg. search and setSeaarch(String value). In the case of search, we generally treat the key="" portion of that struts2 tag like key="button.search". I haven't looked at the code prior to making this post, but if key="button.search" translates to various values depending on the localization being used, wouldn't that mean I would have different setters per language?
        Hide
        Lukasz Lenart added a comment - - edited

        The problem is related to prepare interceptor and modelDriven interceptor - first will call prepare() method which will create instance of ItemSearchCriteria in ItemSearchAction#prepare() then modelDriven will push model onto stack.

        The problem here is with paramsPrepareParamsStack where modelDriven is before prepare and model is null in that case and it wont be pushed onto stack and thus produces mentioned warnings.

        I think we could add a new interceptor - ModelCreateInterceptor - which can be used just before modelDriven to create instance of the model - it will delegate to action to instantiate model.

        Show
        Lukasz Lenart added a comment - - edited The problem is related to prepare interceptor and modelDriven interceptor - first will call prepare() method which will create instance of ItemSearchCriteria in ItemSearchAction#prepare() then modelDriven will push model onto stack. The problem here is with paramsPrepareParamsStack where modelDriven is before prepare and model is null in that case and it wont be pushed onto stack and thus produces mentioned warnings. I think we could add a new interceptor - ModelCreateInterceptor - which can be used just before modelDriven to create instance of the model - it will delegate to action to instantiate model.
        Hide
        Lukasz Lenart added a comment -

        Or you can move creation of model into getModel() method from prepare() method.

        Show
        Lukasz Lenart added a comment - Or you can move creation of model into getModel() method from prepare() method.
        Hide
        Chris Cranford added a comment - - edited

        Or you can move creation of model into getModel() method from prepare() method.

        I don't believe that is a viable option because getModel() is called for every parameter which needs to be set on the model object itself. Putting logic inside that method which needs to determine whether the model needs to be created or not is unnecessary overhead and precisely what I thought prepare() was designed to offer.

        I'm very surprised that others who make use of ModelDriven<T> haven't spoken up about this, perhaps they haven't tried to patch. But we make considerable use of ModelDriven across our entire application and having to alter how models are created in those actions is a serious undertaking, one which after internal discussion would likely leave us at 2.3.4.1 indefinitely.

        Show
        Chris Cranford added a comment - - edited Or you can move creation of model into getModel() method from prepare() method. I don't believe that is a viable option because getModel() is called for every parameter which needs to be set on the model object itself. Putting logic inside that method which needs to determine whether the model needs to be created or not is unnecessary overhead and precisely what I thought prepare() was designed to offer. I'm very surprised that others who make use of ModelDriven<T> haven't spoken up about this, perhaps they haven't tried to patch. But we make considerable use of ModelDriven across our entire application and having to alter how models are created in those actions is a serious undertaking, one which after internal discussion would likely leave us at 2.3.4.1 indefinitely.
        Hide
        Lukasz Lenart added a comment -

        The problem isn't related to ModelDriven but to order of interceptors in paramsPrepareParamsStack and the mentioned warning is just in devMode to alert developer that something potentially can be wrong.

        If you take a look on paramsPrepareParamsStack you will spot that the params interceptor is listed two times - before and after prepare, but there is also modelDriven listed just before the second params which will put non-null Model onto stack.

        The mentioned warnings come from the first params as action doesn't have appropriate setters and Model wasn't yet put onto stack. Even if you add additional modelDriven interceptor before the first params, model still won't be on the stack as it is null. That's why I have suggested to create a dedicated interceptor to solve that uncommon situation or move logic to create Model into getModel() method.

        Or leave everything as is as it's just a warning in devMode

        Show
        Lukasz Lenart added a comment - The problem isn't related to ModelDriven but to order of interceptors in paramsPrepareParamsStack and the mentioned warning is just in devMode to alert developer that something potentially can be wrong. If you take a look on paramsPrepareParamsStack you will spot that the params interceptor is listed two times - before and after prepare , but there is also modelDriven listed just before the second params which will put non-null Model onto stack. The mentioned warnings come from the first params as action doesn't have appropriate setters and Model wasn't yet put onto stack. Even if you add additional modelDriven interceptor before the first params , model still won't be on the stack as it is null. That's why I have suggested to create a dedicated interceptor to solve that uncommon situation or move logic to create Model into getModel() method. Or leave everything as is as it's just a warning in devMode
        Hide
        Chris Cranford added a comment - - edited

        If you take a look on paramsPrepareParamsStack you will spot that the params interceptor is listed two times - before and after prepare, but there is also modelDriven listed just before the second params which will put non-null Model onto stack.

        I don't recall specifically why the interceptor stack was designed that way beyond perhaps being backward compatible. I recall a discussion sometime back where it was decided that the interceptor stack would be designed this way on the mailing list.

        I just applied 2.3.4.1 to the sample application and with devMode=true, I do see where the OGNL exceptions are written to the logs, perhaps I had overlooked them prior. What boggles me however, is that StringIndexOutOfBoundsException isn't thrown as a side affect. And the stack trace related to that exception is coming from LocalizedTextUtil. Why is that just now surfacing but not in prior builds?

        Show
        Chris Cranford added a comment - - edited If you take a look on paramsPrepareParamsStack you will spot that the params interceptor is listed two times - before and after prepare, but there is also modelDriven listed just before the second params which will put non-null Model onto stack. I don't recall specifically why the interceptor stack was designed that way beyond perhaps being backward compatible. I recall a discussion sometime back where it was decided that the interceptor stack would be designed this way on the mailing list. I just applied 2.3.4.1 to the sample application and with devMode=true, I do see where the OGNL exceptions are written to the logs, perhaps I had overlooked them prior. What boggles me however, is that StringIndexOutOfBoundsException isn't thrown as a side affect. And the stack trace related to that exception is coming from LocalizedTextUtil . Why is that just now surfacing but not in prior builds?
        Hide
        Rene Gielen added a comment -

        Both order and having params interceptor twice within the stack (notice the double reference of params in the stack name) are not for backward compatibility, but for providing an advanced use case. Using ModelDriven with PPP isn't a 100% smooth experience, though - but there is not too much room for improvements in that area.

        Show
        Rene Gielen added a comment - Both order and having params interceptor twice within the stack (notice the double reference of params in the stack name) are not for backward compatibility, but for providing an advanced use case. Using ModelDriven with PPP isn't a 100% smooth experience, though - but there is not too much room for improvements in that area.
        Hide
        Lukasz Lenart added a comment - - edited

        Your code below is trying to localise already localised message and as the message contains [], LocalizedTextUtil is trying to parse them. Before there was no such exception and was no problem in LocalizedTextUtil - that's me thoughts

        @Override
        public void addActionMessage(String message) {
          super.addActionMessage(getText(message));
        }
        
        Show
        Lukasz Lenart added a comment - - edited Your code below is trying to localise already localised message and as the message contains [] , LocalizedTextUtil is trying to parse them. Before there was no such exception and was no problem in LocalizedTextUtil - that's me thoughts @Override public void addActionMessage( String message) { super .addActionMessage(getText(message)); }
        Hide
        Lukasz Lenart added a comment - - edited

        My idea is still the same - create dedicated interceptor with connected interface to allow create model - something like:

        public interface ModelDrivenCreateAware {
        
            void createModel();
        
        }
        
        public class ModelDrivenCreateInterceptor {
        
            public String doIntercept(ActionInvocation ai) {
               if (ai.getAction() instanceof ModelDivenCreateAware) {
                   (ModelDivenCreateAware(ai.getAction())).createModel();
               }
            }
        
        }
        

        which should be merged with ModelDriven at some point.

        Show
        Lukasz Lenart added a comment - - edited My idea is still the same - create dedicated interceptor with connected interface to allow create model - something like: public interface ModelDrivenCreateAware { void createModel(); } public class ModelDrivenCreateInterceptor { public String doIntercept(ActionInvocation ai) { if (ai.getAction() instanceof ModelDivenCreateAware) { (ModelDivenCreateAware(ai.getAction())).createModel(); } } } which should be merged with ModelDriven at some point.
        Hide
        Lukasz Lenart added a comment -

        Chris Cranford what's your opinion? What should I do? I'm going to postpone this issue and we can return to it in the future.

        Show
        Lukasz Lenart added a comment - Chris Cranford what's your opinion? What should I do? I'm going to postpone this issue and we can return to it in the future.
        Lukasz Lenart made changes -
        Fix Version/s 2.3.16 [ 12324546 ]
        Fix Version/s 2.3.15 [ 12324267 ]
        Hide
        Chris Cranford added a comment - - edited

        One of the reasons why our base action overrides the action errors, message, and field message methods was because it allowed us to simply call addActionMessage("some.message"); without having to wrap the message itself using getText. That part is what has worked up until the latest builds.

        What I'll propose to our team is that we'll modify all actions to make use of the getText() call within each action where we pass a message to the methods that we were originally overriding and remove their overrides from the base action. That should bring the code base in line with the expectations and avoid the StringIndexOutOfBoundsException error.

        But what still troubles me with this is related to the developer notifications themselves.

        I modified the ItemSearchAction as follows:

        public ItemSearchAction() {
          this.criteria = new ItemSearchCriteria();
        }
        

        Technically, that should instantiate the model prior to any method invocations on the action itself in a similar fashion as your interceptor proposal would insure the model is created before trying to set the parameters on the action. But unfortunately, when I display the action messages which resulted from the form submission to the action, I still get notifications that there were issues with all the form elements.

        Looking at the tags used in the form itself, the submit tag doesn't dictate any attributes as required. We typically use the key attribute so that it does the localization lookup for the value attribute. Using key also sets the name attribute of the tag but with the non-localization value. When using key='button.search', I end up with the following:

        <input type='submit' id='form_button_search' name='button.search' value='Search'>

        Following your developer notifications, that implies I need some object called 'button' inside the model with a property called 'search'. Is the use of the key attribute of a submit tag with a phrase that contains a '.' when the action implements the ModelDriven<T> interface not possible without generating these warnings?

        It seems the only viable options here is either A) don't use a '.' in the key names, B) modify the model to contain the hierarchy so use of '.' is possible, or C) don't use the key attribute at all and simply use the value attribute where it contains:

        value='%{getText('button.search')}'

        What do you suggest?

        Lastly, I still can't seem to submit the values for the itemConditionTypes, itemNumberTypes, itemNumbers, and itemSearchTypes arrays and the excel property without the developer notifications even when the model is instantiated upon the creation of the action itself.

        Are we sure that creating the model with your above proposed solution will 'fix' my test case?

        To clarify - I had not yet removed the 'paramsPrepareParamsStack' interceptor reference as you had indicated you had done. In doing so and using the default struts interceptor stack, I no longer get warnings regarding anything except for the search button even if I instantiate the model during action creation or not.

        So I understand your point about needing to somehow alter the 'paramsPrepareParamsStack' but the default stack seems to work as intended. I'll have to check why we chose to use that particular interceptor stack rather than the default. But can you elaborate on the issue regarding the submit tag and how best to fix it related to the developer notifications?

        Show
        Chris Cranford added a comment - - edited One of the reasons why our base action overrides the action errors, message, and field message methods was because it allowed us to simply call addActionMessage("some.message"); without having to wrap the message itself using getText . That part is what has worked up until the latest builds. What I'll propose to our team is that we'll modify all actions to make use of the getText() call within each action where we pass a message to the methods that we were originally overriding and remove their overrides from the base action. That should bring the code base in line with the expectations and avoid the StringIndexOutOfBoundsException error. But what still troubles me with this is related to the developer notifications themselves. I modified the ItemSearchAction as follows: public ItemSearchAction() { this .criteria = new ItemSearchCriteria(); } Technically, that should instantiate the model prior to any method invocations on the action itself in a similar fashion as your interceptor proposal would insure the model is created before trying to set the parameters on the action. But unfortunately, when I display the action messages which resulted from the form submission to the action, I still get notifications that there were issues with all the form elements. Looking at the tags used in the form itself, the submit tag doesn't dictate any attributes as required. We typically use the key attribute so that it does the localization lookup for the value attribute. Using key also sets the name attribute of the tag but with the non-localization value. When using key='button.search', I end up with the following: <input type='submit' id='form_button_search' name='button.search' value='Search'> Following your developer notifications, that implies I need some object called 'button' inside the model with a property called 'search'. Is the use of the key attribute of a submit tag with a phrase that contains a '.' when the action implements the ModelDriven<T> interface not possible without generating these warnings? It seems the only viable options here is either A) don't use a '.' in the key names, B) modify the model to contain the hierarchy so use of '.' is possible, or C) don't use the key attribute at all and simply use the value attribute where it contains: value='%{getText('button.search')}' What do you suggest? Lastly, I still can't seem to submit the values for the itemConditionTypes, itemNumberTypes, itemNumbers, and itemSearchTypes arrays and the excel property without the developer notifications even when the model is instantiated upon the creation of the action itself. Are we sure that creating the model with your above proposed solution will 'fix' my test case? To clarify - I had not yet removed the 'paramsPrepareParamsStack' interceptor reference as you had indicated you had done. In doing so and using the default struts interceptor stack, I no longer get warnings regarding anything except for the search button even if I instantiate the model during action creation or not. So I understand your point about needing to somehow alter the 'paramsPrepareParamsStack' but the default stack seems to work as intended. I'll have to check why we chose to use that particular interceptor stack rather than the default. But can you elaborate on the issue regarding the submit tag and how best to fix it related to the developer notifications?
        Hide
        Chris Cranford added a comment -

        So I understand your point about needing to somehow alter the 'paramsPrepareParamsStack' but the default stack seems to work as intended. I'll have to check why we chose to use that particular interceptor stack rather than the default.

        In reviewing the struts-default XML configuration, I see specifically why we opted for the paramsPrepareParamsStack. It's been several years since we started the foundation on this application and so I had forgotten the reasons for my decision .

        A majority of our existing ModelDriven<T> actions inspect a set of submitted values and based on those values, determine how the model is to be instantiated. That was the real benefit of this particular interceptor stack for our application's design because it allowed us to set properties on the action for those specific submitted values, do whatever logic inside prepare() was necessary for the model's allocation, and then the second parameters interceptor invocation populated the model accordingly.

        The old way this interceptor stack worked with parameters had little assumptions since parameters could technically exist on the action, model, or a combination of both. I understand the reason to prompt the developer with a warning if the property doesn't exist on the action or the model, but if it exists on at least one of them, I don't believe a developer warning message makes sense in this use case.

        Show
        Chris Cranford added a comment - So I understand your point about needing to somehow alter the 'paramsPrepareParamsStack' but the default stack seems to work as intended. I'll have to check why we chose to use that particular interceptor stack rather than the default. In reviewing the struts-default XML configuration, I see specifically why we opted for the paramsPrepareParamsStack . It's been several years since we started the foundation on this application and so I had forgotten the reasons for my decision . A majority of our existing ModelDriven<T> actions inspect a set of submitted values and based on those values, determine how the model is to be instantiated. That was the real benefit of this particular interceptor stack for our application's design because it allowed us to set properties on the action for those specific submitted values, do whatever logic inside prepare() was necessary for the model's allocation, and then the second parameters interceptor invocation populated the model accordingly. The old way this interceptor stack worked with parameters had little assumptions since parameters could technically exist on the action, model, or a combination of both. I understand the reason to prompt the developer with a warning if the property doesn't exist on the action or the model, but if it exists on at least one of them, I don't believe a developer warning message makes sense in this use case.
        Hide
        Chris Cranford added a comment -

        In doing a bit more of my own testing, I found under 2.3.4.1 the OgnlException exceptions are thrown even during the first params portion of the paramsPrepareParamsStack as seen here:

        WARNING: Error setting expression 'itemSearchTypes[3]' with value '[Ljava.lang.String;@63336706'
        ognl.OgnlException: target is null for setProperty(null, "3", [Ljava.lang.String;@63336706)
        

        It appears then to me the only visible difference here is that no action messages were ever being generated and added to the action's message list for either case of the invocation of the params portion of the interceptor stack in the older builds; where-as in the later builds these exceptions are actually triggering developer warnings. Because these warnings included a '[' character, that is what provoked the getText() method to throw an exception with our original base action implementation.

        But I don't see how the ModelDrivenCreateAware interface solves the problem of where the property exists on the model but not the action, as seen in the demo application added.

        • First ParametersInterceptor call populates the action instance.
        • The ModelDrivenInterceptor simply pushes model from getModel onto the top of the stack
        • Second ParametersInterceptor call populates the model instance.

        If we alter this above functionality in anyway, would it not have a wider impact on those using that particular interceptor stack, right?

        Show
        Chris Cranford added a comment - In doing a bit more of my own testing, I found under 2.3.4.1 the OgnlException exceptions are thrown even during the first params portion of the paramsPrepareParamsStack as seen here: WARNING: Error setting expression 'itemSearchTypes[3]' with value '[Ljava.lang. String ;@63336706' ognl.OgnlException: target is null for setProperty( null , "3" , [Ljava.lang. String ;@63336706) It appears then to me the only visible difference here is that no action messages were ever being generated and added to the action's message list for either case of the invocation of the params portion of the interceptor stack in the older builds; where-as in the later builds these exceptions are actually triggering developer warnings. Because these warnings included a '[' character, that is what provoked the getText() method to throw an exception with our original base action implementation. But I don't see how the ModelDrivenCreateAware interface solves the problem of where the property exists on the model but not the action, as seen in the demo application added. First ParametersInterceptor call populates the action instance. The ModelDrivenInterceptor simply pushes model from getModel onto the top of the stack Second ParametersInterceptor call populates the model instance. If we alter this above functionality in anyway, would it not have a wider impact on those using that particular interceptor stack, right?
        Hide
        Lukasz Lenart added a comment - - edited

        WoW Lots of reading

        I need a time to digest all your comments - anyway I would like to restore the original framework's behaviour in that case (except Developer Notification) as migration to the next minor version shouldn't break backward compatibility.

        Show
        Lukasz Lenart added a comment - - edited WoW Lots of reading I need a time to digest all your comments - anyway I would like to restore the original framework's behaviour in that case (except Developer Notification) as migration to the next minor version shouldn't break backward compatibility.
        Lukasz Lenart made changes -
        Comment [ Just collecting what could potentially break backward compatibility:

        - silly change, just added {{e}} to have the full stacktrace (but for sure changed with wrong commit):
        http://svn.apache.org/viewvc/struts/struts2/trunk/xwork-core/src/main/java/com/opensymphony/xwork2/interceptor/ParametersInterceptor.java?r1=1439410&r2=1456961&diff_format=h ]
        Hide
        Lukasz Lenart added a comment -

        Johno Crawford found the problem. devMode never worked in ParamatersInterceptor before

        Check that commit [1] (line 153-154), as you can see devMode field was static which means it was never set - you can check that starting in debug mode the example app and setting break point in line 319 (Struts 2.3.4.1).

        So, framework's behaviour didn't change at all, you just wasn't informed about that I have changed BaseAction and wrapped super.addActionError(getText(message)); with try...catch block and in catch added super.addActionError(message); to keep the expected behaviour.

        Please forget about my ideas with dedicated interceptor

        [1] http://svn.apache.org/viewvc/struts/struts2/trunk/xwork-core/src/main/java/com/opensymphony/xwork2/interceptor/ParametersInterceptor.java?r1=1456961&r2=1368841&diff_format=h

        Show
        Lukasz Lenart added a comment - Johno Crawford found the problem. devMode never worked in ParamatersInterceptor before Check that commit [1] (line 153-154), as you can see devMode field was static which means it was never set - you can check that starting in debug mode the example app and setting break point in line 319 (Struts 2.3.4.1). So, framework's behaviour didn't change at all, you just wasn't informed about that I have changed BaseAction and wrapped super.addActionError(getText(message)); with try...catch block and in catch added super.addActionError(message); to keep the expected behaviour. Please forget about my ideas with dedicated interceptor [1] http://svn.apache.org/viewvc/struts/struts2/trunk/xwork-core/src/main/java/com/opensymphony/xwork2/interceptor/ParametersInterceptor.java?r1=1456961&r2=1368841&diff_format=h
        Hide
        Johan Ström added a comment -

        Hi,

        I'd like to report that I have similar problems with 2.3.14.2.
        My action implements both ParameterAware and ParameterNameAware, and accepts a set of params via setters. With the WW-3973 change, this now allows setting of a lot of other parameters (if it is an "acceptable name" OR allowed by acceptableParameterName() it is let through). To avoid this and only really allow the ones I want, I tried this in the interceptor stack:

        <interceptor-ref name="params">
        <param name="acceptParamNames">ThisIsNotARealParameter_BUT_It_must_be_set_to_work</param>
        </interceptor-ref>
        

        If the value is blank the acceptParamNames are not even regarded, so I put a dummy value there..

        This however results in failure to set even basic Strings:

        public void setSort(String sort)

        Unknown macro: { .... }

        A simple call to ?simpleparam=abc&sort=name results in this:

        ERROR (com.opensymphony.xwork2.interceptor.ParametersInterceptor:38) - Developer Notification (set struts.devMode to false to disable this message):
        Unexpected Exception caught setting 'sort' on 'class com.myapp.MyActionClass: Error setting expression 'sort' with value ['name', ]
        Error setting expression 'sort' with value ['name', ] - [unknown location]
        

        I do NOT get an error for the simpleparam, which is not OK'ed by the acceptableParamNames() method so that is not expected. But for the simple 'sort' parameter, it refuses to work..

        If I remove the acceptParamNames setting, everything works as expected.

        Show
        Johan Ström added a comment - Hi, I'd like to report that I have similar problems with 2.3.14.2. My action implements both ParameterAware and ParameterNameAware, and accepts a set of params via setters. With the WW-3973 change, this now allows setting of a lot of other parameters (if it is an "acceptable name" OR allowed by acceptableParameterName() it is let through). To avoid this and only really allow the ones I want, I tried this in the interceptor stack: <interceptor-ref name= "params" > <param name= "acceptParamNames" >ThisIsNotARealParameter_BUT_It_must_be_set_to_work</param> </interceptor-ref> If the value is blank the acceptParamNames are not even regarded, so I put a dummy value there.. This however results in failure to set even basic Strings: public void setSort(String sort) Unknown macro: { .... } A simple call to ?simpleparam=abc&sort=name results in this: ERROR (com.opensymphony.xwork2.interceptor.ParametersInterceptor:38) - Developer Notification (set struts.devMode to false to disable this message): Unexpected Exception caught setting 'sort' on 'class com.myapp.MyActionClass: Error setting expression 'sort' with value ['name', ] Error setting expression 'sort' with value ['name', ] - [unknown location] I do NOT get an error for the simpleparam, which is not OK'ed by the acceptableParamNames() method so that is not expected. But for the simple 'sort' parameter, it refuses to work.. If I remove the acceptParamNames setting, everything works as expected.
        Hide
        Lukasz Lenart added a comment -

        Johan Ström in my opinion your case is a bit different, please take a look on WW-4068 and if this is not enough please register a new issue with sample Maven based app (preferable).

        The mentioned ERROR is visible just in DevMode and I think it should be lowered to WARN (as it is just a developer warn), but if you could prepare a demo app I can look into deeper.

        Show
        Lukasz Lenart added a comment - Johan Ström in my opinion your case is a bit different, please take a look on WW-4068 and if this is not enough please register a new issue with sample Maven based app (preferable). The mentioned ERROR is visible just in DevMode and I think it should be lowered to WARN (as it is just a developer warn), but if you could prepare a demo app I can look into deeper.
        Hide
        Johan Ström added a comment - - edited

        Lukasz Lenart: Yes, the original error described in this ticket is not the same. Not sure I followed the changes in the comments correctly, but I thought it could be somewhat related at least..

        To clarify; the setter, which exists, is NOT called in the above example. However, I just noticed that if I add '<param name="acceptParamNames">sort,limit,offset</param>' it works as expected, but if I just let acceptableParameterName() return true for the same values, the setter never gets called. I'm doing some more digging here, I'll get back with further information in a while.

        Show
        Johan Ström added a comment - - edited Lukasz Lenart : Yes, the original error described in this ticket is not the same. Not sure I followed the changes in the comments correctly, but I thought it could be somewhat related at least.. To clarify; the setter, which exists, is NOT called in the above example. However, I just noticed that if I add '<param name="acceptParamNames">sort,limit,offset</param>' it works as expected, but if I just let acceptableParameterName() return true for the same values, the setter never gets called. I'm doing some more digging here, I'll get back with further information in a while.
        Hide
        Lukasz Lenart added a comment -

        Johan Ström I was suspecting that ... and probably know where is the problem. Please fill an issue for that.

        Show
        Lukasz Lenart added a comment - Johan Ström I was suspecting that ... and probably know where is the problem. Please fill an issue for that.
        Hide
        Johan Ström added a comment -

        Found it.

        In ParametersInterceptor, the acceptParamNames config element is used in two places on each interception:

        • to fill up acceptableParameters. This is based on both acceptParamNames config, AND any ParameterNameAware's acceptableParameterName(Strings) method.
        • to setAcceptProperties on the MemberAccessValueStack. This ONLY uses the acceptParamNames config.

        My problem occurs if acceptParamNames config rejects a param, but acceptableParameterName(String) accepts it. The ParameterInterceptor will go try to set the value on the stack (since it was accepted by the ParameterNameAware), but the stack will reject it since setAcceptProperites is based on acceptParamNames only.

        Not sure what the proper fix here is, but at least it explains the problem I'm having.

        Show
        Johan Ström added a comment - Found it. In ParametersInterceptor, the acceptParamNames config element is used in two places on each interception: to fill up acceptableParameters. This is based on both acceptParamNames config, AND any ParameterNameAware's acceptableParameterName(Strings) method. to setAcceptProperties on the MemberAccessValueStack. This ONLY uses the acceptParamNames config. My problem occurs if acceptParamNames config rejects a param, but acceptableParameterName(String) accepts it. The ParameterInterceptor will go try to set the value on the stack (since it was accepted by the ParameterNameAware), but the stack will reject it since setAcceptProperites is based on acceptParamNames only. Not sure what the proper fix here is, but at least it explains the problem I'm having.
        Hide
        Johan Ström added a comment -

        Lukasz Lenart: Ticked separately as WW-4083

        Show
        Johan Ström added a comment - Lukasz Lenart : Ticked separately as WW-4083
        Hide
        Lukasz Lenart added a comment -

        Please register a new issue with your demo app - I will try to solve it asap

        Show
        Lukasz Lenart added a comment - Please register a new issue with your demo app - I will try to solve it asap
        Hide
        Lukasz Lenart added a comment -

        Johno Crawford do you have any more comment?

        Show
        Lukasz Lenart added a comment - Johno Crawford do you have any more comment?
        Hide
        Johno Crawford added a comment - - edited

        Sure, our apps are built on the original behaviour that global rules from struts.xml would be enforced.

        This allows us to avoid exploits such as http://struts.apache.org/release/2.3.x/docs/s2-009.html as the problem with the new behaviour means that implementing ParameterNameAware for an action will ignore rules defined in the acceptParamNames param tag.

        To return the original behaviour we are having to subclass ParametersInterceptor and copy massive chunks of code as there is no easy way to override SecurityMemberAccess (see attachment).

        Frankly speaking I would like to see a configuration option for the "new" behaviour and default to the original behaviour for increased security.

        Show
        Johno Crawford added a comment - - edited Sure, our apps are built on the original behaviour that global rules from struts.xml would be enforced. This allows us to avoid exploits such as http://struts.apache.org/release/2.3.x/docs/s2-009.html as the problem with the new behaviour means that implementing ParameterNameAware for an action will ignore rules defined in the acceptParamNames param tag. To return the original behaviour we are having to subclass ParametersInterceptor and copy massive chunks of code as there is no easy way to override SecurityMemberAccess (see attachment). Frankly speaking I would like to see a configuration option for the "new" behaviour and default to the original behaviour for increased security.
        Johno Crawford made changes -
        Attachment ParametersInterceptor.java [ 12609261 ]
        Hide
        Lukasz Lenart added a comment - - edited

        Johno Crawford I'm going to restore the original behaviour of ParametersInterceptor where parameter must be accepted by ParameterInterceptor AND ParameterNameAware - it was a mistake :\

        Show
        Lukasz Lenart added a comment - - edited Johno Crawford I'm going to restore the original behaviour of ParametersInterceptor where parameter must be accepted by ParameterInterceptor AND ParameterNameAware - it was a mistake :\
        Hide
        Lukasz Lenart added a comment -

        Chris Cranford I'm going to implement system TextProvider which will be separated from default TextProvider and will be used internally by Struts2 and will be safe - I mean no exception like that above.

        Show
        Lukasz Lenart added a comment - Chris Cranford I'm going to implement system TextProvider which will be separated from default TextProvider and will be used internally by Struts2 and will be safe - I mean no exception like that above.
        Hide
        ASF subversion and git services added a comment -

        Commit 1534156 from Lukasz Lenart in branch 'struts2/trunk'
        [ https://svn.apache.org/r1534156 ]

        WW-4066 Solves problem with StringIndexOutOfBoundsException when message was already translated

        Show
        ASF subversion and git services added a comment - Commit 1534156 from Lukasz Lenart in branch 'struts2/trunk' [ https://svn.apache.org/r1534156 ] WW-4066 Solves problem with StringIndexOutOfBoundsException when message was already translated
        Hide
        Lukasz Lenart added a comment -

        Solved, please check with the latest build

        Show
        Lukasz Lenart added a comment - Solved, please check with the latest build
        Lukasz Lenart made changes -
        Status Reopened [ 4 ] Resolved [ 5 ]
        Resolution Fixed [ 1 ]
        Hide
        Johno Crawford added a comment -

        Are you able to give an ETA on 2.3.16 vote?

        Show
        Johno Crawford added a comment - Are you able to give an ETA on 2.3.16 vote?
        Hide
        Hudson added a comment -

        FAILURE: Integrated in Struts2-JDK6 #817 (See https://builds.apache.org/job/Struts2-JDK6/817/)
        WW-4066 Solves problem with StringIndexOutOfBoundsException when message was already translated (lukaszlenart: rev 1534156)

        • /struts/struts2/trunk/xwork-core/src/main/java/com/opensymphony/xwork2/interceptor/ParametersInterceptor.java
        Show
        Hudson added a comment - FAILURE: Integrated in Struts2-JDK6 #817 (See https://builds.apache.org/job/Struts2-JDK6/817/ ) WW-4066 Solves problem with StringIndexOutOfBoundsException when message was already translated (lukaszlenart: rev 1534156) /struts/struts2/trunk/xwork-core/src/main/java/com/opensymphony/xwork2/interceptor/ParametersInterceptor.java
        Hide
        Lukasz Lenart added a comment -

        Around 8 issues left, I think within 2-4 weeks the Vote should be started

        Show
        Lukasz Lenart added a comment - Around 8 issues left, I think within 2-4 weeks the Vote should be started
        Lukasz Lenart made changes -
        Status Resolved [ 5 ] Closed [ 6 ]
        Transition Time In Source Status Execution Times Last Executer Last Execution Date
        Open Open Resolved Resolved
        5d 4h 38m 1 Lukasz Lenart 19/May/13 19:05
        Resolved Resolved Reopened Reopened
        14m 47s 1 Lukasz Lenart 19/May/13 19:20
        Reopened Reopened Resolved Resolved
        154d 19h 30m 1 Lukasz Lenart 21/Oct/13 14:51
        Resolved Resolved Closed Closed
        48d 3h 30m 1 Lukasz Lenart 08/Dec/13 17:21

          People

          • Assignee:
            Lukasz Lenart
            Reporter:
            Chris Cranford
          • Votes:
            0 Vote for this issue
            Watchers:
            7 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development