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

zcourts · 2018-06-30T05:39:54Z

Ignite indices have a char limit of 255 on the name.
The current naming scheme easily exceeds that in complex models because it uses the field names on the entity.
This PR uses a random UUID instead, ensuring that limit is never hit.
I considered using "tableName + UUID" but that just makes it less likely since it's possible to have entity names longer than the 127 chars that the UUID leaves us with.

For reference an example of the current scheme failing manifests itself as in the attached file/stacktrace.
ignite-ogm-index-name-error.txt

Hibernate-CI · 2018-06-30T05:39:55Z

Can one of the admins add this person to the trusted builders? (reply with: "add to whitelist" or "ok to test")

schernolyas · 2018-06-30T05:47:40Z

Hi @zcourts !

Thanks for PR!
But ... before merge the PR ... Could you please create a issue in project's jira ?
Project's Jira URL is http://hibernate.atlassian.net

Also ... Do you use the dialect for your project? What is your impressions?
Thanks for feedback!

zcourts · 2018-06-30T09:03:53Z

Hi @schernolyas
I've created https://hibernate.atlassian.net/browse/OGM-1507

We're migrating an in-house ORM-like library we built to do this with Ignite.
So far it has been ok. No major issue or complaint.
Having never used Hibernate ORM, or OGM at all before this project, it's been more straightforward than I'd expect.
The ignite dialect itself I've only just switched over to using, the initial spike was to investigate only and I did the spike using the Infinispan embedded dialect. The switch over was quick, having learnt what was needed using Infinispan.

The docs are a pain point, If I hadn't done Infinispan first I suspect it'd be a lot harder to use this dialect. The asci docs in the documentation module was invaluable in learning what configs were available to start.

Having the ability to choose an existing Ignite instance started earlier in the JVM was crucial, I'd have had to add that option if it wasn't already present. We have a huge amount of infrastructure already built around Ignite and customising various pieces.

My initial test works fine, but having switched to the patch branch I'm actually now getting an IndexOutOfBoundsException, value.getColumnIterator() in https://github.com/hibernate/hibernate-ogm-ignite/blob/master/ignite/src/main/java/org/hibernate/ogm/datastore/ignite/impl/IgniteCacheInitializer.java#L284 returns more entries than ( (ComponentType) type ).getSubtypes().
I'm investigating, will either report a bug later or another PR/fix if it's not my fault.

My next point of call is going to be investigating how I can provide a custom CacheConfiguration. I've already found https://github.com/hibernate/hibernate-ogm-ignite/blob/master/ignite/src/main/java/org/hibernate/ogm/datastore/ignite/impl/IgniteCacheInitializer.java#L196 but I'm still trying to find my way around the code so it's unclear at the moment how I'll approach this but definitely need to customise the cache configs. Potentially another PR (I keep thinking there's probably a means of exposing dialect specific stuff but haven't found it. I know about unwrap but haven't investigated as a potential route yet)

It's still early days (though I'm hoping a migration to this is completed and ready for integration tests next week). Once I've spent more time, happy to provide feedback.

EDIT:

While I'm at it, is there a reason OGM-1472 Rework of rendering JPQL into Ignite SQL #19 hasn't been merged?
The version in the pom is 5.3.0-SNAPSHOT but 5.3.1-Final is already in the tree. Is that intentional?

DavideD · 2018-07-04T09:46:15Z

While I'm at it, is there a reason #19 hasn't been merged?

There were some conflicts when I review it the first time and then I went on Holiday. I just came back going through the accumulated emails and new PRs.

The version in the pom is 5.3.0-SNAPSHOT but 5.3.1-Final is already in the tree. Is that intentional?

I will check, it seems an error. Thanks for telling us

DavideD

Thanks for providing a PR.
A few notes about the way we work:

A PR should also have a test case, in particular we would need a way to verify what happens when the problem occurs
For commit messages we use the guidelines here: https://chris.beams.io/posts/git-commit/ with the following template (issue code + first letter Uppercase): [OGM-1500] Use an index ...

I'm not convinced about this solution, can't the user choose a different index name using the javax.persistence.@Index annotation?

DavideD · 2018-07-04T09:50:48Z

ignite/src/main/java/org/hibernate/ogm/datastore/ignite/impl/IgniteCacheInitializer.java

@@ -180,7 +181,7 @@ 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(), "_" ) );
+		queryIndex.setName( UUID.randomUUID().toString().replace( "-", "_" ) );


A random name will work but it won't be very user friendly, making it hard to debug when an error occurs or reproduce problems.
Let's use the previous approach and only if the string is bigger than 255 chars do something different. What happens if index name is bigger than 255 chars? Error?

DavideD · 2018-07-04T09:56:52Z

@zcourts I left a few comments about this PR, thanks a lot for your help.

Feel free to ask us for further questions if you something is not clear.
Cheers

Salauyou · 2018-07-04T10:46:14Z

Agree with @DavideD. Index names should be more readable.
For example, if length > 255, you take first N chars and append an autoincremented number to ensure uniqueness, like it's done for aliases in queries:
TABLE_NAME_VERY_LONG...._COL_001
TABLE_NAME_VERY_LONG...._COL_002
etc.
If length <= 255, it would be left uncnahged.

DavideD · 2018-07-04T10:48:39Z

That would work, a simpler solution could be to throw an exception at startup and let the user rewrite the default name. I don't think situation will happen often

DavideD · 2018-07-04T11:02:50Z

In general, we prefer to give the user enough information so that it can easily solve issues based on what's best for him.

Salauyou · 2018-07-04T11:08:30Z

@DavideD agree. A better way would be to throw an exception proposing to reduce table/column names or define this index manually in @Table(indexes =...).

zcourts · 2018-07-10T09:10:51Z

That's fair enough. I'll add a test case for > 255 chars raising an exception and a success case for using javax.persistence.@Index to manually name the index. Will update the PR to address the comments raised.

schernolyas · 2018-08-09T10:40:50Z

Hi @zcourts !

What is status of the PR ?

…generated index name length > 255

zcourts · 2018-08-09T23:26:33Z

Hi @schernolyas
I was busy that week and this slipped off my radar. I just attempted the fix with some tests. It needs a bit of guidance - see the questions I've raised above

zcourts · 2018-08-15T15:40:09Z

@schernolyas / @DavideD ping ^^

DavideD · 2018-08-15T15:55:42Z

see the questions I've raised above

What questions?

zcourts · 2018-08-09T23:21:50Z

ignite/src/main/java/org/hibernate/ogm/datastore/ignite/impl/IgniteCacheInitializer.java

+		String indexName = queryEntity.getTableName() + '_' + org.hibernate.ogm.util.impl.StringHelper.join( fields.keySet(), "_" );
+		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) {


@DavideD / @schernolyas how do you propose this should be handled?
Here I'm just taking the first non-empty @Index name and using it...I'm not convinced that's the way to go but @Index cannot be applied to the classes directly.

Also, should the "columnList" be used as part of the name, if so, how?

edit
What about org.hibernate.search.annotations.Index - it's allowed on classes?

What about org.hibernate.search.annotations.Index - it's allowed on classes?

It's for full-text searches and only if one want to use Hibernate Search.
It's not suitable for this case, I think.

I don't understand what's going on with this part of the code, why would you take the first empty index name? Isn't an index related to a field? What's the logic here?

It's possible to create indexes in two ways:

@Table( indexes = {@Index( columnList= "field1,field2", name="" )})

@Index( ... ) on a field of the class

zcourts · 2018-08-09T23:23:55Z

ignite/src/test/java/org/hibernate/ogm/datastore/ignite/test/cfg/LongIndexNameTest.java

+ */
+public class LongIndexNameTest extends OgmTestCase {
+
+	@Test(expected = HibernateException.class)


@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?

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();

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

zcourts · 2018-08-11T21:49:42Z

ignite/src/main/java/org/hibernate/ogm/datastore/ignite/impl/IgniteCacheInitializer.java

@@ -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() );


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.

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

zcourts · 2018-08-16T15:20:02Z

Didn't realise "PENDING" meant I had to submit before anyone else could see the comments

DavideD · 2018-08-16T16:13:16Z

ignite/src/main/java/org/hibernate/ogm/datastore/ignite/impl/IgniteCacheInitializer.java

+				}
+			}
+		}
+		if ( indexName.getBytes().length > 255 ) {


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

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

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.

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.

DavideD · 2018-08-23T12:00:57Z

ignite/src/test/java/org/hibernate/ogm/datastore/ignite/test/cfg/LongIndexNameTest.java

+		}
+	}
+
+	@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")


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

DavideD · 2018-08-23T12:01:12Z

ignite/src/test/java/org/hibernate/ogm/datastore/ignite/test/cfg/LongIndexNameTest.java

+		@OneToMany(targetEntity = LongEntityName.class)
+		private Set<LongEntityName> longEntityNames = new HashSet<>();
+
+		@javax.persistence.JoinTable(name = "joinLongEntityName")


Is this annotation required?

DavideD · 2018-08-23T12:26:50Z

...t/java/org/hibernate/ogm/datastore/ignite/test/cfg/LongIndexNameWithIndexAnnotationTest.java

+		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" );


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".

I think we need some additional test cases:

What happens when the index name looks like something like :
Table(indexes = {@Index(name = "Idx @#\\@ weird chars", columnList = "id")})
Is that even possible?

What happens if there are two indexes created on different fields in the same class?

What happens if two indexes have the same name except for the the case? Example: "IndexNAME" and "Indexname"

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?

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

DavideD · 2018-08-23T12:42:19Z

ignite/src/test/java/org/hibernate/ogm/datastore/ignite/test/cfg/LongIndexNameTest.java

+
+	@Test(expected = HibernateException.class)
+	public void testLongEntityIndexName() throws Exception {
+		fail( "The length of the registered entity's cache name should've failed this already" );


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.

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.

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

DavideD · 2018-08-23T12:53:33Z

ignite/src/main/java/org/hibernate/ogm/datastore/ignite/impl/IgniteCacheInitializer.java

+			}
+		}
+		if ( indexName.getBytes().length > 255 ) {
+			throw log.indexNameTooLong( indexName, queryEntity.getTableName() );


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.

DavideD

Thanks a lot!

There are still some problems with this PR.

I'm not convinced about the generated name because it seems more complicated than we need (but I'm open to discuss it).

I hope the test I linked might be a good example to solve some of the problems in the test you added.

Cheers

zcourts · 2018-08-23T19:01:13Z

That's great feedback, thank you and thanks for the pointer to that file. I'm out of the office until Tuesday, I'll take a look at all of the points you've raised and address them next week.

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

717b6ef

DavideD suggested changes Jul 4, 2018

View reviewed changes

OGM-1507 Use custom @Index if available and fail if either custom or …

d85297f

…generated index name length > 255

OGM-1507 Use hash of an entity's fields to reduce index name length

b2e5d71

zcourts commented Aug 16, 2018

View reviewed changes

DavideD reviewed Aug 16, 2018

View reviewed changes

DavideD reviewed Aug 23, 2018

View reviewed changes

DavideD suggested changes Aug 23, 2018

View reviewed changes

Base automatically changed from master to main March 19, 2021 14:42

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

Are you sure you want to change the base?

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

Conversation

zcourts commented Jun 30, 2018

Hibernate-CI commented Jun 30, 2018

schernolyas commented Jun 30, 2018 • edited Loading

zcourts commented Jun 30, 2018 • edited Loading

DavideD commented Jul 4, 2018

DavideD left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DavideD commented Jul 4, 2018

Salauyou commented Jul 4, 2018 • edited Loading

DavideD commented Jul 4, 2018

DavideD commented Jul 4, 2018

Salauyou commented Jul 4, 2018 • edited Loading

zcourts commented Jul 10, 2018

schernolyas commented Aug 9, 2018

zcourts commented Aug 9, 2018

zcourts commented Aug 15, 2018

DavideD commented Aug 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DavideD Aug 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DavideD Aug 16, 2018 • 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

zcourts commented Aug 16, 2018

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

DavideD Aug 23, 2018 • 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

DavideD left a comment

Choose a reason for hiding this comment

zcourts commented Aug 23, 2018

schernolyas commented Jun 30, 2018 •

edited

Loading

zcourts commented Jun 30, 2018 •

edited

Loading

Salauyou commented Jul 4, 2018 •

edited

Loading

Salauyou commented Jul 4, 2018 •

edited

Loading

DavideD commented Aug 15, 2018 •

edited

Loading

DavideD Aug 16, 2018 •

edited

Loading

DavideD Aug 16, 2018 •

edited

Loading

DavideD Aug 23, 2018 •

edited

Loading