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

Impossible to configure default display URL provider but keep redirection (regression in 2.3.4)

    • display-url-api 2.3.9

      Context

      The GitHub Checks plugin calls DisplayURLProvider.get() and then uses DisplayURLProvider.getRunURL and DisplayURLProvider.getJobURL to get URLs that it publishes to GitHub. These URLs normally end with "display/redirect". When a user clicks such a link on GitHub, the Display URL API plugin redirects to Blue Ocean UI or to the classic UI. If Blue Ocean has been installed, then that is the default, but logged-in users can configure their user account and choose a different option under "Notification URL".

      In https://github.com/jenkins-infra/helpdesk/issues/2833, they wanted to change the default so that users would be redirected to the classic UI by default, but logged-in users would still be able to choose Blue Ocean. To implement this, https://github.com/jenkins-infra/jenkins-infra/pull/2143 added -Djenkins.displayurl.provider=org.jenkinsci.plugins.displayurlapi.ClassicDisplayURLProvider to the Java startup options.

      Problem

      After the jenkins.displayurl.provider property had been set, the URLs that were published to GitHub no longer included the "display/redirect" suffix. Instead, the hyperlinks pointed directly to the classic UI. Users who clicked these links did not get the Blue Ocean UI, even if they had logged in to Jenkins and configured "Notification URL".

      DisplayURLProvider.DisplayURLProviderImpl (source) overrides methods to append DISPLAY_POSTFIX = "display/redirect", and classes like RunDisplayAction (source) then call AbstractDisplayAction.lookupProvider(StaplerRequest) (source) to choose the provider that selects the redirect URL. But if the jenkins.displayurl.provider property points to org.jenkinsci.plugins.displayurlapi.ClassicDisplayURLProvider (source), then DisplayURLProvider.DisplayURLProviderImpl is not used and does not get a chance to add DISPLAY_POSTFIX.

      Requested changes

      Include "display/redirect" in URLs even if the default display URL provider has been configured with the system property or with the environment variable.

      When a request comes to the URL with "display/redirect", and the user has not configured "Notification URL", use the default from the system property or the environment variable. This part may already work that way.

      Could also define a separate system property, to keep the current behavior available. I don't know whether anyone really wants it, though.

          [JENKINS-69006] Impossible to configure default display URL provider but keep redirection (regression in 2.3.4)

          Huh, DisplayURLProvider.get() actually used to return DisplayURLProviderImpl.INSTANCE always, but that was changed in https://github.com/jenkinsci/display-url-api-plugin/pull/42 to fix JENKINS-64119. That makes this a regression… I'll switch the type of this issue from Improvement to Bug.

          Kalle Niemitalo added a comment - Huh, DisplayURLProvider.get() actually used to return DisplayURLProviderImpl.INSTANCE always, but that was changed in  https://github.com/jenkinsci/display-url-api-plugin/pull/42 to fix JENKINS-64119 . That makes this a regression… I'll switch the type of this issue from Improvement to Bug.

          A simple revert of https://github.com/jenkinsci/display-url-api-plugin/pull/42 merge commit d25900536fb87e239c658f2576866d08ce85436d results in conflicts and would not be OK anyway:

          • pom.xml:
            • The PR removed <tag>display-url-api-2.3.3</tag>. Do not revert.
          • src/main/java/org/jenkinsci/plugins/displayurlapi/DisplayURLProvider.java:
            • DisplayURLProvider.get(): The PR made this call getPreferredProvider(). Revert.
            • DisplayURLProvider.getRoot(): The PR made this call Jenkins.get() rather than the deprecated Jenkins.getInstance(). Do not revert.
            • DisplayURLProvider.getPreferredProvider(): The PR made this check also the per-user setting rather than only the per-controller setting. Not sure whether to revert. This is called from DisplayURLProvider.lookupProvider() (which checks the per-user setting on its own), from DisplayURLProvider.get() (in which the call should be reverted), and from DisplayURLProvider.getDefault() (which is called from AbstractDisplayAction.lookupProvider(), which checks the per-user setting on its own, and from blueocean-display-url-plugin). I think it would be nicer to revert this, just to reduce the redundant checking of per-user settings. GitHub code search does not show this being called by other plugins.
            • DisplayURLProvider.getUserPreferredProviderProperty(): The PR added this method. Do not revert, because other plugins may now be using it (although I don't see why any would).
          • src/main/java/org/jenkinsci/plugins/displayurlapi/actions/AbstractDisplayAction.java:
            • AbstractDisplayAction.getUserPreferredProviderProperty(): The PR deprecated this method and made it call DisplayURLProvider.getUserPreferredProviderProperty(). If the latter method is not removed or deprecated, then this change should not be reverted either.
          • src/test/java/org/jenkinsci/plugins/displayurlapi/DisplayURLProviderTest.java:
            • DisplayURLProviderTest.TestSysPropDisplayURLProvider: The PR added this class. Do not revert, because it will be needed in the rewritten urlsWithSysPropProvider().
            • DisplayURLProviderTest.urlsWithSysPropProvider(): The PR added this test method. Rewrite to assert that JENKINS-69006 is fixed.
            • DisplayURLProviderTest.TestUserDisplayURLProvider: The PR added this class. Do not revert, because it will be needed in the rewritten urlsWithUserDefinedProvider().
            • DisplayURLProviderTest.urlsWithUserDefinedProvider(): The PR added this test method. Rewrite to assert that JENKINS-69006 is fixed.
            • DisplayURLProviderTest.decoration(): The PR made this use try-with-resources syntax. Do not revert.

          Kalle Niemitalo added a comment - A simple revert of https://github.com/jenkinsci/display-url-api-plugin/pull/42 merge commit d25900536fb87e239c658f2576866d08ce85436d results in conflicts and would not be OK anyway: pom.xml: The PR removed <tag>display-url-api-2.3.3</tag> . Do not revert. src/main/java/org/jenkinsci/plugins/displayurlapi/DisplayURLProvider.java: DisplayURLProvider.get(): The PR made this call getPreferredProvider(). Revert. DisplayURLProvider.getRoot(): The PR made this call Jenkins.get() rather than the deprecated Jenkins.getInstance() . Do not revert. DisplayURLProvider.getPreferredProvider(): The PR made this check also the per-user setting rather than only the per-controller setting. Not sure whether to revert. This is called from DisplayURLProvider.lookupProvider() (which checks the per-user setting on its own), from DisplayURLProvider.get() (in which the call should be reverted), and from DisplayURLProvider.getDefault() (which is called from AbstractDisplayAction.lookupProvider(), which checks the per-user setting on its own, and from blueocean-display-url-plugin). I think it would be nicer to revert this, just to reduce the redundant checking of per-user settings. GitHub code search does not show this being called by other plugins. DisplayURLProvider.getUserPreferredProviderProperty(): The PR added this method. Do not revert , because other plugins may now be using it (although I don't see why any would). src/main/java/org/jenkinsci/plugins/displayurlapi/actions/AbstractDisplayAction.java: AbstractDisplayAction.getUserPreferredProviderProperty(): The PR deprecated this method and made it call DisplayURLProvider.getUserPreferredProviderProperty(). If the latter method is not removed or deprecated, then this change should not be reverted either. src/test/java/org/jenkinsci/plugins/displayurlapi/DisplayURLProviderTest.java: DisplayURLProviderTest.TestSysPropDisplayURLProvider: The PR added this class. Do not revert , because it will be needed in the rewritten urlsWithSysPropProvider(). DisplayURLProviderTest.urlsWithSysPropProvider(): The PR added this test method. Rewrite to assert that JENKINS-69006 is fixed. DisplayURLProviderTest.TestUserDisplayURLProvider: The PR added this class. Do not revert , because it will be needed in the rewritten urlsWithUserDefinedProvider(). DisplayURLProviderTest.urlsWithUserDefinedProvider(): The PR added this test method. Rewrite to assert that JENKINS-69006 is fixed. DisplayURLProviderTest.decoration(): The PR made this use try-with-resources syntax. Do not revert.

          I suspect https://github.com/jenkinsci/display-url-api-plugin/pull/42 can even cause infinite recursion.

          In BlueOceanDisplayURLImpl.java, public String getRunURL(Run<?, ?> run) calls DisplayURLProvider.getDefault().getRunURL(run) if it does not recognize the type of the Run. In DisplayURLProvider.java, public static DisplayURLProvider getDefault() can now return the BlueOceanDisplayURLImpl instance again, causing BlueOceanDisplayURLImpl.getRunURL to call itself. Likewise in other methods of BlueOceanDisplayURLImpl.

          Kalle Niemitalo added a comment - I suspect https://github.com/jenkinsci/display-url-api-plugin/pull/42 can even cause infinite recursion. In BlueOceanDisplayURLImpl.java , public String getRunURL(Run<?, ?> run) calls DisplayURLProvider.getDefault().getRunURL(run) if it does not recognize the type of the Run. In DisplayURLProvider.java , public static DisplayURLProvider getDefault() can now return the BlueOceanDisplayURLImpl instance again, causing BlueOceanDisplayURLImpl.getRunURL to call itself. Likewise in other methods of BlueOceanDisplayURLImpl.

          Basil Crow added a comment -

          kon You all but implemented the change in English rather than in a commit, so what is stopping you from opening a pull request with the fix?

          Basil Crow added a comment - kon You all but implemented the change in English rather than in a commit, so what is stopping you from opening a pull request with the fix?

          I'm not interested in taking this forward.

          Kalle Niemitalo added a comment - I'm not interested in taking this forward.

          Basil Crow added a comment -

          Yet you were interested enough to write JENKINS-69006 (comment).

          Basil Crow added a comment - Yet you were interested enough to write JENKINS-69006 (comment) .

          I was having my summer vacation then.

          Kalle Niemitalo added a comment - I was having my summer vacation then.

          Basil Crow added a comment -

          When you find the time and interest to submit a pull request, we will be ready for your contribution.

          Basil Crow added a comment - When you find the time and interest to submit a pull request, we will be ready for your contribution.

          Devin Nusbaum added a comment -

          For what it's worth I ran into this independently while adding a GUI configuration to control the default provider instead of having to set environment variables/system properties, see various comments in https://github.com/jenkinsci/display-url-api-plugin/pull/202. I agree with the overall analysis by kon and will try to fix things.

          Devin Nusbaum added a comment - For what it's worth I ran into this independently while adding a GUI configuration to control the default provider instead of having to set environment variables/system properties, see various comments in https://github.com/jenkinsci/display-url-api-plugin/pull/202 . I agree with the overall analysis by kon and will try to fix things.

          Devin Nusbaum added a comment -

          Devin Nusbaum added a comment - I tried to untangle the mess in https://github.com/jenkinsci/display-url-api-plugin/pull/202 .

            dnusbaum Devin Nusbaum
            kon Kalle Niemitalo
            Votes:
            0 Vote for this issue
            Watchers:
            6 Start watching this issue

              Created:
              Updated:
              Resolved: