Improve how identifiers are treated #84

stIncMale · 2025-04-04T22:31:07Z

No description provided.

stIncMale · 2025-04-04T22:34:31Z

src/main/java/com/mongodb/hibernate/dialect/MongoDialect.java

+
+    @Override
+    public @Nullable String toQuotedIdentifier(@Nullable String name) {
+        return name;


This single change makes all the new tests pass, and without it, they all fail. Nothing else related to quoting both in Dialect (including Dialect.buildIdentifierHelper) and in DatabaseMetaData had any observable effect on the tests added.

It is a great news! Thanks for your PR. With some minor changes it could be merged.

stIncMale · 2025-04-04T22:38:02Z

src/integrationTest/java/com/mongodb/hibernate/IdentifierIntegrationTests.java

+        static final String COLLECTION_NAME = "\"collection in double quotes Mixed Case\"";
+        static final String FIELD_NAME = "\"field in double quotes Mixed Case\"";
+        static final String ACTUAL_COLLECTION_NAME = "collection in double quotes Mixed Case";
+        static final String ACTUAL_FIELD_NAME = "field in double quotes Mixed Case";


Unfortunately, it seems impossible to prevent Hibernate ORM from stripping identifiers like \"collection in double quotes Mixed Case\" of their quotes. Apparently, it has logic somewhere that ignores any configuration/overrides related to quoting, and strips double and single quotes anyway.

That place seems to be here https://github.com/hibernate/hibernate-orm/blob/0a4d551b32801ae15e99d2f8ac0750303ecda2b6/hibernate-core/src/main/java/org/hibernate/boot/model/naming/Identifier.java#L159-L173 - it always considers identifiers enclosed in a set of statically-defined characters as quoted.

yeah, it seems so. If the Hibernate quoting at the start and end is hard-coded or not configurable, I think we could remove those MixedCases testing cases for we don't need to test Hibernate if its behaviour is working as expected.

NathanQingyangXu · 2025-04-06T18:39:37Z

src/integrationTest/java/com/mongodb/hibernate/IdentifierIntegrationTests.java

+    @Table(name = WithSpaceAndDotMixedCase.COLLECTION_NAME)
+    static class WithSpaceAndDotMixedCase {
+        static final String COLLECTION_NAME = "collection with space and .dot Mixed Case";
+        static final String FIELD_NAME = "field with space Mixed Case";


so the dot was intentionally excluded in field name, but should we still use Mixed Case if only space is there?
It would be great to leave some comment here to explain; otherwise for newcomer it seems some overlooking instead.

I think Mixed Case is only meant for mixture of both lower and upper cases. Seems Mixed suffices to express the intentions in this class.

Seems nontrivial to adhere to defensitive programming to provide relevant exception when field name contains .. Currently insertion is allowed but exception is thrown as follows from MongoResultSet:

java.sql.SQLException: Failed to get value from column [index: x]

for this PR one-liner comment to explain the rationale of missing dot seems enough.

it seems more sensible to separate collection name identifier from field identifier classes, though I am ok with current status.

NathanQingyangXu · 2025-04-06T18:50:47Z

src/integrationTest/java/com/mongodb/hibernate/IdentifierIntegrationTests.java

+
+    @Entity
+    @Table(name = InSingleQuotesMixedCase.COLLECTION_NAME)
+    static class InSingleQuotesMixedCase {


the MixedCase is a little bit unclear, though further code reading shows that the Mixed case simply means single quotes both at the beginning and at the end.

Optional. Avoid the confusing MixedCase here by using more intuitive verbiage like InSingleQuotesDelimiters or simply StartingAndEndingWithSingleQuote.

NathanQingyangXu · 2025-04-06T19:06:49Z

src/integrationTest/java/com/mongodb/hibernate/IdentifierIntegrationTests.java

+        static final String COLLECTION_NAME = "`collection in single quotes Mixed Case`";
+        static final String FIELD_NAME = "`field in single quotes Mixed Case`";
+        static final String ACTUAL_COLLECTION_NAME = "collection in single quotes Mixed Case";
+        static final String ACTUAL_FIELD_NAME = "field in single quotes Mixed Case";


yeah, Hibernate has its own quotes so the above ACTUAL IDENTIFIER makes sense, as long as the quote pairs surround the whole identifier. For completeness, we could cover [] as well.

NathanQingyangXu · 2025-04-06T19:09:26Z

src/integrationTest/java/com/mongodb/hibernate/IdentifierIntegrationTests.java

+    }
+
+    @Entity
+    @Table(name = InSingleQuotesMixedCase.COLLECTION_NAME)


technically speaking, ``` is not the same as '. So more accurate verbiage is `backtick`, not `single quote`.

NathanQingyangXu · 2025-04-06T19:24:07Z

src/integrationTest/java/com/mongodb/hibernate/IdentifierIntegrationTests.java

+        static final String COLLECTION_NAME = "\"collection in double quotes Mixed Case\"";
+        static final String FIELD_NAME = "\"field in double quotes Mixed Case\"";
+        static final String ACTUAL_COLLECTION_NAME = "collection in double quotes Mixed Case";
+        static final String ACTUAL_FIELD_NAME = "field in double quotes Mixed Case";


yeah, it seems so. If the Hibernate quoting at the start and end is hard-coded or not configurable, I think we could remove those MixedCases testing cases for we don't need to test Hibernate if its behaviour is working as expected.

NathanQingyangXu · 2025-04-06T19:26:56Z

src/integrationTest/java/com/mongodb/hibernate/IdentifierIntegrationTests.java

+        @Column(name = EndingWithDoubleQuote.FIELD_NAME)
+        int v;
+    }
+}


seems EndingWithDoubleQuoteOnly might be more accurate for the MixedCase ends in the same way as well, right?

NathanQingyangXu · 2025-04-06T19:27:32Z

src/main/java/com/mongodb/hibernate/dialect/MongoDialect.java

+
+    @Override
+    public @Nullable String toQuotedIdentifier(@Nullable String name) {
+        return name;


It is a great news! Thanks for your PR. With some minor changes it could be merged.

NathanQingyangXu · 2025-04-06T19:31:41Z

src/integrationTest/java/com/mongodb/hibernate/IdentifierIntegrationTests.java

+                .containsExactly(new BsonDocument()
+                        .append(ID_FIELD_NAME, new BsonInt32(item.id))
+                        .append(EndingWithDoubleQuote.FIELD_NAME, new BsonInt32(item.v)));
+        sessionFactoryScope.inTransaction(session -> session.get(EndingWithDoubleQuote.class, item.id));


the above testing cases seem to follow similar pattern and is paramerized by two inputs: 1) entity class; 2) identifier name supplier, so it is doable to avoid the duplication by JUnit 5's feature.

but this is optional and over-abstracting in testing code not necessarily helps on code maintaining.

stIncMale requested a review from NathanQingyangXu April 4, 2025 22:31

stIncMale self-assigned this Apr 4, 2025

Improve how identifiers are treated

4f9ef0e

stIncMale force-pushed the improveHowItentifiersAreTreated branch from 2ef6d47 to 4f9ef0e Compare April 4, 2025 22:33

stIncMale commented Apr 4, 2025

View reviewed changes

NathanQingyangXu requested changes Apr 6, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve how identifiers are treated #84

Improve how identifiers are treated #84

stIncMale commented Apr 4, 2025

stIncMale Apr 4, 2025

NathanQingyangXu Apr 6, 2025 •

edited

Loading

stIncMale Apr 4, 2025 •

edited

Loading

stIncMale Apr 4, 2025 •

edited

Loading

NathanQingyangXu Apr 6, 2025

NathanQingyangXu Apr 6, 2025

NathanQingyangXu Apr 6, 2025

NathanQingyangXu Apr 6, 2025

NathanQingyangXu Apr 6, 2025

NathanQingyangXu Apr 6, 2025

NathanQingyangXu Apr 6, 2025

NathanQingyangXu Apr 6, 2025

NathanQingyangXu Apr 6, 2025

NathanQingyangXu Apr 6, 2025 •

edited

Loading

NathanQingyangXu Apr 6, 2025

Improve how identifiers are treated #84

Are you sure you want to change the base?

Improve how identifiers are treated #84

Conversation

stIncMale commented Apr 4, 2025

Choose a reason for hiding this comment

NathanQingyangXu Apr 6, 2025 • edited Loading

Choose a reason for hiding this comment

stIncMale Apr 4, 2025 • edited Loading

Choose a reason for hiding this comment

stIncMale Apr 4, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NathanQingyangXu Apr 6, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NathanQingyangXu Apr 6, 2025 •

edited

Loading

stIncMale Apr 4, 2025 •

edited

Loading

stIncMale Apr 4, 2025 •

edited

Loading

NathanQingyangXu Apr 6, 2025 •

edited

Loading