HSEARCH-3667 Envers compatibility

[dcdh]

https://hibernate.atlassian.net/browse/HSEARCH-3667

[dcdh]

I've create this pull request to fix an issue when using Hibernate Search with Envers.

However I've got two remarks to do about my PR:

1. I did not find a way to implement tests from your one;
2. No dependency is present on Envers into Search which is logic. I add to hard code the full qualified name for DefaultRevisionEntity which may vary in time and introduce a specific code dependency on an another unexplicit dependency which I am not fan...

[yrodiere]

Thanks for this patch. The implementation looks good, I only have a minor comment.

I'm not overly concerned about the hardcoded class name as long as we implement a test for it, but as I mentioned below I'm not sure this part of the PR is necessary, so it might be hard to test :)

Regarding tests, we don't currently have a dependency to Envers in our tests and I think it would be wise to keep it that way, so that we're sure Search works correctly without any other dependency.
But obviously we need to test compatibility with Envers, so I'd be inclined to create a separate module for that:

• Create a new folder: integrationtest/mapper/orm-envers
• Create a POM in this folder. You can copy/paste the one from integrationtest/mapper/orm and adapt it.
• Reference the module from integrationtest/pom.xml: add a <module>mapper/orm-envers</module> line in the <modules> section.
• Create the directory structure of your new module. The root package should probably be org.hibernate.search.integrationtest.mapper.orm.envers
• Create a test. You can take org.hibernate.search.integrationtest.mapper.orm.model.GenericPropertyIT as an example, it's not too complicated and should be close to what you'll need for the Envers test. Just remove the entity classes other than IndexedEntity, make IndexedEntity auditable with a single, indexed text property, and adapt the schema expectations in setup and document expectations in index.
• Run the test: mvn clean install -pl 'integrationtest/mapper/orm-envers' -am

There are two things to keep in mind when dealing with ORM integration tests:

• Hibernate ORM bootstrap is handled by a helper (ormSetupHelper in your test), which offers simple configuration APIs adapted to our use cases, so we don't have to copy-paste tons of code in every single test.

• The tests are backend-independent: they do not use Lucene or Elasticsearch. Instead, they use a "stub" backend, which will simply check that the information sent by Hibernate Search to the backend is the one we expect. Thus, in your test, you will have to define an expected index schema, and an expected document when indexing. To that end, you will call a method on the backendMock, and define the schema/document using a lambda accepting a builder:

  • For the schema, backendMock.expectSchema( IndexedEntity.INDEX, b -> b.field( "text", String.class ) ) defines the expectation that the schema of the index named IndexedEntity.INDEX will only contain a single field named "text", of type String, with default options. An assertion error will be thrown if this doesn't happen, or if a different schema is defined.
  • For the document, backendMock.expectWorks( IndexedEntity.INDEX ).add( "1", b -> b.field( "text", "some text" ) defines the expectation that a document will be added to the index with the identifier "1" and with a single field named "text" containing the value "some text". An assertion error will be thrown if this doesn't happen, or if a different document is indexed.

If you need any further help, don't hestitate to ask.

[yrodiere]

@dcdh Hello!

Do you think you'll be able to add a test to this PR, so we can merge it?

In case you want to do it, I provided some guidance in my earlier comment regarding how to add the test.

If you don't have the time, please let us know so we can plan accordingly.

Thanks.

[dcdh]

Hello Yoann, I'm sorry for my delay to response. I was quite busy. I will give you tonight some remarks about it. Regards, Damien Clément d'Huart Le jeu. 5 sept. 2019 à 09:15, Yoann Rodière <notifications@github.com> a écrit :

…

@dcdh <https://github.com/dcdh> Hello! Do you think you'll be able to add a test to this PR, so we can merge it? In case you want to do it, I provided some guidance in my earlier comment regarding how to add the test. If you don't have the time, please let us know so we can plan accordingly. Thanks. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2067?email_source=notifications&email_token=ABHS735IGIQJCNWXVB6M6DTQICWY7A5CNFSM4IPXGLP2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD56DDNQ#issuecomment-528232886>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABHS737NXGYKJWZG3VLJQ5DQICWY7ANCNFSM4IPXGLPQ> .

[dcdh]

Hello Yoann,

Sorry for the late reply. I'm working on my free time to integrate hibernate ORM + Envers + Search under Quarkus.

Regarding Hibernate it seems that Hibernate Search is decoupled from Hibernate Core. The link is made in Quarkus via the HibernateSearchElasticsearchRecorder which initialize Search after Core.
Regarding tests, firstly I was thinking that I should made it as a bootstrap. However it was before I understand about integration in Quarkus and the use of HibernateSearchElasticsearchRecorder.

Hibernate Search 6 is a full rewrite, some code remains in legacy dealing with Envers so maybe that in a futur a bootstrap between Search and Envers will be provided avoiding vendor to create a custom one ? I may be wrong.

So I admit I am stuck on testing this PR...

[yrodiere]

Ok, thanks for the answer. We'll give it a try.

[yrodiere]

Alright, I implemented a few (simple) tests, ported from Search 5.

Goods news is: your patch does solve the problem, and Envers seems to be working now. Thank you very much for bringing this to our attention and taking the time to fix it!

I just launched a full build of this PR, testing in all supported environments (multiple databases, in particular). Let's see how it turns out, and if it succeeds I will merge.

Regarding Quarkus, one thing to keep in mind is that Quarkus takes Hibernate Search and performs extra steps to integrate it and make it compatible with all of its improvements (native images, compile-time boot, ...). The integration tests in the Hibernate Search project only run on a "traditional" hotspot JVM without any of the Quarkus bits. So in the context of this PR, Quarkus is not relevant.

That being said, there will be a need to test the Envers integration in Quarkus. I think you should be able to discuss that in quarkusio/quarkus#3690 . @gsmet is the person to talk to if anything doesn't work.

[yrodiere]

Merged! This fix will be part of the next release. Thanks again!