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

Add support for COUNT queries and NUMBER and BOOLEAN property mapping #169

Merged
merged 4 commits into from
Feb 6, 2024

Conversation

pavelhoral
Copy link
Member

@pavelhoral pavelhoral commented Feb 3, 2024

This PR contains several changes made to the openidm-repo-jdbc module:

  • complete overhaul of most of its components (i.e. written from scratch)
  • testcontainers integration tests so that we can be sure we are compatible with supported database engines (SQL integration tests #115)
  • support for counting matching objects when using query filters
  • support for BOOLEAN and NUMBER column mapping to native SQL data types (see bellow)
  • properties tables are no longer necessary for PostgreSQL (they were not used previously, but they had to be present)

The refactor is split into multiple commits:

  • first commit introduces Testcontainers ITs to test the legacy components
  • second commit makes JDBCRepoService use newly written components
  • third commit "unlocks" new features mentioned above

As I was on my Windows machine one of the commit also fixes Windows build. All these commits have to be kept separate (so no squashing). I want to be able to get back to the legacy implementation and run the tests again if we find out something is missing from the refactor or that something behaves a bit differently.

Few notes on compatibility:

  • My Windows build fix breaks compatibility in glob pattern matching for ZIP archives defined in launcher.json. I don't think that is a feature anyone uses, but we should state it somewhere. Maybe a comment in GitHub release (and copied to the blog post) will be enough.
  • I want to write documentation for the JDBC repository module next, so details about new features (including new configuration options) will be there.

@pavelhoral pavelhoral mentioned this pull request Feb 4, 2024
@pavelhoral pavelhoral force-pushed the feat-query-count branch 4 times, most recently from 7248599 to 2d61427 Compare February 5, 2024 17:44
@pavelhoral pavelhoral changed the title Add support for COUNT queries and NUMBER and BOOLEAN property mapping Draft: Add support for COUNT queries and NUMBER and BOOLEAN property mapping Feb 5, 2024
@pavelhoral pavelhoral marked this pull request as draft February 5, 2024 21:10
@pavelhoral pavelhoral changed the title Draft: Add support for COUNT queries and NUMBER and BOOLEAN property mapping Add support for COUNT queries and NUMBER and BOOLEAN property mapping Feb 5, 2024
@pavelhoral pavelhoral marked this pull request as ready for review February 6, 2024 12:06
This commit adds new implementation of many openidm-repo-jdbc
components. Most components ertr written from scratch with only
a few pieces directly taken from the original code. The overall
module logic stays the same and the overall comoponent structure
as well.

To make sure the DB integration works and the refactor does not
introduce any inconsistency a testcontainer integration tests
are added. These tests can be run on the legacy components as well
as the new components. This commit intentionally leaves both
implementations so that it is possible to return to this state and
recheck legacy behavior.
Copy link
Member

@karelmaxa karelmaxa left a comment

Choose a reason for hiding this comment

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

LGTM. Excellent work.

@karelmaxa karelmaxa merged commit c602122 into WrenSecurity:main Feb 6, 2024
2 checks passed
@karelmaxa karelmaxa deleted the feat-query-count branch September 30, 2024 11:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants