Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Design, implement and document how an application can configure the product #31
base: main
Are you sure you want to change the base?
Design, implement and document how an application can configure the product #31
Changes from all commits
582857c
7be84a5
c2e7d27
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
not sure why we need to emphasize the combination. To me the two classes are not tightly coupled. Traditionally
Dialect
is used to customize programmatically (as opposed tohibernate.properties
config file).ConnectionProvider
customization is in the scope of properties file without corresponding Dialect method to be overridden.But it is ok to retain it. Now I think your point is the two mandatory entries in hibernate.properties:
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.
If #27 is merged before this PR, then this PR should replace the usage of
new Configuration()
inBasicInsertionTests
with the usage ofMongoDialectSettings
.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.
This API is named and designed after
MongoClientSettings
. Given that it allows an application to accessMongoClientSettings.Builder
, this approach seems reasonable to me.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.
With the design as is, we don't need to expose neither
MongoDialectSettings
norMongoDialectSettings.builder
as part of our API. However, hiding them requires taking an approach different from that of theMongoClientSettings
. I think, I am fine with the approach I propose in this PR (because I am trying to makeMongoDialectSettings
similar toMongoClientSettings
here). But if there are concerns about exposingMongoDialectSettings
, please bring them up.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.
We may trivially support configuring an external
MongoClient
viaMongoDialectSettings
, as opposed to creating an internal one, enalbing applications to reuse clients.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 am not sure
autoCommit
is a good candidate for it seems an open question now. In JDBC, regardless of whether commit is auto, commit happens all the time. Currently we only commit whenautoCommit
value is false (perMongoStatement
implementation for now).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 doubt whether we need to include autoCommit() for it is internal Hibernate implememntatoin detail and its business logic might be complex that "we set a Connection's autoCommit status after it was created". That part might interfere with Hibernate's internal working. For instance, without our doing anything, the Connection would be set autoCommit as expected.
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 think given
configProperties
is not marked as@Nullable
, we could save the usage ofnotNull()
and trustNullAway
to cover our asses (in theory at least any usage inconsistent with thejspecify
annotation would be treated as bug during compiling phase). This is one of improvements in this project compared to Java driver project.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.
In Hibernate codebase, usually the naming of the map is
configurationValues
orconfigValues
for Hibernate does allow for ad-hoc value to be used as value, not restrictive to javaProperties
.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.
Here is an ad-hoc config usage in Hibernate ORM codebase: https://github.com/hibernate/hibernate-orm/blob/main/hibernate-core/src/main/java/org/hibernate/boot/cfgxml/internal/CfgXmlAccessServiceImpl.java
configProperties
is fine but it might mislead reader into thinking that it comes fromhibernate.properties
file exclusively. Actually it could be populated in ad-hoc way during Hibernate bootstrapping.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 understand the introduction of JDBC schema is to pave the way for future ticket, but this linkage is confusing for me and seems not compelling. Our end user might simply wants a database name without understanding why JDBC schema is involved.
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.
Do we need to emphasize the confusing
DatabaseMetadata#getSchemaTerm()
? Even in Hibernate ORM codebase this method is never used at all.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.
seems unnecessary if we trust
NullAway
's protection.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.
Hibernate has internal configuration parsing helper class: https://github.com/hibernate/hibernate-orm/blob/main/hibernate-core/src/main/java/org/hibernate/internal/util/config/ConfigurationHelper.java
Seems it will throw a specific exception:
ConfigurationException
. It might be an idea to copy such util class in ourinternal
package for reuse in other scenarios in the future. But if we decided to centralize all configs in thisMongoDialectSettings
, current design is fine.