Make sure our extension does not interfere when bootstrapping is unrelated to it #146

stIncMale · 2025-11-12T16:33:04Z

stIncMale · 2025-11-12T20:34:41Z

src/test/java/com/mongodb/hibernate/internal/translate/mongoast/filter/AstEmptyFilterTests.java

Test classes should have the "Tests" suffix. This one had, but the name of the file in which it was declared was "Test".

stIncMale · 2025-11-12T21:21:38Z

src/integrationTest/java/com/mongodb/hibernate/boot/FailedBootstrappingIntegrationTests.java

This file was renamed to NativeBootstrappingIntegrationTests. The code was also simplified, taking into account that MongoExtension now disables fail points.

stIncMale · 2025-11-12T21:23:08Z

...ionTest/java/com/mongodb/hibernate/boot/JakartaPersistenceBootstrappingIntegrationTests.java

-            var item = new Item();
-            item.id = 1;
-            em.persist(item);
-        });


Only drive-by changes here to make the test code simpler.

stIncMale · 2025-11-12T21:42:59Z

src/integrationTest/java/com/mongodb/hibernate/ExceptionHandlingIntegrationTests.java

Test classes should have the "Tests" suffix.

stIncMale · 2025-11-12T21:43:38Z

src/main/java/com/mongodb/hibernate/internal/boot/MongoAdditionalMappingContributor.java

    @Override
    public void contribute(
            AdditionalMappingContributions contributions,
            InFlightMetadataCollector metadata,
            ResourceStreamLocator resourceStreamLocator,
            MetadataBuildingContext buildingContext) {
+        if (!(metadata.getDatabase().getDialect() instanceof MongoDialect)) {
+            // avoid interfering with bootstrapping not related to the MongoDB Extension for Hibernate ORM
+            return;
+        }


This is the actual fix.

stIncMale · 2025-11-12T21:45:49Z

src/main/java/com/mongodb/hibernate/internal/service/StandardServiceRegistryScopedState.java

@@ -14,7 +14,7 @@
 * limitations under the License.
 */

-package com.mongodb.hibernate.internal.extension.service;


Most of the program elements we have represent part of the extension. The extension in the package name does not make sense, though originally I thought it would make sense.

stIncMale · 2025-11-12T21:47:17Z

src/main/java/com/mongodb/hibernate/jdbc/MongoConnectionProvider.java

    private @Nullable StandardServiceRegistryScopedState standardServiceRegistryScopedState;
    private transient @Nullable MongoClient mongoClient;

+    public MongoConnectionProvider() {}


Made the constructor used by Hibernate ORM explicit, like we do in other places.

stIncMale · 2025-11-12T21:57:56Z

src/main/java/com/mongodb/hibernate/internal/boot/MongoAdditionalMappingContributor.java

 */

-package com.mongodb.hibernate.internal.extension;
+package com.mongodb.hibernate.internal.boot;


Most of the program elements we have represent part of the extension. The extension in the package name does not make sense, though originally I thought it would make sense.

org.hibernate.boot.spi.AdditionalMappingContributor is in the boot package, which is why I put MongoAdditionalMappingContributor to the boot package.

…ed to it HIBERNATE-135

src/integrationTest/java/com/mongodb/hibernate/boot/NativeBootstrappingIntegrationTests.java

HIBERNATE-135

vbabanin · 2025-11-19T21:45:08Z

As discussed offline, we could use also check if dialect is MongoDB dialect in our ServiceContributor via settings, so that if the user makes a mistake and uses MongoConnectionProvider with non MongoDB Dialect we will fail (because it requires StandardServiceRegistryScopedState) with a user-friendly message.

…der is not plugged in HIBERNATE-135

stIncMale · 2025-11-21T23:37:00Z

As discussed offline, we could use also check if dialect is MongoDB dialect in our ServiceContributor via settings, so that if the user makes a mistake and uses MongoConnectionProvider with non MongoDB Dialect we will fail (because it requires StandardServiceRegistryScopedState) with a user-friendly message.

Done in f7dbe58.

stIncMale · 2025-11-21T23:38:37Z

src/test/java/com/mongodb/hibernate/boot/NativeBootstrappingTests.java

+     * MongoDB Extension for Hibernate ORM.
+     */
+    @Test
+    void testMongoAdditionalMappingContributorIsSkipped() {


This new test fails without the changes done to MongoAdditionalMappingContributor.contribute.

Previously in this PR this test was in PostgreSQLBootstrappingIntegrationTests, but I realized that it can be made a unit test without any dependency on the PostgreSQL driver.

stIncMale · 2025-11-21T23:43:53Z

src/test/java/com/mongodb/hibernate/boot/NativeBootstrappingTests.java

+
+class NativeBootstrappingTests {
+    @Test
+    void testMongoDialectNotPluggedIn() {


Note how there is no testMongoConnectionProviderNotPluggedIn test in NativeBootstrappingTests. That is because it's impossible to write such a test without actually connecting to a DBMS, and that DBMS would have to be not MongoDB, as the connection provider cannot be ours.

However, StandardServiceRegistryScopedStateTests has both testMongoDialectNotPluggedIn and testMongoConnectionProviderNotPluggedIn.

One possible way to simulate this failure path without connecting to a real DBMS is to inject a mocked ConnectionProvider in this case, along with a mocked Connection. The bootstrap will still fail at AbstractMqlTranslator.<init>(AbstractMqlTranslator.java:220) because it depends on StandardServiceRegistryScopedState, which is the expected behavior.

Example

@Test void test() throws SQLException { var standardServiceRegistryBuilder = new StandardServiceRegistryBuilder(new BootstrapServiceRegistryBuilder().build()); standardServiceRegistryBuilder.clearSettings(); ConnectionProvider connectionProvider = mock(ConnectionProvider.class, Mockito.RETURNS_SMART_NULLS); Connection connection = mock(Connection.class, Mockito.RETURNS_SMART_NULLS); when(connectionProvider.getConnection()).thenReturn(connection); assertThatThrownBy(() -> new MetadataSources() .addAnnotatedClass(Item.class) .buildMetadata( standardServiceRegistryBuilder .applySetting(DIALECT, "com.mongodb.hibernate.dialect.MongoDialect") .applySetting(JAKARTA_JDBC_URL, "mongodb://localhost/mongo-hibernate-test?directConnection=false") .applySetting(CONNECTION_PROVIDER, connectionProvider) .applySetting(ALLOW_METADATA_ON_BOOT, false) .build() ) .buildSessionFactory() .close() ).hasRootCauseMessage( "com.mongodb.hibernate.jdbc.MongoConnectionProvider must be plugged in, " + "for example, via the [hibernate.connection.provider_class] configuration property" ); }

This doesn’t reproduce the full end-to-end scenario, but it might be sufficient to exercise the “connection provider not plugged in” branch when provider is not ours.

stIncMale · 2025-11-21T23:44:32Z

src/integrationTest/java/com/mongodb/hibernate/SessionFactoryIntegrationTests.java

+                .buildMetadata(new StandardServiceRegistryBuilder()
+                        .applySettings(settings)
+                        .build())
+                .buildSessionFactory();


A drive-by simplification.

stIncMale · 2025-11-21T23:45:39Z

src/integrationTest/java/com/mongodb/hibernate/SessionFactoryIntegrationTests.java

-                        Map.of(JAKARTA_JDBC_URL, "jdbc:postgresql://localhost/test"))
-                .close());
+        assertThrows(
+                ServiceException.class, () -> buildSessionFactory(Map.of(JAKARTA_JDBC_URL, "jdbc:postgresql://host/"))


Changed the ULR such that there is obviously no chance in practice to successfully connect using it.

stIncMale · 2025-11-21T23:49:17Z

src/main/java/com/mongodb/hibernate/internal/service/StandardServiceRegistryScopedState.java

                public StandardServiceRegistryScopedState initiateService(
                        Map<String, Object> configurationValues, ServiceRegistryImplementor serviceRegistry) {
+                    checkMongoDialectIsPluggedIn(configurationValues, serviceRegistry);
+                    checkMongoConnectionProviderIsPluggedIn(configurationValues);


Originally I wanted to put the checkMongoConnectionProviderIsPluggedIn check into MongoDialect. Unfortunately, there is no way, as far as I can see, to check configuration properties from MongoDialect. The closest to the dialect place where we can do that is the constructor of AbstractMqlTranslator. However, that constructor requires StandardServiceRegistryScopedState, which is why I put the checkMongoConnectionProviderIsPluggedIn check here, close to the checkMongoDialectIsPluggedIn check, which should be here anyway.

One possible option could be to require a MongoConnectionProvider service in the contribute method of MongoDialect, so that bootstrap fails if MongoConnectionProvider is not present. However, this is not an ideal place for this validation either.

Given that, i agree that AbstractMqlTranslator requiring StandardServiceRegistryScopedState seems sufficient.

stIncMale · 2025-11-21T23:50:02Z

src/test/java/com/mongodb/hibernate/cfg/spi/MongoConfigurationContributorTests.java

+        return new MetadataSources()
+                .buildMetadata(new StandardServiceRegistryBuilder()
                        .addService(MongoConfigurationContributor.class, mongoConfigurationContributor)
                        .build())
-                .getMetadataBuilder()
-                .build()
-                .getSessionFactoryBuilder()
-                .build();
+                .buildSessionFactory();


A drive-by simplification.

HIBERNATE-135

vbabanin · 2025-11-28T23:03:20Z

src/main/java/com/mongodb/hibernate/internal/service/StandardServiceRegistryScopedState.java

+                        || (dialect instanceof Class<?> dialectClass
+                                && MongoDialect.class.isAssignableFrom(dialectClass))
+                        || (dialect instanceof String dialectName
+                                && dialectName.startsWith("com.mongodb.hibernate")))) {


Should we use an exact name match here, given that we currently support only a single dialect?

Suggested change

&& dialectName.startsWith("com.mongodb.hibernate")))) {

&& dialectName.equals(MongoDialect.class.getName())))) {

vbabanin · 2025-11-29T18:53:41Z

src/test/java/com/mongodb/hibernate/boot/NativeBootstrappingTests.java

+
+class NativeBootstrappingTests {
+    @Test
+    void testMongoDialectNotPluggedIn() {


One possible way to simulate this failure path without connecting to a real DBMS is to inject a mocked ConnectionProvider in this case, along with a mocked Connection. The bootstrap will still fail at AbstractMqlTranslator.<init>(AbstractMqlTranslator.java:220) because it depends on StandardServiceRegistryScopedState, which is the expected behavior.

Example

@Test void test() throws SQLException { var standardServiceRegistryBuilder = new StandardServiceRegistryBuilder(new BootstrapServiceRegistryBuilder().build()); standardServiceRegistryBuilder.clearSettings(); ConnectionProvider connectionProvider = mock(ConnectionProvider.class, Mockito.RETURNS_SMART_NULLS); Connection connection = mock(Connection.class, Mockito.RETURNS_SMART_NULLS); when(connectionProvider.getConnection()).thenReturn(connection); assertThatThrownBy(() -> new MetadataSources() .addAnnotatedClass(Item.class) .buildMetadata( standardServiceRegistryBuilder .applySetting(DIALECT, "com.mongodb.hibernate.dialect.MongoDialect") .applySetting(JAKARTA_JDBC_URL, "mongodb://localhost/mongo-hibernate-test?directConnection=false") .applySetting(CONNECTION_PROVIDER, connectionProvider) .applySetting(ALLOW_METADATA_ON_BOOT, false) .build() ) .buildSessionFactory() .close() ).hasRootCauseMessage( "com.mongodb.hibernate.jdbc.MongoConnectionProvider must be plugged in, " + "for example, via the [hibernate.connection.provider_class] configuration property" ); }

This doesn’t reproduce the full end-to-end scenario, but it might be sufficient to exercise the “connection provider not plugged in” branch when provider is not ours.

vbabanin · 2025-11-29T19:03:00Z

src/main/java/com/mongodb/hibernate/internal/service/StandardServiceRegistryScopedState.java

+                var dialect = configurationValues.get(AvailableSettings.DIALECT);
+                if (!((dialect instanceof MongoDialect)
+                        || (dialect instanceof Class<?> dialectClass
+                                && MongoDialect.class.isAssignableFrom(dialectClass))


Should we use equals given that MongoDialect is final?

stIncMale requested a review from vbabanin November 12, 2025 16:33

stIncMale self-assigned this Nov 12, 2025

stIncMale commented Nov 12, 2025

View reviewed changes

stIncMale changed the title ~~Make sure our extension does not interfere when bootstrapping is not related to it~~ Make sure our extension does not interfere when bootstrapping unrelated to it Nov 12, 2025

stIncMale commented Nov 12, 2025

View reviewed changes

stIncMale force-pushed the HIBERNATE-135 branch from c2269ef to 76835a8 Compare November 12, 2025 21:59

Make sure our extension does not interfere when bootstrapping unrelat…

9db8582

…ed to it HIBERNATE-135

stIncMale force-pushed the HIBERNATE-135 branch from 76835a8 to 9db8582 Compare November 13, 2025 00:53

stIncMale marked this pull request as ready for review November 13, 2025 00:54

stIncMale requested a review from a team as a code owner November 13, 2025 00:54

stIncMale changed the title ~~Make sure our extension does not interfere when bootstrapping unrelated to it~~ Make sure our extension does not interfere when bootstrapping is unrelated to it Nov 18, 2025

vbabanin reviewed Nov 19, 2025

View reviewed changes

src/integrationTest/java/com/mongodb/hibernate/boot/NativeBootstrappingIntegrationTests.java Outdated Show resolved Hide resolved

Remove unnecessary @SuppressWarnings("try")

9d11a0b

HIBERNATE-135

stIncMale requested a review from vbabanin November 19, 2025 08:11

Throw meaningful exceptions if either our dialect or connection provi…

f7dbe58

…der is not plugged in HIBERNATE-135

stIncMale commented Nov 21, 2025

View reviewed changes

Use static imports

6e18954

HIBERNATE-135

vbabanin reviewed Nov 29, 2025

View reviewed changes

Merge branch 'main' into HIBERNATE-135

a9837ed

	&& dialectName.startsWith("com.mongodb.hibernate")))) {
	&& dialectName.equals(MongoDialect.class.getName())))) {

Make sure our extension does not interfere when bootstrapping is unrelated to it #146

Are you sure you want to change the base?

Make sure our extension does not interfere when bootstrapping is unrelated to it #146

Uh oh!

Conversation

stIncMale commented Nov 12, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vbabanin commented Nov 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stIncMale commented Nov 21, 2025

Uh oh!

stIncMale Nov 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stIncMale Nov 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

vbabanin commented Nov 19, 2025 •

edited

Loading

stIncMale Nov 21, 2025 •

edited

Loading

stIncMale Nov 21, 2025 •

edited

Loading