Don't do any extra parsing, put the responsibility on the caller #10489
Conversation
|
I don't know if anything is currently using this, but I think we should inform them and make this breaking change. The code was technically broken before this anyway, and could've had questionable results. |
|
@shueybubbles I heard this might break the work you're doing. Let's go over this so we don't get stuck maintaining bad logic forever. |
| profile.authenticationType = args.integrated ? Constants.integrated : args.aad ? Constants.azureMFA : (profile.userName.length > 0) ? Constants.sqlLogin : Constants.integrated; | ||
| profile.databaseName = args.database ?? ''; | ||
| profile.userName = args.user ?? ''; | ||
| profile.authenticationType = args.authenticationType ?? Constants.integrated; |
There was a problem hiding this comment.
I guess we'll just be fine with requiring outside callers to get the casing right too? All our internal code pretty much assumes that. Might be useful to do case insensitive check here and then convert to defined case-sensitive version in the profile.
There was a problem hiding this comment.
I'm not sure I'm a fan of that. We could explicitly say this is what we're expecting, these are the values that aren't going to change.
When I call any REST api for example, the REST api doesn't correct me if I make a casing mistake, it just flat out fails and tells me to go fix my stuff. That's kinda the philosophy I'm following here, if that makes sense?
There was a problem hiding this comment.
Yeah that's fine. I personally hate it when stuff requires me to have a specific casing - but I realize it's a pretty normal thing and the code is simpler because of it so it seems fine.
What happens if you give a auth type with invalid casing? The main problem I guess I see is that I highly doubt we handle it in any sane manner so it'd likely just fail in some obscure way along the line. But adding in special logic just for this case isn't super great either so I'm fine either way.
There was a problem hiding this comment.
While I was testing I actually made the mistake of incorrect casing. It simply pops up the connection pane with "unrecognized auth method" or something along those lines.
|
If anyone has any objections, I can revert this PR later on! |
The responsibility to send valid URI should be on the caller. We shouldn't be doing weird parsing to try to make sense of it.
We should also avoid using 'boolean' for types of stuff that come externally. There is no guarantee they're a boolean in runtime. (e.g. in this case)