Skip to content
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

refactor(NODE-5419): move csfle source and tests into the driver #3770

Merged
merged 36 commits into from
Jul 20, 2023

Conversation

baileympearson
Copy link
Contributor

@baileympearson baileympearson commented Jul 11, 2023

Description

What is changing?

This PR depends on mongodb/libmongocrypt#667.

This is a large PR but there's not much that can be done about that. I'll highlight the relevant changes here.

  • FLE code has been moved from encryption into the src. The FLE logic was basically ported directly as-is into src, with three modifications
    -- we no longer inject mongodb into any of the files. Instead, each file is a top-level module.
    -- we no longer inject BSON into any FLE logic. Each FLE class requires from bson directly now. And the _bson properties have been removed from each class as well.
    -- I converted the imports to import syntax instead of require syntax. I did this while debugging, it wasn't necessary, but once it was done I figured I'd leave it because it's one fewer thing to adjust when I convert the files to TS (up next). I can revert this now though, if y'all would like.
  • the tests have been ported into integration/node-specific and unit/client-side-encryption.
    -- the unit tests were mostly left as-is, I plan to update them more when they're converted to TS. I did have to fix imports though. I also deleted some redundant tests.
    -- any tests that required a "live server" have been moved into integration.
    -- shared library tests were moved into integration, because we have integration testing environments that have the shared library installed.

Some other misc changes

  • obviously, the driver has been updated to consume AutoEncrypter directly from src. changes were pretty minimal here
  • I moved the FLE import logic into deps.ts to align with our existing optional deps pattern.
  • the shared library tests now spawn subprocesses, since loading the shared library is a process-wide change and the tests were interfering with each other.
  • a new filter was added, GenericPredicateFilter, that abstracts away the need to manually skip tests in before each hooks by running a specified predicate function in the filter.

NOTE FOR REVIEWERS

Until an alpha is release of mongodb-client-encryption containing the changes to the FLE bindings, pretty much all CI is red. The important tests here are the custom dependency tests with a pinned commit, which pass.

All the integration tests and unit tests pass locally against a local copy the changes in mongodb/libmongocrypt#667 too. Once we merge the bindings changes, I'll release an alpha, pull the alpha into this PR and rerun CI.

To test the changes:

  • make sure you've checked out PR#667 from libmongocrypt.
  • make sure your environment is set up for FLE tests (kms servers, credentials, ect)
  • build and link the bindings: FLE_BINDINGS_PATH=<> ./etc/tooling/fle.sh link --build-libmongocrypt
  • run the tests normally
Is there new documentation needed for these changes?

Not yet.

What is the motivation for this change?

Release Highlight

Fill in title or leave empty for no highlight

Double check the following

  • Ran npm run check:lint script
  • Self-review completed using the steps outlined here
  • PR title follows the correct format: type(NODE-xxxx)[!]: description
    • Example: feat(NODE-1234)!: rewriting everything in coffeescript
  • Changes are covered by tests
  • New TODOs have a related JIRA ticket

@baileympearson baileympearson changed the title move csfle src into driver and tests passing refactor(NODE-5419): move csfle source and tests into the driver Jul 11, 2023
@baileympearson baileympearson marked this pull request as ready for review July 11, 2023 19:36
@baileympearson baileympearson force-pushed the no-story-rely-on-driver-fle-logic branch from b1da8a4 to cecfdfe Compare July 12, 2023 18:36
Copy link
Contributor

@nbbeeken nbbeeken left a comment

Choose a reason for hiding this comment

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

A few small fix ups, and from slack if we can get the CI green with skips (as long as the cursom version keeps working) we're good to go

src/client-side-encryption/autoEncrypter.js Outdated Show resolved Hide resolved
src/client-side-encryption/autoEncrypter.js Show resolved Hide resolved
src/client-side-encryption/clientEncryption.js Outdated Show resolved Hide resolved
src/client-side-encryption/stateMachine.js Show resolved Hide resolved
test/types/client-side-encryption.test-d.ts Outdated Show resolved Hide resolved
test/unit/client-side-encryption/autoEncrypter.test.js Outdated Show resolved Hide resolved
test/unit/client-side-encryption/stateMachine.test.js Outdated Show resolved Hide resolved
@nbbeeken nbbeeken self-assigned this Jul 14, 2023
@nbbeeken nbbeeken added the Primary Review In Review with primary reviewer, not yet ready for team's eyes label Jul 14, 2023
Copy link
Contributor

@nbbeeken nbbeeken left a comment

Choose a reason for hiding this comment

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

We've introduced a bunch of linter exemptions for the imports, let's just double check their used consistently eventually someone will h to have to clean them up.

Q about the FLE import change.

src/deps.ts Show resolved Hide resolved
@nbbeeken nbbeeken added Team Review Needs review from team and removed Primary Review In Review with primary reviewer, not yet ready for team's eyes labels Jul 19, 2023
nbbeeken
nbbeeken previously approved these changes Jul 20, 2023
@nbbeeken nbbeeken merged commit 579219c into main Jul 20, 2023
1 check passed
@nbbeeken nbbeeken deleted the no-story-rely-on-driver-fle-logic branch July 20, 2023 16:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Team Review Needs review from team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants