[JENKINS-72028] deprecated URL instantiation applied

[abhishekmaity]

See JENKINS-72028.

Testing done

Proposed changelog entries

• Entry 1: Issue, human-readable text

Proposed upgrade guidelines

N/A

Submitter checklist

Beta Give feedback

1. The Jira issue, if it exists, is well-described.
2. The changelog entries and upgrade guidelines are appropriate for the audience affected by the change (users or developers, depending on the change) and are in the imperative mood (see examples). Fill in the Proposed upgrade guidelines section only if there are breaking changes or changes that may require extra steps from users during upgrade.
3. There is automated testing or an explanation as to why this change has no tests.
4. New public classes, fields, and methods are annotated with @Restricted or have @since TODO Javadocs, as appropriate.
5. New deprecations are annotated with @Deprecated(since = "TODO") or @Deprecated(forRemoval = true, since = "TODO"), if applicable.
6. New or substantially changed JavaScript is not defined inline and does not call eval to ease future introduction of Content Security Policy (CSP) directives (see documentation).
7. For dependency updates, there are links to external changelogs and, if possible, full differentials.
8. For new APIs and extension points, there is a link to at least one consumer.

Desired reviewers

@mention

Before the changes are marked as ready-for-merge:

Maintainer checklist

Beta Give feedback

1. There are at least two (2) approvals for the pull request and no outstanding requests for change.
2. Conversations in the pull request are over, or it is explicit that a reviewer is not blocking the change.
3. Changelog entries in the pull request title and/or Proposed changelog entries are accurate, human-readable, and in the imperative mood.
4. Proper changelog labels are set so that the changelog can be generated automatically.
5. If the change needs additional upgrade steps from users, the upgrade-guide-needed label is set and there is a Proposed upgrade guidelines section in the pull request title (see example).
6. If it would make sense to backport the change to LTS, a Jira issue must exist, be a Bug or Improvement, and be labeled as lts-candidate to be considered (see query).

[abhishekmaity]

Request @NotMyFault @Vlatombe @MarkEWaite @jglick for review and close the PR

[timja]

looks ok, one question though

[NotMyFault]

Looks good so far, thanks for taking on the issue :)

needs another look

[abhishekmaity]

/label needs-more-reviews

[abhishekmaity]

/label java

[abhishekmaity]

@jtnord any feedback on the above changes made.

[abhishekmaity]

@jtnord I have made changes based on the review. Re-request to have a look and ready PR for merge.

[jtnord]

There are many similar issues throughout the PR, I have been commenting on occurrences of the types of issue but not all the issues but this has seemingly caused confusion thinking they where only the issues.

So.

---

unless you are explicitly redacting/hiding information (which is not the case in this PR)

Do not write code like:

try {
  // something
catch (SomeException e) {
  throw new SomeException(....)
}

as it makes the code worse - especially when the code inside the try block is long and the exception could have been thrown on any one of a number of lines.

rather just keep the code as

// something

---

non internal code (ie anything protected or public) can not have its method signature changed. This includes adding a new Exception type to the declared throws that is not a subclass of an existing declared one.

e.g. if the method was

public void doSomething(String url) throws MalformedURLException {
 // something
 }

then you can not change it to

public void doSomething(String url) throws MalformedURLException, URISyntaxException {
 // something
 }

in this case and so that callers can react the same way to an "invalid URL" you should probably change the code so that it looks like

public void doSomething(String url) throws MalformedURLException {
  try {
    // something
  } catch (URISyntaxException e) {
    MalformedURLException mex = new MalformedURLException(e.getMessage());
    mex.initCause(e);
    throw mex;
  }
 }

---

Internal code can have its method changed without breaking plugins.

However if you change the signature to throw a URISyntaxException you need to ensure that every caller that calls the method handles a URISyntaxException in exactly the same way as if it had caught a MalformedURLException (if it has special handling for this);

As MalformedURLException is an IOException this may not be obvious.

You could handle the URISyntaxException differently, but this would need a case by case analysis and would generally only be applicable if the code was catching an IOException, but even in this case the code needs to take appropriate action.

[jtnord]

dismissing previous approvaals as the code has moved on so much. (not asking for a review at this stage as it is not ready)

[abhishekmaity]

@jtnord @NotMyFault @timja I am closing this PR and will open a new PR a fresh based on the observation. Many thanks @jtnord for taking out time and highlighting the issues. Instead of a single PR, I am thinking to do the task via smaller PRs.