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

feat(sql): add SQL bootstrapper #4376

Conversation

paullatzelsperger
Copy link
Member

@paullatzelsperger paullatzelsperger commented Jul 26, 2024

What this PR changes/adds

This PR adds the SqlSchemaBootstrapper class, which allows sql-related extensions to enqueue DDL statements. In the prepare() phase, the bootstrapper then executes these commands one after the other.

To do that, the schema files (formerly known as schema.sql) were renamed and moved to the resources/ folder. The were given individual names to avoid name clashes.

In addition, all SQL-store tests were updated to read the schema from the resource rather than docs/schema.sql.

Why it does that

easy way to create database structures.

Further notes

  • "edc.sql.schema.autocreate" is false by default

Linked Issue(s)

Closes #4356

Please be sure to take a look at the contributing guidelines and our etiquette for pull requests.

@paullatzelsperger paullatzelsperger added the enhancement New feature or request label Jul 26, 2024
@codecov-commenter
Copy link

codecov-commenter commented Jul 26, 2024

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 83.09859% with 12 lines in your changes missing coverage. Please review.

Project coverage is 74.95%. Comparing base (7f20ba5) to head (ee45e97).
Report is 370 commits behind head on main.

Files Patch % Lines
...ne/store/sql/SqlAccessTokenDataStoreExtension.java 0.00% 3 Missing ⚠️
...ataplane/store/sql/SqlDataPlaneStoreExtension.java 0.00% 3 Missing ⚠️
...se/edc/sql/bootstrapper/SqlDmlStatementRunner.java 91.30% 2 Missing ⚠️
...dc/sql/bootstrapper/SqlSchemaBootstrapperImpl.java 80.00% 2 Missing ⚠️
...l/bootstrapper/SqlSchemaBootstrapperExtension.java 92.30% 0 Missing and 1 partial ⚠️
...itor/store/sql/SqlPolicyMonitorStoreExtension.java 0.00% 1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4376      +/-   ##
==========================================
+ Coverage   71.74%   74.95%   +3.21%     
==========================================
  Files         919     1071     +152     
  Lines       18457    21468    +3011     
  Branches     1037     1174     +137     
==========================================
+ Hits        13242    16092    +2850     
- Misses       4756     4852      +96     
- Partials      459      524      +65     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

extensions/common/sql/sql-bootstrapper/build.gradle.kts Outdated Show resolved Hide resolved
extensions/common/sql/sql-bootstrapper/build.gradle.kts Outdated Show resolved Hide resolved
}
}

public Result<Void> executeSql() {
Copy link
Member

Choose a reason for hiding this comment

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

this method should be put in the interface (that will save the explicit cast in the SqlSchemaBootstrapperExtension)

Copy link
Member Author

@paullatzelsperger paullatzelsperger Jul 29, 2024

Choose a reason for hiding this comment

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

I don't want this method in the interface, for it could entice contributing modules to invoke it, which could potentially cause problems. The cast is ugly, but it should be possible to simply use the variable (as opposed to the getBootstrapper() method) in the prepare() phase.

Copy link
Member

Choose a reason for hiding this comment

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

ok, I understand it, but this necessity is not explicit (and being misinterpreted, e.g. from a future-me :) ), that could be achieved by keeping only the addStatement method in the bootstrapper and moving the execution logic to a module-private service, that won't be registered on the runtime, this way the executeSql will effectively become not callable from other modules.

Copy link
Member Author

Choose a reason for hiding this comment

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

thats a lot of ceremony for one single method. if executeSql() is package-private and documented, it should be clear enough what is done.
In addition, I didn't put the interface in an SPI package on purpose, because it is not intended as an extension point anyway.

Copy link
Member Author

@paullatzelsperger paullatzelsperger Jul 29, 2024

Choose a reason for hiding this comment

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

I tried it, and it looks OK, but I'm not really sure splitting such a trivial task in two classes (and two test classes) is really worth it TBH. LMK what you think.

@@ -42,18 +42,16 @@ static void createDatabase(String participantName) {

var extensionsFolder = TestUtils.findBuildRoot().toPath().resolve("extensions");
Copy link
Member

Choose a reason for hiding this comment

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

at this point the createDatabase could just create the database and leave the bootstrapper to create tables and indexes

Copy link
Member Author

@paullatzelsperger paullatzelsperger Jul 29, 2024

Choose a reason for hiding this comment

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

for the E2E tests this will work but the e2e transfer tests, e.g. TransferPushEndToEndTest, run into a race condition then. I'll give it a shot using @Order(...)

@paullatzelsperger paullatzelsperger force-pushed the feat/4356_add_sql_bootstrapper branch 4 times, most recently from 4ea4410 to 6c3ab12 Compare July 29, 2024 14:03
@paullatzelsperger paullatzelsperger marked this pull request as draft July 29, 2024 14:20
@paullatzelsperger paullatzelsperger marked this pull request as ready for review July 29, 2024 15:32
@paullatzelsperger paullatzelsperger merged commit ae5664e into eclipse-edc:main Jul 30, 2024
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provide SQL Migrations by default
4 participants