-
Notifications
You must be signed in to change notification settings - Fork 129
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
Add db-specific error #521
Conversation
To be more consistent. Because we already return data as is in case of base64 error.
Though it can not be properly triggered yet.
oops, sorry ヾ(_ _。)
...unless `default_data_value` is defined. In that case, make it `default_value`. Also, fix tests.
Merge implementations of encodeText and encodeBinary into one encodeToValue.
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.
finally lgtm. except... can we recognize error message in the python driver's exceptions in integration tests?
// `GetResponseOnFail` | ||
ResponseOnFailEmpty ResponseOnFail = "" | ||
|
||
// ResponseOnFailCiphertext indicates that raw cip value should be returned |
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.
never saw cip
as short form of ciphertext
)) did you see somewhere?
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.
Oops, a lil typo :)
setting: &config.BasicColumnEncryptionSetting{ | ||
Tokenized: false, | ||
DataType: "int32", | ||
ResponseOnFail: "default_value", |
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.
hmm, if you added own type for that with constants, maybe we should use them instead of hardcoded values?
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.
Hard coded values are more readable, imho. Good tests should be readable - because review from the person is the only way to verify correctness of the tests.
But on the other hand, use of constants makes introducing changes (renames) easier. So, it makes sense. I'll do it
tests/test.py
Outdated
self.engine2.execute( | ||
sa.select([self.test_table]) | ||
.where(self.test_table.c.id == data['id'])) | ||
self.assertIn("encoding error", str(ex)) |
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.
self.assertIn("encoding error", str(ex)) | |
self.assertIn("encoding error", str(ex)) |
this check doesn't call) because exception raised on previous row)
plus, better to check self.assertEqual(r'encoding error in column "value_str"\n', ex.exception.args[0])
Co-authored-by: Lagovas <[email protected]>
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.
lgtm
One of the last PRs added support for type awareness. That allows defining types on values, which would be decrypted and returned from Acra, like string or int. It also added default values field in configs, which allows to specify which value to return, if some error (possibly decryption error) occurs.
This PR adds ability to chose, which action should be performed on (decryption) failure: either to return client specific error, like acra censor already can, or return default value.
Default action, if type awareness is enabled, is "error". The user can specify the "default" case together with a default value.
Checklist
with new changes
Benchmark results are attached (if applicable)