Add PropertyPathMapper to PropertyPath creation [DATACMNS-1176]

[spring-projects-issues]

Ales Justin opened DATACMNS-1176 and commented

PropertyPath from Part(Tree) doesn't always directly map class's structure.
e.g. in Hibernate Search (HS) Spring Data implementation we map class's HS fields

@Field(name = "foo")
String bar;

public List<Simple> findByFoo(String foo);

In current impl, this use case fails.
Adding a mapper, we could be less restrictive on the actual "Part" usage.
e.g. not just strictly class's properties / hierarchy

---

No further details from DATACMNS-1176

[spring-projects-issues]

Oliver Drotbohm commented

Can you elaborate on what the use case is for this as this is mostly considered internal API. That said, what you're suggesting exactly works the other way around: query methods follow the domain model with a PersistentPropertyPath obtained from a MappingContext representing the full path including the ability to translate that into a store-specific path that might include renames, just like MongoDB uses property.getFieldName().

Does that help?

[spring-projects-issues]

Ales Justin commented

The problem is, if we go with fields, and not properties, PropertyPath ctor fails, as it cannot find matching property (since we pass-in a field).
Converting property to a field is not a problem -- we could do it in different places (and I already have this kind of impl in place).
But the problem is, Hibernate Search is field agnostic.
And going with fields, it also takes care of multi-field properties, which are not rare in Hibernate Search.

My goal was to workaround the PropertyPath ctor validation in the best possible way ...

[spring-projects-issues]

Oliver Drotbohm commented

Okay, for one: things work as they work. We can't change very core abstractions of Spring Data or extend their responsibilities, just because one store has an arbitrary requirement. Second, we have to become way more concrete if it comes to requiring changes to Spring Data. Please point to your module's code that's overly complex or describe what you'd like to achieve. Query methods are supposed to be defined based on the domain model. This not something we're going to change at any time

[spring-projects-issues]

Yoann Rodiere commented

Hello,

The thing is, translating property names will not work in our case for two reasons:

1. Fields may be generated from an entity as a whole, not a property in particular, by user-provided code. In that case users would not even be able to express the query using property paths, because the fields are not attached to a particular property.
2. There may be multiple fields for a given property. In that case using a property path for a query will be ambiguous.

Maybe a bit of context would help... Hibernate Search is a mapping tool for full-text search on Java entities (generally JPA entities). It allows to transparently update a de-normalized, full-text view of your original model. When querying, you usually refer to the de-normalized view (the full-text index), because each "field" in the index has a dedicated purpose, is meant to be used for a specific search query. For instance you may want to perform full-text search for auto-completion on the name of an entity in one part of your app, and for a classic search form in another part of the app. Those will require two different kinds of pre-processing when indexing, and thus two different fields.
Full-text search is such that mapping two fields to a given entity property is not a corner case, either. In particular, you need two different fields in the full-text index if you want to both sort and search on a text property, because the data will have to be pre-processed differently for sorts than for full-text search (in particular, you don't want to tokenize sort keys)

[spring-projects-issues]

Oliver Drotbohm commented

Such requirements are best implemented using manually declared queries. Be aware that the query derivation mechanism is a tool to get started quickly. It is not even intended to satisfy advanced querying facilities. Take JPA for example. There's no way we support all the joining capabilities JPQL exposes in query derivation. Also, everything that goes beyond a simple keyword on one or at max two properties just makes the method name ridiculously complex and unreadable.

Again, we simply can't change the PropertyPath implementation as it currently implements a guarantee that code using it relies on: if a PropertyPath instance can be successfully created, it means that the path given is valid, i.e. that all properties contained in it actually exist. Changing this would completely break this guarantee

[spring-projects-issues]

Yoann Rodiere commented

Be aware that the query derivation mechanism is a tool to get started quickly. It is not even intended to satisfy advanced querying facilities. [...]

Ok. I guess that rules out most full-text queries, unfortunately.

Again, we simply can't change the PropertyPath implementation as it currently implements a guarantee that code using it relies on:

I'm curious, are we only talking about query code, or also code that would get/set data on actual instances?

I suppose we could introduce new interfaces to abstract the validation about "this property exists" while still preserving the current behavior for existing constructors (we'd just use an implementation of the "validator" that does the exact same thing as today). Would a PR preserving existing methods/constructors/behaviors while adding new ones be acceptable, or is this a strict no?

[spring-projects-issues]

Oliver Drotbohm commented

Ok. I guess that rules out most full-text queries, unfortunately.
Care to elaborate why?

I'm curious, are we only talking about query code, or also code that would get/set data on actual instances?

There's is metamodel API in the mapping package. PersistentEntity exposes a PersistentPropertyAccessor that allows setting and getting values from entity instances. Entity creation is abstracted via the EntityInstantiator API.

As now indicated a couple of times before, this is not how we work. We don't just arbitrarily add methods to types just because a single store implementation (or their maintainers) thinks they're needed. Also, don't forget that Spring Data Commons query execution model is an opt in model. The core interface you need to implement is RepositoryQuery. You're free to virtually do anything you want here. We usually let store implementation just build what they need in their own. We then look at those implementations and decide what parts of that might be candidates for transfer into Spring Data Commons. But that's a last step, not a first one.

Did you have a look at other Spring Data implementations fronting search technologies like Spring Data Elasticsearch or Solr? I guess they're solving similar challenges as you do, right?

[spring-projects-issues]

Yoann Rodiere commented

Care to elaborate why?

As mentioned earlier:

Full-text search is such that mapping two fields to a given entity property is not a corner case, either. In particular, you need two different fields in the full-text index if you want to both sort and search on a text property, because the data will have to be pre-processed differently for sorts than for full-text search (in particular, you don't want to tokenize sort keys).

So as soon as you want to be able to search and sort on the same property (even in two different queries), you'll have to use two fields, and then as you said you'll have to use manually declared queries. That's some pretty strong limitations on sorting.
But yes, I suppose users could at least take advantage of derived queries when they want to use the default sort, or when they sort on a field that isn't also used in filtering clauses.

Did you have a look at other Spring Data implementations fronting search technologies like Spring Data Elasticsearch or Solr? I guess they're solving similar challenges as you do, right?

Not exactly the same challenges, actually, since they seem to assume that their entity models follow the index structure very closely, which is not our case (since the entities are typically also mapped in JPA). Also, we don't need to support writes, while they do.
Regardless, I had a look at the Elasticsearch integration earlier and I couldn't find an example of "derived" queries using multi-fields, for instance (it's what you typically use to do both sorting and searching on a text field in Elasticsearch). Actually Multi-field support, while available, does not seem to be documented.
But yes, I didn't spend too much time on this. I'll look harder.

Anyway, we'll have to find solutions that do not involve changing the core, that much is clear. Thanks for your patience

[spring-projects-issues]

Ales Justin commented

Again, we simply can't change the PropertyPath implementation

What about if we just changed what Part takes as propertyPath.
So that instead of PropertyPath, we would have some simple interface, e.g.:

intefrace PathHandle {
  String toDotPath();
  String getSegment();
  Class<?> getType();
}

plus a PathHandleCreator::createPathHandle(...),
which would be pushed into Part(Tree).

This would definitely work for us,
and for the query creation code,
while I would have to check in more details what this means for the mapping metamodel,
but I think it could also easily work out -- as the model itself is already abstracted enough.

I can prepare a PR for this -- if it makes sense

[spring-projects-issues]

Oliver Drotbohm commented

Again, please avoid prematurely creating PRs for Spring Data Commons and focus on the store implementation first. I'd love to see actual code within some context before arbitrarily adding methods or abstractions to core Spring Data abstractions. Please assume that things are in place and work a certain way for a reason. Spring Data itself has a domain model for the domain of implementing repositories, query methods etc. That domain model implements domain constraints and deliberately weakening those constraints or adding new model elements is not something we take lightly ("Hey, let's add this interface!") as there already is a lot of complexity involved and every addition and tweak increases that.

Regarding the suggested interface: how are you planning to obtain the type if there's no (canonical) property on a reference type you could derive that type from?

What's actually wrong with creating a dedicated (read-only) model to map the search index on? That's very much in line with CQRS semantics where a primary (JPA) command model is accompanied by an eventually consistent read model that can have a different structure optimized for particular queries. This sounds way more matching your problem space than trying to unify everything into a canonical model

[spring-projects-issues]

Ales Justin commented

I'd love to see actual code within some context before arbitrarily adding methods or abstractions to core Spring Data abstractions.

https://github.com/snowdrop/spring-data-hibernate-search

But this currently maps against properties, and not (yet) against fields.

Regarding the suggested interface: how are you planning to obtain the type if there's no (canonical) property on a reference type you could derive that type from?

In most cases we could find the property that owns that field.
If not, I think we would still be OK.

What's actually wrong with creating a dedicated (read-only) model to map the search index on? That's very much in line with CQRS semantics where a primary (JPA) command model is accompanied by an eventually consistent read model that can have a different structure optimized for particular queries. This sounds way more matching your problem space than trying to unify everything into a canonical model.

Which model are we talking about here?

My initial problem is that I don't want to re-write / copy-paste PartTree's logic.
And with current impl, I cannot work around PropertyPath validation w/o changing the code.

[spring-projects-issues]

Oliver Drotbohm commented

In most cases we could find the property that owns that field.
If not, I think we would still be OK.

That feels like a very week abstraction then as you basically don't add any real guarantees on top of a simple dot-separated String do you.

Which model are we talking about here?

The user's code. With a search index in place next to a primary backing store, I'd definitely recommend to have a dedicated entity for the primary store and a completely separate domain type and repository to access the index data so that that domain type can actually be mapped much closed to the structure of the index data

[spring-projects-issues]

Ales Justin commented

That feels like a very week abstraction then as you basically don't add any real guarantees on top of a simple dot-separated String do you.

Well, you need to find the field from the Part on your entity model.

Wrt the type, I don't see how that is relevant to the query?
Do you match parameters against it?

The user's code. With a search index in place next to a primary backing store, I'd definitely recommend to have a dedicated entity for the primary store and a completely separate domain type and repository to access the index data so that that domain type can actually be mapped much closed to the structure of the index data.

I don't see why you would need two?
You have one, which has both info; e.g. JPA and HS.

It's just that you in HS repository case, you map queries against HS info (fields),
whereas in JPA, you map it against props.

I do agree with you that this should only handle simple cases (for the good reasons you mention in some previous comment),
where the complex mappings should be handled differently -- yet to think of a good way ...

[spring-projects-issues]

Yoann Rodiere commented

That feels like a very week abstraction then as you basically don't add any real guarantees on top of a simple dot-separated String do you.

We could have such guarantees, we have our own internal model. The model just isn't a java Class, so the current PropertyPath constraints cannot work.

The user's code. With a search index in place next to a primary backing store, I'd definitely recommend to have a dedicated entity for the primary store and a completely separate domain type and repository to access the index data so that that domain type can actually be mapped much closed to the structure of the index data.

Sure, it could work that way. It's just not how Hibernate Search works. Users just annotate their primary entity, build queries targeting the target model, and get results in their primary model. Having separate class definitions for the index model would just be overhead, since those classes would never be instantiated.

In any case... I'm explaining that since we're talking about it, but you already said nothing will change in the core unless we provide a concrete, separate implementation. So I guess we can close this ticket... ?

[spring-projects-issues]

Ales Justin commented

I quickly hacked this:

• https://github.com/alesj/spring-data-commons/tree/ph1
• https://github.com/alesj/hs-data-poc/tree/v2

And it works for me.
(OK, JPA tests fail for unknown reason, but others work :-))