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.

Sign up for GitHub

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 internal docs and do other improvements #80

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Add internal docs and do other improvements #80

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
36b51cd
Add internal docs and do other improvements
stIncMale Mar 26, 2025
11a6545
Document that `MongoTranslatorFactory` is thread-safe.
stIncMale Mar 27, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/main/java/com/mongodb/hibernate/dialect/MongoDialect.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,6 @@ protected DatabaseVersion getMinimumSupportedVersion() {

@Override
public SqlAstTranslatorFactory getSqlAstTranslatorFactory() {
return new MongoTranslatorFactory();
return MongoTranslatorFactory.INSTANCE;
Copy link
Member Author

Choose a reason for hiding this comment

The 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.

Copy link
Contributor

Choose a reason for hiding this comment

The 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).

Copy link
Contributor

Choose a reason for hiding this comment

The 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.

}
}
9 changes: 9 additions & 0 deletions ...main/java/com/mongodb/hibernate/internal/extension/MongoAdditionalMappingContributor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,13 +23,22 @@

import com.mongodb.hibernate.internal.FeatureNotSupportedException;
import org.hibernate.annotations.DynamicInsert;
import org.hibernate.boot.Metadata;
import org.hibernate.boot.MetadataSources;
import org.hibernate.boot.ResourceStreamLocator;
import org.hibernate.boot.registry.BootstrapServiceRegistry;
import org.hibernate.boot.spi.AdditionalMappingContributions;
import org.hibernate.boot.spi.AdditionalMappingContributor;
import org.hibernate.boot.spi.InFlightMetadataCollector;
import org.hibernate.boot.spi.MetadataBuildingContext;
import org.hibernate.mapping.PersistentClass;

/**
* The instance methods like {@link #getContributorName()}, {@link #contribute(AdditionalMappingContributions,
* InFlightMetadataCollector, ResourceStreamLocator, MetadataBuildingContext)} are called multiple times if multiple
* {@link Metadata} instances are {@linkplain MetadataSources#buildMetadata() built} using the same
* {@link BootstrapServiceRegistry}.
Copy link
Contributor

Choose a reason for hiding this comment

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

This Javadoc is visible to public even though it is within internal package, right?

To my best knowledge, I don't expect any Hibernate contributor methods would be called more than once in one SessionFactory. So the above doc and similar ones in this PR really confuse me.

I'd thought you plan to put down such doc outside our code. Putting it here seems to hurt our product for they might clarify some doubts to us but would confuse others (including me).

Copy link
Member Author

Choose a reason for hiding this comment

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

This Javadoc is visible to public even though it is within internal package, right?

Yes, our whole codebase is publicly visible.

To my best knowledge, I don't expect any Hibernate contributor methods would be called more than once in one SessionFactory.

SessionFactory has nothing to do with the invocations of MongoAdditionalMappingContributor.contribute. This method is called neither by SessionFactory, nor even when a SessionFactory instance is built by the SessionFactoryBuilder.build method.

Putting it here seems to hurt our product for they might clarify some doubts to us but would confuse others (including me).

  • This is not our public API documentation, because the documented class is not part of our API. Thus, it does not apply to "others".
  • What do you mean exactly by "confuse"? (Does the proposed documentation say something that is false? Does it make one to believe something that is false because it is ill-formulated?)

Copy link
Contributor

Choose a reason for hiding this comment

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

Why we need to build multiple Metadata? Yeah, Hibernate doesn't disallow, but I dont' understand why we need to consider this case. From my best knowledge, such contributor only needs to be called once.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is not our public API documentation, because the documented class is not part of our API. Thus, it does not apply to "others".

Given our codebase is public, there is still possibility that the doc could be read by others (e.g. community contributor or Hibernate developer's browsing out of curiosity). If the project is private, I would have no such concern.

Copy link
Contributor

Choose a reason for hiding this comment

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

What do you mean exactly by "confuse"? (Does the proposed documentation say something that is false? Does it make one to believe something that is false because it is ill-formulated?)

Let me elaborate on why I got confused.

Hibernate's contributor is to be created by default constructor (via JDK's ServiceLoader mechanism or ClassLoaderService in Hibernate), so it is a typical one-use object in the sense that once it is loaded (or created), its contribute() method is invoked and then that is it and the contributor object is subject to garbage collecting. Next time a new instance will be created and loaded, but I don't think it will be reused more than once.

Such contributor hooking point is hard-coded in Hibernate code as below:


		final Collection<AdditionalMappingContributor> additionalMappingContributors = classLoaderService.loadJavaServices( AdditionalMappingContributor.class );
		additionalMappingContributors.forEach( (contributor) -> {
			contributions.setCurrentContributor( contributor.getContributorName() );
			try {
				contributor.contribute(
						contributions,
						metadataCollector,
						classLoaderService,
						rootMetadataBuildingContext
				);
			}
			finally {
				contributions.setCurrentContributor( null );
			}
		} );

As you can see, for each contributor, its two methods will be called only once after it is loaded or created. So it seems a typical one-use scenario as our various mql translators.

*/
public final class MongoAdditionalMappingContributor implements AdditionalMappingContributor {

public MongoAdditionalMappingContributor() {}
Expand Down
7 changes: 7 additions & 0 deletions .../com/mongodb/hibernate/internal/extension/service/StandardServiceRegistryScopedState.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,9 @@
import java.io.Serial;
import java.util.Map;
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;
Expand All @@ -49,6 +51,11 @@ public MongoConfiguration getConfiguration() {
return config;
}

/**
* The instance methods like {@link #contribute(StandardServiceRegistryBuilder)} are called multiple times if
* multiple {@link StandardServiceRegistry} instances are {@linkplain StandardServiceRegistryBuilder#build() built}
* using the same {@link BootstrapServiceRegistry}.
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

again, I failed to see why multiple StandardServiceRegistry need to be build from one BootstrapServiceRegistry. Could we move the confusing doc out of the code base?

Copy link
Member Author

Choose a reason for hiding this comment

The 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.

Copy link
Contributor

Choose a reason for hiding this comment

The 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 StandardServiceRegistry, which seems to invite trouble.

Copy link
Contributor

@NathanQingyangXu NathanQingyangXu Apr 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

So from my understanding, Hibernate's SessionFactory is pretty heavy-weighted (it is super slow for it tries to prepare everything so runtime perf could be as fast as possible; one example is it tries to finish static SQL translations as much as possible so they could be reused). For that reason, for typical usage, it is seldom created multiple times (in typical Spring JPA usage, one single SessionFactory is bound to the global Spring Bean Context), unless there are good reasons.

If multiple SessionFactory is justified (maybe each one uses a subset of the META-INF/services so they could share the same jar library without conflict; then it requires customized BootstrapServiceRegistry, one of whose components is the ClassLoaderService which could be customized), different BootstrapServiceRegistry is expected to be created for each of them. Different BootstrapServiceRegistry leads to different StandardServiceRegistry (as explained above, maybe due to different class loaders), otherwise why create different SessionFactory in the first place?

During runtime, one SessionFactory has one single ServiceRegistry and its default implementation is StandardServiceRegistry as its API below (in org.hibernate.engine.spi.SessionFactoryImplementor):

ServiceRegistryImplementor getServiceRegistry();

From my understanding, one and only one StandardServiceRegistry is needed, and StandardServiceRegistryBuilder is the de-factor one-time stuff. When it has finished building, it seems only in theory that it could be invoked again (why? one StandardServiceRegistry is not enough?). That is what puzzles me. Yeah, it is possible but why emphasize such fact in Javadoc (arguably not internal doc for it is likely that community contributor needs to read it to figure out something).

Copy link
Contributor

Choose a reason for hiding this comment

The 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() {}

Expand Down
7 changes: 7 additions & 0 deletions src/main/java/com/mongodb/hibernate/internal/translate/MongoTranslatorFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,14 @@
import org.hibernate.sql.model.ast.TableMutation;
import org.hibernate.sql.model.jdbc.JdbcMutationOperation;

/**
* Tread-safe.
*/
public final class MongoTranslatorFactory implements SqlAstTranslatorFactory {
public static MongoTranslatorFactory INSTANCE = new MongoTranslatorFactory();

private MongoTranslatorFactory() {}

@Override
public SqlAstTranslator<JdbcOperationQuerySelect> buildSelectTranslator(
SessionFactoryImplementor sessionFactoryImplementor, SelectStatement selectStatement) {
Expand Down