feat: visually identify CSFLE-decrypted fields COMPASS-5628 #2951
Conversation
There was a problem hiding this comment.
One or two questions, looking good - also handles that long string case nicely ^^
Looking at the maybeDecorateWithDecryptedKeys it looks like we have to repeat that in a lot of places, is there a way we could keep it more in one place? Not sure if that's possible without some larger refactoring so I'm thinking its fine as is?
| * Detemine if this value or any of its children were marked | ||
| * as having been decrypted with CSFLE. | ||
| * | ||
| * Warning: This does *not* apply to the children of these elements! |
There was a problem hiding this comment.
Maybe I'm misunderstanding this function, but aren't we evaluating here if the children of this element have decrypted children?
Should this warning be for isValueDecrypted?
There was a problem hiding this comment.
Maybe I'm misunderstanding this function, but aren't we evaluating here if the children of this element have decrypted children?
I’ve made the comments a bit more explicit – it returns whether a child of this element has been decrypted. That would not apply to a child of a decrypted field, because decrypted fields generally don’t have decrypted children.
I will admit that I’ve never actually checked what happens when nested encryption happens on nested fields. I don’t think anybody would ever really do this on purpose, but I’ve opened COMPASS-5705 to remind myself to look into that, and how we would handle it in Compass. 🙂
Should this warning be for
isValueDecrypted?
It also applies there, that’s true 🙂 Added it.
| <div className={elementDivider} role="presentation"> | ||
| : | ||
| { | ||
| /* TODO: figure out exact placement */ |
There was a problem hiding this comment.
How do we figure out exact placement? Is that pending w/ Claudia? If there is a ticket maybe we can link it.
There was a problem hiding this comment.
Yeah, I’ve opened a ticket and assigned it to her, added the reference here ;)
I don’t think we’ll need to repeat it in more places than the ones that are present here. It is a bit weird that we duplicate the object generation code four times essentially, but I don’t think this PR is making things much worse? 🤷♀️ It’s certainly possible to merge those (I certainly feel like I’m leaning towards DRY a bit more than others) but it might also be okay to keep it simple here. |
2 participants
Note: This only works with the changes in mongodb-js/devtools-connect#9 and mongodb/libmongocrypt@adfdce0.
(For now not placing this on the right-hand side of the screen as we had discussed last week, because we only actually show the types in edit mode currently)
Description
Checklist
Motivation and Context
Open Questions
Dependents
Types of changes