Uploaded image for project: 'OFBiz'
  1. OFBiz
  2. OFBIZ-6219

NPE while rendering content uisng screen widget

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: Release Branch 14.12, Trunk
    • Fix Version/s: 14.12.01, 16.11.01
    • Component/s: content, framework
    • Labels:
      None

      Description

      If we want to render an content that does't have dataResourceId then screen widget throwing NPE

      Caused by: java.lang.NullPointerException
      	at org.ofbiz.widget.model.ModelScreenWidget$Content.renderWidgetString(ModelScreenWidget.java:1399) ~[ofbiz-widget.jar:?]
      	at org.ofbiz.widget.model.ModelScreenWidget.renderSubWidgetsString(ModelScreenWidget.java:98) ~[ofbiz-widget.jar:?]
      	at org.ofbiz.widget.model.ModelScreenWidget$Section.renderWidgetString(ModelScreenWidget.java:280) ~[ofbiz-widget.jar:?]
      	at org.ofbiz.widget.model.ModelScreen.renderScreenString(ModelScreen.java:164) ~[ofbiz-widget.jar:?]
      	at org.ofbiz.widget.renderer.ScreenRenderer.render(ScreenRenderer.java:136) ~[ofbiz-widget.jar:?]
      	at org.ofbiz.widget.renderer.ScreenRenderer.render(ScreenRenderer.java:98) ~[ofbiz-widget.jar:?]
      
      1. OFBIZ-6219.patch
        0.8 kB
        Deepak Dixit
      2. OFBIZ-6219.patch
        0.8 kB
        Adrian Crum
      3. OFBIZ-6219.patch
        0.7 kB
        Deepak Dixit

        Activity

        Hide
        deepak.dixit Deepak Dixit added a comment -

        Here is the patch for this issue, NPE occurred due to isEmpty() call on null object.

        Show
        deepak.dixit Deepak Dixit added a comment - Here is the patch for this issue, NPE occurred due to isEmpty() call on null object.
        Hide
        adrianc@hlmksw.com Adrian Crum added a comment -

        It would be better to ensure the reference is not null - since the isEmpty() call is used throughout the method.

        Show
        adrianc@hlmksw.com Adrian Crum added a comment - It would be better to ensure the reference is not null - since the isEmpty() call is used throughout the method.
        Hide
        deepak.dixit Deepak Dixit added a comment - - edited

        But in this case if dataResourceId is null then it will throw error (as per attached patch)

        "Could not find content with contentId [" + expandedContentId + "] ";
        

        We need to add dataResourceId null check inside the if condition.

        Show
        deepak.dixit Deepak Dixit added a comment - - edited But in this case if dataResourceId is null then it will throw error (as per attached patch) "Could not find content with contentId [" + expandedContentId + "] " ; We need to add dataResourceId null check inside the if condition.
        Hide
        adrianc@hlmksw.com Adrian Crum added a comment -

        Your updated patch duplicates mine.

        Show
        adrianc@hlmksw.com Adrian Crum added a comment - Your updated patch duplicates mine.
        Hide
        deepak.dixit Deepak Dixit added a comment -

        I think its different:
        Your Patch

        -                    if (content != null) {
        +                    if (content != null && content.get("dataResourceId") != null) {
                                 expandedDataResourceId = content.getString("dataResourceId");
                             } else {
                                 String errMsg = "Could not find content with contentId [" + expandedContentId + "] ";
        

        Mine Patch

                             }
                             if (content != null) {
        -                        expandedDataResourceId = content.getString("dataResourceId");
        +                        if (content.get("dataResourceId") != null) expandedDataResourceId = content.getString("dataResourceId");
                             } else {
                                 String errMsg = "Could not find content with contentId [" + expandedContentId + "] ";
                                 Debug.logError(errMsg, module);
        
        Show
        deepak.dixit Deepak Dixit added a comment - I think its different: Your Patch - if (content != null ) { + if (content != null && content.get( "dataResourceId" ) != null ) { expandedDataResourceId = content.getString( "dataResourceId" ); } else { String errMsg = "Could not find content with contentId [" + expandedContentId + "] " ; Mine Patch } if (content != null ) { - expandedDataResourceId = content.getString( "dataResourceId" ); + if (content.get( "dataResourceId" ) != null ) expandedDataResourceId = content.getString( "dataResourceId" ); } else { String errMsg = "Could not find content with contentId [" + expandedContentId + "] " ; Debug.logError(errMsg, module);
        Hide
        adrianc@hlmksw.com Adrian Crum added a comment -

        They both do the same thing. Mine uses a single if block and yours uses two (the second if block can be included in the first).

        Show
        adrianc@hlmksw.com Adrian Crum added a comment - They both do the same thing. Mine uses a single if block and yours uses two (the second if block can be included in the first).
        Hide
        deepak.dixit Deepak Dixit added a comment - - edited

        If we add condition in single if block then if content exists but dataResouceId is null then condition fails and it will return error with message "Could not find content with contentId [" + expandedContentId + "] ";

        Here is the original code

                            if (content != null) {
                                expandedDataResourceId = content.getString("dataResourceId");
                            } else {
                                String errMsg = "Could not find content with contentId [" + expandedContentId + "] ";
                                Debug.logError(errMsg, module);
                                throw new RuntimeException(errMsg);
                            }
        
        Show
        deepak.dixit Deepak Dixit added a comment - - edited If we add condition in single if block then if content exists but dataResouceId is null then condition fails and it will return error with message "Could not find content with contentId [" + expandedContentId + "] "; Here is the original code if (content != null ) { expandedDataResourceId = content.getString( "dataResourceId" ); } else { String errMsg = "Could not find content with contentId [" + expandedContentId + "] " ; Debug.logError(errMsg, module); throw new RuntimeException(errMsg); }
        Hide
        adrianc@hlmksw.com Adrian Crum added a comment -

        This is beginner Java, and I can't believe we are having this discussion. I don't understand the point you are trying to make.

        My patch fixes the problem. Your patch does exactly the same thing but with more code. Just commit the patch and close the issue. This discussion is a waste of time.

        Show
        adrianc@hlmksw.com Adrian Crum added a comment - This is beginner Java, and I can't believe we are having this discussion. I don't understand the point you are trying to make. My patch fixes the problem. Your patch does exactly the same thing but with more code. Just commit the patch and close the issue. This discussion is a waste of time.
        Hide
        adrianc@hlmksw.com Adrian Crum added a comment -

        Okay, I see your concern is with the content of the error message. Okay - use your patch. I apologize for the confusion.

        Show
        adrianc@hlmksw.com Adrian Crum added a comment - Okay, I see your concern is with the content of the error message. Okay - use your patch. I apologize for the confusion.
        Hide
        deepak.dixit Deepak Dixit added a comment -

        Thanks Adrian

        Show
        deepak.dixit Deepak Dixit added a comment - Thanks Adrian
        Hide
        deepak.dixit Deepak Dixit added a comment -

        This has been committed in
        Trunk at r#1669588
        R14.12 at r#1669589

        Finally I am able to convince Adrian (at least) on this ticket

        Show
        deepak.dixit Deepak Dixit added a comment - This has been committed in Trunk at r#1669588 R14.12 at r#1669589 Finally I am able to convince Adrian (at least) on this ticket

          People

          • Assignee:
            deepak.dixit Deepak Dixit
            Reporter:
            deepak.dixit Deepak Dixit
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development