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

[Hardy Ferentschik]

>>> Remember _includePath_ is additive to fields otherwise included via
>>> @IndexedEmbedded _depth_, so it fits nicely for this role:
>> 
>> Is that really true? AFAIK the includePath attribute changes the behaviour of the depth attribute.
>> Depth is ‘0’ per default if ‘includePath’ is used. So you would have to include the id via includePath
>> and also set the depth explicitly to something > 0.
> 
> No it's additive. depth=n will add "all" @Field until depth=n, and it
> *also* will add what is explicitly listed in includePath.

Sure. What I meant that just adding a single include path changes the depth value.
I understand how it a depth > 0 and include path work in combination.

> So asserting that "all" doesn't include fields which are missing
> Hibernate Search indexing annotations is a quite natural implication.

?

> The problem if any is that the default depth value is different
> depending if you use includePath or not

exactly

> but also the alternative
> Integer.MAX_VALUE doesn't make much sense in practice as noone will
> define queries on an unbounded path.

hmmm, I guess you have a point here. At least at query time you need to know which
fields you are targeting. Unless of course we would support an empty prefix for index embedded. 
That’s requested in HSEARCH-183, but at the same time our documentation has a note
saying: "The prefix cannot be set to the empty string.” 

I added a quick test and there seems to be verification that the prefix is not the empty string.
If we allowed an empty prefix then unbounded inclusion makes sense. Either way, probably a good
time to address HSEARCH-183 as well. One way or the other.

> I'd actually like to propose to change the depth default to zero, and
> since includePath also defaults to an empty list, we'd be logging a
> warning as the @IndexedEmbedded annotation would have no effect.

It would be a shame in this case that the default annotation basically is an “invalid” configuration. 
If we go down this path where we either require a depth > 0 or at least one include path, I would 
go further than just logging a warning. I think we should throw an exception in case. This is probably 
also better for people who upgrade from Search 4 to 5. Messages can be ignored or depending on the 
log level even won’t show up and suddenly we get everyone asking why their app does not work anymore.
Throwing an exception would force people to make an active choice.

>> I also find it not intuitive that this means:
>> "index all embedded fields up to the given depth, plus the specified paths”.
>> I never liked how we baked depth and includePath into the same annotation. A dedicated annotation would
>> have been more appropriate.
> 
> We've been there and couldn't agree on a better proposal. Feel free to
> reopen the case on a new thread, if you have a nice name in mind.

I think I lost this one. I have no reason to believe that this has changed. I would have been easy on the names, but
dedicated annotation was what I was after. 

—Hardy