Skip to content

Conversation

@yrodiere
Copy link
Member

@yrodiere yrodiere force-pushed the HSEARCH-3671 branch 3 times, most recently from 1e76dea to 512e996 Compare September 13, 2019 10:11
SearchScope<Book> scope = searchMapping.scope( Book.class ); // <2>

SearchResult<Book> result = scope.search() // <3>
SearchResult<Book> result = scope.search( entityManager ) // <3>
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 mandatory? I must admit I'm not excited by the fact that we have to passe the EMF and the EM. It makes things far less fluid IMHO.

Copy link
Member Author

Choose a reason for hiding this comment

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

Eventually, people who don't want to load entities will be allowed to call scope.search() (if using single-tenancy) or scope.search(tenantId) (if using multi-tenancy). That's not implemented yet.

For people who actually want to load entities, well, we need an entity manager. Yes, we have a factory, but I don't think creating the entity manager ourselves silently would be a good idea. The loaded entities would not be usable anyway.

I could provide a way to pass the scope to a SearchSession, e.g. searchSession.search( scope ), but then you need an entity manager to create the SearchSession, so it's just more verbose.

Also, please keep in mind this is just the syntax with scopes, which is not really the one I want to promote. Let's say it's an alternate, lower-level syntax. The first syntax, with a SearchSession, is much better in most cases.

Copy link
Member Author

Choose a reason for hiding this comment

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

If the only problem you have is with the example itself, and the appearance of code for users who just don't want lambdas, I suppose I could also restore the scope() methods on the session. Then users will be able to trigger the search and create scope from a single object that was created from an entity manager.

However, the scopes will still be session-independent, so the search() method on those will still take an EntityManager in parameter. This method generally won't be useful though, since people can just call search on the session.

@yrodiere
Copy link
Member Author

I just pushed a few other commits to not require passing an entity manager when creating a SearchWriter or a MassIndexer. I'm beginning to think that, maybe, for those two cases, we should promote the mapping + scope syntax rather than the session syntax. I.e.:

MassIndexer indexer = Search.mapping( entityManagerFactory ).scope( MyEntity.class ).massIndexer();

Rather than:

// This entity manager is created for the only purpose of calling our API. It's not really useful.
EntityManager entityManager = entityManagerFactory.createEntityManager();
MassIndexer indexer = Search.session( entityManager ).massIndexer( MyEntity.class );

... but I suppose it's too verbose? :)

Copy link
Contributor

@fax4ever fax4ever left a comment

Choose a reason for hiding this comment

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

Making the scope session-independent has a lot of advantages it term of decoupling. I agree with you the scope should not be related to a session instance.

On the other hand, my concern I think is the same that @gsmet has: make the API less easy to use.

I think that a Hibernate user expects to work on a session instance. IMHO creating a scope from a mapping derived from a session factory, then apply a session instance to it could be seen from him too much complicated.

I know that the user can bypass the scope extraction and, in that case, the API to search is the same, but I have some concern about to give him an API that has too many steps, even if there is an alternative.

…in ToJpaIT

Because this test is about JPA, and we don't need more than an
EntityManagerFactory.
…he "mapping" package

For consistency with the ORM mapper.
They've been deprecated since 6.0.0.Alpha7, and are getting in the way
of new work.
…nager, SessionFactory, ...) to SPI (*Implementor)
…lementor from the MassIndexer

Just to simplify the context passed by the SearchScope to the
MassIndexer.
…nterfaces

Scope creation is about to be moved to the mapping, so we won't be able
to create scope from interfaces anymore (only from implementations).
…lar) to BackendSessionContext

In an attempt to clarify what these interfaces were originally intended
for, at least initially: they are a view of the mapping/session from the
backend.
…m a SearchSession

The resulting scope is still session-independent, which is a bit weird,
but it's a handy shortcut when one wants to perform a search immediately.
@yrodiere
Copy link
Member Author

I reverted some of the changes to keep the previous syntax searchSession.scope( ... ) and introduce a new searchSession.search( scope ) method.

So the syntax with object (which, keep that in mind, is an alternative syntax) will be:

SearchSession searchSession = Search.session( entityManager );
SearchScope<Book> scope = searchSession.scope( Book.class );
return searchSession.search( scope )
    .predicate( scope.predicate().matchAll().toPredicate() )
    .fetchHits( 20 );

... with the possibility to create the scope from the mapping directly, if need be (no example in the documentation yet, I'll introduce some when it's actually useful).

It is, I think, fairly similar to what we used to have:

SearchSession searchSession = Search.session( entityManager );
SearchScope<Book> scope = searchSession.scope( Book.class );
return scope.search()
    .predicate( scope.predicate().matchAll().toPredicate() )
    .fetchHits( 20 );

…earch(scope)

The implementation is ugly and requires casting, but at least the syntax
is easier on the eye for users.
It was forgotten in a previous PR.
@yrodiere yrodiere requested a review from fax4ever September 17, 2019 07:47
@yrodiere
Copy link
Member Author

@fax4ever Actually never mind, I think you already have enough review work. I addressed your comments with the last few commits, so I'll merge once the CI build succeeds.

@fax4ever
Copy link
Contributor

Anyway @yrodiere. seeing the API, not handling a full review of the changes, I can say we have reached a good compromise.

@yrodiere yrodiere merged commit e3e18e9 into hibernate:master Sep 17, 2019
@yrodiere
Copy link
Member Author

Thanks @gsmet and @fax4ever ! Merged.

@yrodiere yrodiere deleted the HSEARCH-3671 branch September 18, 2019 15:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants