[hibernate-dev] [Search] Index embedded and id property of embedded entity

[Emmanuel Bernard]

On 27 May 2014, at 12:29, Hardy Ferentschik <hardy at hibernate.org> wrote:

> Thanks for the input. Comments inline.
> 
> On 27 Jan 2014, at 12:01, Emmanuel Bernard <emmanuel at hibernate.org> wrote:
> 
>> 
>> On 06 May 2014, at 21:43, Hardy Ferentschik <hardy at hibernate.org> wrote:
>> 
>>> Where are we at in this discussion?
>>> 
>>> I think we basically have to main proposals.
>>> 
>>> #1 Don’t include the embedded id per default. If @IndexEmbedded is used via the depth parameter there is no way to include the embedded id.
>>>   In order to include the id you would need to use includePaths. depth and includePaths can be used in combination, so something like this 
>>>   is possible @IndexedEmbedded( depth = 1, includePaths = “foo.id” ). This will include all configured fields of the embedded entity, plus its id.
>>> #2 Don’t include the embedded id per default, but have an additional flag on @IndexedEmbedded to include the embedded id. The default would be false.
>>>  To include the id would be something like @IndexedEmbedded( depth = 1, includeEmbeddedId = true ) or 
>>>  @IndexedEmbedded(  includePaths = {“foo.a”, “foo.b”, “foo.c”} , includeEmbeddedId = true ).
>> 
>> I like #2 much better. It is too complex to ask of the user to understand how the combination of depth and includePaths work based on fairly arbitrary rules.
>> I thing the property should be named includeEmbeddedObjectsIds because embeddedId has a precise meaning in ORM.
> 
> Cool. Seems we have now several votes for this approach. I will start implementing it then. 
> +1 also for ‘includeEmbeddedObjectsIds'
> 
>> One question in that case, what does this one do
>> 
>>   @IndexedEmbedded( includePath = { “foo.id” }, includeEmbeddedId=false //default )
>> 
>> I think we should honour includePath according to the additive rule mentioned earlier and the path of least surprise.
> 
> Sounds reasonable.
> 
>>> On top of this we seem to agree that it is a good idea to set the default depth value of @IndexedEmbedded to 0. This avoids the change in default values,
>>> when includePaths is used. As a side effect the default @IndexedEmbedded annotation becomes a no-op and should be treated as an invalid configuration.
>>> The choice here is between:
>>> 
>>> #1 Log warning and move on
>>> #2 Throw an exception due to invalid configuration
>>> 
>>> My choice would be #2 again.
>> 
>> Is that really a good idea? As you guys said, @IndexedEmbedded becomes a (silently or not) broken mapping :(
>> It also means that the simplest @IE usage requires to understand depth and / or includePath.
>> I don’t like that at all. It makes things more complex than they should be for the simple case.
> 
> So what is your take on this then? Leave as is and keep the fact that the default depth value changes its default value depending 
> on whether or not includePaths is used? That would be option
> 
> #3 Keep status quo for value of depth parameter
> 
> I raised the concern that the simple @IndexEmbedded is now not valid anymore as well before. I guess the question is what you give more importance,
> ability to use the annotation with its default values or have consistent default values which don’t change. 
> 
> I still think consistency is more important and #2 is the better approach. However, before going to #1, I would rather join you and keep the status quo
> with #3.
> 
> Btw, enforcing a depth or includePath value might have the advantage of creating smaller (more targeted) indexes, since we less likely include 
> fields which are need targeted by a query.

#3 then #2 for me. I really like #3 better though for the reason I explained.
We should bet at horse races together ;)