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

Thanks for the input. Comments inline. On 27 Jan 2014, at 12:01, Emmanuel Bernard <emmanuel(a)hibernate.org&gt; wrote: > > On 06 May 2014, at 21:43, Hardy Ferentschik <hardy(a)hibernate.org&gt; 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.