feat(connection-form): add support for FLE KMS options COMPASS-5639

[addaleax]

Description

Checklist

• New tests and/or benchmarks are included
• Documentation is changed or added

Motivation and Context

• Bugfix
• New feature
• Dependency update
• Misc

Open Questions

Dependents

Types of changes

• Backport Needed
• Patch (non-breaking change which fixes an issue)
• Minor (non-breaking change which adds functionality)
• Major (fix or feature that would cause existing functionality to change)

[Anemy]

Nice! Couple comments/suggestions/thoughts. Looking good. Nice react work!

[Anemy]

I'm receiving an error when typing in this field:

Is that expected for now?

[Anemy]

Since this action does some more logic with the textToEncryptedFieldConfig function do we want to add a test for this component that types into this editor and spies on the response to the onChange prop we pass to the component?
An example of a test like this: https://github.com/mongodb-js/compass/blob/main/packages/compass-find-in-page/src/components/find-in-page-input.spec.tsx#L121 - there are also probably a few in this package to reference as well.

[addaleax]

Ugh, thanks for noticing this error. No, it’s not expected, it’s a weird quirk of our ejson-shell-parser package that I cannot understand at all, where invalid input is mapped to an empty string as a result, rather than an exception.

(This is worrying me a little bit, because it means that we might be reading an empty string where we expect something else in other places as well. Here this would have been particularly bad, since accidentally omitting a clien-provided EncryptedFieldConfig can be dangerous from a security perspective.)

Since this action does some more logic with the textToEncryptedFieldConfig function do we want to add a test for this component that types into this editor and spies on the response to the onChange prop we pass to the component?

I’ve tinkered around a bit with writing a test like this, but in the end I’m not sure how to make this work with the ace editor rather than a text input.

Fwiw, the bulk of the work here is done by textToEncryptedFieldConfig() and encryptedFieldConfigToText(), and those are tested quite well, so I’ve moved the remainder of the text conversions there as well.

---

Somewhat relatedly, I noticed that we also need to use the logic here for reviving EncryptedFieldConfig when double-clicking through the sidebar, where otherwise the stored data in the model would have led to errors. I’ve exposed a wrapper around adjustCSFLEParams() in the connection-form exports (see 5f8f819), it doesn’t feel like an ideal solution here though and I’m happy to take suggestions on where a better place for this would be.

[Anemy]

Gotcha - nice find.

For the location of adjustCSFLEParams - would it make sense to be in the data-service instead?
Something on load/save? I'm thinking then the connect layer would only interact with the expected connectionOptions.
https://github.com/mongodb-js/compass/blob/main/packages/data-service/src/legacy/legacy-connection-model.ts#L206
Used here: https://github.com/mongodb-js/compass/blob/main/packages/data-service/src/connection-storage.ts#L46
Not sure if that's a better spot, just a thought. I think ideally we want this connection form to be usable in places like VSCode or other projects down the line - it would make sense to keep the connection form logic/usage to be minimal where we can then.

[addaleax]

Mhh … I thought about that, and actually wrote a version that placed it in data-service at some point, but:

• This is solving a problem with how options are stored (i.e. not in a way that properly serializes/deserializes BSON values), not with how we connect
• It makes sense to me to keep the $compass.xxx special field handling as localized as possible, not spread across packages

The ideal way to handle this would probably be adjusting the connection storage to handle BSON data well, e.g. by using EJSON instead of JSON, but that might not be trivial (since some of the data might not be handled well by EJSON, and we’re aiming to move the storage layer away from Ampersand entirely at some point).

[Anemy]

lgtm! Couple styling thoughts/suggestions, not blockers. Nice work – I like how you've made each dropdown accordion show the state and error state if there is one.

[Anemy]

How would you feel about wrapping this in the FormFieldContainer so that it gets a little bit of top margin?

We might be able to remove the first withMarginStyles from that component then
https://github.com/mongodb-js/compass/pull/2928/files#diff-02ecb09ae0d4678618b443da67f6a9cdf5c3043d424727b39d494a2dc53fb9a9R55

[addaleax]

Sure, done – I’ve removed these everywhere and used FormFieldContainer instead. I’m not sure if this is what you had in mind exactly, but in any case these feel like things that are fairly easy to adjust :)