getEndpointAuthorization does not throw when authorization was not set and is not optional

[SimonAlling]

Environment

azure-pipelines-task-lib version: 3.1.4

Issue Description

Excerpt from the JSDoc comment about getEndpointAuthorization in node/task.ts:

If the authorization was not set and is not optional, it will throw.

I believe this is incorrect.

Expected behaviour

The function throws if the authorization was not set and is not optional.

Actual behaviour

The function does not throw.

Steps to reproduce

import * as tl from "azure-pipelines-task-lib/task"

try {
  console.log("VALUE:", tl.getEndpointAuthorization("NONEXISTENT", false))
} catch (err) {
  console.warn("CAUGHT:")
  console.warn(err)
}

Output when run on my local machine:

##vso[task.debug]agent.TempDirectory=undefined
##vso[task.debug]agent.workFolder=undefined
##vso[task.debug]loading inputs and endpoints
##vso[task.debug]loaded 0
##vso[task.debug]Agent.ProxyUrl=undefined
##vso[task.debug]Agent.CAInfo=undefined
##vso[task.debug]Agent.ClientCert=undefined
##vso[task.debug]Agent.SkipCertValidation=undefined
##vso[task.debug]task result: Failed
##vso[task.issue type=error;]Endpoint auth data not present: NONEXISTENT
##vso[task.complete result=Failed;]Endpoint auth data not present: NONEXISTENT
##vso[task.debug]NONEXISTENT exists true
VALUE: undefined

Note that it also says NONEXISTENT exists true. In fact, it always does that, because the expression being logged is

id + ' exists ' + (aval !== null)

and aval has type string | undefined, so its value will probably never be null.

[SimonAlling]

Any updates on this?

It may be worth mentioning that corresponding claims (not set and not optional ⇒ throws) are made about getEndpointUrl, getEndpointDataParameter, getEndpointAuthorizationScheme and getEndpointAuthorizationParameter, and that these functions do indeed throw as expected.

[AnnaOpareva]

Hello, @SimonAlling.
Thank you very much for your comments. I would like to clarify is it required for you this function to throw an exception if authorization was not set? If so we will add an additional option to it that will allow throwing exceptions instead of just returning an undefined value. If the main concern here is that comment doesn't correspond to the actual behavior we will just update it to correspond to the actual behavior.
The main concern in changing the behavior of this function on our end is that it is quite widely used by in-the-box tasks and common packages and currently they rely on that this function does not throw an exception. So it will be a breaking change if we just make this function to throw an exception if authorization was not set.

[SimonAlling]

Hello, @SimonAlling.
Thank you very much for your comments. I would like to clarify is it required for you this function to throw an exception if authorization was not set?

Not really, I just expected the function to throw because the comment said it would. I've since moved to getEndpointAuthorizationParameter instead, which does throw as stated.

If so we will add an additional option to it that will allow throwing exceptions instead of just returning an undefined value. If the main concern here is that comment doesn't correspond to the actual behavior we will just update it to correspond to the actual behavior.

I don't think such an option should be added. Updating the comment might be enough; however, I'm quite surprised that getEndpointAuthorization behaves differently from getEndpointUrl, getEndpointDataParameter, getEndpointAuthorizationScheme and getEndpointAuthorizationParameter in this respect.

The main concern in changing the behavior of this function on our end is that it is quite widely used by in-the-box tasks and common packages and currently they rely on that this function does not throw an exception.

I can neither confirm nor dispute that because I'm not very familiar with the in-the-box tasks, but I can easily imagine that being the case.

So it will be a breaking change if we just make this function to throw an exception if authorization was not set.

Indeed it would!

I might add that TypeScript's powerful abilities aren't being leveraged in the library as of today: undefined is always included in the return type of the functions mentioned above, even when undefined can't actually be returned, forcing library users to write unnecessary "plumbing code":

const url = tl.getEndpointUrl(serviceEndpointID, false)
const username = tl.getEndpointAuthorizationParameter(serviceEndpointID, "username", false)
const password = tl.getEndpointAuthorizationParameter(serviceEndpointID, "password", false)
// `url`, `username` and `password` all have type `string | undefined`, even though an error would already have been thrown if any of them were `undefined`.
if (url === undefined || username === undefined || password === undefined) {
  // This entire `if` statement is only needed to make the type checker understand that it need not worry about `undefined`.
  // We could have used type assertions (`… as string`), but that may cause nasty bugs down the road.
  throw new Error("The impossible happened.")
}
// `url`, `username` and `password` now have type `string`.

This can easily be fixed by adding a few overloads, for example:

+export function getEndpointUrl(id: string, optional: false): string;
+export function getEndpointUrl(id: string, optional: boolean): string | undefined;
 export function getEndpointUrl(id: string, optional: boolean): string | undefined {
     var urlval = process.env['ENDPOINT_URL_' + id];
 
     if (!optional && !urlval) {
         throw new Error(loc('LIB_EndpointNotExist', id));
     }
 
     debug(id + '=' + urlval);
     return urlval;
 }

Probably Possibly also a breaking change, though, but certainly a helpful one.

[AnnaOpareva]

Hello, @SimonAlling.
Thank you very much for your active participation in refactoring this function and for noticed inaccuracies in the comments. As part of this issue, it seems to us that the main points have been fixed. At the moment we close this issue. Thanks again for your participation. If you have any other comments, be free to write to us and create new issues