Skip to content
This repository has been archived by the owner on Nov 28, 2024. It is now read-only.

use an index name that is guaranteed to be no more than 255 chars #21

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

use an index name that is guaranteed to be no more than 255 chars #21

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
717b6ef
use an index name that is guaranteed to be no more than 255 chars
zcourts Jun 30, 2018
d85297f
OGM-1507 Use custom @Index if available and fail if either custom or …
zcourts Aug 9, 2018
b2e5d71
OGM-1507 Use hash of an entity's fields to reduce index name length
zcourts Aug 11, 2018
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
17 changes: 16 additions & 1 deletion ignite/src/main/java/org/hibernate/ogm/datastore/ignite/impl/IgniteCacheInitializer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
import java.util.LinkedHashMap;
import java.util.Optional;
import java.util.Set;
import java.util.UUID;

import org.apache.ignite.cache.CacheAtomicityMode;
import org.apache.ignite.cache.QueryEntity;
Expand Down Expand Up @@ -180,7 +181,21 @@ private void appendIndex(QueryEntity queryEntity, AssociationKeyMetadata associa
fields.put( realColumnName, true );
}
queryIndex.setFields( fields );
queryIndex.setName( queryEntity.getTableName() + '_' + org.hibernate.ogm.util.impl.StringHelper.join( fields.keySet(), "_" ) );
String indexName = queryEntity.getTableName() + '_' + UUID.nameUUIDFromBytes( org.hibernate.ogm.util.impl.StringHelper.join( fields.keySet(), "_" ).getBytes() );
Copy link
Author

Choose a reason for hiding this comment

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

Is this sufficient without the use of the @Index? It makes it extremely unlikely for this to happen and it was already a rare case.

Copy link
Member

Choose a reason for hiding this comment

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

Why wouldn't you simply concatenating the fields? What's the benefit in adding the UUID as suffix?

Class<?> tableClass = context.getTableEntityTypeMapping().get( associationKeyMetadata.getAssociatedEntityKeyMetadata().getEntityKeyMetadata().getTable() );
javax.persistence.Table tableAnnotation = tableClass.getAnnotation( javax.persistence.Table.class );
if ( tableAnnotation != null && tableAnnotation.indexes().length > 0 ) {
for ( javax.persistence.Index index: tableAnnotation.indexes() ) {
if ( !index.name().isEmpty() ) {
indexName = index.name();
break;
}
}
}
if ( indexName.getBytes().length > 255 ) {
Copy link
Member

Choose a reason for hiding this comment

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

How come aren't you using indexName.length()?

Copy link
Author

Choose a reason for hiding this comment

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

Because Ignite's limit is of bytes not char length. String.length will give num of chars in the string but isn't necessarily the same as num of bytes.
Just verified in the Ignite code base as well and realised this still has a bug, Ignite specifically uses UTF_8 https://github.com/apache/ignite/blob/cc370d6cfef4a9d82761cc70fcb3bbeb0f91ab94/modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/IndexStorageImpl.java#L108 so I'll update this to do the same

Copy link
Member

Choose a reason for hiding this comment

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

Imagine a table MyTable with 100 columns, and at some point you see the error:
Encoded index name is too long for index: MyTable_189bbbb0-0c5f-3fb7-bba9-ad9285f193d1.

How do you know which column caused the issue?

Wouldn't you prefer something like:
Encoded index name is too long for index MyTable_Column23_column34

We could also add in the message the name of the table and the column used to create the index as additional information.

Copy link
Member

Choose a reason for hiding this comment

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

You also didn't add the charset StandardCharsets.UTF_8:
byte[] idxNameBytes = idxName.getBytes(StandardCharsets.UTF_8);

This means that if OGM runs on a machine with a different charset the validation might not be correct.

throw log.indexNameTooLong( indexName, queryEntity.getTableName() );
Copy link
Member

Choose a reason for hiding this comment

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

Would it be possible to collect all the indexes with an invalid name and then throw an exception at the end?
It's not a critical problem but when there is more than one error, the user can see all the problems in one go instead of
fix and rerun the application everytime an index fails the validation.

}
queryIndex.setName( indexName );

Set<QueryIndex> indexes = new HashSet<>( queryEntity.getIndexes() );
indexes.add( queryIndex );
Expand Down
3 changes: 3 additions & 0 deletions ignite/src/main/java/org/hibernate/ogm/datastore/ignite/logging/impl/Log.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,4 +49,7 @@ public interface Log extends org.hibernate.ogm.util.impl.Log {

@Message(id = 1710, value = "Neither " + IgniteProperties.CONFIGURATION_RESOURCE_NAME + " nor " + IgniteProperties.CONFIGURATION_CLASS_NAME + " properties are set")
HibernateException configurationNotSet();

@Message(id = 1711, value = "Invalid index name '%s' for entity '%s'. It must not be longer than 255 bytes. Use javax.persistence.@Index to override")
HibernateException indexNameTooLong(String indexName, String entityName);
}
59 changes: 59 additions & 0 deletions ignite/src/test/java/org/hibernate/ogm/datastore/ignite/test/cfg/LongIndexNameTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
/*
* Hibernate OGM, Domain model persistence for NoSQL datastores
*
* License: GNU Lesser General Public License (LGPL), version 2.1 or later
* See the lgpl.txt file in the root directory or <http://www.gnu.org/licenses/lgpl-2.1.html>.
*/
package org.hibernate.ogm.datastore.ignite.test.cfg;

import org.hibernate.HibernateException;
import org.hibernate.ogm.utils.OgmTestCase;
import org.junit.Test;

import javax.persistence.Entity;
import javax.persistence.Id;
import javax.persistence.OneToMany;
import java.util.HashSet;
import java.util.Set;

import static junit.framework.TestCase.fail;

/**
* Entities with long names that results in an index with greater than 255 chars is invalid in Ignite.
* This ensures we raise an error and provide useful information to the user
*/
public class LongIndexNameTest extends OgmTestCase {

@Test(expected = HibernateException.class)
Copy link
Author

Choose a reason for hiding this comment

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

@DavideD / @schernolyas - I need a bit of guidance on how to achieve this.
The exception HibernateException is raised as expected but because the OgmTestCase does setup before calling the test itself, this doesn't get caught and the exception causes the test to fail. How do I allow the exception to be raised and verify that it did to pass the test?

Copy link
Member

@DavideD DavideD Aug 16, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

There are several examples in our test base about checking for exceptions, look for the following line in test classes:

	@Rule
	public ExpectedException thrown = ExpectedException.none();

Copy link
Member

Choose a reason for hiding this comment

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

I think this class can be a good example: https://github.com/hibernate/hibernate-ogm/blob/a97bb73a3619ff37d02f88d628306bf8e9ac097f/core/src/test/java/org/hibernate/ogm/backendtck/jpa/JPAAPIWrappingTest.java

public void testLongEntityIndexName() throws Exception {
fail( "The length of the registered entity's cache name should've failed this already" );
Copy link
Member

Choose a reason for hiding this comment

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

This test is not good enough, The failure might be caused by something else.
How do you know that it failed because of the longer index? Actually, I don't think that's the cause of the failure. I think It's because you use @Entity( name="..." ) instead of @Table(name-=""). But I might be wrong because I didn't run the test.

Copy link
Member

Choose a reason for hiding this comment

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

I think JPAAPIWrappingTest#testUndefinedPU() might look similar to what you need.
You might want to have the following assertions (using the JUnit rule):

thrown.expect( HibernateException.class );
thrown.expectMessage( ... );

THere are multiple example of tests in hibernate ogm core using ExpectedException junit rule in the sources of hibernate-ogm core project.

Copy link
Member

Choose a reason for hiding this comment

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

By the way, sometimes the JUnit rule is not easy to use. he try catch block is still a valid option

}

@Override
protected Class<?>[] getAnnotatedClasses() {
return new Class<?>[] {
LongEntityName.class,
LongEntityContainer.class
};
}

@Entity
public static class LongEntityContainer {
@Id
private String id;

@OneToMany(targetEntity = LongEntityName.class)
private Set<LongEntityName> longEntityNames = new HashSet<>();

@javax.persistence.JoinTable(name = "joinLongEntityName")
Copy link
Member

Choose a reason for hiding this comment

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

Is this annotation required?

public Set<LongEntityName> getLongEntityNames() {
return longEntityNames;
}
}

@Entity(name = "Lorem ipsum dolor sit amet, consectetur adipiscing elit. Donec dapibus cursus vestibulum. Quisque eu justo non mi tincidunt sagittis. Donec tincidunt facilisis placerat. Sed placerat urna eget tristique faucibus. Curabitur maximus gravida enim, vitae sed")
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be @Table( name = "...") ? I don't think the attribute name in @Entity will affect the mapping in Ignite

public static class LongEntityName {
@Id
private String id;
}
}
68 changes: 68 additions & 0 deletions ...ava/org/hibernate/ogm/datastore/ignite/test/cfg/LongIndexNameWithIndexAnnotationTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
/*
* Hibernate OGM, Domain model persistence for NoSQL datastores
*
* License: GNU Lesser General Public License (LGPL), version 2.1 or later
* See the lgpl.txt file in the root directory or <http://www.gnu.org/licenses/lgpl-2.1.html>.
*/
package org.hibernate.ogm.datastore.ignite.test.cfg;

import org.apache.ignite.cache.QueryIndex;
import org.hibernate.ogm.OgmSession;
import org.hibernate.ogm.datastore.ignite.utils.IgniteTestHelper;
import org.hibernate.ogm.utils.OgmTestCase;
import org.junit.Test;

import javax.persistence.Entity;
import javax.persistence.Id;
import javax.persistence.Index;
import javax.persistence.OneToMany;
import javax.persistence.Table;
import java.util.HashSet;
import java.util.Set;

import static org.fest.assertions.Assertions.assertThat;

/**
* Entities with long names that results in an index with greater than 255 chars is invalid in Ignite.
* This ensures we raise an error and provide useful information to the user
*/
public class LongIndexNameWithIndexAnnotationTest extends OgmTestCase {

@Test
public void testLongEntityIndexName() throws Exception {
try ( OgmSession session = openSession() ) {
Set<QueryIndex> indexes = IgniteTestHelper.getIndexes( session.getSessionFactory(), EntityWithCustomIndex.class );
assertThat( indexes.size() ).isEqualTo( 1 );
assertThat( indexes.iterator().next().getName() ).isEqualToIgnoringCase( "SimpleIndexName" );
Copy link
Member

@DavideD DavideD Aug 23, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Nice!

You could make this test even better using:
assertThat( indexes ).onProperty( "name" ).containsOnly( "SimpleIndexName" )

instead of

assertThat( indexes.size() ).isEqualTo( 1 );
assertThat( indexes.iterator().next().getName() ).isEqualToIgnoringCase( "SimpleIndexName" );

By the way, is Ignite case insensitive? What happens if we have two indexes on two different columns with the same name and dfferent case. For example: "IndexNAME" and "Indexname".

Copy link
Member

Choose a reason for hiding this comment

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

I think we need some additional test cases:

  1. What happens when the index name looks like something like : 
    Table(indexes = {@Index(name = "Idx @#\\@    weird chars", columnList = "id")})
    Is that even possible?
  2. What happens if there are two indexes created on different fields in the same class?
  3. What happens if two indexes have the same name except for the the case? Example: "IndexNAME" and "Indexname"
  4. What happens when the index span multiple columns? Will it work using @Index( columnList = "field1,field2")?

This is just a question because I'm not familiar with it: Can we support the unique property in the @Index annotation?

Copy link
Member

Choose a reason for hiding this comment

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

  1. Test that the the index with the default name has the name we expect

}
}

@Override
protected Class<?>[] getAnnotatedClasses() {
return new Class<?>[] {
EntityWithCustomIndex.class,
EntityWithCustomContainer.class
};
}

@Entity
public static class EntityWithCustomContainer {
@Id
private String id;

@OneToMany(targetEntity = EntityWithCustomIndex.class)
private Set<EntityWithCustomIndex> entityWithCustomIndices = new HashSet<>();

@javax.persistence.JoinTable(name = "joinLongEntityName")
public Set<EntityWithCustomIndex> getEntityWithCustomIndices() {
return entityWithCustomIndices;
}
}

@Entity(name = "Lorem ipsum dolor sit amet, consectetur adipiscing elit. Donec dapibus cursus vestibulum. Quisque eu justo non mi tincidunt sagittis. Donec tincidunt facilisis placerat. Sed placerat urna eget tristique faucibus. Curabitur maximus gravida enim, vitae sed")
@Table(indexes = {@Index(name = "SimpleIndexName", columnList = "id")})
public static class EntityWithCustomIndex {
@Id
private String id;
}
}