-
Notifications
You must be signed in to change notification settings - Fork 13
Make sure our extension does not interfere when bootstrapping is unrelated to it #146
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
base: main
Are you sure you want to change the base?
Changes from all commits
9db8582
9d11a0b
f7dbe58
6e18954
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -36,9 +36,9 @@ void testSuccess() { | |
|
|
||
| @Test | ||
| void testInvalidConnectionString() { | ||
| assertThrows(ServiceException.class, () -> buildSessionFactory( | ||
| Map.of(JAKARTA_JDBC_URL, "jdbc:postgresql://localhost/test")) | ||
| .close()); | ||
| assertThrows( | ||
| ServiceException.class, () -> buildSessionFactory(Map.of(JAKARTA_JDBC_URL, "jdbc:postgresql://host/")) | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Changed the ULR such that there is obviously no chance in practice to successfully connect using it. |
||
| .close()); | ||
| } | ||
|
|
||
| @Test | ||
|
|
@@ -60,12 +60,10 @@ private static SessionFactory buildSessionFactory() throws ServiceException { | |
| } | ||
|
|
||
| private static SessionFactory buildSessionFactory(Map<String, Object> settings) throws ServiceException { | ||
| var standardServiceRegistry = | ||
| new StandardServiceRegistryBuilder().applySettings(settings).build(); | ||
| return new MetadataSources(standardServiceRegistry) | ||
| .getMetadataBuilder() | ||
| .build() | ||
| .getSessionFactoryBuilder() | ||
| .build(); | ||
| return new MetadataSources() | ||
| .buildMetadata(new StandardServiceRegistryBuilder() | ||
| .applySettings(settings) | ||
| .build()) | ||
| .buildSessionFactory(); | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A drive-by simplification. |
||
| } | ||
| } | ||
This file was deleted.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,6 +16,7 @@ | |
|
|
||
| package com.mongodb.hibernate.boot; | ||
|
|
||
| import static com.mongodb.hibernate.BasicCrudIntegrationTests.Item.COLLECTION_NAME; | ||
| import static org.assertj.core.api.Assertions.assertThat; | ||
|
|
||
| import com.mongodb.client.MongoCollection; | ||
|
|
@@ -41,18 +42,12 @@ | |
| annotatedClasses = JakartaPersistenceBootstrappingIntegrationTests.Item.class) | ||
| @ExtendWith(MongoExtension.class) | ||
| class JakartaPersistenceBootstrappingIntegrationTests { | ||
| private static final String COLLECTION_NAME = "items"; | ||
|
|
||
| @InjectMongoCollection(COLLECTION_NAME) | ||
| private static MongoCollection<BsonDocument> mongoCollection; | ||
|
|
||
| @Test | ||
| void smoke(EntityManagerFactoryScope scope) { | ||
| scope.inTransaction(em -> { | ||
| var item = new Item(); | ||
| item.id = 1; | ||
| em.persist(item); | ||
| }); | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Only drive-by changes here to make the test code simpler. |
||
| scope.inTransaction(em -> em.persist(new Item(1))); | ||
| assertThat(mongoCollection.find()).containsExactly(BsonDocument.parse("{_id: 1}")); | ||
| } | ||
|
|
||
|
|
@@ -61,5 +56,9 @@ void smoke(EntityManagerFactoryScope scope) { | |
| static class Item { | ||
| @Id | ||
| int id; | ||
|
|
||
| Item(int id) { | ||
| this.id = id; | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,56 @@ | ||
| /* | ||
| * Copyright 2024-present MongoDB, Inc. | ||
| * | ||
| * Licensed under the Apache License, Version 2.0 (the "License"); | ||
| * you may not use this file except in compliance with the License. | ||
| * You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| */ | ||
|
|
||
| package com.mongodb.hibernate.boot; | ||
|
|
||
| import static org.assertj.core.api.Assertions.assertThatThrownBy; | ||
|
|
||
| import com.mongodb.client.MongoClient; | ||
| import com.mongodb.hibernate.junit.InjectMongoClient; | ||
| import com.mongodb.hibernate.junit.MongoExtension; | ||
| import org.bson.BsonDocument; | ||
| import org.hibernate.boot.MetadataSources; | ||
| import org.junit.jupiter.api.Test; | ||
| import org.junit.jupiter.api.extension.ExtendWith; | ||
|
|
||
| @ExtendWith(MongoExtension.class) | ||
| class NativeBootstrappingIntegrationTests { | ||
| @InjectMongoClient | ||
| private static MongoClient mongoClient; | ||
|
|
||
| @Test | ||
| void testCouldNotInstantiateDialectExceptionMessage() { | ||
| assertThatThrownBy(() -> { | ||
| BsonDocument failPointCommand = BsonDocument.parse( | ||
| """ | ||
| { | ||
| "configureFailPoint": "failCommand", | ||
| "mode": { | ||
| "times": 1 | ||
| }, | ||
| "data": { | ||
| "failCommands": ["buildInfo"], | ||
| "errorCode": 1 | ||
| } | ||
| } | ||
| """); | ||
| mongoClient.getDatabase("admin").runCommand(failPointCommand); | ||
| new MetadataSources().buildMetadata(); | ||
| }) | ||
| .hasRootCauseMessage( | ||
| "Could not instantiate [com.mongodb.hibernate.dialect.MongoDialect], see the earlier exceptions to find out why"); | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,7 +14,7 @@ | |
| * limitations under the License. | ||
| */ | ||
|
|
||
| package com.mongodb.hibernate.internal.extension; | ||
| package com.mongodb.hibernate.internal.boot; | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
|
|
||
| import static com.mongodb.hibernate.internal.MongoAssertions.assertFalse; | ||
| import static com.mongodb.hibernate.internal.MongoAssertions.assertInstanceOf; | ||
|
|
@@ -23,6 +23,7 @@ | |
| import static com.mongodb.hibernate.internal.MongoConstants.MONGO_DBMS_NAME; | ||
| import static java.lang.String.format; | ||
|
|
||
| import com.mongodb.hibernate.dialect.MongoDialect; | ||
| import com.mongodb.hibernate.internal.FeatureNotSupportedException; | ||
| import jakarta.persistence.Embeddable; | ||
| import java.sql.Time; | ||
|
|
@@ -83,6 +84,10 @@ public void contribute( | |
| InFlightMetadataCollector metadata, | ||
| ResourceStreamLocator resourceStreamLocator, | ||
| MetadataBuildingContext buildingContext) { | ||
| if (!(metadata.getDatabase().getDialect() instanceof MongoDialect)) { | ||
| // avoid interfering with bootstrapping unrelated to the MongoDB Extension for Hibernate ORM | ||
| return; | ||
| } | ||
| forbidEmbeddablesWithoutPersistentAttributes(metadata); | ||
| metadata.getEntityBindings().forEach(persistentClass -> { | ||
| checkPropertyTypes(persistentClass); | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -14,17 +14,20 @@ | |||||
| * limitations under the License. | ||||||
| */ | ||||||
|
|
||||||
| package com.mongodb.hibernate.internal.extension.service; | ||||||
| package com.mongodb.hibernate.internal.service; | ||||||
|
|
||||||
| import static com.mongodb.hibernate.internal.VisibleForTesting.AccessModifier.PRIVATE; | ||||||
| import static java.lang.String.format; | ||||||
| import static org.hibernate.cfg.AvailableSettings.DIALECT_RESOLVERS; | ||||||
| import static org.hibernate.cfg.AvailableSettings.JAKARTA_JDBC_URL; | ||||||
| import static org.hibernate.cfg.AvailableSettings.JAVA_TIME_USE_DIRECT_JDBC; | ||||||
| import static org.hibernate.cfg.AvailableSettings.PREFERRED_INSTANT_JDBC_TYPE; | ||||||
|
|
||||||
| import com.mongodb.hibernate.dialect.MongoDialect; | ||||||
| import com.mongodb.hibernate.internal.VisibleForTesting; | ||||||
| import com.mongodb.hibernate.internal.cfg.MongoConfiguration; | ||||||
| import com.mongodb.hibernate.internal.cfg.MongoConfigurationBuilder; | ||||||
| import com.mongodb.hibernate.jdbc.MongoConnectionProvider; | ||||||
| import com.mongodb.hibernate.service.spi.MongoConfigurationContributor; | ||||||
| import java.io.IOException; | ||||||
| import java.io.NotSerializableException; | ||||||
|
|
@@ -35,6 +38,8 @@ | |||||
| import org.hibernate.HibernateException; | ||||||
| import org.hibernate.boot.registry.StandardServiceInitiator; | ||||||
| import org.hibernate.boot.registry.StandardServiceRegistryBuilder; | ||||||
| import org.hibernate.cfg.AvailableSettings; | ||||||
| import org.hibernate.engine.jdbc.dialect.spi.DialectFactory; | ||||||
| import org.hibernate.service.Service; | ||||||
| import org.hibernate.service.UnknownServiceException; | ||||||
| import org.hibernate.service.spi.ServiceRegistryImplementor; | ||||||
|
|
@@ -75,17 +80,52 @@ public Class<StandardServiceRegistryScopedState> getServiceInitiated() { | |||||
| @Override | ||||||
| public StandardServiceRegistryScopedState initiateService( | ||||||
| Map<String, Object> configurationValues, ServiceRegistryImplementor serviceRegistry) { | ||||||
| checkMongoDialectIsPluggedIn(configurationValues, serviceRegistry); | ||||||
| checkMongoConnectionProviderIsPluggedIn(configurationValues); | ||||||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Originally I wanted to put the
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. One possible option could be to require a Given that, i agree that |
||||||
| return new StandardServiceRegistryScopedState( | ||||||
| createMongoConfiguration(configurationValues, serviceRegistry)); | ||||||
| } | ||||||
| }); | ||||||
| } | ||||||
|
|
||||||
| private MongoConfiguration createMongoConfiguration( | ||||||
| private static void checkMongoDialectIsPluggedIn( | ||||||
| Map<String, Object> configurationValues, ServiceRegistryImplementor serviceRegistry) { | ||||||
| var dialectFactory = serviceRegistry.getService(DialectFactory.class); | ||||||
| if ((dialectFactory == null | ||||||
| || dialectFactory.getClass().getPackageName().startsWith("org.hibernate")) | ||||||
| && configurationValues.get(DIALECT_RESOLVERS) == null) { | ||||||
| // If `DialectFactory` is different from the ones Hibernate ORM provides, or if | ||||||
| // `DIALECT_RESOLVERS` is specified, then we cannot detect whether `MongoDialect` is plugged in. | ||||||
| // Otherwise, we know that `DIALECT` is the only way to plug `MongoDialect` in, | ||||||
| // and we can detect whether it is plugged in. | ||||||
| var dialect = configurationValues.get(AvailableSettings.DIALECT); | ||||||
| if (!((dialect instanceof MongoDialect) | ||||||
| || (dialect instanceof Class<?> dialectClass | ||||||
| && MongoDialect.class.isAssignableFrom(dialectClass)) | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we use |
||||||
| || (dialect instanceof String dialectName | ||||||
| && dialectName.startsWith("com.mongodb.hibernate")))) { | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we use an exact name match here, given that we currently support only a single dialect?
Suggested change
|
||||||
| throw new RuntimeException("%s must be plugged in, for example, via the [%s] configuration property" | ||||||
| .formatted(MongoDialect.class.getName(), AvailableSettings.DIALECT)); | ||||||
| } | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| private static void checkMongoConnectionProviderIsPluggedIn(Map<String, Object> configurationValues) { | ||||||
| var connectionProvider = configurationValues.get(AvailableSettings.CONNECTION_PROVIDER); | ||||||
| if (!((connectionProvider instanceof MongoConnectionProvider) | ||||||
| || (connectionProvider instanceof Class<?> connectionProviderClass | ||||||
| && MongoConnectionProvider.class.isAssignableFrom(connectionProviderClass)) | ||||||
| || (connectionProvider instanceof String connectionProviderName | ||||||
| && connectionProviderName.startsWith("com.mongodb.hibernate")))) { | ||||||
| throw new RuntimeException("%s must be plugged in, for example, via the [%s] configuration property" | ||||||
| .formatted(MongoConnectionProvider.class.getName(), AvailableSettings.CONNECTION_PROVIDER)); | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| private static MongoConfiguration createMongoConfiguration( | ||||||
| Map<String, Object> configurationValues, ServiceRegistryImplementor serviceRegistry) { | ||||||
| var jdbcUrl = configurationValues.get(JAKARTA_JDBC_URL); | ||||||
| MongoConfigurationContributor mongoConfigurationContributor = | ||||||
| getMongoConfigurationContributor(serviceRegistry); | ||||||
| var mongoConfigurationContributor = getMongoConfigurationContributor(serviceRegistry); | ||||||
| if (jdbcUrl == null && mongoConfigurationContributor == null) { | ||||||
| throw new HibernateException(format( | ||||||
| "Configuration property [%s] is required unless %s is provided", | ||||||
|
|
@@ -99,7 +139,7 @@ private MongoConfiguration createMongoConfiguration( | |||||
| return mongoConfigurationBuilder.build(); | ||||||
| } | ||||||
|
|
||||||
| private @Nullable MongoConfigurationContributor getMongoConfigurationContributor( | ||||||
| private static @Nullable MongoConfigurationContributor getMongoConfigurationContributor( | ||||||
| ServiceRegistryImplementor serviceRegistry) { | ||||||
| MongoConfigurationContributor result = null; | ||||||
| try { | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -25,7 +25,7 @@ | |
| import com.mongodb.client.MongoClients; | ||
| import com.mongodb.hibernate.internal.BuildConfig; | ||
| import com.mongodb.hibernate.internal.VisibleForTesting; | ||
| import com.mongodb.hibernate.internal.extension.service.StandardServiceRegistryScopedState; | ||
| import com.mongodb.hibernate.internal.service.StandardServiceRegistryScopedState; | ||
| import java.io.IOException; | ||
| import java.io.NotSerializableException; | ||
| import java.io.ObjectOutputStream; | ||
|
|
@@ -58,6 +58,8 @@ public final class MongoConnectionProvider implements ConnectionProvider, Stoppa | |
| private @Nullable StandardServiceRegistryScopedState standardServiceRegistryScopedState; | ||
| private transient @Nullable MongoClient mongoClient; | ||
|
|
||
| public MongoConnectionProvider() {} | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Made the constructor used by Hibernate ORM explicit, like we do in other places. |
||
|
|
||
| @Override | ||
| public Connection getConnection() throws SQLException { | ||
| try { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test classes should have the "Tests" suffix.