Skip to content

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.

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

Open
wants to merge 9 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 2 commits
Commits
Show all changes
9 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
8d47b4d
Merge branch 'main' into addInternalDocsAndDoOtherImprovements
stIncMale Nov 4, 2025
02cdb67
Remove our workaround for HHH-19320
stIncMale Nov 4, 2025
1fc2c92
Refine the docs on `MongoAdditionalMappingContributor`, `StandardServ…
stIncMale Nov 4, 2025
2678e97
Add the `@mongoCme` `javadoc` tag
stIncMale Nov 5, 2025
278b519
Delete `ConcurrencyMutabilityExecutionTaglet`
stIncMale Nov 5, 2025
f83a1df
Add the missing `.` in the `internal` packages docs, also add som mis…
stIncMale Nov 5, 2025
94b6169
Improve the `@mongoCme` documentation on `AbstractMqlTranslator` and …
stIncMale Nov 5, 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

@stIncMale stIncMale Mar 26, 2025

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

@NathanQingyangXu NathanQingyangXu Apr 6, 2025

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

@NathanQingyangXu NathanQingyangXu Apr 6, 2025

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.

Copy link
Member Author

@stIncMale stIncMale Nov 4, 2025

Choose a reason for hiding this comment

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

}
}
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

@NathanQingyangXu NathanQingyangXu Mar 27, 2025

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

@stIncMale stIncMale Apr 1, 2025

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

@NathanQingyangXu NathanQingyangXu Apr 6, 2025

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

@NathanQingyangXu NathanQingyangXu Apr 6, 2025

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

@NathanQingyangXu NathanQingyangXu Apr 6, 2025

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.

Copy link
Member

@vbabanin vbabanin Apr 18, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Next time a new instance will be created and loaded, but I don't think it will be reused more than once.

JDK ServiceLoader does perform some caching, as described in the "Timing of provider discovery" section of the official documentation.

Hibernate also caches loaded services and reuses the same instance of MongoAdditionalMappingContributor in the same BootstrapServiceRegistry. Relevant code snippet from AggregatedServiceLoader:

@Override
public Collection<S> getAll() {
    if ( cache == null ) {
        /*
         * loadAll() uses ServiceLoader.Provider.get(), which doesn't cache the instance internally,
         * unlike ServiceLoader.iterator().
         * Looking at how https://hibernate.atlassian.net/browse/HHH-8363 was solved,
         * waiting for Hibernate ORM to shut down before clearing the service caches,
         * it seems caching of service instances is important, or at least used to be important.
         * Thus we cache service instances ourselves to avoid any kind of backward-incompatibility.
         * If one day we decide caching isn't important, this cache can be removed...
         */
        cache = Collections.unmodifiableCollection( loadAll() );
    }
    return cache;
}

As long as it reflects how things actually work and isn't misleading, which is not in this case - I think there is no harm in documenting it.

Copy link
Contributor

@NathanQingyangXu NathanQingyangXu Apr 19, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Good point and food for thoughts.
I reconsidered the specific service provider in question, i.e. MongoAdditionalMappingContributor. Yeah, once it is loaded into cache the same service provider could be reused later. That is something new I got from your feedback and I appreciate it.

However, from my knowledge, for one specific SessionFactory, all the AdditionalMappingContributor service provider instances would be ONLY used for once. Suppose they (we could create multiple AdditionalMappingContributor service providers per good practice; currently we reuse MongoAdditionalMappingContributor for all concerns, including _id hardcoding and supportability checking) are cached internally, the cached providers would not have opportunity to be reused again.

AggregatedServiceLoader has internal cache but again, we don't reuse AdditionalMappingContributor for one Hibernate bootstrapping, so whether its service providers are cached or not seems not important for our concern.

*/
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

@NathanQingyangXu NathanQingyangXu Mar 27, 2025

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

@stIncMale stIncMale Apr 1, 2025

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

@NathanQingyangXu NathanQingyangXu Apr 6, 2025

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

@NathanQingyangXu NathanQingyangXu Apr 6, 2025

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.
Copy link
Member

@vbabanin vbabanin Apr 19, 2025

Choose a reason for hiding this comment

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

Suggested change
* Tread-safe.
* Thread-safe.

Copy link
Member Author

@stIncMale stIncMale Nov 5, 2025

Choose a reason for hiding this comment

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

Done.

*/
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