Skip to content

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

Merged
merged 10 commits into from
Mar 11, 2025

Conversation

CDThomas
Copy link
Contributor

@CDThomas CDThomas commented Mar 10, 2025

Changes:

  • Combine plaintext, columnName, tableName, and lockContext args into a single opts object for encrypt
    • The opts type, EncryptPayload, is shared between encrypt and encryptBulk
  • Return JS objects from encrypt/encryptBulk instead of strings
  • Make encryptSchema arg for newClient required
    • This was already the behaviour on the Rust side, but now the types match the behaviour
  • Make the table property passed to encrypt and encryptBulk required
    • This was already the behaviour on the Rust side, but now the types match the behaviour
  • Remove unused EncryptedEqlPayload and BulkEncryptedEqlPayload types
  • Push logic for handling undefined cstTokens from TS to Rust
  • Add integration tests for encryptBulk and decryptBulk
  • Add integration tests for passing in undefined for ctsToken

@CDThomas CDThomas changed the title Chore/clean up interface Refactor encrypt interface and various TS types Mar 10, 2025
columnName: string,
tableName?: string,
context?: Context,
plaintext: EncryptPayload,
Copy link
Contributor Author

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) => {
Copy link
Contributor Author

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.

@CDThomas CDThomas requested a review from calvinbrewer March 10, 2025 05:04
@CDThomas CDThomas marked this pull request as ready for review March 10, 2025 05:04
Copy link
Contributor

@calvinbrewer calvinbrewer left a 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!

@CDThomas
Copy link
Contributor Author

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 declare module './load.cjs' block, so TypeScript types should still work. I'm removing the wrapper functions in indext.cts because they looked to be a workaround for optional args not being handled properly on the Rust side. I still need to update decrypt to handle optional args on the Rust side, so I haven't removed the wrapper function, yet, but my intent is to do that (potentially in a follow up PR).

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.

@CDThomas CDThomas requested a review from calvinbrewer March 10, 2025 21:21
@calvinbrewer
Copy link
Contributor

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 declare module './load.cjs' block, so TypeScript types should still work. I'm removing the wrapper functions in indext.cts because they looked to be a workaround for optional args not being handled properly on the Rust side. I still need to update decrypt to handle optional args on the Rust side, so I haven't removed the wrapper function, yet, but my intent is to do that (potentially in a follow up PR).

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!

@CDThomas
Copy link
Contributor Author

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 declare module './load.cjs' block, so TypeScript types should still work. I'm removing the wrapper functions in indext.cts because they looked to be a workaround for optional args not being handled properly on the Rust side. I still need to update decrypt to handle optional args on the Rust side, so I haven't removed the wrapper function, yet, but my intent is to do that (potentially in a follow up PR).
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 tsc that show that the types for protect-ffi functions get picked up (and aren't defaulting to any).

CDThomas and others added 8 commits March 11, 2025 16:10
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.
@CDThomas CDThomas force-pushed the chore/clean-up-interface branch from 057e48d to 4e3ef27 Compare March 11, 2025 05:16
@@ -127,11 +73,15 @@ export type Context = {
identityClaim: string[]
}

export type EncryptedEqlPayload = {
export type Encrypted = {
Copy link
Contributor

@calvinbrewer calvinbrewer Mar 11, 2025

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?

Copy link
Contributor Author

@CDThomas CDThomas Mar 11, 2025

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.

Copy link
Contributor

@calvinbrewer calvinbrewer left a 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

@CDThomas CDThomas merged commit ea85c88 into main Mar 11, 2025
3 checks passed
@CDThomas CDThomas deleted the chore/clean-up-interface branch March 11, 2025 22:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants