💥 [BREAKING] enable TLS if api key provided

[THardy98]

##What was changed
DWISOTT

1. Part of Assume TLS enabled if API key provided in SDKs features#687

How was this tested:
Unit tests

Any docs updates needed?
Maybe

[cretz]

Suggested change

	boolean enableHttps;
	if (this.enableHttpsExplicitlySet) {
	enableHttps = this.enableHttps;
	} else {
	enableHttps = this.apiKeyProvided;
	}
	boolean enableHttps = this.enableHttps;
	if (!this.enableHttpsExplicitlySet && this.apiKeyProvided && this.sslContext != null && this.channel != null) {
	enableHttps = true;
	}

I think this is better. IIUC, you should only default enableHttps if no SSL/channel-related things are set. I think this way of defining the logic is also clearer that you are setting to true in this one rare situation.

[THardy98]

That makes sense to me. I've basically done this but removed the "explicit" flag in favor of changing enableHttps to Boolean

[cretz]

This PR should be marked as 💥 COMPAT BREAK. Perhaps even more in Java than other languages, there may be users setting API keys and expecting non-TLS by default (e.g. to their proxies). So we need to make very clear in release notes this is a breaking change.

[Quinn-With-Two-Ns]

Why are you assuming this is always true if a builder is created from an options?

[THardy98]

Yeah, I figured this would be a safe-ish way to assume enableHttpsExplicitlySet but it doesn't make sense.
It's not a good way to represent the tri-state of: not set, explicitly false, set.
I've instead opted to make enableHttps a Boolean instead of a boolean.

[cretz]

LGTM, nothing blocking (but added a couple of notes). Will leave approval up to Java SDK owners though.

[cretz]

I assume we don't want to support a user calling setEnableHttps(null) to restore to "default"? I don't mind, just want to confirm

[cretz]

Here's a contrived situation:

• I create service stubs options with no API key and having set nothing about TLS whatsoever, thereby making enableHttps default to false on the service stub options
• I want to create new service stub options build from the existing options, so now enableHttps is false even though I never set it
• I do a addApiKey on this new service stub options I have never set any TLS setting on, but it still won't default to TLS because with the current code, it's impossible to create a builder from existing options and retain default TLS behavior

I don't mind if we break and/or don't-support this scenario, just wanted to mention it

[Quinn-With-Two-Ns]

I think we should at least document that scenario since it is confusing. Arguably if enableHttps wasn't explicitly set when the service stub options was created we should preserve that information when we go back to a builder

[THardy98]

Hmm, maybe should revert the change:
this.enableHttps != null ? this.enableHttps : false,

and make enableHttps a Boolean in ServiceStubOptions as well

The semantics might be clearer this way tbh

[cretz]

If we don't consider changing the return type of ServiceStubOptions.getEnableHttps() to Boolean instead of boolean to be too big of a compat break, I agree that is the better/clearer approach

[Quinn-With-Two-Ns]

Hm I think changing the return type to a Boolean is a bit dangerous since if we return a null and users expect it to be a bool then they could get a null pointer exception. Can we just internally keep track of it as a Boolean, but keep the return type the same?

[cretz]

I didn't figure it'd be that common, but meh. This works for me, but what should be the return value of getEnableHttps be if enableHttps is null? Would the logic be return (this.enableHttps != null && this.enableHttps) || (enableHttps == null && this.apiKeyProvided && this.sslContext == null && this.channel == null)? I also assume users may never want to differentiate whether it's explicitly set or not? Maybe a different field name/getter and getEnableHttps is that more advanced logic but the getter still exists for the wrapper?