-
Notifications
You must be signed in to change notification settings - Fork 0
Refactor encrypt
interface and various TS types
#19
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
encrypt
interface and various TS types
columnName: string, | ||
tableName?: string, | ||
context?: Context, | ||
plaintext: EncryptPayload, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the change that fixes up errors on the Rust side when the token from JS is undefined
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
@@ -127,11 +73,15 @@ export type Context = { | |||
identityClaim: string[] | |||
} | |||
|
|||
export type EncryptedEqlPayload = { | |||
export type Encrypted = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice. Yes, I like that approach. Let's add that in a follow-up PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice this is working well and validated typing is working as expected here: cipherstash/protectjs@5e12152
Changes:
plaintext
,columnName
,tableName
, andlockContext
args into a single opts object forencrypt
EncryptPayload
, is shared betweenencrypt
andencryptBulk
encrypt
/encryptBulk
instead of stringsencryptSchema
arg fornewClient
requiredtable
property passed toencrypt
andencryptBulk
requiredEncryptedEqlPayload
andBulkEncryptedEqlPayload
typesundefined
cstToken
s from TS to RustencryptBulk
anddecryptBulk
undefined
forctsToken