Refactor encrypt interface and various TS types#19
Conversation
encrypt interface and various TS types
| columnName: string, | ||
| tableName?: string, | ||
| context?: Context, | ||
| plaintext: EncryptPayload, |
There was a problem hiding this comment.
plaintext, columnName, tableName, and context have been combined into a single arg of type EncryptPayload. encrypt takes a single EncryptPayload and encryptBulk takes an EncryptPayload[].
Is this similar to what you had in mind @calvinbrewer? I can do something similar for decrypt/decryptBulk as well if that's helpful (but we'd only be combining ciphertext and lockContext into a DecryptPayload.
| if let Some(service_token) = value { | ||
| let service_token: Handle<JsObject> = service_token.downcast_or_throw(cx)?; | ||
| match value { | ||
| Some(service_token) if is_defined(service_token, cx) => { |
There was a problem hiding this comment.
This is the change that fixes up errors on the Rust side when the token from JS is undefined.
calvinbrewer
left a comment
There was a problem hiding this comment.
I'm a bit confused on what you are doing with the index.cts file. Based on my understanding of the neon framework, we have to explicitly define the functions and types in the module definition in order to support TypeScript in this package.
E.g.
export type argTypes {
...
}
export function encrypt(args: argTypes) {
return addon.encrypt(args)
}You left the decrypt function in there so I'm just confused on your direction!
@calvinbrewer, my intent is to leave the types defined in the TypeScript types show up properly for me in the integration tests, but we can confirm in Protect.js as well before merging this PR. Part of the purpose of the integration tests is to be able to be able to pick up on typing issues before testing things out in Protect.js. |
Sounds good - let's do a pre release and test it out in Protect.js! |
@calvinbrewer I have a branch up in Protect.js that demonstrates the new types in this PR (released as v0.13.0-0) are working. Here's an example of errors from |
This change refactors the `encrypt` function to combine the `plaintext`, `columnName`, `tableName`, and `context` args into a single `EncryptPayload` (similar to `encryptBulk`). This change also makes the `table` property required in the `EncryptPayload` and allows for passing in `serviceToken` as `undefined` to the Rust addon functions.
057e48d to
4e3ef27
Compare
| } | ||
|
|
||
| export type EncryptedEqlPayload = { | ||
| export type Encrypted = { |
There was a problem hiding this comment.
Should we lift this logic in Protect.js to this repo? https://github.com/cipherstash/protectjs/blob/main/packages/protect/generateEqlSchema.ts
Then leverage the EqlSchema upstream?
There was a problem hiding this comment.
Nice. Yes, I like that approach. Let's add that in a follow-up PR.
calvinbrewer
left a comment
There was a problem hiding this comment.
Nice this is working well and validated typing is working as expected here: cipherstash/stack@5e12152
Changes:
plaintext,columnName,tableName, andlockContextargs into a single opts object forencryptEncryptPayload, is shared betweenencryptandencryptBulkencrypt/encryptBulkinstead of stringsencryptSchemaarg fornewClientrequiredtableproperty passed toencryptandencryptBulkrequiredEncryptedEqlPayloadandBulkEncryptedEqlPayloadtypesundefinedcstTokens from TS to RustencryptBulkanddecryptBulkundefinedforctsToken