Uploaded image for project: 'Jenkins'
  1. Jenkins
  2. JENKINS-56792

Change constructor in Issue (TreeString)

    Details

    • Similar Issues:
    • Released As:
      5.0.0 (analysis-model and warnings-ng)

      Description

      Issue has a constructor with String values for fileName, packageName, message, description. Another constructor is required (same number of parameters) that uses a TreeString for these arguments.

      In a second step, IssueBuilder should be refined to store the members fileName, packageName, message, description in a TreeString as well.

      Issue has a constructor with String values for fileName, packageName, message, description. The constructor has to be changed to required (same number of parameters) that uses a TreeString for these arguments.

      All normalization and transformation logic that is currently part of the constructor of Issue should be moved to IssueBuilder#build. The Issue constructor then needs to be changed as described above.

        Attachments

          Activity

          Hide
          nengelbrecht Nils Engelbrecht added a comment - - edited

          Do the values of those parameters (now being TreeStrings) have to be 'normalized' within the Issue class as well?
          If so, I would either have to implement the normalizeFeature within the TreeString.class or have to retrieve the String of e.g the fileName, normalize it and then create a new TreeString via TreeStringBuilder.

          Or should the normalization happen before being passed to the Issue-Constructor ( within the IssueBuilder)?

          Example for 'normalization':
          Issue.java(l. 305): this.fileName = builder.intern(normalizeFileName(fileName));

           

          Show
          nengelbrecht Nils Engelbrecht added a comment - - edited Do the values of those parameters (now being TreeStrings) have to be 'normalized' within the Issue class as well? If so, I would either have to implement the normalizeFeature within the TreeString.class or have to retrieve the String of e.g the fileName, normalize it and then create a new TreeString via TreeStringBuilder. Or should the normalization happen before being passed to the Issue-Constructor ( within the IssueBuilder)? Example for 'normalization': Issue.java(l. 305): this.fileName = builder.intern(normalizeFileName(fileName));  
          Hide
          drulli Ulli Hafner added a comment -

          Good catch! Actually the normalization needs to be done outside of Issue. Otherwise the caching effect of TreeString would not pay off. A TreeString actually is a kind of String cache that tries to minimize memory consumption. In order to build up that cache the TreeStringBuilder instance must be created once and reused for all issues (i.e. it is a kind of static field). This is currently broken as the builder is created for each instance.

          My idea about this issue was to provide the first part of the required refactoring. But after thinking again I think it makes more sense to rework that part completely. If it is not too complex for you we can rephrase this issue in the following way:
          all normalization and transformation logic that is currently part of the constructor of Issue should be moved to IssueBuilder#build. The Issue constructor then needs to be changed as described above. That means no new constructor.

          Then an IssueBuilder holds a TreeStringBuilder for each of the 4 fields and performs the whole transformation logic (and the rest of the String normalization). So the Issue constructor basically does nothing, just assigns the fields. Note that null should be not allowed anymore for these parameters, this should be ensured by the IssueBuilder.

          Does this sound reasonable? If the work around that gets to complex we can also drop the second issue... Changing IssueBuilder and Issue in such a way will break several test cases, but I think it should be easy to spot what to do in those tests.

          Show
          drulli Ulli Hafner added a comment - Good catch! Actually the normalization needs to be done outside of Issue . Otherwise the caching effect of TreeString would not pay off. A TreeString actually is a kind of String cache that tries to minimize memory consumption. In order to build up that cache the TreeStringBuilder instance must be created once and reused for all issues (i.e. it is a kind of static field). This is currently broken as the builder is created for each instance. My idea about this issue was to provide the first part of the required refactoring. But after thinking again I think it makes more sense to rework that part completely. If it is not too complex for you we can rephrase this issue in the following way: all normalization and transformation logic that is currently part of the constructor of Issue should be moved to IssueBuilder#build . The Issue constructor then needs to be changed as described above. That means no new constructor. Then an IssueBuilder holds a TreeStringBuilder for each of the 4 fields and performs the whole transformation logic (and the rest of the String normalization). So the Issue constructor basically does nothing, just assigns the fields. Note that null should be not allowed anymore for these parameters, this should be ensured by the IssueBuilder . Does this sound reasonable? If the work around that gets to complex we can also drop the second issue... Changing IssueBuilder and Issue in such a way will break several test cases, but I think it should be easy to spot what to do in those tests.
          Hide
          nengelbrecht Nils Engelbrecht added a comment -

          I understand. Sounds good to me!
          After reworking this for the fileName parameter about 8 test cases are failing.

          Am I allowed to delete test cases such as IssueTest#testFileNameBackslashConversion (therefore of course I would create a new one in the IssueBuilderTest class) ?

          Another question I got is how to cope with failure of the test case IssueTest#shouldReadIssueFromOldSerialization ?
          I assume this is due to the type-change of fileName from String to TreeString.

          Show
          nengelbrecht Nils Engelbrecht added a comment - I understand. Sounds good to me! After reworking this for the fileName parameter about 8 test cases are failing. Am I allowed to delete test cases such as IssueTest#testFileNameBackslashConversion (therefore of course I would create a new one in the IssueBuilderTest class) ? Another question I got is how to cope with failure of the test case IssueTest#shouldReadIssueFromOldSerialization ? I assume this is due to the type-change of fileName from String to TreeString.
          Hide
          drulli Ulli Hafner added a comment -

          Am I allowed to delete test cases such as IssueTest#testFileNameBackslashConversion (therefore of course I would create a new one in the IssueBuilderTest class) ?

          Yes, just move the tests to the IssueBuilder class.

          Another question I got is how to cope with failure of the test case IssueTest#shouldReadIssueFromOldSerialization ?
          I assume this is due to the type-change of fileName from String to TreeString.

          Hmm, does the serialization change? The type changed only in the constructor, but the field still is TreeString. Or do I miss something?

          Show
          drulli Ulli Hafner added a comment - Am I allowed to delete test cases such as IssueTest#testFileNameBackslashConversion (therefore of course I would create a new one in the IssueBuilderTest class) ? Yes, just move the tests to the IssueBuilder class. Another question I got is how to cope with failure of the test case IssueTest#shouldReadIssueFromOldSerialization ? I assume this is due to the type-change of fileName from String to TreeString. Hmm, does the serialization change? The type changed only in the constructor, but the field still is TreeString . Or do I miss something?
          Hide
          nengelbrecht Nils Engelbrecht added a comment - - edited

          Ok thank you - I’ll do that.

          My bad! You’re right: the fileName was already of type TreeString before.
          I guess the test fails because I changed the type of the parameter of the setter for fileName and removed the logic of normalization.
          Might that be the reason?

          I actually don't know why, but this specific serialization test mentioned above now passes.
          But now ReportTest#shouldReadIssueFromOldSerialization fails...

          Show
          nengelbrecht Nils Engelbrecht added a comment - - edited Ok thank you - I’ll do that. My bad! You’re right: the fileName was already of type TreeString before. I guess the test fails because I changed the type of the parameter of the setter for fileName and removed the logic of normalization. Might that be the reason? I actually don't know why, but this specific serialization test mentioned above now passes. But now ReportTest#shouldReadIssueFromOldSerialization fails...

            People

            • Assignee:
              nengelbrecht Nils Engelbrecht
              Reporter:
              drulli Ulli Hafner
            • Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: