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

Improve how identifiers are treated #84

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Improve how identifiers are treated #84

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
250 changes: 250 additions & 0 deletions src/integrationTest/java/com/mongodb/hibernate/IdentifierIntegrationTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,250 @@
/*
* Copyright 2025-present MongoDB, Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package com.mongodb.hibernate;

import static com.mongodb.hibernate.internal.MongoConstants.ID_FIELD_NAME;
import static org.assertj.core.api.Assertions.assertThat;

import com.mongodb.client.MongoCollection;
import com.mongodb.hibernate.junit.InjectMongoCollection;
import com.mongodb.hibernate.junit.MongoExtension;
import jakarta.persistence.Column;
import jakarta.persistence.Entity;
import jakarta.persistence.Id;
import jakarta.persistence.Table;
import org.bson.BsonDocument;
import org.bson.BsonInt32;
import org.hibernate.testing.orm.junit.DomainModel;
import org.hibernate.testing.orm.junit.SessionFactory;
import org.hibernate.testing.orm.junit.SessionFactoryScope;
import org.hibernate.testing.orm.junit.SessionFactoryScopeAware;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.ExtendWith;

@SessionFactory(exportSchema = false)
@DomainModel(
annotatedClasses = {
IdentifierIntegrationTests.WithSpaceAndDotMixedCase.class,
IdentifierIntegrationTests.InSingleQuotesMixedCase.class,
IdentifierIntegrationTests.StartingWithSingleQuote.class,
IdentifierIntegrationTests.EndingWithSingleQuote.class,
IdentifierIntegrationTests.InDoubleQuotesMixedCase.class,
IdentifierIntegrationTests.StartingWithDoubleQuote.class,
IdentifierIntegrationTests.EndingWithDoubleQuote.class
})
@ExtendWith(MongoExtension.class)
class IdentifierIntegrationTests implements SessionFactoryScopeAware {
@InjectMongoCollection(WithSpaceAndDotMixedCase.COLLECTION_NAME)
private static MongoCollection<BsonDocument> mongoCollectionWithSpaceAndDotMixedCase;

@InjectMongoCollection(InSingleQuotesMixedCase.ACTUAL_COLLECTION_NAME)
private static MongoCollection<BsonDocument> mongoCollectionInSingleQuotesMixedCase;

@InjectMongoCollection(StartingWithSingleQuote.COLLECTION_NAME)
private static MongoCollection<BsonDocument> mongoCollectionStartingWithSingleQuote;

@InjectMongoCollection(EndingWithSingleQuote.COLLECTION_NAME)
private static MongoCollection<BsonDocument> mongoCollectionEndingWithSingleQuote;

@InjectMongoCollection(InDoubleQuotesMixedCase.ACTUAL_COLLECTION_NAME)
private static MongoCollection<BsonDocument> mongoCollectionInDoubleQuotesMixedCase;

@InjectMongoCollection(StartingWithDoubleQuote.COLLECTION_NAME)
private static MongoCollection<BsonDocument> mongoCollectionStartingWithDoubleQuote;

@InjectMongoCollection(EndingWithDoubleQuote.COLLECTION_NAME)
private static MongoCollection<BsonDocument> mongoCollectionEndingWithDoubleQuote;

private SessionFactoryScope sessionFactoryScope;

@Test
void withSpaceAndDotMixedCase() {
var item = new WithSpaceAndDotMixedCase();
sessionFactoryScope.inTransaction(session -> session.persist(item));
assertThat(mongoCollectionWithSpaceAndDotMixedCase.find())
.containsExactly(new BsonDocument()
.append(ID_FIELD_NAME, new BsonInt32(item.id))
.append(WithSpaceAndDotMixedCase.FIELD_NAME, new BsonInt32(item.v)));
sessionFactoryScope.inTransaction(session -> session.get(WithSpaceAndDotMixedCase.class, item.id));
}

@Test
void inSingleQuotesMixedCase() {
var item = new InSingleQuotesMixedCase();
sessionFactoryScope.inTransaction(session -> session.persist(item));
assertThat(mongoCollectionInSingleQuotesMixedCase.find())
.containsExactly(new BsonDocument()
.append(ID_FIELD_NAME, new BsonInt32(item.id))
.append(InSingleQuotesMixedCase.ACTUAL_FIELD_NAME, new BsonInt32(item.v)));
sessionFactoryScope.inTransaction(session -> session.get(InSingleQuotesMixedCase.class, item.id));
}

@Test
void startingWithSingleQuote() {
var item = new StartingWithSingleQuote();
sessionFactoryScope.inTransaction(session -> session.persist(item));
assertThat(mongoCollectionStartingWithSingleQuote.find())
.containsExactly(new BsonDocument()
.append(ID_FIELD_NAME, new BsonInt32(item.id))
.append(StartingWithSingleQuote.FIELD_NAME, new BsonInt32(item.v)));
sessionFactoryScope.inTransaction(session -> session.get(StartingWithSingleQuote.class, item.id));
}

@Test
void endingWithSingleQuote() {
var item = new EndingWithSingleQuote();
sessionFactoryScope.inTransaction(session -> session.persist(item));
assertThat(mongoCollectionEndingWithSingleQuote.find())
.containsExactly(new BsonDocument()
.append(ID_FIELD_NAME, new BsonInt32(item.id))
.append(EndingWithSingleQuote.FIELD_NAME, new BsonInt32(item.v)));
sessionFactoryScope.inTransaction(session -> session.get(EndingWithSingleQuote.class, item.id));
}

@Test
void inDoubleQuotesMixedCase() {
var item = new InDoubleQuotesMixedCase();
sessionFactoryScope.inTransaction(session -> session.persist(item));
assertThat(mongoCollectionInDoubleQuotesMixedCase.find())
.containsExactly(new BsonDocument()
.append(ID_FIELD_NAME, new BsonInt32(item.id))
.append(InDoubleQuotesMixedCase.ACTUAL_FIELD_NAME, new BsonInt32(item.v)));
sessionFactoryScope.inTransaction(session -> session.get(InDoubleQuotesMixedCase.class, item.id));
}

@Test
void startingWithDoubleQuote() {
var item = new StartingWithDoubleQuote();
sessionFactoryScope.inTransaction(session -> session.persist(item));
assertThat(mongoCollectionStartingWithDoubleQuote.find())
.containsExactly(new BsonDocument()
.append(ID_FIELD_NAME, new BsonInt32(item.id))
.append(StartingWithDoubleQuote.FIELD_NAME, new BsonInt32(item.v)));
sessionFactoryScope.inTransaction(session -> session.get(StartingWithDoubleQuote.class, item.id));
}

@Test
void endingWithDoubleQuote() {
var item = new EndingWithDoubleQuote();
sessionFactoryScope.inTransaction(session -> session.persist(item));
assertThat(mongoCollectionEndingWithDoubleQuote.find())
.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));
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 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.

}

@Override
public void injectSessionFactoryScope(SessionFactoryScope sessionFactoryScope) {
this.sessionFactoryScope = sessionFactoryScope;
}

@Entity
@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";
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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


@Id
int id;

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

@Entity
@Table(name = InSingleQuotesMixedCase.COLLECTION_NAME)
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.

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

static class InSingleQuotesMixedCase {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

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";
Copy link
Contributor

Choose a reason for hiding this comment

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

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.


@Id
int id;

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

@Entity
@Table(name = StartingWithSingleQuote.COLLECTION_NAME)
static class StartingWithSingleQuote {
static final String COLLECTION_NAME = "`collection starting with single quote";
static final String FIELD_NAME = "`field starting with single quote";

@Id
int id;

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

@Entity
@Table(name = EndingWithSingleQuote.COLLECTION_NAME)
static class EndingWithSingleQuote {
static final String COLLECTION_NAME = "collection ending with single quote`";
static final String FIELD_NAME = "field ending with single quote`";

@Id
int id;

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

@Entity
@Table(name = InDoubleQuotesMixedCase.COLLECTION_NAME)
static class InDoubleQuotesMixedCase {
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";
Copy link
Member Author

@stIncMale stIncMale Apr 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@stIncMale stIncMale Apr 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.


@Id
int id;

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

@Entity
@Table(name = StartingWithDoubleQuote.COLLECTION_NAME)
static class StartingWithDoubleQuote {
static final String COLLECTION_NAME = "\"collection starting with double quote";
static final String FIELD_NAME = "\"field starting with double quote";

@Id
int id;

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

@Entity
@Table(name = EndingWithDoubleQuote.COLLECTION_NAME)
static class EndingWithDoubleQuote {
static final String COLLECTION_NAME = "collection ending with double quote\"";
static final String FIELD_NAME = "field ending with double quote\"";

@Id
int id;

@Column(name = EndingWithDoubleQuote.FIELD_NAME)
int v;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

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

6 changes: 6 additions & 0 deletions 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 @@ -29,6 +29,7 @@
import org.hibernate.engine.jdbc.dialect.spi.DialectResolutionInfo;
import org.hibernate.service.ServiceRegistry;
import org.hibernate.sql.ast.SqlAstTranslatorFactory;
import org.jspecify.annotations.Nullable;

/**
* A MongoDB {@link Dialect} for {@linkplain #getMinimumSupportedVersion() version 6.0 and above}. Must be used together
Expand Down Expand Up @@ -91,4 +92,9 @@ public void contribute(TypeContributions typeContributions, ServiceRegistry serv
typeContributions.contributeJavaType(ObjectIdJavaType.INSTANCE);
typeContributions.contributeJdbcType(ObjectIdJdbcType.INSTANCE);
}

@Override
public @Nullable String toQuotedIdentifier(@Nullable String name) {
return name;
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 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.

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.

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

}
}