HSEARCH-**** Attempt to make the migration simpler

[marko-bekhta]

[Please describe here what your change is about]

---

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license
and can be relicensed under the terms of the LGPL v2.1 license in the future at the maintainers' discretion.
For more information on licensing, please check here.

---

[hibernate-github-bot]

Thanks for your pull request!

This pull request does not follow the contribution rules. Could you have a look?

❌ All commit messages should start with a JIRA issue key matching pattern HSEARCH-\d+
    ↳ Offending commits: [e34f809, edd2090, fef0bdf, edeac21, a93ff9b, f98865b, 30297a8]

› This message was automatically generated.

[marko-bekhta]

if we do the SearchProjectionFactory<R, E> extends TypedSearchProjectionFactory<NonStaticMetamodelScope, R, E> then these definitions (similar for predicates will receive the typed version as at the time user creates the definitions we've got no idea what the scope will be... so with that users would need to update their definitions if they were creating local variables from these factories (in case of predicates) or simply update the create method signature in case of a projection..
cc @yrodiere 🙈

[yrodiere]

It seems to me that in order for type-safe factories to be useful in these definitions, we'd need the implementor of the definition to be able to check/require a specific root type, no? E.g.:

public class BookAuthorNamesProjectionDef implements ProjectionDefinition<Book__, P> {
		@Override
		public SearchProjection<List<String>> create(SearchProjectionFactory<Book__, ?, ?> factory,
				ProjectionDefinitionContext context) {
			return factory.field( Book__.authors.name )
					.collector( ProjectionCollector.list() ) // <4>
					.toProjection();
		}
}

IMO, either this ^ can be made to work, or typed scopescan be ignored here.

Either way... this is incubating API. I'd say it's fine to do whatever is easier here, and create a follow-up issue?

[marko-bekhta]

Hey @yrodiere

soooo here I've tried to explore what can be done about the predicate/projection definitions...
I didn't extend TypedPredicateDefinition with PredicateDefinition or vice-versa as that makes things more cumbersome .... since they expect the context (which has a factory either typed or not) we'd have to add the context type to the type arguments and that makes defining the typed one as:

new TypedPredicateDefinition<SomeScope, TypedPredicateDefinitionContext<SomeScope>> {
   SearchPredicate create(TypedPredicateDefinitionContext<SomeScope> context) {
   ...
   }
}

with this approach there's some ambiguity when creating these definitions as lambdas as it's unclear which context user wants in root.namedPredicate(name definition). Having a root.namedTypedPredicate() ... didn't look nice to me 😄.

and then in the actual impl... since this SR is only in the type definition and we never pass it anywhere... we can just do an "ugly-unchecked-cast".
I wonder if we should keep the scope type at runtime and make additional checks, or if it's not worth it, as the main point here was to have a compile-level check? WDYT?

[yrodiere]

I think additional runtime checks would make sense, if possible.

I also think having the definition context type as a generic type parameter is a bit odd.

And... I think it's fine to leave this for later, too, as it's incubating API that you can break and break again :)

[sonarqubecloud]

Quality Gate failed

Failed conditions
63.5% Coverage on New Code (required ≥ 80%)
11.6% Duplication on New Code (required ≤ 3%)

See analysis details on SonarQube Cloud

[marko-bekhta]

I added this method to rescope the factory in this extended interface so it wouldn't be right in front of the user all the time (I thought about creating another spi interface and moving it there ... but then we already have so many interfaces I wasn't sure if that would be a good solution..)

[marko-bekhta]

Here's how "rescoping" works ^

[yrodiere]

I'm not entirely sure how safe downcasting would be, but... whatever, if it's incubating, let's try.

[yrodiere]

I'm not entirely sure how safe downcasting would be, but... whatever, if it's incubating, let's try.

[yrodiere]

So IIUC, it's no longer possible to pass an ElasticsearchSearchPredicateFactory to a method expecting a SearchPredicateFactory? E.g. a method like the one in this example of the documentation?

private SearchPredicate matchFirstAndLastName(SearchPredicateFactory f,
        String firstName, String lastName) {
    return f.and(
            f.match().field( "firstName" ) 
                    .matching( firstName ),
            f.match().field( "lastName" )
                    .matching( lastName )
    )
            .toPredicate();
}

It's fine if this is a voluntary limitation, i.e. if it's a compromise you found between complexity and compatibility. But I thought you should be aware :)