Skip to content

Conversation

@islamaliev
Copy link
Contributor

Relevant issue(s)

Resolves #3754

Description

Add create doc options to collection create-doc-related methods.
The options allow specifying parameters for doc encryption.

As opposed to the previous approach with passing the context, this moves parameters handing into the interface and unifies it for all clients.

@islamaliev islamaliev added this to the DefraDB v0.18 milestone Jun 3, 2025
@islamaliev islamaliev requested a review from a team June 3, 2025 07:02
@islamaliev islamaliev self-assigned this Jun 3, 2025
@islamaliev islamaliev added bug Something isn't working security Related to security labels Jun 3, 2025
@islamaliev islamaliev changed the title Add create option to collection with encryption params fix: Add create option to collection with encryption params Jun 3, 2025
@islamaliev islamaliev force-pushed the fix/doc-encryption-for-embedded branch from a7ffcda to 50f941f Compare June 3, 2025 07:03
@islamaliev islamaliev changed the title fix: Add create option to collection with encryption params fix: Enable doc encryption for embedded client Jun 3, 2025
@codecov
Copy link

codecov bot commented Jun 3, 2025

Codecov Report

Attention: Patch coverage is 83.95062% with 13 lines in your changes missing coverage. Please review.

Project coverage is 77.24%. Comparing base (b04272f) to head (38a019c).
Report is 1 commits behind head on develop.

Files with missing lines Patch % Lines
js/client_collection.go 52.00% 10 Missing and 2 partials ⚠️
http/handler_collection.go 80.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #3755      +/-   ##
===========================================
+ Coverage    77.18%   77.24%   +0.05%     
===========================================
  Files          422      423       +1     
  Lines        38257    38289      +32     
===========================================
+ Hits         29528    29573      +45     
+ Misses        6855     6847       -8     
+ Partials      1874     1869       -5     
Flag Coverage Δ
all-tests 77.24% <83.95%> (+0.05%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
cli/collection_create.go 73.26% <100.00%> (-1.19%) ⬇️
client/collection.go 100.00% <100.00%> (ø)
http/client.go 52.05% <ø> (-0.24%) ⬇️
http/client_collection.go 51.69% <100.00%> (+0.55%) ⬆️
internal/db/collection.go 68.70% <100.00%> (+0.50%) ⬆️
internal/planner/create.go 78.12% <100.00%> (+0.47%) ⬆️
js/utils.go 41.43% <ø> (-9.17%) ⬇️
http/handler_collection.go 71.97% <80.00%> (+0.13%) ⬆️
js/client_collection.go 39.92% <52.00%> (+1.10%) ⬆️

... and 12 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b04272f...38a019c. Read the comment docs.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Member

@jsimnz jsimnz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, minor suggestion raised on some of the param types.

One thing I wanted to highlight as a general convo (non blocker to this PR) - Long term, how much do we want to expose the manual API for (field-level) doc encryption vs soley relying on the policy/schema to determine and inform the rest of the system when docs and fields should be encrypted. (We can have this discussion in discord if prefered)

@islamaliev
Copy link
Contributor Author

how much do we want to expose the manual API for (field-level) doc encryption vs soley relying on the policy/schema to determine and inform the rest of the system when docs and fields should be encrypted.

Encrypt a document or not is a decision made for each individual document, the same applies to fields. Policy is something that is globally applied to all documents. Are you suggesting abandoning the idea of having field-level encryption individually configurable?

Copy link
Member

@nasdf nasdf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Don't forget to update the JS client as well. It is using the context method like the CLI was.

@AndrewSisley
Copy link
Contributor

Are you suggesting abandoning the idea of having field-level encryption individually configurable?

This does sound like an easily avoidable user-complication, but I've not thought a whole load about it and I cant remember a previous discussion around it.

Copy link
Contributor

@AndrewSisley AndrewSisley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM :)

@islamaliev islamaliev force-pushed the fix/doc-encryption-for-embedded branch from 3ace050 to 924b38a Compare June 3, 2025 18:59
@islamaliev islamaliev merged commit 9738f61 into sourcenetwork:develop Jun 4, 2025
44 of 46 checks passed
ChrisBQu pushed a commit to ChrisBQu/defradb that referenced this pull request Jun 16, 2025
## Relevant issue(s)

Resolves [sourcenetwork#3754](sourcenetwork#3754)

## Description

Add create doc options to collection create-doc-related methods. 
The options allow specifying parameters for doc encryption.

As opposed to the previous approach with passing the context, this moves
parameters handing into the interface and unifies it for all clients.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working security Related to security

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Doc Encryption: enable doc encryption for embedded go

5 participants