- 
                Notifications
    You must be signed in to change notification settings 
- Fork 12
Support native query without parameters #128
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
Conversation
6572fbb    to
    72dd7e0      
    Compare
  
            
          
                src/main/java/com/mongodb/hibernate/internal/type/ObjectIdJdbcType.java
              
                Outdated
          
            Show resolved
            Hide resolved
        
      | typeContributions.contributeJavaType(ObjectIdJavaType.INSTANCE); | ||
| typeContributions.contributeJdbcType(ObjectIdJdbcType.INSTANCE); | ||
| var objectIdTypeCode = MqlType.OBJECT_ID.getVendorTypeNumber(); | ||
| var objectIdTypeCode = ObjectIdJdbcType.MQL_TYPE.getVendorTypeNumber(); | 
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.
MqlType was made package-access. Furthermore, ObjectIdJdbcType.MQL_TYPE explicitly points to our implementation of the JdbcType for ObjectId, which improves readability.
| throw exceptionDomainTypeUnsupportedOrMustBeExplicit(value, domainType); | ||
| } | ||
|  | ||
| public static @Nullable Object toDomainValue(BsonValue value) throws SQLFeatureNotSupportedException { | 
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 PR only needed this new method. However, I also wanted to stop using Object.class to represent "unknown domain type", which caused the rest of the changes.
72dd7e0    to
    510ae1f      
    Compare
  
    | @Table(name = COLLECTION_NAME) | ||
| @SqlResultSetMapping( | ||
| name = ItemWithFlattenedValue.MAPPING_FOR_FLATTENED_VALUE, | ||
| columns = { | 
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 tried also using SqlResultSetMapping.classes, but I don't think Hibernate ORM implements that one correctly: instead of trying to find the corresponding constructor, it uses the no-argument constructor and then tries to populate the object, searching for the field/property based on the ColumnResult.name, which represents the column name, and does not have to match the name of a field/property.
02c39c7    to
    51b953e      
    Compare
  
    51b953e    to
    6fb9e8c      
    Compare
  
            
          
                src/integrationTest/java/com/mongodb/hibernate/ArrayAndCollectionIntegrationTests.java
          
            Show resolved
            Hide resolved
        
              
          
                src/integrationTest/java/com/mongodb/hibernate/query/select/NativeQueryIntegrationTests.java
          
            Show resolved
            Hide resolved
        
      | if (fieldName.equals(ID_FIELD_NAME)) { | ||
| excludeId = true; | ||
| } | 
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.
[nit/optional]
| if (fieldName.equals(ID_FIELD_NAME)) { | |
| excludeId = true; | |
| } | |
| excludeId |= ID_FIELD_NAME.equals(fieldName); | 
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.
Done in 942a177.
| * Returning DTOs (Data Transfer Objects)</a>, {@link QueryProducer#createNativeQuery(String, Class)}. | ||
| */ | ||
| @Nested | ||
| class Dto { | 
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.
As I see, Hibernate supports DTO mapping both via Tuple and directly to an instance of DTO class with the help of @ConstructorResult. I suggest adding tests for latter approach as well to have wider use-case coverage.
To keep things simple, we could add @ConstructorResult to existing entities used in these tests (e.g., BasicCrudIntegrationTests.Item, etc.) rather than introducing new DTO-only classes (it works in both cases). Then we could extend the Dto nested test class with constructor-based variants (e.g., testBasicValues that uses constructor mapping):
() -> assertEq(
    item,
    session.createNativeQuery(mql, Item.CONSTRUCTOR_MAPPING_FOR_ITEM, Item.class)
           .getSingleResult());
Findings so far: constructor mapping works with entities that have only basic types, embeddables, or structs. It fails with Collection or subtypes - Hibernate ORM throws a NullPointerException because CollectionJavaType returns a null as recommended type. If array is used in @ColumnResult for a parameter that is Collection, the parametrized constructor is not invoked and Hibernate falls back to using the default constructor and field mapping.
@Override
	public JdbcType getRecommendedJdbcType(JdbcTypeIndicators context) {
		// none
		return null;
	}
That looks like a Hibernate bug (regardless of whether collections are intended to be supported, given the non-descriptive error).
Given that, we can add/extend tests for:
ItemWithFlattenedValue
ItemWithNestedValue
ItemWithNestedValueHavingArraysAndCollections (collections are inside a struct, which our type handles)
BasicCrudIntegrationTests.Item
Partially implemented suggestion (excludes ItemWithNestedValueHavingArraysAndCollections): 4fc30d2
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.
Done in c78dcde. I based this commit on your commit (and there is credit for that), but I changed a fair bit.
Discussing it over Zoom may be faster.
HIBERNATE-60
Co-authored-by: Viacheslav Babanin <[email protected]>
ac23fc3    to
    c78dcde      
    Compare
  
    …tyWithAggregateEmbeddableValue` HIBERNATE-60
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.
LGTM!
| More conflicts with  | 
HIBERNATE-60