Add internal docs and do other improvements #80
Conversation
| @@ -46,6 +46,6 @@ protected DatabaseVersion getMinimumSupportedVersion() { | |||
|
|
|||
| @Override | |||
| public SqlAstTranslatorFactory getSqlAstTranslatorFactory() { | |||
| return new MongoTranslatorFactory(); | |||
| return MongoTranslatorFactory.INSTANCE; | |||
There was a problem hiding this comment.
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.
| @@ -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 { | |||
There was a problem hiding this comment.
Hibernate ORM does not document that implementations of SqlAstTranslatorFactory must be thread-safe, so we should document that for our MongoTranslatorFactory.
There was a problem hiding this comment.
For such bootstrapping stuff, usually no need to sepcify type safety for everything will be done sequentially with well-defined dependency or order. There is no need of concurrency for bootstrapping only happens once and its perf could be compromised a little bit.
There was a problem hiding this comment.
sorry, I made a mistake here. Yeah, I think SqlAstTranslatorFactory should be thread-safe for it will be invoked concurrently.
You are right. This doc is good to have.
| * 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}. |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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?)
| * 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}. | ||
| */ |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
| @@ -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 { | |||
There was a problem hiding this comment.
For such bootstrapping stuff, usually no need to sepcify type safety for everything will be done sequentially with well-defined dependency or order. There is no need of concurrency for bootstrapping only happens once and its perf could be compromised a little bit.
This PR is a follow-up for #69 (comment).
MongoDialect, because it is documented onorg.hibernate.dialect.Dialect.SqlAstTranslators created byMongoTranslatorFactoryare single-use, because it is documented onorg.hibernate.sql.ast.SqlAstTranslatorFactory.