Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

HSEARCH-3319 WIP: Type-safe field references v2 #4156

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

marko-bekhta
Copy link
Member

@marko-bekhta marko-bekhta force-pushed the feat/HSEARCH-3319-static-metamodel-field-references-v2 branch from de353a5 to aaf4fff Compare May 15, 2024 09:35
Comment on lines 383 to 386
@Incubating
default SearchPredicateFactory<SR> withRoot(ObjectFieldReference<? super SR> objectFieldReference) {
return withRoot( objectFieldReference.absolutePath() );
}
Copy link
Member Author

Choose a reason for hiding this comment

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

Even though I've added these withRoot(..) methods to factories, I am not sure that they make much sense with the approach references are taking...

Assuming we somehow address the paths, the factory would still point to the same <SR> root type, as the properties in the embedded objects of the "generated" metamodel will use them:

public static class EntityA_ {
	public static ValueFieldReference1<EntityA_, String, String, String> stringA;

	public static SomeNestedObject_ someNestedObject;

	public static RootReferenceScope<EntityA_, EntityA> scope;

	static {
		stringA = ValueFieldReference1.of( "stringA", EntityA_.class, String.class, String.class, String.class );
		someNestedObject = new SomeNestedObject_();

		scope = RootReferenceScopeImpl.of( EntityA_.class, EntityA.class );
	}

	private static class SomeNestedObject_ implements ObjectFieldReference<EntityA_> {

		public final ValueFieldReference1<EntityA_, String, String, String> nestedStringA;

		public SomeNestedObject_() {
			this.nestedStringA = ValueFieldReference1.of( "someNestedObject.stringA", EntityA_.class, String.class, String.class, String.class );
		}

		@Override
		public String absolutePath() {
			return "someNestedObject";
		}

		@Override
		public Class<EntityA_> scopeRootType() {
			return EntityA_.class;
		}
	}
}

If we go with the different <SR> for nestedStringA then we wouldn't be able to use it in a regular case e.g.:

...
.where( f -> f.match().field( EntityA_.someNestedObject.nestedStringA ).matching( "a" ) )

Also, if we change the SR on withRoot call, the current idea for these embedded objects to create a class-per-field would not fly 😔.

If we ignore the <SR> type change, then any property can be passed to the factory methods, which makes the static metamodel pointless 😔 (in this case)

Copy link
Member

Choose a reason for hiding this comment

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

Ideally, I'd expect after using withRoot(), people using the factory would use references such as SomeNestedObject_.nestedStringA, with nestedStringA being a static field. Just like they would on root types.

So you'd have two classes per object field:

  • SomeNestedObject$Absolute (the name doesn't matter much, but must not conflict with other classes), which would be the type of EntityA_.someNestedObject, and which would expose non-static "sub" fields of type ValueFieldReference<EntityA_, ...>.
  • SomeNestedObject_ which would be the type users of withRoot() would rely on, and which would expose non-static "sub" fields of type ValueFieldReference<SomeNestedObject_, ...>.

I don't think you need an inheritance relationship between these two classes, but you may need to redefine ObjectFieldReference to have two generic type parameters: the first being its "holding" type, and the second being the SR of factories obtained by calling withRoot.

That being said... Since SomeNestedObject_ is completely specific to the nestedString field of EntityA, I'm wondering how useful withRoot() would be. The main point of withRoot is to obtain a factory that can be passed to some util method, which is then able to build a predicate. That method is supposed to be used with different object fields that are assumed to have a similar schema.
If that method works for a specific object field only (with a specific metamodel type, e.g. SomeNestedObject_), then... what's the point?

I suppose it all comes down to the "mapped superclass"/inheritance support I mentioned the other time. If we could make sure that object field metamodel uses inheritance, i.e. the metamodel type EntityB_.SomeNestedObject_ extends EntityA_.SomeNestedObject_ as soon as EntityB extends EntityA, then you'd at least be able to create a root from EntityB_.someNestedObject and then use EntityA_.SomeNestedObject_.nestedStringA on the resulting factory.

Union types at the object field level could be a thing too, but that seems... hard. We'd essentially need SomeNestedObject_ to extend all its union types. That means multiple inheritance, which means SomeNestedObject_ must be an interface. Possible, since interfaces can have static fields, but needs investigation.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm ok I've come up with the next example. Assuming we have a model as

@Indexed
	public static class EntityA {

		@IndexedEmbedded(includePaths = { "a", "b" })
		public EmbeddedThing embA;

		@IndexedEmbedded(includePaths = { "b", "c" })
		public EmbeddedThing embB;
	}

	public static class EmbeddedThing {
		@FullTextField
		String a;
		@FullTextField
		String b;
		@FullTextField
		String c;
	}

and want to have an util to filter by smth.b using this withRoot approach...

we can generate the metamodel as:

	public static class EntityA_ {

		public static a_ embA;
		public static b_ embB;

		// These two are for the absolute paths: 
		public static class a_ extends ObjectFieldReference<EntityA_, EntityA_a_>{
			public static ValueFieldReference<EntityA_, String, String, String> a; // path = embA.a
			public static ValueFieldReference<EntityA_, String, String, String> b; // path = embA.b
		}

		public static class b_ extends ObjectFieldReference<EntityA_, EntityA_b_>{
			public static ValueFieldReference<EntityA_, String, String, String> b; // path = embB.b
			public static ValueFieldReference<EntityA_, String, String, String> c; // path = embB.c
		}
	}

	// These two are for "withRoot"/  relative paths 
	public static class EntityA_a_ {
		public static ValueFieldReference<EntityA_a_, String, String, String> a; // path = a
		public static ValueFieldReference<EntityA_a_, String, String, String> b; // path = b
	}

	public static class EntityA_b_ {
		public static ValueFieldReference<EntityA_b_, String, String, String> b; // path = b
		public static ValueFieldReference<EntityA_b_, String, String, String> c; // path = c
	}

That won't help much for the with withRoot ... so we can try:

	// ideally I guess we'd want to generate:
	public static class EntityA_a_b_common_ {
		public static ValueFieldReference<EntityA_a_b_common_, String, String, String> b; // path = b
	}

	public static class EntityA_a_ extends EntityA_a_b_common_ {
		public static ValueFieldReference<EntityA_a_, String, String, String> b; // path = b
	}
	
	public static class EntityA_b_ extends EntityA_a_b_common_ {
		public static ValueFieldReference<EntityA_b_, String, String, String> c; // path = c
	}
	
	// and then the util method could use  EntityA_a_b_common_ with access to `b` 
	//   and we pass withRoot(EntityA_.embA) withRoot(EntityA_.embB) to the util method ... 

that could probably work.. but if I understand it correctly... currently that withRoot is not tied to a particular embedded type, meaning we can have:

public static class EntityA {
	@IndexedEmbedded
	EmbeddedThing1 e1;
	@IndexedEmbedded
	EmbeddedThing1 e2;
}

public static class EmbeddedThing1 {
	@FullTextField
	String a;
	// some other fields
}

public static class EmbeddedThing2 {
	@FullTextField
	String a;
	// some other fields maybe different from EmbeddedThing1
}

public static class EntityB {
	@IndexedEmbedded
	EmbeddedThing3 e3;
}

public static class EmbeddedThing3 {
	@FullTextField
	String a;
	// some other fields maybe different from EmbeddedThing1/EmbeddedThing2
}

//util method
private SearchPredicate matchFirstAndLastName(SearchPredicateFactory f, String avalue) {
	return f.match().field( "a" ).matching( avalue ).toPredicate();
}

it is ok now to pass any of

withRoot("e1");
withRoot("e2");
withRoot("e3");

but I'm not sure how we'd tie them all to something in the metamodel to be used in that util... 😕

Copy link
Member

Choose a reason for hiding this comment

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

but I'm not sure how we'd tie them all to something in the metamodel to be used in that util... 😕

Essentially, to go beyond simple entity inheritance, we'll need to introduce union types at the object field level.

That's what I was talking about here:

Union types at the object field level could be a thing too, but that seems... hard. We'd essentially need SomeNestedObject_ to extend all its union types. That means multiple inheritance, which means SomeNestedObject_ must be an interface. Possible, since interfaces can have static fields, but needs investigation.

Copy link
Member Author

Choose a reason for hiding this comment

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

ok next iteration 🙈 I've tried to look at the interfaces and whether we can somehow use them to our advantage. This time, the model is:

@MappedSuperclass
public static class MappedSuperclassThing {
	@Id
	Long id;

	@FullTextField
	String a;
}

@Indexed
@Entity
public static class ContainingA extends MappedSuperclassThing {
	@IndexedEmbedded
	EmbeddedThing1 e1;
	@IndexedEmbedded
	EmbeddedThing1 e2;
}

public static class EmbeddedThing1 {
	@FullTextField
	String a;
	@FullTextField
	String b;
	// some other fields
}

public static class EmbeddedThing2 {
	@FullTextField
	String a;
	// some other fields maybe different from EmbeddedThing1
}

@Entity
@Indexed
public static class ContainingB extends MappedSuperclassThing{
	@IndexedEmbedded
	EmbeddedThing3 e3;
}

public static class EmbeddedThing3 {
	@GenericField
	Integer a;
	// some other fields maybe different from EmbeddedThing1/EmbeddedThing2
}

There are string/int fields a and some embeddables. Ultimately, we'd want to be able to have a util method as:

public static SearchPredicate utilMethodForPredicate(SearchPredicateFactory<? extends Property_String_a_> factory) {
	return factory.match().field( Property_String_a_.a ).matching( "a" ).toPredicate();
}

where we want to accept the factories for "scope roots" that have a string property a in them. Which can be based on ContainingA/ContainingB/EmbeddedThing1/EmbeddedThing2. For the util to work a should be a part of the Property_String_a_:

interface Property_String_a_ {
	ValueFieldReference1<Property_String_a_, String, String, String> a = ....;
}

then we can have the metamodel as:

public static class MappedSuperclassThing_ implements Property_String_a_ {

	public static HibernateOrmRootReferenceScope<MappedSuperclassThing_, MappedSuperclassThing> scope;

	static {
		//a = ValueFieldReference1.of( "a", MappedSuperclassThing_.class, String.class, String.class, String.class );

		scope = RootReferenceScopeImpl.of( MappedSuperclassThing_.class, MappedSuperclassThing.class );
	}
}

public static class ContainingA_ extends MappedSuperclassThing_ {
	public static e1_.Absolute e1;
	public static e2_.Absolute e2;

	public static HibernateOrmRootReferenceScope<ContainingA_, ContainingA> scope;

	static {
		e1 = new e1_.Absolute();
		e2 = new e2_.Absolute();

		scope = RootReferenceScopeImpl.of( ContainingA_.class, ContainingA.class );
	}

	public static class e1_ implements ObjectFieldReference<ContainingA_>, Property_String_a_, Property_String_b_ {

		@Override
		public String absolutePath() {
			return "e1";
		}

		@Override
		public Class<ContainingA_> scopeRootType() {
			return ContainingA_.class;
		}

		static class Absolute extends e1_ {
			public static ValueFieldReference1<ContainingA_, String, String, String> a;
			public static ValueFieldReference1<ContainingA_, String, String, String> b;

			static {
				a = ValueFieldReference1.of( "e1.a", ContainingA_.class, String.class, String.class, String.class );
				b = ValueFieldReference1.of( "e1.b", ContainingA_.class, String.class, String.class, String.class );
			}
		}
	}

	public static class e2_ implements ObjectFieldReference<ContainingA_>, Property_String_a_, Property_String_b_ {

		@Override
		public String absolutePath() {
			return "e2";
		}

		@Override
		public Class<ContainingA_> scopeRootType() {
			return ContainingA_.class;
		}

		static class Absolute extends e2_ {
			public static ValueFieldReference1<ContainingA_, String, String, String> a;
			public static ValueFieldReference1<ContainingA_, String, String, String> b;

			static {
				a = ValueFieldReference1.of( "e2.a", ContainingA_.class, String.class, String.class, String.class );
				b = ValueFieldReference1.of( "e2.b", ContainingA_.class, String.class, String.class, String.class );
			}
		}
	}
}

Since we use ? super SR we can do

SearchScope<ContainingA_, ContainingA> scope = ContainingA_.scope.create( searchSession );

searchSession.search( scope )
		.select( f -> f.field( ContainingA_.a ) )
		.where( f -> utilMethodForPredicate( f ) )
		.fetchHits( 20 )

as ContainingA_ extends Property_String_a_ it works both for projection and util.
Hence, I'm thinking - we can create an interface-per-filed, where we reuse the interfaces with the same name and capabilities(traits)...

@@ -38,7 +38,8 @@
*
* @param <E> A supertype of all types in this scope.
*/
public interface SearchScope<E> {
@SuppressWarnings("deprecation")
public interface SearchScope<E> extends org.hibernate.search.engine.mapper.scope.SearchScope<E, org.hibernate.search.mapper.orm.common.EntityReference> {
Copy link
Member Author

Choose a reason for hiding this comment

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

Obviously, JQAssistant does not like these changes 😔 😃. However, I couldn't come up with an alternative to allow scope to be created from the "generated" static metamodel classes. Unless we generate a model specific to the mapper 😖 😕

@yrodiere if you can take a quick look at the approach in this PR whenever you have some free time 😃

Copy link
Member

Choose a reason for hiding this comment

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

Unless we generate a model specific to the mapper 😖 😕

That seems like a very valid solution to me?

Not for every Hibernate Search type in the metamodel, of course -- i.e. I wouldn't make ValueFieldReference specific to each mapper. But it seems perfectly fine to me to give EntityA_.scope a mapper-specific type.

Copy link
Member Author

Choose a reason for hiding this comment

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

But it seems perfectly fine to me to give EntityA_.scope a mapper-specific type.

mm ok, we'd need to tell the plugin generating these to know which mappers are expected and then have

EntityA_.ormScope
EntityA_.standaloneScope

we probably can detect that from the dependencies (which mappers are present) and have an explicit property if needed I suppose 🤔

Copy link
Member

Choose a reason for hiding this comment

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

mm ok, we'd need to tell the plugin generating these to know which mappers are expected and then have

The plugin would already need to know, since it needs to start Hibernate Search in offline mode to create backends and their index descriptors.

Copy link
Member

@yrodiere yrodiere May 15, 2024

Choose a reason for hiding this comment

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

(Because, yeah, annotation parsing is not enough: there are binders to take into account, and programmatic mapping :x)

@marko-bekhta marko-bekhta force-pushed the feat/HSEARCH-3319-static-metamodel-field-references-v2 branch 3 times, most recently from 8555146 to 296c173 Compare May 20, 2024 14:48
Copy link

sonarcloud bot commented May 22, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
78.2% Coverage on New Code (required ≥ 80%)

See analysis details on SonarCloud

Copy link
Member Author

@marko-bekhta marko-bekhta left a comment

Choose a reason for hiding this comment

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

Hey @yrodiere 😃 the approach we discussed last Friday seems to work well so far. (see the new inline comments or just take a look at the FieldReferenceIT )

One thing I'm not sure how to deal with is named predicates. They are defined in binders, where there's no notion of search scope. Since binders would define the index structure, does it make sense to use the generated metamodel in them? as its generation depends on the binder ... 🤔 🙈

// IMPL_NOTE: note cannot use the EntityClassName_ since ORM picks it up and tries to its thing...
// so we'd need to come up with a different naming strategy...
public static class ContainingA__
Copy link
Member Author

Choose a reason for hiding this comment

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

The entityName-underscore pattern for these "generated" classes won't work because of the ORM, even if that model is not generated... sooo should we prepend the underscore, use some prefix/suffix oooor do something else?

Copy link
Member

@yrodiere yrodiere May 23, 2024

Choose a reason for hiding this comment

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

I'd suggest looking at what's been done for Jakarta Data, because we need to avoid conflict with that as well.

Maybe SearchContainingA? ContainingA_Search?

// IMPL_NOTE: Maybe let's use the INDEX name?
// also I'm thinking we can make it configurable and let the user decide how to call this variable...
public static final ContainingA__ INDEX = new ContainingA__();
Copy link
Member Author

Choose a reason for hiding this comment

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

I thought about the naming for this constant, and INDEX seemed like an ok alternative to INSTANCE, and as the comment suggests, we can make this configurable and let the user pick the name.

Copy link
Member

Choose a reason for hiding this comment

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

But then for union scopes, you'd have... INDEXES?

Wouldn't SCOPE be a better alternative? Then you'd have ContainingA__ implement SearchScopeReferenceWhatever.

Copy link
Member

@yrodiere yrodiere May 23, 2024

Choose a reason for hiding this comment

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

Or SCOPE_REF, or a static scopeRef(), or...

Comment on lines 396 to 426
public interface ContainingA_e1_e2_e3_Intersection<SR> {

TraitsIntersection<SR, String> a();

interface TraitsIntersection<SR, T>
extends MatchPredicateFieldReference<SR, T>,
ExistsPredicateFieldReference<SR>,
QueryStringPredicateFieldReference<SR, T> {

@Override
default ValueConvert valueConvert() {
return ValueConvert.YES;
}
}
}

// the same field with the same traits:
public interface ContainingA_e1_e2_Intersection<SR> {

TraitsIntersection<SR, String, String> a();

interface TraitsIntersection<SR, T, P>
extends FieldProjectionFieldReference<SR, P>,
MatchPredicateFieldReference<SR, T>,
ExistsPredicateFieldReference<SR>,
QueryStringPredicateFieldReference<SR, T> {

@Override
default ValueConvert valueConvert() {
return ValueConvert.YES;
}
}
}
Copy link
Member Author

Choose a reason for hiding this comment

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

These are examples of intersection interfaces. I'm thinking that it makes sense to keep the trait intersection interface as an inner class, then if there are multiple intersection interfaces per one scope (ContainingA_) there's no ambiguity.

Copy link
Member

Choose a reason for hiding this comment

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

Are you sure there's a need to have special handling for the TraitsInteresection interface?

I mean, we already need to generate interfaces/classes for combination of traits, so we already assume something will be generating a FieldProjectionAndMatchPredicateAndExistsPredicateAndQueryStringPredicateFieldReference; can't we use that here, too?

Unrelated note: naming these "trait combination" interfaces will be a challenge in itself.

Copy link
Member Author

Choose a reason for hiding this comment

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

Are you sure there's a need to have special handling for the TraitsInteresection interface?

thinking more about it, yeah we can probably reuse the interfaces... I was thinking about a scenario:

@KeywordField(projectable = Projectable.YES, sortable = Sortable.NO)
private String title;

@KeywordField(projectable = Projectable.NO, sortable = Sortable.YES)
private String title;

where the intersection would be just predicates and we wouldn't have it since it is not needed for an actual field, but we could just keep creating these combinations and add them to a lookup for reuse, so +1 😃

Unrelated note: naming these "trait combination" interfaces will be a challenge in itself.

yeah... At some point I was thinking to have a sorted list of trait interfaces and then "map" them to some numbered keys, e.g. T1, T2... Tn, and the name of a combination will be based on these keys, e.g. T1T5T24FieldReferece

@marko-bekhta
Copy link
Member Author

So, in the new commit, I've tried adding a Maven plugin that would generate things. The approach I've taken here is to have model+ search config classes in their own jar that is passed as a dependency to the Maven plugin. The plugin then starts the Search. I've used some of our test helpers to start things since, well, it's just to test how it'll work.

It generates something like:

package org.hibernate.search.metamodel.generator.model;

class MyEntity__ {

	public String text;	
}
package org.hibernate.search.metamodel.generator.model;

class MyProgrammaticEntity__ {

	public String number;
	public String text;	
}

And since it happens on the generate sources phase, these are getting compiled automatically by Maven itself.

As for the plugin configuration:

<configuration>
    <annotatedTypes>
        <type>org.hibernate.search.metamodel.generator.model.MyEntity</type>
        <type>org.hibernate.search.metamodel.generator.model.MyProgrammaticEntity</type>
    </annotatedTypes>
    <properties>
        <hibernate.search.mapping.configurer>org.hibernate.search.metamodel.generator.model.MyConfigurer</hibernate.search.mapping.configurer>
    </properties>
</configuration>

There's a way to pass any properties that should be "forwarded" to the Search through properties. In this case, I've tried passing a configurer to apply some programmatic mapping 🙈.

now.... next thing I'd want to try is to run the plugin in the same module that the model is. From what I've seen so far, it is easy to get the source root, and the dependencies (things that were missing in the AP approach).

@marko-bekhta marko-bekhta force-pushed the feat/HSEARCH-3319-static-metamodel-field-references-v2 branch from 0c8366e to 30f9d6a Compare May 28, 2024 16:15
@marko-bekhta marko-bekhta force-pushed the feat/HSEARCH-3319-static-metamodel-field-references-v2 branch 2 times, most recently from 5789bc7 to 159347c Compare August 21, 2024 13:19
@marko-bekhta marko-bekhta force-pushed the feat/HSEARCH-3319-static-metamodel-field-references-v2 branch 2 times, most recently from baae623 to b8ad37a Compare December 5, 2024 17:42
@marko-bekhta marko-bekhta force-pushed the feat/HSEARCH-3319-static-metamodel-field-references-v2 branch from b8ad37a to b5a16ea Compare December 12, 2024 21:26
@hibernate-github-bot
Copy link

hibernate-github-bot bot commented Dec 12, 2024

Thanks for your pull request!

This pull request appears to follow the contribution rules.

› This message was automatically generated.

@marko-bekhta marko-bekhta force-pushed the feat/HSEARCH-3319-static-metamodel-field-references-v2 branch from b5a16ea to cbfcec9 Compare December 13, 2024 12:38
Copy link

sonarcloud bot commented Dec 13, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
72.8% Coverage on New Code (required ≥ 80%)

See analysis details on SonarQube Cloud

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants