-
-
Notifications
You must be signed in to change notification settings - Fork 30
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
feat: Add support for filtering encrypted field by list of values #120
feat: Add support for filtering encrypted field by list of values #120
Conversation
Pull Request Test Coverage Report for Build 10309403851Details
💛 - Coveralls |
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.
Thanks for this feature!
A couple of things could do with some changes (field naming and checking for arrays), otherwise it looks good.
@@ -67,7 +67,7 @@ export function encryptOnWrite<Models extends string, Actions extends string>( | |||
models, | |||
function encryptFieldValue({ | |||
fieldConfig, | |||
value: clearText, | |||
value: unHashedValue, |
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.
suggestion: Keep the original clearText
name, as the value is not only used for hashing, but also for encryption in most (writing) cases.
src/encryption.ts
Outdated
if (typeof unHashedValue === 'object') { | ||
hash = (unHashedValue as string[]).map((value) => hashString(value, fieldConfigHash)); |
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.
suggestion: Use the Array.isArray
function, to make the intent clearer. It also removes the need for the as string[]
type casting.
src/encryption.ts
Outdated
|
||
// Encrypting list values is not yet supported | ||
if (typeof unHashedValue !== 'string') { | ||
return; |
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.
suggestion: Could we add a log point to this code path, for debugging? It's unlikely we'll hit it with Prisma's current API AFAIK, but it would make it easier to catch issues in the future.
( | ||
// Used for where: { field: in: []} queries | ||
typeof (node as any)?.[specialSubField] === 'string' || | ||
typeof (node as any)?.[specialSubField] === 'object' |
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.
suggestion: Same test here, using Array.isArray
.
- Apply Prettier formatting
@SteveKekacs Can we please move forward with this PR? If you do not have time I could take over and apply changes from comments. I really need this feature in my project. Thanks. |
Hi @m-przybylski, apologies, I'm no longer using this library so go ahead and take over. |
Superseded by #136. |
Took a stab at implementing a fix for #119.