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

StreamTaskListener#getCharset() may violate API and return null

    XMLWordPrintable

    Details

    • Similar Issues:

      Description

      Current state:

      • API users may get NPE if they handle TaskListener according to the requirements

      Nonnull annotation has been introduced by Jesse Glick in https://github.com/jenkinsci/jenkins/commit/9ed6b01317424f3f441022b3a2f23fbbc5ae1543

      I see two options:

      • Relax requirements in TaskListener#getCharset() and allow null returns. It is a "breaking change" from API PoV (now we require API users to check for nulls)
      • Rework StreamTaskListener to disallow nulls and to default to something (UTF-8). May break some use-cases as well since now it defaults to the system's default charset

        Attachments

          Activity

          Hide
          oleg_nenashev Oleg Nenashev added a comment -

          Likely NVM, because TaskListener#getCharset() is restricted and allows overrides only, so we can safely change the annotation to CheckForNull. All API calls are under our control

          Show
          oleg_nenashev Oleg Nenashev added a comment - Likely NVM, because TaskListener#getCharset() is restricted and allows overrides only, so we can safely change the annotation to CheckForNull. All API calls are under our control
          Hide
          jglick Jesse Glick added a comment -

          A null value for the charset field is fine—just means platform default. We need merely fix the getCharset() override to fall back to Charset.defaultCharset(). Alternately, make getCharset() to be @CheckForNull. Unlikely to break anything since it is anyway only used by TaskListener._error, which already checks for null. The former option is a little safer.

          Show
          jglick Jesse Glick added a comment - A null value for the charset field is fine—just means platform default. We need merely fix the getCharset() override to fall back to Charset.defaultCharset() . Alternately, make getCharset() to be @CheckForNull . Unlikely to break anything since it is anyway only used by TaskListener._error , which already checks for null. The former option is a little safer.
          Hide
          oleg_nenashev Oleg Nenashev added a comment -

          Agreed

          Show
          oleg_nenashev Oleg Nenashev added a comment - Agreed
          Hide
          oleg_nenashev Oleg Nenashev added a comment -

          Released in 2.128. Will mark as LTS candidate though there is no major risk

          Show
          oleg_nenashev Oleg Nenashev added a comment - Released in 2.128. Will mark as LTS candidate though there is no major risk

            People

            • Assignee:
              Unassigned
              Reporter:
              oleg_nenashev Oleg Nenashev
            • Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: