feat(connection-form): use loose connection string validation #2839
Conversation
Use loose validation in the connection form UI. This allows editing in situations like when the URI contains no username but a password.
| message: parsingError | ||
| ? parsingError.message | ||
| : `Username cannot be empty: "${parsingError.message}"`, | ||
| : 'Username cannot be empty if password is present', |
There was a problem hiding this comment.
Does this error here without username because we want it to fail earlier?
| ); | ||
|
|
||
| if (parsingError) { | ||
| const hasPasswordWithoutUsername = |
There was a problem hiding this comment.
For context, this is not necessary anymore, it was a "special" error handling mechanism necessary only for those updates which were breaking the connection string, now we can move this check with all the others in validations.ts.
There was a problem hiding this comment.
Oh, I see – that’s nice 👍 Are there other cases like these?
There was a problem hiding this comment.
Could even be already covered:
There was a problem hiding this comment.
Alright, done – I think this works well enough at this point :)
There was a problem hiding this comment.
I think i used
new ConnectionString()somewhere in legacy-connection-model to remove the appName@mcasimir That’s fine though, right? We don’t actually want to be able to store invalid connection strings?
That's a good question. Well not pineapple maybe cause in that case we wouldn't be able to identify secrets, but something well-formed with a password but not a username can be ok. That aligns with all the other "validations" and with the general philosophy of new the form of trying to limit the user as least as possible. If someone wants to save a connection without the username cause they don't remember it, they have to jump to something else and just continue later, etc .. then they will be able to. As long as I'm not missing some downside I'd let users do that.
There was a problem hiding this comment.
I see so you mean that we would save string that would then break later, and we won't be able to rely on the fact that
new ConnectionString(str)is safe.
Not so much “later”, but rather “in other parts of the code” (than the connection form).
That is possible to validate, why not. The solution would be the same (
new ConnectionString(str)right before saving and set the error similarly to whatvalidatedoes).
I’m not sure I understand – Compass currently already fails when trying to save invalid connection strings?
My dream would be to have
new ConnectionString(str)having the sole responsability to parse a string and not validating anything, including the scheme. Then a shareduri.validate()or something of that effect that we can actually call and returning errors that we can relate to the string params similar to what a schema validation would do (throw new InvalidConnectionStringError([{path: 'params.tls', message: 'value should be one of ...'}])).
I think that would be really nice to have, yes. But since this is not the status quo of how we use this or the Node.js driver uses it, we can maybe bring this up with the Node.js team at sync today?
There was a problem hiding this comment.
Also, note that the ConnectionString constructor does actually do some validation that is directly related to parsing (for example, that username and password contain no unescaped characters).
There was a problem hiding this comment.
Compass currently already fails when trying to save invalid connection strings?
Not that I know, up till now it wasn't possible to build an invalid connection string:
We could try to new ConnectionString(connectionInfo.connectionOptions.connectionString) before saving, the error should be handled already nicely enough there, a toast will show reporting an error.
There was a problem hiding this comment.
Hm yeah – it reports an error saving the connection (presumably as part of the secrets extraction?), but it does add it to the favorites list. Attempting to remove it again throws an uncaught exception then – that’s the kind of scenario that I’m worried about here.
For now, I’ve pushed a change here that forbids saving invalid connection strings. I think that’s fair as a requirement for making sure that parts of Compass outside the connect-form can safely interact with the saved connection strings.
There was a problem hiding this comment.
that’s the kind of scenario that I’m worried about here
yeah but it also wouldn't happen if new ConnectionString(str) wouldn't do validations
@mcasimir That’s fine though, right? We don’t actually want to be able to store invalid connection strings? |
| try { | ||
| connectionString = new ConnectionString(connectionOptions.connectionString); | ||
| } catch (err: any) { | ||
| parserWarning = [{ message: err.message }]; |
There was a problem hiding this comment.
Technically we can just do this in the validateErrors since it would prevent the connection. Warnings are for cases where the user may be able to connect, also since there is no guarantee for the exception to be informative and suggest a fix probably is better to validate case-by-case ie. validateSchemaIsKnown ...
3 participants
Use loose validation in the connection form UI. This allows
editing in situations like when the URI contains no username
but a password.
Description
Checklist
Motivation and Context
Open Questions
Dependents
Types of changes