-
Notifications
You must be signed in to change notification settings - Fork 13
Add internal docs and do other improvements #80
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 7 commits
36b51cd
11a6545
8d47b4d
02cdb67
1fc2c92
2678e97
278b519
f83a1df
94b6169
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 |
|---|---|---|
|
|
@@ -151,6 +151,9 @@ | |
| * <p>For the documentation on the supported <a | ||
| * href="https://docs.jboss.org/hibernate/orm/6.6/userguide/html_single/Hibernate_User_Guide.html#hql-exp-functions">HQL | ||
| * functions</a> see {@link #initializeFunctionRegistry(FunctionContributions)}. | ||
| * | ||
| * @mongoCme Must be immutable, as per the documentation of {@link Dialect}. It is unclear whether it should be | ||
| * shallowly or deeply immutable; most likely—shallowly. | ||
| */ | ||
| @Sealed | ||
| public class MongoDialect extends Dialect { | ||
|
|
@@ -196,7 +199,7 @@ protected void checkVersion() { | |
|
|
||
| @Override | ||
| public SqlAstTranslatorFactory getSqlAstTranslatorFactory() { | ||
| return new MongoTranslatorFactory(); | ||
| return MongoTranslatorFactory.INSTANCE; | ||
|
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. This way we work around the issue fixed in hibernate/hibernate-orm#9900, because we are not getting that fix in Hibernate ORM 6.x. 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. Thanks for your raising the issue and it is a quick fix. On the other hand, it seems trivial to me for we only avoid one unnecessary duplicated object creation (which usually is not a big deal for the creation is not costly). 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. The other id generator defect is more important and hard to fix from Hibernate side. I created a PR at hibernate/hibernate-orm#9965. Let us see. 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. The PR @NathanQingyangXu mentioned was superseded with hibernate/hibernate-orm#9969, and the fix was released in 6.6.14. |
||
| } | ||
|
|
||
| @Override | ||
|
|
@@ -350,6 +353,7 @@ public void initializeFunctionRegistry(FunctionContributions functionContributio | |
| functionRegistry.register("array_includes_nullable", new MongoArrayIncludesFunction(true, typeConfiguration)); | ||
| } | ||
|
|
||
| /** @mongoCme The {@link MutationOperation} returned from this method does not have to be thread-safe. */ | ||
| @Override | ||
| public MutationOperation createOptionalTableUpdateOperation( | ||
| EntityMutationTarget mutationTarget, | ||
|
|
@@ -372,6 +376,7 @@ public void appendDatetimeFormat(SqlAppender appender, String format) { | |
| throw new FeatureNotSupportedException("TODO-HIBERNATE-88 https://jira.mongodb.org/browse/HIBERNATE-88"); | ||
| } | ||
|
|
||
| /** @mongoCme The {@link SQLExceptionConversionDelegate} returned from this method must be thread-safe. */ | ||
| @Override | ||
| public SQLExceptionConversionDelegate buildSQLExceptionConversionDelegate() { | ||
| return (sqlException, exceptionMessage, mql) -> new JDBCException(exceptionMessage, sqlException, mql); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -33,13 +33,17 @@ | |
| import java.util.Map; | ||
| import java.util.Set; | ||
| import org.hibernate.HibernateException; | ||
| import org.hibernate.boot.registry.BootstrapServiceRegistry; | ||
| import org.hibernate.boot.registry.StandardServiceInitiator; | ||
| import org.hibernate.boot.registry.StandardServiceRegistry; | ||
| import org.hibernate.boot.registry.StandardServiceRegistryBuilder; | ||
| import org.hibernate.service.Service; | ||
| import org.hibernate.service.UnknownServiceException; | ||
| import org.hibernate.service.spi.ServiceRegistryImplementor; | ||
| import org.jspecify.annotations.Nullable; | ||
|
|
||
| /** @mongoCme Thread-safe. */ | ||
|
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. In most other places I said "Must be thread-safe", but here it is just "Thread-safe". The difference is that here the behavior is something we provide based on what we need. In other places we must provide thread-safety because of the Hibernate ORM API parts we implement/extend. |
||
| @SuppressWarnings("MissingSummary") | ||
| public final class StandardServiceRegistryScopedState implements Service { | ||
| @Serial | ||
| private static final long serialVersionUID = 1L; | ||
|
|
@@ -61,17 +65,30 @@ private void writeObject(ObjectOutputStream out) throws IOException { | |
| "This class is not designed to be serialized despite it having to implement `Serializable`"); | ||
| } | ||
|
|
||
| /** | ||
| * @mongoCme The instance methods of {@link org.hibernate.service.spi.ServiceContributor} are called multiple times | ||
| * if multiple {@link StandardServiceRegistry} instances are {@linkplain StandardServiceRegistryBuilder#build() | ||
| * built} using the same {@link BootstrapServiceRegistry}. | ||
| */ | ||
|
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. again, I failed to see why multiple 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. I don't think that matters. What does matter is that doing so is possible with the API of Hibernate ORM, and it is not forbidden by the API documentation, so we must take that into account when we are extending Hibernate ORM. 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. that is why I got confused. Yeah, it is not forbidden, but why we need to cover it? I never know of such scenario that we need to create multiple 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. So from my understanding, Hibernate's If multiple During runtime, one From my understanding, one and only one 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. The above comment also explains why I got puzzled by your previous PR; but you modified it to make it aligned with reality so I approved that. |
||
| public static final class ServiceContributor implements org.hibernate.service.spi.ServiceContributor { | ||
| public ServiceContributor() {} | ||
|
|
||
| @Override | ||
| public void contribute(StandardServiceRegistryBuilder serviceRegistryBuilder) { | ||
| serviceRegistryBuilder.addInitiator(new StandardServiceInitiator<StandardServiceRegistryScopedState>() { | ||
| /** | ||
| * @mongoCme This method may be called multiple times when | ||
| * {@linkplain StandardServiceRegistryBuilder#build() building} a single | ||
| * {@link StandardServiceRegistry} instance. | ||
| */ | ||
| @Override | ||
| public Class<StandardServiceRegistryScopedState> getServiceInitiated() { | ||
| return StandardServiceRegistryScopedState.class; | ||
| } | ||
|
|
||
| /** | ||
| * @mongoCme This method is called not more than once per instance of {@link StandardServiceInitiator}. | ||
| */ | ||
| @Override | ||
| public StandardServiceRegistryScopedState initiateService( | ||
| Map<String, Object> configurationValues, ServiceRegistryImplementor serviceRegistry) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -24,40 +24,33 @@ | |
| import org.hibernate.engine.spi.SharedSessionContractImplementor; | ||
| import org.hibernate.generator.BeforeExecutionGenerator; | ||
| import org.hibernate.generator.EventType; | ||
| import org.hibernate.generator.Generator; | ||
| import org.hibernate.generator.GeneratorCreationContext; | ||
| import org.hibernate.id.factory.spi.CustomIdGeneratorCreationContext; | ||
| import org.jspecify.annotations.Nullable; | ||
|
|
||
| /** | ||
| * Tread-safe. | ||
| * | ||
| * @see com.mongodb.hibernate.annotations.ObjectIdGenerator | ||
| * @mongoCme Must be thread-safe. | ||
| */ | ||
| @SuppressWarnings("MissingSummary") | ||
| public final class ObjectIdGenerator implements BeforeExecutionGenerator { | ||
| @Serial | ||
| private static final long serialVersionUID = 1L; | ||
|
|
||
| private static final org.bson.codecs.ObjectIdGenerator GENERATOR = new org.bson.codecs.ObjectIdGenerator(); | ||
|
|
||
| private final boolean forIdentifier; | ||
|
|
||
| /** @see Generator */ | ||
| public ObjectIdGenerator( | ||
| com.mongodb.hibernate.annotations.ObjectIdGenerator config, | ||
| Member annotatedMember, | ||
| CustomIdGeneratorCreationContext context) { | ||
| this(true); | ||
| } | ||
| CustomIdGeneratorCreationContext context) {} | ||
|
|
||
| /** @see Generator */ | ||
| public ObjectIdGenerator( | ||
| com.mongodb.hibernate.annotations.ObjectIdGenerator config, | ||
| Member annotatedMember, | ||
| GeneratorCreationContext context) { | ||
| this(false); | ||
| } | ||
|
|
||
| private ObjectIdGenerator(boolean forIdentifier) { | ||
| this.forIdentifier = forIdentifier; | ||
| } | ||
| GeneratorCreationContext context) {} | ||
|
|
||
| @Override | ||
| public Object generate( | ||
|
|
@@ -67,13 +60,6 @@ public Object generate( | |
| EventType eventType) { | ||
| if (currentValue != null) { | ||
| return currentValue; | ||
| } else if (forIdentifier) { | ||
| // Hibernate ORM provides `null` as `currentValue` when generating an entity identifier value. | ||
| // To work around that behavior we have to read the value explicitly. | ||
| var currentId = session.getEntityPersister(null, owner).getIdentifier(owner, session); | ||
| if (currentId != null) { | ||
| return currentId; | ||
| } | ||
|
Comment on lines
-70
to
-76
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. We don't need this workaround since https://hibernate.atlassian.net/browse/HHH-19320 was fixed in 6.6.14. The fix indeed works:
@NathanQingyangXu, thank you for reporting the aforementioned bug to Hibernate ORM. |
||
| } | ||
| return GENERATOR.generate(); | ||
| } | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
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.
I propose using a
javadoctag, which allows us to express behavior related to thread-safety, mutability, and execution overall in a free form, as opposed to the@ThreadSafeuniform approach used in the driver. This is because the reality is often more subtle than@ThreadSafe/@NotThreadSafe, and it is useful to be able to express it while having thejavadocformatting at hand.The name of the tag is currently
@mongoCme, but I am open to other suggestions.