OAuth2Client: input parameter opts mutated in generateAuthUrl, leading to unexpected behavior

[danslobodan]

Description:

When calling the generateAuthUrl method, the input parameter opts is being mutated. This leads to unexpected behavior when the same opts object is reused in subsequent calls, as it may retain state from previous invocations. This can cause issues when generating URLs for different clients (e.g., Android and iOS) within the same application.

Steps to Reproduce:

1. Create a single opts object.
2. Call generateAuthUrl with the opts object for one client.
3. Call generateAuthUrl again with the same opts object for a different client.
4. Observe that the opts object has been modified by the first call, leading to incorrect behavior in the second call.

Expected Behavior:

The opts parameter should remain unchanged after being passed to the generateAuthUrl function. If modifications are necessary, a copy of the opts object should be made within the function.

Actual Behavior:

The opts object is modified by the generateAuthUrl function, causing unintended side effects when reused.

[danielbankhead]

Hey @danslobodan,

Thanks for filing an issue. When it comes to generateAuthUrl, and numerous other methods in the library in general, it wouldn't be performant to copy or deep-copy on each call. Instead, we treat the provided object as mutable. You could create a shallow-copy for the method like this if you do not prefer the mutations:

client.generateAuthUrl({...opts});

However I think you're highlighting something more important: I think we should have a stronger API contract for what's considered mutable or not.

[danslobodan]

I don't understand why performance is so critical here that you would rather risk mutating the input parameter thus potentially creating bugs that are really hard to diagnose, when in the same file you can find that getTokenAsync creates a new object, and verifyIdTokenAsync passes parameters from the object one by one.

Both of these methods seem to respect the rule that input parameters of functions should not be mutated, especially outward facing (public) functions.

Why then does generateAuthUrl have a different policy?

I already solved the problem on my side in a way that you describe, but I'm not the only one who will fall in this hole, counting on good programming practices to be honored in a widely used library.

[danielbankhead]

I don't understand why performance is so critical here that you would rather risk mutating the input parameter thus potentially creating bugs that are really hard to diagnose, when in the same file you can find that getTokenAsync creates a new object, and verifyIdTokenAsync passes parameters from the object one by one.

Both of these methods seem to respect the rule that input parameters of functions should not be mutated, especially outward facing (public) functions.

Which rule are you referring to?

Why then does generateAuthUrl have a different policy?

I already solved the problem on my side in a way that you describe, but I'm not the only one who will fall in this hole, counting on good programming practices to be honored in a widely used library.

Which ‘good programming’ practices are you referring to?

I’ve seen numerous JS libraries to mutate the provided objects, both in and outside of Google. However, as I said, I think we should improve the public API contract to make this clearer.

[danslobodan]

Sorry about generalising and if I sounded condescending in any way, but I'd really like to consider changing this.

I've already lost time and I don't gain anything by being right or having my way, and my intention is not to annoy strangers on the internet because I'm bored. I think many more people will fall pray to assumption that I had and lose time and introduce bugs into production, not knowing that they've done it.

So please, try to consider my argument.

It could be argued that generateAuthUrl violates Command Query Separation in that it is a query (it takes some input - an object, and returns an output - a string) and as such should not mutate the input.

But for me personally the Principle of Least Astonishment is more appropriate here - it's really unobvious that a function that takes some input and returns a string would mutate it's input. Would you expect that?

I fully support the idea of communicating more strongly what's mutated and what's not, but I don't see how to do this well in this case, and with Javascript. I don't think a better description will change anything and there's no &mut opts (like in Rust), as far as I know.

I think it's better to let people assume that their inputs will not be changed and do something like in 'verifyIdTokenAsync' function, but if you want to apply defaults from the constructor, you could do the same, without creating a new object by passing arguments to the function, like this:

Let's assume this is the function: generateAuthUrlAsync(inputA, inputB, inputC)

And you call it like this: generateAuthUrl(opts.inputA || this.defaults.inputA, opts.inputB || this.defaults.inputB, ...)

If making a new object affects performance that much, this way you don't have to make a new object and you don't modify the input parameters.

It's an easy fix - it would take you less time to fix and test it, than it took you to read all this, I'm sure. And nobody (but me) will thank you for it, because they will never know it worked differently, but it's still going save them time and money and nerves.

If you still don't find this convincing, I will not bother you any more.

Peace.