HSEARCH-3403 + HSEARCH-3511 Polish the entry points to the Search DSL and make them more consistent

[yrodiere]

https://hibernate.atlassian.net//browse/HSEARCH-3403: Remove the inheritance from our Session/Query APIs to Hibernate ORM/JPA APIs.
https://hibernate.atlassian.net//browse/HSEARCH-3511: Make the entry points to the Search API more intuitive

Regarding HSEARCH-3403, note I ended up removing the inheritance in FullTextSession too, and exposing two methods: .toHibernateOrmSession()/toJpaEntityManager(), so that the use case mentioned in the ticket is still covered.

Regarding HSEARCH-3511, I'd really like @gsmet to have a look, please, since you were the one to mention to me that the APIs were not that intuitive when trying to use them without having a look at the documentation. I think I addressed your main concern, but I went a bit beyond that, so it would be great if you could have a quick look at the resulting syntax.

In a nutshell:

• We consistently use the Search prefix in API types.
• We use toQuery to build the query (consistent with predicates, sorts, etc.).
• The concept of "target" has been renamed to "scope": the scope of the search.
• There are now two separate entry points to the query DSL: the simpler one, .search(), will work just fine with the lambda-based syntax. The more "advanced" one, .scope(), will be necessary for the object-based syntax, and we may one day add more operations than just search there: mass indexer, delete queries, ...
• The methods on the SearchQuery interface no longer follow the same naming scheme as JPA/ORM: they are named fetchXXX() and offsets/limits are passed through parameters instead of setters. The reasons are:
  1. JPA naming does not play well with how versatile a "result" can be in Elasticsearch/Lucene. A result is not just hits, it can also include the total hit count, aggregations, facets, ... so having a method named getResultList can be confusing. Even if we decided to deal with it, finding appropriate names would be quite challenging...
  2. The methods actually retrieve data from disk or the network. Naming them getXXX gives the impression that you're just calling a getter.
  3. Some methods are not affected with the limit/offset (e.g. fetchTotalHitCount)... but passing their value through setters gives the impression that every method in SearchQuery is affected.

You can find a few examples in the tests, org.hibernate.search.integrationtest.mapper.orm.search.SearchQueryIT in particular, but below are some simple ones.

I changed this:

FullTextSession ftSession = Search.getFullTextSession( session );
FullTextSearchTarget<Book> target = ftSession.search( Book.class );
SearchQuery<String> query = target.query()
		.asProjection( target.projection().field( "title", String.class ).toProjection() )
		.predicate( target.predicate().matchAll() )
		.build();
List<String> results = query.fetchHits();

to this:

SearchSession searchSession = Search.getSearchSession( session );
SearchScope<Book> scope = searchSession.scope( Book.class );
SearchQuery<String> query = scope.search()
		.asProjection( scope.projection().field( "title", String.class ).toProjection() )
		.predicate( scope.predicate().matchAll() )
		.toQuery();
List<String> results = query.fetchHits();

And I introduced a shortcut so that the lambda syntax (the one we will recommend for most use cases) feels more natural, so this:

FullTextQuery<Book> query = ftSession.search( Book.class ).query()
		.asEntity()
		.predicate( f -> f.matchAll() )
		.build();

became this:

SearchQuery<Book> query = searchSession.search( Book.class ) // No trailing .query() call
		.asEntity()
		.predicate( f -> f.matchAll() )
		.toQuery();

[yrodiere]

@gsmet , @fax4ever : there's a lot of refactoring here, so please don't let the large number of changes scare you. The large commits (the ones with "Rename X to Y" in the commit message) are generally not worth reviewing in details as long as you agree with their purpose.

[coveralls]

Pull Request Test Coverage Report for Build 7

• 271 of 330 (82.12%) changed or added relevant lines in 61 files are covered.
• 1 unchanged line in 1 file lost coverage.
• Overall coverage increased (+0.04%) to 88.874%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
backend/elasticsearch/src/main/java/org/hibernate/search/backend/elasticsearch/search/query/impl/ElasticsearchIndexSearchScope.java	5	6	83.33%
backend/lucene/src/main/java/org/hibernate/search/backend/lucene/search/query/impl/LuceneIndexSearchScope.java	6	7	85.71%
engine/src/main/java/org/hibernate/search/engine/common/impl/MappedIndexSearchScopeImpl.java	8	9	88.89%
mapper/orm/src/main/java/org/hibernate/search/mapper/orm/mapping/impl/HibernateOrmMappingImpl.java	2	3	66.67%
backend/elasticsearch/src/main/java/org/hibernate/search/backend/elasticsearch/index/impl/ElasticsearchIndexManagerImpl.java	4	6	66.67%
backend/lucene/src/main/java/org/hibernate/search/backend/lucene/index/impl/LuceneIndexManagerImpl.java	4	6	66.67%
mapper/javabean/src/main/java/org/hibernate/search/mapper/javabean/search/query/SearchQuery.java	2	4	50.0%
mapper/javabean/src/main/java/org/hibernate/search/mapper/javabean/search/query/impl/JavaBeanSearchQuery.java	4	6	66.67%
mapper/orm/src/main/java/org/hibernate/search/mapper/orm/search/impl/SearchScopeImpl.java	3	5	60.0%
mapper/orm/src/main/java/org/hibernate/search/mapper/orm/search/query/SearchQuery.java	6	8	75.0%

Files with Coverage Reduction	New Missed Lines	%
backend/elasticsearch/src/main/java/org/hibernate/search/backend/elasticsearch/orchestration/impl/ElasticsearchBatchingSharedWorkOrchestrator.java	1	70.24%

Totals
Change from base Build 164:	0.04%
Covered Lines:	16207
Relevant Lines:	18236

---

💛 - Coveralls

[gsmet]

I added some comments.

My main issue is about this result/hit thing. And it goes far beyond what I mentioned as you also have setFirstResult and so on. Not sure if my proposal is a good idea but we need to think carefully about it.

[gsmet]

So we still inherit from AbstractProducedQuery? Does it bring something to us?

[yrodiere]

It mainly provides more resilience to API changes in ORM, which can be useful since the Query contract is not meant to be implemented by third parties, thus they could add methods unexpectedly.

Also, it allows to more easily implement methods such as getSingleResult() in a way that throws the correct exceptions.

Since we're splitting the interfaces, I could create a wrapper class, separate from this one, and instantiate it when toJpaQuery is called? That would clean up the code of SearcheQueryImpl quite a lot.
The main drawback is that we wouldn't notice when our own interfaces deviate from JPA/ORM interfaces in an incompatible way (same signature with different return type). But I suppose it's not such a big deal, since we're unlikely to go back to making our interfaces extend JPA/ORM interfaces.

[yrodiere]

I split the implementations and moved the adaptor to a separate class.

[gsmet]

Any reason why we do not have varargs versions for search() and scope()?

[yrodiere]

Yes: generics in varargs lead to call-site warnings ("heap pollution yada yada"). Usually the solution to avoid these warnings is to add @SafeVarargs, but this can only be used on static or final methods, so not here in an interface.

For the JavaBean mapper, we could get rid of the generics, but not for the ORM mapper, where they are actually useful.

Our only other option would be to declare one method per number of parameters: one type, two types, etc. I'm really not thrilled by the idea.

[gsmet]

Hum. This is annoying :/.

What makes them useful for the ORM case?

[yrodiere]

The generics, you mean? In short it allows for typed queries when requesting an entity result type. Without it, we wouldn't know which type to put in SearchQuery<T>.
It's not useful in the JavaBean mapper because it cannot load entities from the database, since there isn't any database.

[gsmet]

Does it make sense, now? Or is it used for loading the elements from the database?

[yrodiere]

is it used for loading the elements from the database

Yes.

[gsmet]

I don't see how that makes sense. I mean you can't translate the query to a JPA one, which is more or less what people would expect with this method, wouldn't they?

[yrodiere]

I'm not sure I'm following, we did agree it was useful to be able to use Hibernate Search queries through JPA/ORM interfaces, didn't we? I just wanted to move this away from our main interfaces because we know that it will always be just a hack and we cannot implement all the methods of TypedQuery.

It's not a JPA query, it's a representation of a Hibernate Search query as a JPA query. The only purpose of this method is to allow using JPA interfaces to manipulate Hibernate Search queries, thereby integrating more easily into existing frameworks that are used to Hibernate ORM.

[gsmet]

Hmmm. There's something weird with having getResult() and getOptionalResult/getSingleResult not doing the same thing at all. At least the Optional one could be seen as the same thing as getResult().

Maybe we should rename the others to getOptionalHit, getSingleHit, getHits? That would be consistent with what you have inside SearchResult?

[yrodiere]

So it seems we're on the same page, but just a reminder for readers: what we call "result" in Hibernate Search is the whole thing: hits, total hit count, aggregations, facets, ... While a "hit" is only the part of the result that represents a document matching the query.

I agree the names you proposed are more correct. What I was trying to do here is to still more or less fit the JPA naming, so as not to be too confusing.

If we are to give up on that, then yes, I would pick different names. But I would go even further, because I want to make it extra clear that calling one of these methods does not just "get" the hits from a Java object that is already there, fully populated: it actually executes the query.
I would also get rid of the getSingleHit variant, since returning an Optional provides the same feature, and more.

So ideally, if we forget about using the same naming as JPA, I would use:

• SearchResult<T> fetch()
• List<T> fetchHits() (=> fetch().getHits())
• Optional<T> fetchSingleHit() (=> fetch().getHits(), but returns an Optional for 0 or 1 hit, throws an exception if more than 1 hit)
• long fetchCount() (=> fetch().getHitCount(), but optimized to not fetch any hit)
• variants of fetch() and fetchHits accepting two paramters to handle paging: fetch(long limit), fetch(long limit, long offset)... OR at least rename setFirstResult to setHitOffset and setMaxResults to setHitLimit.

[gsmet]

TBH, I don't know.

I suppose people would expect an API similar to what they are used to with JPA but it's really 2 completely different things.

Another thing is that a lot of people are using layers on top of JPA so maybe they are not that used to the JPA API.

I like your fetch(limit, offset) proposal.

[yrodiere]

I added a few commits to use fetch(...) as I proposed.

[gsmet]

Should we remove them instead? My rationale is that people will have a tendency to use them considering you don't need the L and they would have a warning.

[yrodiere]

If I remove them, people trying to pass an int (either a literal without a trailing L, or a variable) will get a compilation error. There is no automatic boxing from int to Long, just from long to Long.

If I removed the int variants and changed the parameter type from Long to long, then users would benefit from automatic casting from int to long. But then they couldn't reset the value by passing null to setFirstResult or setMaxResults anymore. Which I suppose is fine, since they could pass 0 to setFirstResult and Long.MAX_VALUE to setMaxResults`?

[gsmet]

Well, what I mean is: either you support it and you don't have them deprecated or you don't.

But what you're saying in your first paragraph means they will end up with a deprecation warning.

[yrodiere]

Ok, so I suppose "them" meant the @Deprecated annotations, not the methods. Anyway... It's probably not relevant anymore if we switch to a fetch(limit, offset) method.

[yrodiere]

I removed the deprecation, we now have two variants of each fetch method: one accepting longs and one accepting integers.

[gsmet]

I think the API is better this way.

History will tell if people really want an API similar to JPA or not :).

[fax4ever]

I approve all the changes to the APIs. I think we've got a great step forward with them. I have just few comments, very few if compared with the change size.

[yrodiere]

@fax4ever Thanks for the review. I applied all of your suggestions.

Waiting for CI, then I'll merge.

[yrodiere]

Merged. Thanks a lot for your reviews, @gsmet and @fax4ever !